On Oct 23, 2022, at 10:33 PM, NeilBrown [off-list ref] wrote:

On Wed, 19 Oct 2022, Chuck Lever wrote:

quoted

NFSv4 operations manage the lifetime of nfsd_file items they use by
means of NFSv4 OPEN and CLOSE. Hence there's no need for them to be
garbage collected.

Introduce a mechanism to enable garbage collection for nfsd_file
items used only by NFSv2/3 callers.

Note that the change in nfsd_file_put() ensures that both CLOSE and
DELEGRETURN will actually close out and free an nfsd_file on last
reference of a non-garbage-collected file.

Link: https://bugzilla.linux-nfs.org/show_bug.cgi?id=394
Suggested-by: Trond Myklebust <redacted>
Tested-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
fs/nfsd/filecache.c |   61 +++++++++++++++++++++++++++++++++++++++++++++------
fs/nfsd/filecache.h |    3 +++
fs/nfsd/nfs3proc.c  |    4 ++-
fs/nfsd/trace.h     |    3 ++-
fs/nfsd/vfs.c       |    4 ++-
5 files changed, 63 insertions(+), 12 deletions(-)

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index b7aa523c2010..87fce5c95726 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c

@@ -63,6 +63,7 @@ struct nfsd_file_lookup_key {

	struct net			*net;
	const struct cred		*cred;
	unsigned char			need;
+	unsigned char			gc:1;
	enum nfsd_file_lookup_type	type;
};

@@ -162,6 +163,8 @@ static int nfsd_file_obj_cmpfn(struct rhashtable_compare_arg *arg,

			return 1;
		if (!nfsd_match_cred(nf->nf_cred, key->cred))
			return 1;
+		if (test_bit(NFSD_FILE_GC, &nf->nf_flags) != key->gc)
+			return 1;
		if (test_bit(NFSD_FILE_HASHED, &nf->nf_flags) == 0)
			return 1;
		break;

@@ -297,6 +300,8 @@ nfsd_file_alloc(struct nfsd_file_lookup_key *key, unsigned int may)

		nf->nf_flags = 0;
		__set_bit(NFSD_FILE_HASHED, &nf->nf_flags);
		__set_bit(NFSD_FILE_PENDING, &nf->nf_flags);
+		if (key->gc)
+			__set_bit(NFSD_FILE_GC, &nf->nf_flags);
		nf->nf_inode = key->inode;
		/* nf_ref is pre-incremented for hash table */
		refcount_set(&nf->nf_ref, 2);

@@ -428,16 +433,27 @@ nfsd_file_put_noref(struct nfsd_file *nf)

	}
}

+static void
+nfsd_file_unhash_and_put(struct nfsd_file *nf)
+{
+	if (nfsd_file_unhash(nf))
+		nfsd_file_put_noref(nf);
+}
+
void
nfsd_file_put(struct nfsd_file *nf)
{
	might_sleep();

-	nfsd_file_lru_add(nf);
+	if (test_bit(NFSD_FILE_GC, &nf->nf_flags) == 1)

Clearly this is a style choice on which sensible people might disagree,
but I much prefer to leave out the "== 1" That is what most callers in
fs/nfsd/ do - only exceptions are here in filecache.c.
Even "!= 0" would be better than "== 1".
I think test_bit() is declared as a bool, but it is hard to be certain.

The definitions of test_bit() I've seen return "int" which is why
I wrote it this way.

quoted

@@ -1017,12 +1033,14 @@ nfsd_file_is_cached(struct inode *inode)

static __be32
nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
-		     unsigned int may_flags, struct nfsd_file **pnf, bool open)
+		     unsigned int may_flags, struct nfsd_file **pnf,
+		     bool open, int want_gc)

I too would prefer "bool" for all intstance of gc and want_gc.

OK. I think I've figured out a way to do that and still deal
safely with the test_bit() return type silliness.

v5 coming soon.


--
Chuck Lever