Re: [PATCH 3/3] NFS/localio: nfs_uuid_put() fix the wake up after unlinking the file

[Trond Myklebust <trondmy@kernel.org>]

On Wed, 2025-07-16 at 11:31 +1000, NeilBrown wrote:
> On Wed, 16 Jul 2025, Trond Myklebust wrote:
> > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > 
> > After setting nfl->nfs_uuid to NULL, dereferences of nfl should be
> > avoided, since there are no further guarantees that the memory is
> > still
> > allocated.
> 
> nfl is not being dereferenced here.  The difference between using
> "nfl"
> and "&nfl->nfs_uuid" as the event variable is simply some address
> arithmetic.  As long as both are the same it doesn't matter which is
> used.
> 
> 
> > Also change the wake_up_var_locked() to be a regular wake_up_var(),
> > since nfs_close_local_fh() cannot retake the nfs_uuid->lock after
> > dropping it.
> 
> The point of wake_up_var_locked() is to document why wake_up_var() is
> safe.  In general you need a barrier between an assignment and a
> wake_up_var().  I would like to eventually remove all wake_up_var()
> calls, replacing them with other calls which document why the wakeup
> is
> safe (e.g.  store_release_wake_up(), atomic_dec_and_wake_up()).  In
> this
> case it is safe because both the waker and the waiter hold the same
> spinlock.  I would like that documentation to remain.


The documentation is wrong. The waiter and waker do not both hold the
spin lock.

nfs_close_local_fh() calls wait_var_event() after it has dropped the
nfs_uuid->lock, and it has no guarantee that nfs_uuid still exists
after that is the case.
In order to guarantee that, it would have to go through the whole
rcu_dereference(nfl->nfs_uuid) rhumba from beginning of the call again.

The point of the rcu_assign_pointer() is therefore to add the barrier
that is missing before the call to wake_up_var().

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trondmy@kernel.org, trond.myklebust@hammerspace.com