Re: [PATCH 1/2] file-posix: Use OFD lock only if the filesystem supports the lock

[Masayoshi Mizuma]

On Thu, Nov 19, 2020 at 11:44:42AM +0100, Kevin Wolf wrote:
> Am 18.11.2020 um 20:48 hat Masayoshi Mizuma geschrieben:
> > On Wed, Nov 18, 2020 at 02:10:36PM -0500, Masayoshi Mizuma wrote:
> > > On Wed, Nov 18, 2020 at 04:42:47PM +0100, Kevin Wolf wrote:
> > > > Am 06.11.2020 um 05:01 hat Masayoshi Mizuma geschrieben:
> > > > > From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
> > > > > 
> > > > > locking=auto doesn't work if the filesystem doesn't support OFD lock.
> > > > > In that situation, following error happens:
> > > > > 
> > > > >   qemu-system-x86_64: -blockdev 
> > > > > driver=qcow2,node-name=disk,file.driver=file,file.filename=/mnt/guest.qcow2,file.locking=auto:
> > > > >  Failed to lock byte 100
> > > > > 
> > > > > qemu_probe_lock_ops() judges whether qemu can use OFD lock
> > > > > or not with doing fcntl(F_OFD_GETLK) to /dev/null. So the
> > > > > error happens if /dev/null supports OFD lock, but the filesystem
> > > > > doesn't support the lock.
> > > > > 
> > > > > Lock the actual file, not /dev/null, using F_OFD_SETLK and if that
> > > > > fails, then fallback to F_SETLK.
> > > > > 
> > > > > Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
> 
> > > > > -bool qemu_has_ofd_lock(void)
> > > > > -{
> > > > > -    qemu_probe_lock_ops();
> > > > >  #ifdef F_OFD_SETLK
> > > > > -    return fcntl_op_setlk == F_OFD_SETLK;
> > > > > +static int _qemu_lock_fcntl(int fd, struct flock *fl)
> > > > > +{
> > > > > +    int ret;
> > > > > +    bool ofd_lock = true;
> > > > > +
> > > > > +    do {
> > > > > +        if (ofd_lock) {
> > > > > +            ret = fcntl(fd, F_OFD_SETLK, fl);
> > > > > +            if ((ret == -1) && (errno == EINVAL)) {
> > > > > +                ofd_lock = false;
> > > > > +            }
> > > > > +        }
> > > > > +
> > > > > +        if (!ofd_lock) {
> > > > > +            /* Fallback to POSIX lock */
> > > > > +            ret = fcntl(fd, F_SETLK, fl);
> > > > > +        }
> > > > > +    } while (ret == -1 && errno == EINTR);
> > > > > +
> > > > > +    return ret == -1 ? -errno : 0;
> > > > > +}
> > > > >  #else
> > > > > -    return false;
> > > > > -#endif
> > > > > +static int _qemu_lock_fcntl(int fd, struct flock *fl)
> > > > > +{
> > > > > +    int ret;
> > > > > +
> > > > > +    do {
> > > > > +        ret = fcntl(fd, F_SETLK, fl);
> > > > > +    } while (ret == -1 && errno == EINTR);
> > > > > +
> > > > > +    return ret == -1 ? -errno : 0;
> > > > >  }
> > > > > +#endif
> > > > 
> > > > The logic looks fine to me, at least assuming that EINVAL is really what
> > > > we will consistently get from the kernel if OFD locks are not supported.
> > > > Is this documented anywhere? The fcntl manpage doesn't seem to mention
> > > > this case.
> > 
> > The man page of fcntl(2) says:
> > 
> >        EINVAL The value specified in cmd is not recognized by this kernel.
> > 
> > So I think EINVAL is good enough to check whether the filesystem supports
> > OFD locks or not...
> 
> A kernel not knowing the cmd at all is a somewhat different case (and
> certainly a different code path) than a filesystem not supporting it.
> 
> I just had a look at the kernel code, and to me it seems that the
> difference between POSIX locks and OFD locks is handled entirely in
> filesystem independent code. A filesystem driver would in theory have
> ways to distinguish both, but I don't see any driver in the kernel tree
> that actually does this (and there is probably little reason for a
> driver to do so).
> 
> So now I wonder what filesystem you are using? I'm curious what I
> missed.

I'm using a proprietary filesystem, which isn't in the linux kernel.
The filesystem supports posix lock only, doesn't support OFD lock...

Thanks,
Masa