> On Oct 24, 2022, at 6:57 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote: > > 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. rhash does have per-bucket serialization using bit-spin-locks, as far as I can tell. -- Chuck Lever
