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

[Daniel P . Berrangé]

On Fri, Dec 11, 2020 at 12:38:19AM +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".
> 
> Add a new function 'qemu_has_file_lock' to detect if the filesystem supports 
> locks
> or not.
> And when the drive is auto mode, use the 'qemu_has_file_lock' to set the 
> toggle.
> 
> Signed-off-by: Li Feng <fengli@smartx.com>
> ---
> v3: don't call the qemu_has_ofd_lock, use a new function instead.
> v2: remove the refactoring.
> ---
>  block/file-posix.c   | 30 +++++++++++++++++-------------
>  include/qemu/osdep.h |  1 +
>  util/osdep.c         | 29 +++++++++++++++++++++++++++++
>  3 files changed, 47 insertions(+), 13 deletions(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 806764f7e3..48f9a32de2 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -606,7 +606,7 @@ static int raw_open_common(BlockDriverState *bs, QDict 
> *options,
>          s->use_lock = false;
>          break;
>      case ON_OFF_AUTO_AUTO:
> -        s->use_lock = qemu_has_ofd_lock();
> +        s->use_lock = qemu_has_file_lock(filename);

This is not good - it causes us to always use locks by default, where
as previously we only used them if OFD was available. It neds to test
both here, except opening + closing filename to test for fnctl support
risks releasing any locks QEMU already holds on filename if OFD is not
supported.

>          break;
>      default:
>          abort();
> @@ -2388,6 +2388,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 +2429,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(file_opts->filename);

This cause QEMU to open and close filename. If another thread
already had filename open, and OFD is not support, we've just
lock the locks we held. We need to use 'fd' which is already
open.

> +    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;
> +        }
>      }
>  
>      /* Clear the file by truncating it to 0 */


> +bool qemu_has_file_lock(const char *filename)

IMO thisshould just accept a pre-opened 'int fd'

> +{
> +#ifdef F_OFD_SETLK
> +    int cmd = F_OFD_GETLK;
> +#else
> +    int cmd = F_GETLK;
> +#endif
> +        int fd;
> +        int ret;
> +        struct flock fl = {
> +            .l_whence = SEEK_SET,
> +            .l_start  = 0,
> +            .l_len    = 0,
> +            .l_type   = F_WRLCK,
> +        };
> +
> +        fd = open(filename, O_RDWR);
> +        if (fd < 0) {
> +            fprintf(stderr,
> +                    "Failed to open %s for OFD lock probing: %s\n",
> +                    filename,
> +                    strerror(errno));
> +            return false;
> +        }
> +        ret = fcntl(fd, cmd, &fl);
> +        close(fd);
> +        return ret == 0;
> +}
> +
>  bool qemu_has_ofd_lock(void)
>  {
>      qemu_probe_lock_ops();

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