On Thu, Nov 05, 2020 at 11:01:01PM -0500, Masayoshi Mizuma wrote: > From: Masayoshi Mizuma <m.miz...@jp.fujitsu.com> > > 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 <m.miz...@jp.fujitsu.com> > --- > block/file-posix.c | 56 ++++++++-------- > include/qemu/osdep.h | 2 +- > util/osdep.c | 149 ++++++++++++++++++++++++++++--------------- > 3 files changed, 125 insertions(+), 82 deletions(-)
> 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); Consider that we're *not* using OFD locks, and QEMU already has 'foo.qcow2' open for an existing disk backend, and it is locked. Now someone tries to hot-add 'foo.qcow2' for a second disk by mistake. Doing this qemu_dup + qemu_close will cause the existing locks to be removed AFAICT. > + 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); THis loop is confusing to read. I'd suggest creating a wrapper qemu_fcntl() that does the while (ret == -1 && errno == EINTR) loop, so that this locking code can be clearer without the loop. > + > + 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 Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|