On Tue, 2022-10-11 at 21:02 +0200, Miklos Szeredi wrote:
> On Tue, 11 Oct 2022 at 20:04, Al Viro <viro@xxxxxxxxxxxxxxxxxx> 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")
>
