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

[Vladimir Sementsov-Ogievskiy]

15.12.2020 13:53, 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>
> ---
> v5: simplify the code.
> 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   | 61 +++++++++++++++++++++++---------------------
>   include/qemu/osdep.h |  1 +
>   util/osdep.c         | 14 ++++++++++
>   3 files changed, 47 insertions(+), 29 deletions(-)
>
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 806764f7e3..4e00111031 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);
> @@ -606,7 +621,7 @@ static int raw_open_common(BlockDriverState *bs, QDict 
> *options,
>           s->use_lock = false;
>           break;

In case of ON_OFF_AUTO_ON: we do check qemu_has_ofd_lock() and print a warning.

Probably we can also check new qemu_has_file_lock() and just do early fail, not 
waiting for permissions update..

>       case ON_OFF_AUTO_AUTO:
> -        s->use_lock = qemu_has_ofd_lock();
> +        s->use_lock = qemu_has_file_lock(s->fd) && qemu_has_ofd_lock();
>           break;
>       default:
>           abort();
> @@ -625,22 +640,6 @@ static int raw_open_common(BlockDriverState *bs, QDict 
> *options,
>       s->drop_cache = qemu_opt_get_bool(opts, "drop-cache", true);
>       s->check_cache_dropped = qemu_opt_get_bool(opts, "x-check-cache-dropped",
>                                                  false);
> -
> -    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;
> -
>       /* 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);
> @@ -2388,6 +2387,7 @@ raw_co_create(BlockdevCreateOptions *options, Error 
> **errp)
>       int fd;
>       uint64_t perm, shared;
>       int result = 0;
> +    bool use_lock;
>   
>       /* Validate options and set default values */
>       assert(options->driver == BLOCKDEV_DRIVER_FILE);
> @@ -2428,19 +2428,22 @@ raw_co_create(BlockdevCreateOptions *options, Error 
> **errp)
>       perm = BLK_PERM_WRITE | BLK_PERM_RESIZE;
>       shared = BLK_PERM_ALL & ~BLK_PERM_RESIZE;
>   
> -    /* Step one: Take locks */
> -    result = raw_apply_lock_bytes(NULL, fd, perm, ~shared, false, errp);
> -    if (result < 0) {
> -        goto out_close;
> -    }
> +    use_lock = qemu_has_file_lock(fd);
> +    if (use_lock) {
> +        /* Step one: Take locks */
> +        result = raw_apply_lock_bytes(NULL, fd, perm, ~shared, false, errp);
> +        if (result < 0) {
> +            goto out_close;
> +        }
>   
> -    /* Step two: Check that nobody else has taken conflicting locks */
> -    result = raw_check_lock_bytes(fd, perm, shared, errp);
> -    if (result < 0) {
> -        error_append_hint(errp,
> -                          "Is another process using the image [%s]?\n",
> -                          file_opts->filename);
> -        goto out_unlock;
> +        /* Step two: Check that nobody else has taken conflicting locks */
> +        result = raw_check_lock_bytes(fd, perm, shared, errp);
> +        if (result < 0) {
> +            error_append_hint(errp,
> +                              "Is another process using the image [%s]?\n",
> +                              file_opts->filename);
> +            goto out_unlock;
> +        }
>       }

In raw_co_create(), I think you should also update code under "out_unlock:", we 
shouldn't
call raw_apply_lock_bytes(), when use_lock is false.


Another thing is call to raw_apply_lock_bytes() in raw_check_perm(). Looks like a 
preexisting bug. Why don't we check for s->use_lock?

>   
>       /* Clear the file by truncating it to 0 */
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index f9ec8c84e9..c7587be99d 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -513,6 +513,7 @@ 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_file_lock(int fd);
>   #endif
>   
>   #if defined(__HAIKU__) && defined(__i386__)
> diff --git a/util/osdep.c b/util/osdep.c
> index 66d01b9160..dee1f076da 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -225,6 +225,20 @@ static void qemu_probe_lock_ops(void)
>       }
>   }
>   
> +bool qemu_has_file_lock(int fd)
> +{
> +    int ret;
> +    struct flock fl = {
> +        .l_whence = SEEK_SET,
> +        .l_start  = 0,
> +        .l_len    = 0,
> +        .l_type   = F_WRLCK,
> +    };
> +
> +    ret = fcntl(fd, F_GETLK, &fl);

I think we need

    qemu_probe_lock_ops();
    ret = fcntl(fd, fcntl_op_getlk, &fl);

pattern instead, like in qemu_lock_fd_test(). Otherwise, what we check may 
differ with what we are going to use.

Also, it's  mostly a duplication of qemu_lock_fd_test(), so some refactoring 
may be done, but it may be done later.

> +    return ret == 0;
> +}
> +
>   bool qemu_has_ofd_lock(void)
>   {
>       qemu_probe_lock_ops();


--
Best regards,
Vladimir