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

[Daniel P . Berrangé]

On Wed, Dec 16, 2020 at 11:22:38AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 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.

No, we explicitly do *not* want that.  This function is *only*
about checking whether traditional fcntl locks work or not on
this specific file handle.

Support for OFD locks is a separate check, and its result
applies system wide.


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