> On Jul 8, 2022, at 3:29 PM, Jeff Layton <jlayton@xxxxxxxxxx> 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@xxxxxxxxxx> >> Reported-by: Wang Yugui <wangyugui@xxxxxxxxxxxx> >> Link: https://bugzilla.linux-nfs.org/show_bug.cgi?id=386 >> Suggested-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx> >> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx> >> --- >> 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. >> 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@xxxxxxxxxx> -- Chuck Lever
