Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
From:   "NeilBrown" <neilb@suse.de>
To:     "Chuck Lever III" <chuck.lever@oracle.com>
Cc:     "Jeff Layton" <jlayton@poochiereds.net>,
        "Linux NFS Mailing List" <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v4 5/7] NFSD: Use rhashtable for managing nfs4_file objects
In-reply-to: <1DE04392-E8C9-4D39-BF23-BD1A59DB4FE3@oracle.com>
References: <166612295223.1291.11761205673682408148.stgit@manet.1015granger.net>,
 <166612313084.1291.5764156173845222109.stgit@manet.1015granger.net>,
 <cdad5af44b1c62c8e2ceae27cb2b646f7b2ec0bd.camel@poochiereds.net>,
 <1DE04392-E8C9-4D39-BF23-BD1A59DB4FE3@oracle.com>
Date:   Mon, 24 Oct 2022 13:07:10 +1100
Message-id: <166657723034.12462.8422170607830380805@noble.neil.brown.name>
Precedence: bulk

On Thu, 20 Oct 2022, Chuck Lever III wrote:
> 
> > On Oct 19, 2022, at 7:39 AM, Jeff Layton <jlayton@poochiereds.net> 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