On Jan 5, 2023, at 9:29 AM, Jeff Layton [off-list ref] wrote:

On Thu, 2023-01-05 at 14:18 +0000, Chuck Lever III wrote:

quoted

Hi Jeff-

quoted

On Jan 5, 2023, at 7:15 AM, Jeff Layton [off-list ref] wrote:

Since v4 files are expected to be long-lived, there's little value in
closing them out of the cache when there is conflicting access.

Seems like targeting long-lived nfsd_file items could actually
be a hazardous behavior. Are you sure it's safe to leave it in
stable kernels?

Basically it just means we end up doing more opens than are needed in
this situation.

The notifiers just unnecessarily unhash a nfsd_file in this case, even
though it doesn't have any chance of freeing it until the client issues
a CLOSE, due to the persistent references held by the stateids.

So, this really is just an optimization and not a "real bug".

Good to know. Let's add this to the patch description when you
redrive...?

quoted

quoted

Rename NFSD_FILE_KEY_INODE to NFSD_FILE_KEY_INODE_GC,

I'd prefer to keep the name as it is. It's for searching for
inodes, and adding the ".gc = true" to the search criteria is
enough to show what you are looking for.

Ok.

quoted

quoted

and change the
comparator to also match the gc value in the key. Change both of the
current users of that key to set the gc value in the key to "true".

Also, test_bit returns bool, AFAICT, so I think we're ok to drop the
!! from the condition later in the comparator.

And I'd prefer that you leave this clean up for another patch.

Ok, I'll drop that hunk.

quoted

quoted

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

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index 9fff1fa09d08..a67b22579c6e 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c

@@ -78,7 +78,7 @@ static struct rhashtable		nfsd_file_rhash_tbl

						____cacheline_aligned_in_smp;

enum nfsd_file_lookup_type {
-	NFSD_FILE_KEY_INODE,
+	NFSD_FILE_KEY_INODE_GC,
	NFSD_FILE_KEY_FULL,
};

@@ -174,7 +174,9 @@ static int nfsd_file_obj_cmpfn(struct rhashtable_compare_arg *arg,

	const struct nfsd_file *nf = ptr;

	switch (key->type) {
-	case NFSD_FILE_KEY_INODE:
+	case NFSD_FILE_KEY_INODE_GC:
+		if (test_bit(NFSD_FILE_GC, &nf->nf_flags) != key->gc)
+			return 1;
		if (nf->nf_inode != key->inode)
			return 1;
		break;

@@ -187,7 +189,7 @@ 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)
+		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;

@@ -681,8 +683,9 @@ static void

nfsd_file_queue_for_close(struct inode *inode, struct list_head *dispose)
{
	struct nfsd_file_lookup_key key = {
-		.type	= NFSD_FILE_KEY_INODE,
+		.type	= NFSD_FILE_KEY_INODE_GC,
		.inode	= inode,
+		.gc	= true,
	};
	struct nfsd_file *nf;

@@ -1057,8 +1060,9 @@ bool

nfsd_file_is_cached(struct inode *inode)
{
	struct nfsd_file_lookup_key key = {
-		.type	= NFSD_FILE_KEY_INODE,
+		.type	= NFSD_FILE_KEY_INODE_GC,
		.inode	= inode,
+		.gc	= true,
	};
	bool ret = false;

-- 
2.39.0

--
Chuck Lever

-- 
Jeff Layton [off-list ref]

--
Chuck Lever