Re: [PATCH 2/4] nfsd: NFSD_FILE_KEY_INODE only needs to find GC'ed entries

[Jeff Layton <jlayton@kernel.org>]

On Thu, 2023-01-05 at 14:18 +0000, Chuck Lever III wrote:
> Hi Jeff-
> 
> 
> > On Jan 5, 2023, at 7:15 AM, Jeff Layton <jlayton@kernel.org> 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".

> 
> > 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.

> 
> > 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.

> 
> > 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 <jlayton@kernel.org>