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

[Daniel P . Berrangé]

On Wed, Dec 16, 2020 at 03:03:08PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 16.12.2020 13:41, Daniel P. Berrangé wrote:
> > On Wed, Dec 16, 2020 at 01:25:36PM +0300, Vladimir Sementsov-Ogievskiy 
> > wrote:
> > > 16.12.2020 12:49, Daniel P. Berrangé wrote:
> > > > 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.
> > > > > > 


> > > > > > 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.
> > > 
> > > Hmm, than may be name the function qemu_has_posix_lock(), to stress that 
> > > fact? All other qemu*lock*(fd) API functions do rely on 
> > > fcnt_op_getlk/fcntl_op_setlk and work with lock type determined by 
> > > qemu_probe_lock_ops().
> > > 
> > > > 
> > > > Support for OFD locks is a separate check, and its result
> > > > applies system wide.
> > > > 
> > > 
> > > Still, I don't follow, why should we check posix lock, when we are
> > > going to use ofd locks. What if OFD locks are supported by kernel,
> > > but specific file-system supports posix lock, but not ofd? Than
> > > we'll fail the same way as described in commit message and the
> > > patch doesn't help. Or what I miss?
> > 
> > That's not a scenario that exists. OFD locks are implemented by the
> > kernel in the generic VFS layer, so apply to all filesystems. The
> > filesystems merely have to support traditiaonl fcntl locks, and then
> > they get OFD for free.
> > 
> > IOW, there are two separate questions the code needs answers to
> > 
> >   1. Does this specific filesystem support locking
> >   2. Does the operating system support OFD locking
> > 
> > The problem in the commit message is because the original code was asking
> > question 2 only and geting the correct answer that the OS supports OFD.
> > The image was stored on a filesystem, however, that does not support fnctl
> > locks at all and hence locking failed. This failure would happen regardless
> > of whether OFD or traditional fcntl locks were used.
> > 
> > The problem is solved by also asking the first question before enabling
> > use of locks.
> > 
> 
> OK, thanks for explanation. Sorry, I was not attentive to previous versions, 
> but I remember that something about this was discussed, so probably you 
> explain this thing not the first time;( Hmm, still, what's wrong with 
> checking fs by OFD lock trying? It will fail anyway? Or, it will NOT fail, 
> because OFD knows that there is no locks, and will not ask filesystem driver 
> and we will fail only later, when try to set the lock? If so, it worth a 
> comment in file-posix.c.. As it looks like a design flaw of OFD locks.

We want to cache the OFD result check so we don't have to repeat the
probe every time. If we check OFD per-FS it makes caching much more
complex than it needs to be.


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