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

[Feng Li]

Hi, Daniel
Thanks for your reply.
I have just ended my trip, sorry for my late response.
I will send out the v4.

Daniel P. Berrangé <berrange@redhat.com> 于2020年12月11日周五 上午12:55写道：
>
> 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.
Yes, check the qemu_has_ofd_lock and qemu_has_file_lock both, and
set the use_lock to false when the os supports the OFD lock, but the
filesystem doesn't support.

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

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

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