Re: [PATCH v3 15/32] NFSD: Leave open files out of the filecache LRU

From: Jeff Layton <jlayton@redhat.com>
To: Chuck Lever III <chuck.lever@oracle.com>
Cc: Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"david@fromorbit.com" <david@fromorbit.com>,
	"tgraf@suug.ch" <tgraf@suug.ch>
Subject: Re: [PATCH v3 15/32] NFSD: Leave open files out of the filecache LRU
Date: Mon, 11 Jul 2022 07:39:07 -0400	[thread overview]
Message-ID: <33688832c658a15d35af6321030f8ebd98cfea8a.camel@redhat.com> (raw)
In-Reply-To: <BC4E1A0C-1404-4C50-ADD0-3999DEE01066@oracle.com>

On Sat, 2022-07-09 at 20:45 +0000, Chuck Lever III wrote:
> 
> > On Jul 8, 2022, at 3:29 PM, Jeff Layton <jlayton@redhat.com> wrote:
> > 
> > On Fri, 2022-07-08 at 14:25 -0400, Chuck Lever wrote:
> > > There have been reports of problems when running fstests generic/531
> > > against Linux NFS servers with NFSv4. The NFS server that hosts the
> > > test's SCRATCH_DEV suffers from CPU soft lock-ups during the test.
> > > Analysis shows that:
> > > 
> > > fs/nfsd/filecache.c
> > > 482 ret = list_lru_walk(&nfsd_file_lru,
> > > 483 nfsd_file_lru_cb,
> > > 484 &head, LONG_MAX);
> > > 
> > > causes nfsd_file_gc() to walk the entire length of the filecache LRU
> > > list every time it is called (which is quite frequently). The walk
> > > holds a spinlock the entire time that prevents other nfsd threads
> > > from accessing the filecache.
> > > 
> > > What's more, for NFSv4 workloads, none of the items that are visited
> > > during this walk may be evicted, since they are all files that are
> > > held OPEN by NFS clients.
> > > 
> > > Address this by ensuring that open files are not kept on the LRU
> > > list.
> > > 
> > > Reported-by: Frank van der Linden <fllinden@amazon.com>
> > > Reported-by: Wang Yugui <wangyugui@e16-tech.com>
> > > Link: https://bugzilla.linux-nfs.org/show_bug.cgi?id=386
> > > Suggested-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > > ---
> > > fs/nfsd/filecache.c | 24 +++++++++++++++++++-----
> > > fs/nfsd/trace.h | 2 ++
> > > 2 files changed, 21 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> > > index 37373b012276..6e9e186334ab 100644
> > > --- a/fs/nfsd/filecache.c
> > > +++ b/fs/nfsd/filecache.c
> > > @@ -269,6 +269,7 @@ nfsd_file_flush(struct nfsd_file *nf)
> > > 
> > > static void nfsd_file_lru_add(struct nfsd_file *nf)
> > > {
> > > +	set_bit(NFSD_FILE_REFERENCED, &nf->nf_flags);
> > > 	if (list_lru_add(&nfsd_file_lru, &nf->nf_lru))
> > > 		trace_nfsd_file_lru_add(nf);
> > > }
> > > @@ -298,7 +299,6 @@ nfsd_file_unhash(struct nfsd_file *nf)
> > > {
> > > 	if (test_and_clear_bit(NFSD_FILE_HASHED, &nf->nf_flags)) {
> > > 		nfsd_file_do_unhash(nf);
> > > -		nfsd_file_lru_remove(nf);
> > > 		return true;
> > > 	}
> > > 	return false;
> > > @@ -319,6 +319,7 @@ nfsd_file_unhash_and_release_locked(struct nfsd_file *nf, struct list_head *disp
> > > 	if (refcount_dec_not_one(&nf->nf_ref))
> > > 		return true;
> > > 
> > > +	nfsd_file_lru_remove(nf);
> > > 	list_add(&nf->nf_lru, dispose);
> > > 	return true;
> > > }
> > > @@ -330,6 +331,7 @@ nfsd_file_put_noref(struct nfsd_file *nf)
> > > 
> > > 	if (refcount_dec_and_test(&nf->nf_ref)) {
> > > 		WARN_ON(test_bit(NFSD_FILE_HASHED, &nf->nf_flags));
> > > +		nfsd_file_lru_remove(nf);
> > > 		nfsd_file_free(nf);
> > > 	}
> > > }
> > > @@ -339,7 +341,7 @@ nfsd_file_put(struct nfsd_file *nf)
> > > {
> > > 	might_sleep();
> > > 
> > > -	set_bit(NFSD_FILE_REFERENCED, &nf->nf_flags);
> > > +	nfsd_file_lru_add(nf);
> > 
> > Do you really want to add this on every put? I would have thought you'd
> > only want to do this on a 2->1 nf_ref transition.
> 
> My measurements indicate that 2->1 is the common case, so checking
> that this is /not/ a 2->1 transition doesn't confer much if any
> benefit.
> 
> Under load, I don't see any contention on the LRU locks, which is
> where I'd expect to see a problem if this design were not efficient.
> 
> 

Fair enough. I guess the idea is to throw it onto the LRU and the
scanner will just (eventually) take it off again without reaping it.

You can add my Reviewed-by: to this one as well.

> > > 	if (test_bit(NFSD_FILE_HASHED, &nf->nf_flags) == 0) {
> > > 		nfsd_file_flush(nf);
> > > 		nfsd_file_put_noref(nf);
> > > @@ -439,8 +441,18 @@ nfsd_file_dispose_list_delayed(struct list_head *dispose)
> > > 	}
> > > }
> > > 
> > > -/*
> > > +/**
> > > + * nfsd_file_lru_cb - Examine an entry on the LRU list
> > > + * @item: LRU entry to examine
> > > + * @lru: controlling LRU
> > > + * @lock: LRU list lock (unused)
> > > + * @arg: dispose list
> > > + *
> > > * Note this can deadlock with nfsd_file_cache_purge.
> > > + *
> > > + * Return values:
> > > + * %LRU_REMOVED: @item was removed from the LRU
> > > + * %LRU_SKIP: @item cannot be evicted
> > > */
> > > static enum lru_status
> > > nfsd_file_lru_cb(struct list_head *item, struct list_lru_one *lru,
> > > @@ -462,8 +474,9 @@ nfsd_file_lru_cb(struct list_head *item, struct list_lru_one *lru,
> > > 	 * That order is deliberate to ensure that we can do this locklessly.
> > > 	 */
> > > 	if (refcount_read(&nf->nf_ref) > 1) {
> > > +		list_lru_isolate(lru, &nf->nf_lru);
> > > 		trace_nfsd_file_gc_in_use(nf);
> > > -		return LRU_SKIP;
> > > +		return LRU_REMOVED;
> > 
> > Interesting. So you wait until the LRU scanner runs to remove these
> > entries? I expected to see you do this in nfsd_file_get, but this does
> > seem likely to be more efficient.
> > 
> > > 	}
> > > 
> > > 	/*
> > > @@ -1020,6 +1033,7 @@ nfsd_do_file_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > > 		goto retry;
> > > 	}
> > > 
> > > +	nfsd_file_lru_remove(nf);
> > > 	this_cpu_inc(nfsd_file_cache_hits);
> > > 
> > > 	if (!(may_flags & NFSD_MAY_NOT_BREAK_LEASE)) {
> > > @@ -1055,7 +1069,6 @@ nfsd_do_file_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > > 	refcount_inc(&nf->nf_ref);
> > > 	__set_bit(NFSD_FILE_HASHED, &nf->nf_flags);
> > > 	__set_bit(NFSD_FILE_PENDING, &nf->nf_flags);
> > > -	nfsd_file_lru_add(nf);
> > > 	hlist_add_head_rcu(&nf->nf_node, &nfsd_file_hashtbl[hashval].nfb_head);
> > > 	++nfsd_file_hashtbl[hashval].nfb_count;
> > > 	nfsd_file_hashtbl[hashval].nfb_maxcount = max(nfsd_file_hashtbl[hashval].nfb_maxcount,
> > > @@ -1080,6 +1093,7 @@ nfsd_do_file_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > > 	 */
> > > 	if (status != nfs_ok || inode->i_nlink == 0) {
> > > 		bool do_free;
> > > +		nfsd_file_lru_remove(nf);
> > > 		spin_lock(&nfsd_file_hashtbl[hashval].nfb_lock);
> > > 		do_free = nfsd_file_unhash(nf);
> > > 		spin_unlock(&nfsd_file_hashtbl[hashval].nfb_lock);
> > > diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
> > > index 1cc1133371eb..54082b868b72 100644
> > > --- a/fs/nfsd/trace.h
> > > +++ b/fs/nfsd/trace.h
> > > @@ -929,7 +929,9 @@ DEFINE_EVENT(nfsd_file_gc_class, name,					\
> > > 	TP_ARGS(nf))
> > > 
> > > DEFINE_NFSD_FILE_GC_EVENT(nfsd_file_lru_add);
> > > +DEFINE_NFSD_FILE_GC_EVENT(nfsd_file_lru_add_disposed);
> > > DEFINE_NFSD_FILE_GC_EVENT(nfsd_file_lru_del);
> > > +DEFINE_NFSD_FILE_GC_EVENT(nfsd_file_lru_del_disposed);
> > > DEFINE_NFSD_FILE_GC_EVENT(nfsd_file_gc_in_use);
> > > DEFINE_NFSD_FILE_GC_EVENT(nfsd_file_gc_writeback);
> > > DEFINE_NFSD_FILE_GC_EVENT(nfsd_file_gc_referenced);
> > > 
> > > 
> > 
> > -- 
> > Jeff Layton <jlayton@redhat.com>
> 
> --
> Chuck Lever
> 
> 
> 

-- 
Jeff Layton <jlayton@redhat.com>