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 15:07 +1000, NeilBrown wrote:
> On Wed, 16 Jul 2025, Trond Myklebust wrote:
> > 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.
> 
> True.  In that case it would make sense to use
> store_release_wake_up()
> in nfs_uuid_put().  Though that doesn't have the right rcu
> annotations....
> I think 
>     store_release_wake_up(&nfl->nfs_uuid, RCU_INITIALIZER(NULL));
> would be correct.
> 
> > 
> > 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().
> 
> rcu_assign_pointer()s add a barrier before the assignment. 
> wake_up_var()
> requires a barrier after the assignment.
> In fact, when the val is NULL, rcu_assign_pointer() doesn't even
> include
> that barrier - it acts exactly like RCU_INIT_POINTER() - interesting.
> 

Fair enough. I have a v2 of this patchset that Mike is testing out, and
that should fix this issue as well as the ones raised by the second
patch. I'll post once he is satisfied.

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