Re: [PATCH 1/2] file-posix: Use OFD lock only if the filesystem supports the lock

[Masayoshi Mizuma <msys.mizuma@gmail.com>]

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
>