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. 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@xxxxxxxxxx, trond.myklebust@xxxxxxxxxxxxxxx
