Re: [PATCH v4] file-posix: detect the lock using the real file

[Daniel P . Berrangé]

On Tue, Dec 15, 2020 at 03:09:28PM +0800, Li Feng wrote:
> This patch addresses this issue:
> When accessing a volume on an NFS filesystem without supporting the file lock,
> tools, like qemu-img, will complain "Failed to lock byte 100".
> 
> In the original code, the qemu_has_ofd_lock will test the lock on the
> "/dev/null" pseudo-file. Actually, the file.locking is per-drive property,
> which depends on the underlay filesystem.
> 
> In this patch, add a new 'qemu_has_file_lock' to detect whether the
> file supports the file lock. And disable the lock when the underlay file
> system doesn't support locks.
> 
> Signed-off-by: Li Feng <fengli@smartx.com>
> ---
> v4: use the fd as the qemu_has_file_lock argument.
> v3: don't call the qemu_has_ofd_lock, use a new function instead.
> v2: remove the refactoring.
> ---
>  block/file-posix.c   | 66 +++++++++++++++++++++++++-------------------
>  include/qemu/osdep.h |  1 +
>  util/osdep.c         | 19 +++++++++++++
>  3 files changed, 58 insertions(+), 28 deletions(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 806764f7e3..9708212f01 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -584,6 +584,21 @@ static int raw_open_common(BlockDriverState *bs, QDict 
> *options,
>      s->use_linux_io_uring = (aio == BLOCKDEV_AIO_OPTIONS_IO_URING);
>  #endif
>  
> +    s->open_flags = open_flags;
> +    raw_parse_flags(bdrv_flags, &s->open_flags, false);
> +
> +    s->fd = -1;
> +    fd = qemu_open(filename, s->open_flags, errp);
> +    ret = fd < 0 ? -errno : 0;
> +
> +    if (ret < 0) {
> +        if (ret == -EROFS) {
> +            ret = -EACCES;
> +        }
> +        goto fail;
> +    }
> +    s->fd = fd;
> +
>      locking = qapi_enum_parse(&OnOffAuto_lookup,
>                                qemu_opt_get(opts, "locking"),
>                                ON_OFF_AUTO_AUTO, &local_err);
> @@ -607,6 +622,13 @@ static int raw_open_common(BlockDriverState *bs, QDict 
> *options,
>          break;
>      case ON_OFF_AUTO_AUTO:
>          s->use_lock = qemu_has_ofd_lock();
> +        if (s->use_lock && !qemu_has_file_lock(s->fd)) {
> +            /*
> +             * When the os supports the OFD lock, but the filesystem doesn't
> +             * support, just disable the file lock.
> +             */
> +            s->use_lock = false;
> +        }

This should just be

  s->use_lock = qemu_has_file_lock(s->fd) && qemu_has_ofd_lock();



> diff --git a/util/osdep.c b/util/osdep.c
> index 66d01b9160..07de97e2c5 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -225,6 +225,25 @@ static void qemu_probe_lock_ops(void)
>      }
>  }
>  
> +bool qemu_has_file_lock(int fd)
> +{
> +#ifdef F_OFD_SETLK
> +    int cmd = F_OFD_GETLK;
> +#else
> +    int cmd = F_GETLK;
> +#endif

This should unconditionally use  F_GETLK.

> +    int ret;
> +    struct flock fl = {
> +        .l_whence = SEEK_SET,
> +        .l_start  = 0,
> +        .l_len    = 0,
> +        .l_type   = F_WRLCK,
> +    };
> +
> +    ret = fcntl(fd, cmd, &fl);
> +    return ret == 0;
> +}

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 :|