> On Oct 23, 2022, at 10:07 PM, NeilBrown <neilb@xxxxxxx> 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. > >> >> >> 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. I've been testing a version all weekend that removes the use of state_lock. I haven't found any issues with it. I'll post what I have in a moment. -- Chuck Lever
