On Wed, 2022-11-30 at 13:46 +0100, Miklos Szeredi wrote:
> On Mon, 28 Nov 2022 at 16:03, Jeff Layton <jlayton@xxxxxxxxxx> 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@xxxxxxxxxx>
