On Oct 27, 2022, at 6:55 PM, NeilBrown [off-list ref] wrote:

On Fri, 28 Oct 2022, Jeff Layton wrote:

quoted

On Fri, 2022-10-28 at 09:20 +1100, NeilBrown wrote:

quoted

On Fri, 28 Oct 2022, Jeff Layton wrote:

quoted

Currently, nfsd_files live on the LRU once they are added until they are
unhashed. There's no need to keep ones that are actively in use there.

Is that true?
nfsd_file_do_acquire() calls nfsd_file_lru_remove()
Isn't that enough to keep the file off the lru while it is active?

Thanks,
NeilBrown

After patch #1, it doesn't call that anymore. That's probably a (minor)
regression then.

Yes, I eventually found that - thanks.

quoted

After patch #1, the LRU holds a reference. If you successfully remove it
from the LRU, you need to transfer or put that reference. Doing the LRU
handling in the get and put routines seems more natural, I think.

Maybe.  But then you need a __get as well as a get.
Though it might seem asymmetric, I would prefer removing from the lru in
'acquire' and adding to the lru in put.

That's exactly the design introduced by commit 4a0e73e635e3
("NFSD: Leave open files out of the filecache LRU"). I also
would like to keep that behavior -- that's what a real LRU
is for.

quoted

Maybe I just need to squash this patch into #1?

Or do the "put" if lru_remove succeeds in the first patch.  Then revise
it all in the second.

Thanks,
NeilBrown

quoted

quoted

quoted

Before incrementing the refcount, do a lockless check for nf_lru being
empty. If it's not then attempt to remove the entry from the LRU. If
that's successful, claim the LRU reference and return it. If the removal
fails (or if the list_head was empty), then just increment the counter
as we normally would.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
fs/nfsd/filecache.c | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index e63534f4b9f8..d2bbded805d4 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c

@@ -420,14 +420,31 @@ nfsd_file_unhash(struct nfsd_file *nf)

	return false;
}

-struct nfsd_file *
-nfsd_file_get(struct nfsd_file *nf)
+static struct nfsd_file *
+__nfsd_file_get(struct nfsd_file *nf)
{
	if (likely(refcount_inc_not_zero(&nf->nf_ref)))
		return nf;
	return NULL;
}

+struct nfsd_file *
+nfsd_file_get(struct nfsd_file *nf)
+{
+	/*
+	 * Do a lockless list_empty check first, before attempting to
+	 * remove it, so we can avoid the spinlock when it's not on the
+	 * list.
+	 *
+	 * If we successfully remove it from the LRU, then we can just
+	 * claim the LRU reference and return it. Otherwise, we need to
+	 * bump the counter the old-fashioned way.
+	 */
+	if (!list_empty(&nf->nf_lru) && nfsd_file_lru_remove(nf))
+		return nf;
+	return __nfsd_file_get(nf);
+}
+
/**
 * nfsd_file_unhash_and_queue - unhash a file and queue it to the dispose list
 * @nf: nfsd_file to be unhashed and queued

@@ -449,7 +466,7 @@ nfsd_file_unhash_and_queue(struct nfsd_file *nf, struct list_head *dispose)

		 * to take a reference. If that fails, just ignore
		 * the file altogether.
		 */
-		if (!nfsd_file_lru_remove(nf) && !nfsd_file_get(nf))
+		if (!nfsd_file_lru_remove(nf) && !__nfsd_file_get(nf))
			return false;
		list_add(&nf->nf_lru, dispose);
		return true;
-- 
2.37.3

-- 
Jeff Layton [off-list ref]

--
Chuck Lever