[PATCH 0/4] nfsd: filecache cleanups and optimizations

[PATCH 0/4] nfsd: filecache cleanups and optimizations

[Jeff Layton]

This is just a small set of filecache fixes and cleanups that I put
together while going over this code. None of these fix critical
problems, so these are probably v6.3 material.

Jeff Layton (4):
  nfsd: don't open-code clear_and_wake_up_bit
  nfsd: NFSD_FILE_KEY_INODE only needs to find GC'ed entries
  nfsd: don't kill nfsd_files because of lease break error
  nfsd: add some comments to nfsd_file_do_acquire

 fs/nfsd/filecache.c | 52 ++++++++++++++++++++++++++-------------------
 1 file changed, 30 insertions(+), 22 deletions(-)

-- 
2.39.0

[PATCH 1/4] nfsd: don't open-code clear_and_wake_up_bit

[Jeff Layton]

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

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index 6674a86e1917..9fff1fa09d08 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c
@@ -1184,9 +1184,7 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		status = nfserr_jukebox;
 	if (status != nfs_ok)
 		nfsd_file_unhash(nf);
-	clear_bit_unlock(NFSD_FILE_PENDING, &nf->nf_flags);
-	smp_mb__after_atomic();
-	wake_up_bit(&nf->nf_flags, NFSD_FILE_PENDING);
+	clear_and_wake_up_bit(NFSD_FILE_PENDING, &nf->nf_flags);
 	goto out;
 }
 
-- 
2.39.0

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

[Jeff Layton]

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.

Rename NFSD_FILE_KEY_INODE to NFSD_FILE_KEY_INODE_GC, 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.

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

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

[Chuck Lever III]

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?


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


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


> 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

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

[Jeff Layton]

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>

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

[Chuck Lever III]

> On Jan 5, 2023, at 9:29 AM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> 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".

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


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

--
Chuck Lever

[PATCH 3/4] nfsd: don't kill nfsd_files because of lease break error

[Jeff Layton]

An error from break_lease is non-fatal, so we needn't destroy the
nfsd_file in that case. Just put the reference like we normally would
and return the error.

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

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index a67b22579c6e..f0ca9501edb2 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c
@@ -1113,7 +1113,7 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	nf = nfsd_file_alloc(&key, may_flags);
 	if (!nf) {
 		status = nfserr_jukebox;
-		goto out_status;
+		goto out;
 	}
 
 	ret = rhashtable_lookup_insert_key(&nfsd_file_rhash_tbl,
@@ -1122,13 +1122,11 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	if (likely(ret == 0))
 		goto open_file;
 
-	nfsd_file_slab_free(&nf->nf_rcu);
-	nf = NULL;
 	if (ret == -EEXIST)
 		goto retry;
 	trace_nfsd_file_insert_err(rqstp, key.inode, may_flags, ret);
 	status = nfserr_jukebox;
-	goto out_status;
+	goto construction_err;
 
 wait_for_construction:
 	wait_on_bit(&nf->nf_flags, NFSD_FILE_PENDING, TASK_UNINTERRUPTIBLE);
@@ -1138,28 +1136,24 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		trace_nfsd_file_cons_err(rqstp, key.inode, may_flags, nf);
 		if (!open_retry) {
 			status = nfserr_jukebox;
-			goto out;
+			goto construction_err;
 		}
 		open_retry = false;
-		if (refcount_dec_and_test(&nf->nf_ref))
-			nfsd_file_free(nf);
 		goto retry;
 	}
-
 	this_cpu_inc(nfsd_file_cache_hits);
 
 	status = nfserrno(nfsd_open_break_lease(file_inode(nf->nf_file), may_flags));
+	if (status != nfs_ok) {
+		nfsd_file_put(nf);
+		nf = NULL;
+	}
+
 out:
 	if (status == nfs_ok) {
 		this_cpu_inc(nfsd_file_acquisitions);
 		*pnf = nf;
-	} else {
-		if (refcount_dec_and_test(&nf->nf_ref))
-			nfsd_file_free(nf);
-		nf = NULL;
 	}
-
-out_status:
 	put_cred(key.cred);
 	trace_nfsd_file_acquire(rqstp, key.inode, may_flags, nf, status);
 	return status;
@@ -1189,6 +1183,13 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	if (status != nfs_ok)
 		nfsd_file_unhash(nf);
 	clear_and_wake_up_bit(NFSD_FILE_PENDING, &nf->nf_flags);
+	if (status == nfs_ok)
+		goto out;
+
+construction_err:
+	if (refcount_dec_and_test(&nf->nf_ref))
+		nfsd_file_free(nf);
+	nf = NULL;
 	goto out;
 }
 
-- 
2.39.0

Re: [PATCH 3/4] nfsd: don't kill nfsd_files because of lease break error

[Jeff Layton]

On Thu, 2023-01-05 at 07:15 -0500, Jeff Layton wrote:
> An error from break_lease is non-fatal, so we needn't destroy the
> nfsd_file in that case. Just put the reference like we normally would
> and return the error.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/nfsd/filecache.c | 29 +++++++++++++++--------------
>  1 file changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index a67b22579c6e..f0ca9501edb2 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -1113,7 +1113,7 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  	nf = nfsd_file_alloc(&key, may_flags);
>  	if (!nf) {
>  		status = nfserr_jukebox;
> -		goto out_status;
> +		goto out;
>  	}
>  
>  	ret = rhashtable_lookup_insert_key(&nfsd_file_rhash_tbl,
> @@ -1122,13 +1122,11 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  	if (likely(ret == 0))
>  		goto open_file;
>  
> -	nfsd_file_slab_free(&nf->nf_rcu);
> -	nf = NULL;
>  	if (ret == -EEXIST)
>  		goto retry;
>  	trace_nfsd_file_insert_err(rqstp, key.inode, may_flags, ret);
>  	status = nfserr_jukebox;
> -	goto out_status;
> +	goto construction_err;
>  
>  wait_for_construction:
>  	wait_on_bit(&nf->nf_flags, NFSD_FILE_PENDING, TASK_UNINTERRUPTIBLE);
> @@ -1138,28 +1136,24 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  		trace_nfsd_file_cons_err(rqstp, key.inode, may_flags, nf);
>  		if (!open_retry) {
>  			status = nfserr_jukebox;
> -			goto out;
> +			goto construction_err;
>  		}
>  		open_retry = false;
> -		if (refcount_dec_and_test(&nf->nf_ref))
> -			nfsd_file_free(nf);
>  		goto retry;
>  	}
> -
>  	this_cpu_inc(nfsd_file_cache_hits);
>  
>  	status = nfserrno(nfsd_open_break_lease(file_inode(nf->nf_file), may_flags));
> +	if (status != nfs_ok) {
> +		nfsd_file_put(nf);
> +		nf = NULL;
> +	}
> +
>  out:
>  	if (status == nfs_ok) {
>  		this_cpu_inc(nfsd_file_acquisitions);
>  		*pnf = nf;
> -	} else {
> -		if (refcount_dec_and_test(&nf->nf_ref))
> -			nfsd_file_free(nf);
> -		nf = NULL;
>  	}
> -
> -out_status:
>  	put_cred(key.cred);
>  	trace_nfsd_file_acquire(rqstp, key.inode, may_flags, nf, status);
>  	return status;
> @@ -1189,6 +1183,13 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  	if (status != nfs_ok)
>  		nfsd_file_unhash(nf);
>  	clear_and_wake_up_bit(NFSD_FILE_PENDING, &nf->nf_flags);
> +	if (status == nfs_ok)
> +		goto out;
> +
> +construction_err:
> +	if (refcount_dec_and_test(&nf->nf_ref))
> +		nfsd_file_free(nf);
> +	nf = NULL;
>  	goto out;
>  }
>  

Chuck, ping?

You never responded to this patch and I don't see it in your tree. Any
thoughts?
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 3/4] nfsd: don't kill nfsd_files because of lease break error

[Chuck Lever III]

> On Jan 18, 2023, at 12:54 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Thu, 2023-01-05 at 07:15 -0500, Jeff Layton wrote:
>> An error from break_lease is non-fatal, so we needn't destroy the
>> nfsd_file in that case. Just put the reference like we normally would
>> and return the error.
>> 
>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>> ---
>> fs/nfsd/filecache.c | 29 +++++++++++++++--------------
>> 1 file changed, 15 insertions(+), 14 deletions(-)
>> 
>> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
>> index a67b22579c6e..f0ca9501edb2 100644
>> --- a/fs/nfsd/filecache.c
>> +++ b/fs/nfsd/filecache.c
>> @@ -1113,7 +1113,7 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
>> 	nf = nfsd_file_alloc(&key, may_flags);
>> 	if (!nf) {
>> 		status = nfserr_jukebox;
>> -		goto out_status;
>> +		goto out;
>> 	}
>> 
>> 	ret = rhashtable_lookup_insert_key(&nfsd_file_rhash_tbl,
>> @@ -1122,13 +1122,11 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
>> 	if (likely(ret == 0))
>> 		goto open_file;
>> 
>> -	nfsd_file_slab_free(&nf->nf_rcu);
>> -	nf = NULL;
>> 	if (ret == -EEXIST)
>> 		goto retry;
>> 	trace_nfsd_file_insert_err(rqstp, key.inode, may_flags, ret);
>> 	status = nfserr_jukebox;
>> -	goto out_status;
>> +	goto construction_err;
>> 
>> wait_for_construction:
>> 	wait_on_bit(&nf->nf_flags, NFSD_FILE_PENDING, TASK_UNINTERRUPTIBLE);
>> @@ -1138,28 +1136,24 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
>> 		trace_nfsd_file_cons_err(rqstp, key.inode, may_flags, nf);
>> 		if (!open_retry) {
>> 			status = nfserr_jukebox;
>> -			goto out;
>> +			goto construction_err;
>> 		}
>> 		open_retry = false;
>> -		if (refcount_dec_and_test(&nf->nf_ref))
>> -			nfsd_file_free(nf);
>> 		goto retry;
>> 	}
>> -
>> 	this_cpu_inc(nfsd_file_cache_hits);
>> 
>> 	status = nfserrno(nfsd_open_break_lease(file_inode(nf->nf_file), may_flags));
>> +	if (status != nfs_ok) {
>> +		nfsd_file_put(nf);
>> +		nf = NULL;
>> +	}
>> +
>> out:
>> 	if (status == nfs_ok) {
>> 		this_cpu_inc(nfsd_file_acquisitions);
>> 		*pnf = nf;
>> -	} else {
>> -		if (refcount_dec_and_test(&nf->nf_ref))
>> -			nfsd_file_free(nf);
>> -		nf = NULL;
>> 	}
>> -
>> -out_status:
>> 	put_cred(key.cred);
>> 	trace_nfsd_file_acquire(rqstp, key.inode, may_flags, nf, status);
>> 	return status;
>> @@ -1189,6 +1183,13 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
>> 	if (status != nfs_ok)
>> 		nfsd_file_unhash(nf);
>> 	clear_and_wake_up_bit(NFSD_FILE_PENDING, &nf->nf_flags);
>> +	if (status == nfs_ok)
>> +		goto out;
>> +
>> +construction_err:
>> +	if (refcount_dec_and_test(&nf->nf_ref))
>> +		nfsd_file_free(nf);
>> +	nf = NULL;
>> 	goto out;
>> }
>> 
> 
> Chuck, ping?
> 
> You never responded to this patch and I don't see it in your tree. Any
> thoughts?

It's been in nfsd-next for a bit, but I had to drop a bunch of patches
from nfsd-next until yesterday's -rc PR was applied upstream because
they depended on one of Dai's crasher fixes.

That's been sorted, and I've restored these to nfsd-next.

--
Chuck Lever

[PATCH 4/4] nfsd: add some comments to nfsd_file_do_acquire

[Jeff Layton]

David Howells mentioned that he found this bit of code confusing, so
sprinkle in some comments to clarify.

Reported-by: David Howells <dhowells@redhat.com>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/filecache.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index f0ca9501edb2..5724baad09ec 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c
@@ -1105,6 +1105,11 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	rcu_read_unlock();
 
 	if (nf) {
+		/*
+		 * If the nf is on the LRU then it holds an extra reference
+		 * that must be put if it's removed. It had better not be
+		 * the last one however, since we should hold another.
+		 */
 		if (nfsd_file_lru_remove(nf))
 			WARN_ON_ONCE(refcount_dec_and_test(&nf->nf_ref));
 		goto wait_for_construction;
-- 
2.39.0

Re: [PATCH 0/4] nfsd: filecache cleanups and optimizations

[Chuck Lever III]

> On Jan 5, 2023, at 7:15 AM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> This is just a small set of filecache fixes and cleanups that I put
> together while going over this code. None of these fix critical
> problems, so these are probably v6.3 material.
> 
> Jeff Layton (4):
>  nfsd: don't open-code clear_and_wake_up_bit
>  nfsd: NFSD_FILE_KEY_INODE only needs to find GC'ed entries
>  nfsd: don't kill nfsd_files because of lease break error
>  nfsd: add some comments to nfsd_file_do_acquire
> 
> fs/nfsd/filecache.c | 52 ++++++++++++++++++++++++++-------------------
> 1 file changed, 30 insertions(+), 22 deletions(-)
> 
> -- 
> 2.39.0
> 

These have been applied to nfsd's for-next. Thanks!


--
Chuck Lever