On Thu, 20 Oct 2022, Chuck Lever III wrote: > > > On Oct 19, 2022, at 7:39 AM, Jeff Layton <jlayton@xxxxxxxxxxxxxxx> wrote: > > > >> - fp = find_or_add_file(open->op_file, current_fh); > >> + rcu_read_lock(); > >> + fp = insert_nfs4_file(open->op_file, current_fh); > >> + rcu_read_unlock(); > > > > It'd probably be better to push this rcu_read_lock down into > > insert_nfs4_file. You don't need to hold it over the actual insertion, > > since that requires the state_lock. > > I used this arrangement because: > > insert_nfs4_file() invokes only find_nfs4_file() and the > insert_file() helper. Both find_nfs4_file() and the > insert_file() helper invoke rhltable_lookup(), which > must be called with the RCU read lock held. > > And this is the reason why put_nfs4_file() no longer takes > the state_lock: it would take the state_lock first and > then the RCU read lock (which is implicitly taken in > rhltable_remove()), which results in a lock inversion > relative to insert_nfs4_file(), which takes the RCU read > lock first, then the state_lock. It doesn't make any sense to talk about lock inversion with rcu_read_lock(). It isn't really a lock in any traditional sense in that it can never block (which is what cause lock-inversion problems). I prefer to think for rcu_read_lock() as taking a reference on some global state. > > > I'm certainly not an expert, so I'm willing to listen to > alternative approaches. Can we rely on only the RCU read > lock for exclusion on hash insertion? Probably we can. I'll read through all the patches now and provide some review. NeilBrown
