[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/2] file-posix: Use OFD lock only if the filesystem supports
|
From: |
Masayoshi Mizuma |
|
Subject: |
Re: [PATCH 1/2] file-posix: Use OFD lock only if the filesystem supports the lock |
|
Date: |
Wed, 18 Nov 2020 10:06:31 -0500 |
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
>