Hello, Would you review this patch when you have a chance?
Thanks! Masa On Thu, Nov 05, 2020 at 11:01:01PM -0500, Masayoshi Mizuma wrote: > From: Masayoshi Mizuma <[email protected]> > > locking=auto doesn't work if the filesystem doesn't support OFD lock. > In that situation, following error happens: > > qemu-system-x86_64: -blockdev > driver=qcow2,node-name=disk,file.driver=file,file.filename=/mnt/guest.qcow2,file.locking=auto: > Failed to lock byte 100 > > qemu_probe_lock_ops() judges whether qemu can use OFD lock > or not with doing fcntl(F_OFD_GETLK) to /dev/null. So the > error happens if /dev/null supports OFD lock, but the filesystem > doesn't support the lock. > > Lock the actual file, not /dev/null, using F_OFD_SETLK and if that > fails, then fallback to F_SETLK. > > Signed-off-by: Masayoshi Mizuma <[email protected]> > --- > block/file-posix.c | 56 ++++++++-------- > include/qemu/osdep.h | 2 +- > util/osdep.c | 149 ++++++++++++++++++++++++++++--------------- > 3 files changed, 125 insertions(+), 82 deletions(-) > > diff --git a/block/file-posix.c b/block/file-posix.c > index c63926d592..a568dbf125 100644 > --- a/block/file-posix.c > +++ b/block/file-posix.c > @@ -584,34 +584,6 @@ static int raw_open_common(BlockDriverState *bs, QDict > *options, > s->use_linux_io_uring = (aio == BLOCKDEV_AIO_OPTIONS_IO_URING); > #endif > > - locking = qapi_enum_parse(&OnOffAuto_lookup, > - qemu_opt_get(opts, "locking"), > - ON_OFF_AUTO_AUTO, &local_err); > - if (local_err) { > - error_propagate(errp, local_err); > - ret = -EINVAL; > - goto fail; > - } > - switch (locking) { > - case ON_OFF_AUTO_ON: > - s->use_lock = true; > - if (!qemu_has_ofd_lock()) { > - warn_report("File lock requested but OFD locking syscall is " > - "unavailable, falling back to POSIX file locks"); > - error_printf("Due to the implementation, locks can be lost " > - "unexpectedly.\n"); > - } > - break; > - case ON_OFF_AUTO_OFF: > - s->use_lock = false; > - break; > - case ON_OFF_AUTO_AUTO: > - s->use_lock = qemu_has_ofd_lock(); > - break; > - default: > - abort(); > - } > - > str = qemu_opt_get(opts, "pr-manager"); > if (str) { > s->pr_mgr = pr_manager_lookup(str, &local_err); > @@ -641,6 +613,34 @@ static int raw_open_common(BlockDriverState *bs, QDict > *options, > } > s->fd = fd; > > + locking = qapi_enum_parse(&OnOffAuto_lookup, > + qemu_opt_get(opts, "locking"), > + ON_OFF_AUTO_AUTO, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + ret = -EINVAL; > + goto fail; > + } > + switch (locking) { > + case ON_OFF_AUTO_ON: > + s->use_lock = true; > + if (!qemu_has_ofd_lock(s->fd)) { > + warn_report("File lock requested but OFD locking syscall is " > + "unavailable, falling back to POSIX file locks"); > + error_printf("Due to the implementation, locks can be lost " > + "unexpectedly.\n"); > + } > + break; > + case ON_OFF_AUTO_OFF: > + s->use_lock = false; > + break; > + case ON_OFF_AUTO_AUTO: > + s->use_lock = qemu_has_ofd_lock(s->fd); > + break; > + default: > + abort(); > + } > + > /* Check s->open_flags rather than bdrv_flags due to auto-read-only */ > if (s->open_flags & O_RDWR) { > ret = check_hdev_writable(s->fd); > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h > index f9ec8c84e9..222138a81a 100644 > --- a/include/qemu/osdep.h > +++ b/include/qemu/osdep.h > @@ -512,7 +512,7 @@ int qemu_dup(int fd); > int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive); > int qemu_unlock_fd(int fd, int64_t start, int64_t len); > int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive); > -bool qemu_has_ofd_lock(void); > +bool qemu_has_ofd_lock(int orig_fd); > #endif > > #if defined(__HAIKU__) && defined(__i386__) > diff --git a/util/osdep.c b/util/osdep.c > index 66d01b9160..454e8ef9f4 100644 > --- a/util/osdep.c > +++ b/util/osdep.c > @@ -117,9 +117,6 @@ int qemu_mprotect_none(void *addr, size_t size) > > #ifndef _WIN32 > > -static int fcntl_op_setlk = -1; > -static int fcntl_op_getlk = -1; > - > /* > * Dups an fd and sets the flags > */ > @@ -187,68 +184,87 @@ static int qemu_parse_fdset(const char *param) > return qemu_parse_fd(param); > } > > -static void qemu_probe_lock_ops(void) > +bool qemu_has_ofd_lock(int orig_fd) > { > - if (fcntl_op_setlk == -1) { > #ifdef F_OFD_SETLK > - int fd; > - int ret; > - struct flock fl = { > - .l_whence = SEEK_SET, > - .l_start = 0, > - .l_len = 0, > - .l_type = F_WRLCK, > - }; > - > - fd = open("/dev/null", O_RDWR); > - if (fd < 0) { > + int fd; > + int ret; > + struct flock fl = { > + .l_whence = SEEK_SET, > + .l_start = 0, > + .l_len = 0, > + .l_type = F_RDLCK, > + }; > + > + fd = qemu_dup(orig_fd); > + if (fd >= 0) { > + ret = fcntl_setfl(fd, O_RDONLY); > + if (ret) { > fprintf(stderr, > - "Failed to open /dev/null for OFD lock probing: %s\n", > - strerror(errno)); > - fcntl_op_setlk = F_SETLK; > - fcntl_op_getlk = F_GETLK; > - return; > - } > - ret = fcntl(fd, F_OFD_GETLK, &fl); > - close(fd); > - if (!ret) { > - fcntl_op_setlk = F_OFD_SETLK; > - fcntl_op_getlk = F_OFD_GETLK; > - } else { > - fcntl_op_setlk = F_SETLK; > - fcntl_op_getlk = F_GETLK; > + "Failed to fcntl for OFD lock probing.\n"); > + qemu_close(fd); > + return false; > } > + } > + > + ret = fcntl(fd, F_OFD_GETLK, &fl); > + qemu_close(fd); > + > + if (ret == 0) { > + return true; > + } else { > + return false; > + } > #else > - fcntl_op_setlk = F_SETLK; > - fcntl_op_getlk = F_GETLK; > + return false; > #endif > - } > } > > -bool qemu_has_ofd_lock(void) > -{ > - qemu_probe_lock_ops(); > #ifdef F_OFD_SETLK > - return fcntl_op_setlk == F_OFD_SETLK; > +static int _qemu_lock_fcntl(int fd, struct flock *fl) > +{ > + int ret; > + bool ofd_lock = true; > + > + do { > + if (ofd_lock) { > + ret = fcntl(fd, F_OFD_SETLK, fl); > + if ((ret == -1) && (errno == EINVAL)) { > + ofd_lock = false; > + } > + } > + > + if (!ofd_lock) { > + /* Fallback to POSIX lock */ > + ret = fcntl(fd, F_SETLK, fl); > + } > + } while (ret == -1 && errno == EINTR); > + > + return ret == -1 ? -errno : 0; > +} > #else > - return false; > -#endif > +static int _qemu_lock_fcntl(int fd, struct flock *fl) > +{ > + int ret; > + > + do { > + ret = fcntl(fd, F_SETLK, fl); > + } while (ret == -1 && errno == EINTR); > + > + return ret == -1 ? -errno : 0; > } > +#endif > > static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type) > { > - int ret; > struct flock fl = { > .l_whence = SEEK_SET, > .l_start = start, > .l_len = len, > .l_type = fl_type, > }; > - qemu_probe_lock_ops(); > - do { > - ret = fcntl(fd, fcntl_op_setlk, &fl); > - } while (ret == -1 && errno == EINTR); > - return ret == -1 ? -errno : 0; > + > + return _qemu_lock_fcntl(fd, &fl); > } > > int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive) > @@ -261,22 +277,49 @@ int qemu_unlock_fd(int fd, int64_t start, int64_t len) > return qemu_lock_fcntl(fd, start, len, F_UNLCK); > } > > -int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive) > +#ifdef F_OFD_SETLK > +static int _qemu_lock_fd_test(int fd, struct flock *fl) > { > int ret; > + > + ret = fcntl(fd, F_OFD_GETLK, fl); > + if ((ret == -1) && (errno != EINVAL)) { > + return -errno; > + > + } else if ((ret == -1) && (errno == EINVAL)) { > + /* Fallback to POSIX lock */ > + ret = fcntl(fd, F_GETLK, fl); > + if (ret == -1) { > + return -errno; > + } > + } > + > + return fl->l_type == F_UNLCK ? 0 : -EAGAIN; > +} > +#else > +static int _qemu_lock_fd_test(int fd, struct flock *fl) > +{ > + int ret; > + > + ret = fcntl(fd, F_GETLK, fl); > + if (ret == -1) { > + return -errno; > + } else { > + return fl->l_type == F_UNLCK ? 0 : -EAGAIN; > + } > +} > +#endif > + > +int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive) > +{ > struct flock fl = { > .l_whence = SEEK_SET, > .l_start = start, > .l_len = len, > .l_type = exclusive ? F_WRLCK : F_RDLCK, > }; > - qemu_probe_lock_ops(); > - ret = fcntl(fd, fcntl_op_getlk, &fl); > - if (ret == -1) { > - return -errno; > - } else { > - return fl.l_type == F_UNLCK ? 0 : -EAGAIN; > - } > + > + return _qemu_lock_fd_test(fd, &fl); > } > #endif > > -- > 2.27.0 >
