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@xxxxxxxxxxxxxxx>
> > >
> > > 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.
Thanks,
NeilBrown
>
> --
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trondmy@xxxxxxxxxx, trond.myklebust@xxxxxxxxxxxxxxx
>
