| Message ID | 20201106040102.13892-1-msys.mizuma@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] file-posix: Use OFD lock only if the filesystem supports the lock | expand |
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 <m.mizuma@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.mizuma@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/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 >
On Thu, Nov 05, 2020 at 11:01:01PM -0500, Masayoshi Mizuma wrote: > From: Masayoshi Mizuma <m.mizuma@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.mizuma@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 :|
Am 06.11.2020 um 05:01 hat Masayoshi Mizuma geschrieben: > From: Masayoshi Mizuma <m.mizuma@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.mizuma@jp.fujitsu.com> CCing qemu-block, which is the relevant mailing list. You can use the scripts/get_maintainer.pl script to find out who should be CCed on your patches. As qemu-devel itself is a very high traffic list, it's easy for a patch to get lost if it's only sent there. > 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); I don't understand this part. Why are you trying to reopen the file descriptor read-only? Shouldn't the test work fine with a read-write file descriptor? /dev/null was opened O_RDWR in the old code. > + 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); F_OFD_GETLK doesn't modify the state, so it seems to me that even the qemu_dup() is unnecessary and we could just directly try F_OFD_GETLK on the passed file descriptor (orig_fd). > + > + if (ret == 0) { > + return true; > + } else { > + return false; > + } This should be written shorter as return ret == 0; > #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 The logic looks fine to me, at least assuming that EINVAL is really what we will consistently get from the kernel if OFD locks are not supported. Is this documented anywhere? The fcntl manpage doesn't seem to mention this case. Anyway, I think I would try to minimise the duplication by having only a small #ifdef section inside the function, maybe like this: #ifdef F_OFD_SETLK ret = fcntl(fd, F_OFD_SETLK, fl); if ((ret == -1) && (errno == EINVAL)) { ofd_lock = false; } #else ofd_lock = false; #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; > + Please remove this empty line. The parentheses in the condition (above and below) are not stricly necessary. > + } 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 Same idea as above: #ifdef only around the fcntl(F_OFD_GETLK) call can minimise the code duplication. > +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 After moving the #ifdef into the function, you can inline _qemu_lock_fd_test() and and _qemu_lock_fcntl() again. This is also good because identifiers starting with an underscore are reserved in the C standard. Kevin
On Wed, Nov 18, 2020 at 03:16:53PM +0000, Daniel P. Berrangé wrote: > On Thu, Nov 05, 2020 at 11:01:01PM -0500, Masayoshi Mizuma wrote: > > From: Masayoshi Mizuma <m.mizuma@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.mizuma@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. Thank you for pointing it out. I'll remove this qemu_dup() and check orig_fd directly. > > > + 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. Great idea. I'll make qemu_fcntl(). Thanks! Masa > > > + > > + 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 :| >
On Wed, Nov 18, 2020 at 04:42:47PM +0100, Kevin Wolf wrote: > Am 06.11.2020 um 05:01 hat Masayoshi Mizuma geschrieben: > > From: Masayoshi Mizuma <m.mizuma@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.mizuma@jp.fujitsu.com> > > CCing qemu-block, which is the relevant mailing list. You can use the > scripts/get_maintainer.pl script to find out who should be CCed on your > patches. > > As qemu-devel itself is a very high traffic list, it's easy for a patch > to get lost if it's only sent there. Thank you for letting me know. I'll do scripts/get_maintainer.pl to get the mailing list before posting patches. > > > 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); > > I don't understand this part. Why are you trying to reopen the file > descriptor read-only? Shouldn't the test work fine with a read-write > file descriptor? /dev/null was opened O_RDWR in the old code. > > > + 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); > > F_OFD_GETLK doesn't modify the state, so it seems to me that even the > qemu_dup() is unnecessary and we could just directly try F_OFD_GETLK on > the passed file descriptor (orig_fd). OK, I'll change to try F_OFD_GETLK of orig_fd directly. > > > + > > + if (ret == 0) { > > + return true; > > + } else { > > + return false; > > + } > > This should be written shorter as return ret == 0; > > > #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 > > The logic looks fine to me, at least assuming that EINVAL is really what > we will consistently get from the kernel if OFD locks are not supported. > Is this documented anywhere? The fcntl manpage doesn't seem to mention > this case. > > Anyway, I think I would try to minimise the duplication by having only > a small #ifdef section inside the function, maybe like this: > > #ifdef F_OFD_SETLK > ret = fcntl(fd, F_OFD_SETLK, fl); > if ((ret == -1) && (errno == EINVAL)) { > ofd_lock = false; > } > #else > ofd_lock = false; > #endif Great! I'll make this. > > > 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; > > + > > Please remove this empty line. > > The parentheses in the condition (above and below) are not stricly > necessary. Got it. > > > + } 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 > > Same idea as above: #ifdef only around the fcntl(F_OFD_GETLK) call can > minimise the code duplication. > > > +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 > > After moving the #ifdef into the function, you can inline > _qemu_lock_fd_test() and and _qemu_lock_fcntl() again. This is also good > because identifiers starting with an underscore are reserved in the C > standard. Got it, thanks! I'll post v2. - Masa
On Wed, Nov 18, 2020 at 02:10:36PM -0500, Masayoshi Mizuma wrote: > On Wed, Nov 18, 2020 at 04:42:47PM +0100, Kevin Wolf wrote: > > Am 06.11.2020 um 05:01 hat Masayoshi Mizuma geschrieben: > > > From: Masayoshi Mizuma <m.mizuma@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.mizuma@jp.fujitsu.com> > > > > CCing qemu-block, which is the relevant mailing list. You can use the > > scripts/get_maintainer.pl script to find out who should be CCed on your > > patches. > > > > As qemu-devel itself is a very high traffic list, it's easy for a patch > > to get lost if it's only sent there. > > Thank you for letting me know. > I'll do scripts/get_maintainer.pl to get the mailing list before posting patches. > > > > > > 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); > > > > I don't understand this part. Why are you trying to reopen the file > > descriptor read-only? Shouldn't the test work fine with a read-write > > file descriptor? /dev/null was opened O_RDWR in the old code. > > > > > + 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); > > > > F_OFD_GETLK doesn't modify the state, so it seems to me that even the > > qemu_dup() is unnecessary and we could just directly try F_OFD_GETLK on > > the passed file descriptor (orig_fd). > > OK, I'll change to try F_OFD_GETLK of orig_fd directly. > > > > > > + > > > + if (ret == 0) { > > > + return true; > > > + } else { > > > + return false; > > > + } > > > > This should be written shorter as return ret == 0; > > > > > #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 > > > > The logic looks fine to me, at least assuming that EINVAL is really what > > we will consistently get from the kernel if OFD locks are not supported. > > Is this documented anywhere? The fcntl manpage doesn't seem to mention > > this case. The man page of fcntl(2) says: EINVAL The value specified in cmd is not recognized by this kernel. So I think EINVAL is good enough to check whether the filesystem supports OFD locks or not... Thanks, Masa > > > > Anyway, I think I would try to minimise the duplication by having only > > a small #ifdef section inside the function, maybe like this: > > > > #ifdef F_OFD_SETLK > > ret = fcntl(fd, F_OFD_SETLK, fl); > > if ((ret == -1) && (errno == EINVAL)) { > > ofd_lock = false; > > } > > #else > > ofd_lock = false; > > #endif > > Great! I'll make this. > > > > > > 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; > > > + > > > > Please remove this empty line. > > > > The parentheses in the condition (above and below) are not stricly > > necessary. > > Got it. > > > > > > + } 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 > > > > Same idea as above: #ifdef only around the fcntl(F_OFD_GETLK) call can > > minimise the code duplication. > > > > > +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 > > > > After moving the #ifdef into the function, you can inline > > _qemu_lock_fd_test() and and _qemu_lock_fcntl() again. This is also good > > because identifiers starting with an underscore are reserved in the C > > standard. > > Got it, thanks! I'll post v2. > > - Masa
Am 18.11.2020 um 20:48 hat Masayoshi Mizuma geschrieben: > On Wed, Nov 18, 2020 at 02:10:36PM -0500, Masayoshi Mizuma wrote: > > On Wed, Nov 18, 2020 at 04:42:47PM +0100, Kevin Wolf wrote: > > > Am 06.11.2020 um 05:01 hat Masayoshi Mizuma geschrieben: > > > > From: Masayoshi Mizuma <m.mizuma@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.mizuma@jp.fujitsu.com> > > > > -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 > > > > > > The logic looks fine to me, at least assuming that EINVAL is really what > > > we will consistently get from the kernel if OFD locks are not supported. > > > Is this documented anywhere? The fcntl manpage doesn't seem to mention > > > this case. > > The man page of fcntl(2) says: > > EINVAL The value specified in cmd is not recognized by this kernel. > > So I think EINVAL is good enough to check whether the filesystem supports > OFD locks or not... A kernel not knowing the cmd at all is a somewhat different case (and certainly a different code path) than a filesystem not supporting it. I just had a look at the kernel code, and to me it seems that the difference between POSIX locks and OFD locks is handled entirely in filesystem independent code. A filesystem driver would in theory have ways to distinguish both, but I don't see any driver in the kernel tree that actually does this (and there is probably little reason for a driver to do so). So now I wonder what filesystem you are using? I'm curious what I missed. Kevin
On Thu, Nov 19, 2020 at 11:44:42AM +0100, Kevin Wolf wrote: > Am 18.11.2020 um 20:48 hat Masayoshi Mizuma geschrieben: > > On Wed, Nov 18, 2020 at 02:10:36PM -0500, Masayoshi Mizuma wrote: > > > On Wed, Nov 18, 2020 at 04:42:47PM +0100, Kevin Wolf wrote: > > > > Am 06.11.2020 um 05:01 hat Masayoshi Mizuma geschrieben: > > > > > From: Masayoshi Mizuma <m.mizuma@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.mizuma@jp.fujitsu.com> > > > > > > -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 > > > > > > > > The logic looks fine to me, at least assuming that EINVAL is really what > > > > we will consistently get from the kernel if OFD locks are not supported. > > > > Is this documented anywhere? The fcntl manpage doesn't seem to mention > > > > this case. > > > > The man page of fcntl(2) says: > > > > EINVAL The value specified in cmd is not recognized by this kernel. > > > > So I think EINVAL is good enough to check whether the filesystem supports > > OFD locks or not... > > A kernel not knowing the cmd at all is a somewhat different case (and > certainly a different code path) than a filesystem not supporting it. > > I just had a look at the kernel code, and to me it seems that the > difference between POSIX locks and OFD locks is handled entirely in > filesystem independent code. A filesystem driver would in theory have > ways to distinguish both, but I don't see any driver in the kernel tree > that actually does this (and there is probably little reason for a > driver to do so). > > So now I wonder what filesystem you are using? I'm curious what I > missed. I'm using a proprietary filesystem, which isn't in the linux kernel. The filesystem supports posix lock only, doesn't support OFD lock... Thanks, Masa
Am 20.11.2020 um 00:56 hat Masayoshi Mizuma geschrieben: > On Thu, Nov 19, 2020 at 11:44:42AM +0100, Kevin Wolf wrote: > > Am 18.11.2020 um 20:48 hat Masayoshi Mizuma geschrieben: > > > On Wed, Nov 18, 2020 at 02:10:36PM -0500, Masayoshi Mizuma wrote: > > > > On Wed, Nov 18, 2020 at 04:42:47PM +0100, Kevin Wolf wrote: > > > > > The logic looks fine to me, at least assuming that EINVAL is really what > > > > > we will consistently get from the kernel if OFD locks are not supported. > > > > > Is this documented anywhere? The fcntl manpage doesn't seem to mention > > > > > this case. > > > > > > The man page of fcntl(2) says: > > > > > > EINVAL The value specified in cmd is not recognized by this kernel. > > > > > > So I think EINVAL is good enough to check whether the filesystem supports > > > OFD locks or not... > > > > A kernel not knowing the cmd at all is a somewhat different case (and > > certainly a different code path) than a filesystem not supporting it. > > > > I just had a look at the kernel code, and to me it seems that the > > difference between POSIX locks and OFD locks is handled entirely in > > filesystem independent code. A filesystem driver would in theory have > > ways to distinguish both, but I don't see any driver in the kernel tree > > that actually does this (and there is probably little reason for a > > driver to do so). > > > > So now I wonder what filesystem you are using? I'm curious what I > > missed. > > I'm using a proprietary filesystem, which isn't in the linux kernel. > The filesystem supports posix lock only, doesn't support OFD lock... Do you know why that proprietary filesystem driver makes a difference between POSIX locks and OFD locks? The main difference between both types is when they are released automatically, and this is handled by generic kernel code and not the filesystem driver. From a filesystem perspective, I don't see any reason to even distuingish. So unless there are good reasons for making the distinction, I'm currently inclined to view this as a filesystem driver bug. It makes handling this case hard because when the case isn't even supposed to exist, of course there won't be a defined error code. Kevin
On Fri, Nov 20, 2020 at 04:42:28PM +0100, Kevin Wolf wrote: > Am 20.11.2020 um 00:56 hat Masayoshi Mizuma geschrieben: > > On Thu, Nov 19, 2020 at 11:44:42AM +0100, Kevin Wolf wrote: > > > Am 18.11.2020 um 20:48 hat Masayoshi Mizuma geschrieben: > > > > On Wed, Nov 18, 2020 at 02:10:36PM -0500, Masayoshi Mizuma wrote: > > > > > On Wed, Nov 18, 2020 at 04:42:47PM +0100, Kevin Wolf wrote: > > > > > > The logic looks fine to me, at least assuming that EINVAL is really what > > > > > > we will consistently get from the kernel if OFD locks are not supported. > > > > > > Is this documented anywhere? The fcntl manpage doesn't seem to mention > > > > > > this case. > > > > > > > > The man page of fcntl(2) says: > > > > > > > > EINVAL The value specified in cmd is not recognized by this kernel. > > > > > > > > So I think EINVAL is good enough to check whether the filesystem supports > > > > OFD locks or not... > > > > > > A kernel not knowing the cmd at all is a somewhat different case (and > > > certainly a different code path) than a filesystem not supporting it. > > > > > > I just had a look at the kernel code, and to me it seems that the > > > difference between POSIX locks and OFD locks is handled entirely in > > > filesystem independent code. A filesystem driver would in theory have > > > ways to distinguish both, but I don't see any driver in the kernel tree > > > that actually does this (and there is probably little reason for a > > > driver to do so). > > > > > > So now I wonder what filesystem you are using? I'm curious what I > > > missed. > > > > I'm using a proprietary filesystem, which isn't in the linux kernel. > > The filesystem supports posix lock only, doesn't support OFD lock... > > Do you know why that proprietary filesystem driver makes a difference > between POSIX locks and OFD locks? The main difference between both > types is when they are released automatically, and this is handled by > generic kernel code and not the filesystem driver. > > From a filesystem perspective, I don't see any reason to even > distuingish. So unless there are good reasons for making the > distinction, I'm currently inclined to view this as a filesystem > driver bug. > > It makes handling this case hard because when the case isn't even > supposed to exist, of course there won't be a defined error code. Hi Kevin, The filesystem team found a locking issue in the filesystem. Your comments were very helpful! I really appriciate it. Thanks, Masa
Hi Masa, Am 10.02.2021 um 17:43 hat Masayoshi Mizuma geschrieben: > Hi Kevin, > > The filesystem team found a locking issue in the filesystem. > Your comments were very helpful! I really appriciate it. > > Thanks, > Masa I'm glad that I could help you to find the root cause. Thanks for reporting back! Kevin
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