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@kernel.org>,
        "Linux NFS Mailing List" <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v3 3/4] nfsd: close race between unhashing and LRU addition
In-reply-to: <08778EE0-CBDC-467B-ACA6-9D8E6719E1BB@oracle.com>
References: <20221028185712.79863-1-jlayton@kernel.org>,
 <20221028185712.79863-4-jlayton@kernel.org>,
 <08778EE0-CBDC-467B-ACA6-9D8E6719E1BB@oracle.com>
Date:   Mon, 31 Oct 2022 08:45:09 +1100
Message-id: <166716630911.13915.14442550645061536898@noble.neil.brown.name>
Precedence: bulk

On Sat, 29 Oct 2022, Chuck Lever III wrote:
> 
> > On Oct 28, 2022, at 2:57 PM, Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > The list_lru_add and list_lru_del functions use list_empty checks to see
> > whether the object is already on the LRU. That's fine in most cases, but
> > we occasionally repurpose nf_lru after unhashing. It's possible for an
> > LRU removal to remove it from a different list altogether if we lose a
> > race.
> 
> I've never seen that happen. lru field re-use is actually used in other
> places in the kernel. Shouldn't we try to find and fix such races?
> 
> Wasn't the idea to reduce the complexity of nfsd_file_put ?
> 

I think the nfsd filecache is different from those "other places"
because of nfsd_file_close_inode() and related code.  If I understand
correctly, nfsd can remove a file from the filecache while it is still
in use.  In this case it needs to be unhashed and removed from the lru -
and then added to a dispose list - while it might still be active for
some IO request.

Prior to Commit 8d0d254b15cc ("nfsd: fix nfsd_file_unhash_and_dispose")
unhash_and_dispose() wouldn't add to the dispose list unless the
refcount was one.  I'm not sure how racy this test was, but it would
mean that it is unlikely for an nfsd_file to be added to the dispose list
if it was still in use.

After that commit it seems more likely that a nfsd_file will be added to
a dispose list while it is in use.
As we add/remove things to a dispose list without a lock, we need to be
careful that no other thread will add the nfsd_file to an lru at the
same time.  refcounts will no longer provide that protection.  Maybe we
should restore the refcount protection, or maybe we need a bit as Jeff
has added here.

NeilBrown