Re: [RFC] fl_owner_t and use of filp_close() in nfs4_free_lock_stateid()

[Jeff Layton <jlayton@redhat.com>]

On Tue, 2022-10-11 at 21:02 +0200, Miklos Szeredi wrote:
> On Tue, 11 Oct 2022 at 20:04, Al Viro <viro@zeniv.linux.org.uk> wrote:
> 
> > Another interesting question is about FUSE ->flush() - how is the
> > server supposed to use the value it gets from
> >         inarg.lock_owner = fuse_lock_owner_id(fm->fc, id);
> > in fuse_flush()?  Note that e.g. async write might be followed by
> > close() before the completion.  Moreover, it's possible to start
> > async write and do unshare(CLONE_FILES); if the descriptor table
> > used to be shared and all other threads exit after our unshare,
> > it's possible to get
> >         async write begins, fuse_send_write() called with current->files as owner
> >         flush happens, with current->files as id
> >         what used to be current->files gets freed and memory reused
> >         async write completes
> > 
> > Miklos, could you give some braindump on that?
> 
> The lock_owner in flush is supposed to be used for remote posix lock
> release [1].   I don't like posix lock semantics the least bit, and in
> hindsight it would have been better to just not try to support remote
> posix locks (nfs doesn't, so why would anyone care for it in fuse?)
> Anyway, it's probably too late to get rid of this wart now.
> 

The NFS client maintains lock records in the local VFS. When a file is
closed, the VFS issues a whole file unlock. You're probably getting
bitten by this in locks_remove_posix:

        ctx =  smp_load_acquire(&inode->i_flctx);
        if (!ctx || list_empty(&ctx->flc_posix))
                return;

Because FUSE doesn't set any locks in the local kernel, that final
unlock never occurs.

There are a couple of options here: You could have FUSE start setting
local lock records, or you could look at pushing the above check down
into the individual ->lock ops.

> The lock_owner field in read/write/setattr was added for mandatory
> locking [2].  Now that support for mandatory locking has been removed
> this is dead code, I guess.  Will clean up in fuse as well.
> 

That sounds like a good plan.

> 
> [1] v2.6.18: 7142125937e1 ("[PATCH] fuse: add POSIX file locking support")
> [2] v2.6.24: f33321141b27 ("fuse: add support for mandatory locking")
>