Re: [RFC PATCH] fs: drop the fl_owner_t argument from ->flush

[Jeff Layton <jlayton@kernel.org>]

On Wed, 2022-11-30 at 13:46 +0100, Miklos Szeredi wrote:
> On Mon, 28 Nov 2022 at 16:03, Jeff Layton <jlayton@kernel.org> wrote:
> 
> > It would be nice to do this, but the FUSE thing may make it impossible.
> > 
> > Is there a way to audit the various flush operations in userland FUSE
> > filesystems and see whether they do anything with the lock_owner? I did
> > take a peek at ceph-fuse and it doesn't care about the lock_owner in its
> > flush op. I'm at a loss as to how to check this more broadly.
> > 
> > If we can't do this right away, then perhaps we could try to change this
> > as part of the FUSE userland API first, and follow up with a change like
> > this later.
> > 
> > Thoughts?
> 
> So fuse is alone in trying to support remote POSIX locks?
> 

No. Several network filesystems support POSIX locking. This is about the
->flush file_operation. That op is called from filp_close, just before
we remove POSIX locks:

-------------8<-----------------
int filp_close(struct file *filp, fl_owner_t id)
{
        int retval = 0;

        if (!file_count(filp)) {
                printk(KERN_ERR "VFS: Close: file count is 0\n");
                return 0;
        }

        if (filp->f_op->flush)
                retval = filp->f_op->flush(filp, id);

        if (likely(!(filp->f_mode & FMODE_PATH))) {
                dnotify_flush(filp, id);
                locks_remove_posix(filp, id);
        }
        fput(filp);
        return retval;
}
-------------8<-----------------

None of the in-kernel filesystems that define a ->flush operation do
anything with the fl_owner_t arg, aside from FUSE which just passes it
along.

ceph-fuse also seems to ignore the lock_owner in its ->flush (not that
it's necessarily a representative sample, but it does support file
locking). My suspicion is that most, if not all FUSE filesystems don't
pay attention to the lock_owner in their ->flush ops.

To be clear, fuse_common.h has this in struct fuse_file_info:

        /** Lock owner id.  Available in locking operations and flush */
        uint64_t lock_owner;

After the proposed change, we'd just change the comment to say:

	/** Lock owner id.  Available in locking operations */

The big question is whether this would negatively affect any FUSE
filesystems in the field.


> There's a feature flag in fuse: FUSE_POSIX_LOCKS.  In theory it's okay
> to remove this feature from the INIT flags, which tells the server
> that the client doesn't support remote POSIX locks.  At that point
> flush wouldn't need the lock owner.
> 
> I'm not sure if there are any users of this feature, and whether they
> check the feature flag.
> 
> So it's not easy to deprecate, but maybe we could  start by adding a
> CONFIG_FUSE_REMOTE_POSIX_LOCKS with an appropriate notice.
> 

This wouldn't obviate FUSE's support for POSIX locks, but I do see a
problem for FUSE here. It looks like FUSE doesn't record POSIX locks in
the local inode. Without that, it'd never unlock due to the optimization
at the start of locks_remove_posix.

That'd be pretty simple to fix though. We could add a fstype flag that
just says "don't trust that list_empty check and always unlock". We'd
set that for FUSE and you'd still get the whole-file unlock call down to
the fs for any inode with a file_lock_context.
-- 
Jeff Layton <jlayton@kernel.org>