On Mon, 2022-10-24 at 13:07 +1100, NeilBrown wrote: > 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. > Right. To be clear, you can deadlock with synchronize_rcu if you use it improperly, but the rcu_read_lock itself never blocks. > > > > > > 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. > The rcu_read_lock provides _no_ exclusion whatsoever, so it's not usually suitable for things that need exclusive access (like a hash insertion). If each rhl hash chain has its own lock though, then we may not require other locking to serialize insertions. -- Jeff Layton <jlayton@xxxxxxxxxx>
