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

[Li Feng]

Daniel P. Berrangé <berrange@redhat.com> 于2020年12月15日周二 下午6:08写道：
>
> 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();
>
Acked.

>
>
> > 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.
Acked.

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