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