[PATCH] nfsd: remove unnecessary cache_put from idmap_lookup

[PATCH] nfsd: remove unnecessary cache_put from idmap_lookup

[Guoqing Jiang]

It is not needed given cache_check already calls cache_put on failure,
otherwise we call cache_put twice in case of -ETIMEDOUT.   

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 fs/nfsd/nfs4idmap.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/nfsd/nfs4idmap.c b/fs/nfsd/nfs4idmap.c
index 7a806ac13e31..08a354783d57 100644
--- a/fs/nfsd/nfs4idmap.c
+++ b/fs/nfsd/nfs4idmap.c
@@ -521,7 +521,6 @@ idmap_lookup(struct svc_rqst *rqstp,
 		*item = lookup_fn(detail, key);
 		if (*item != prev_item)
 			goto retry;
-		cache_put(&(*item)->h, detail);
 	}
 	return ret;
 }
-- 
2.35.3

Re: [PATCH] nfsd: remove unnecessary cache_put from idmap_lookup

[NeilBrown]

On Mon, 05 Aug 2024, Guoqing Jiang wrote:
> It is not needed given cache_check already calls cache_put on failure,
> otherwise we call cache_put twice in case of -ETIMEDOUT.   

cache_check() was called on a different *item.

This cache_put() puts the *item returned by lookup_fn() two lines
earlier.

The current code is correct.

NeilBrown

> 
> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
> ---
>  fs/nfsd/nfs4idmap.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/fs/nfsd/nfs4idmap.c b/fs/nfsd/nfs4idmap.c
> index 7a806ac13e31..08a354783d57 100644
> --- a/fs/nfsd/nfs4idmap.c
> +++ b/fs/nfsd/nfs4idmap.c
> @@ -521,7 +521,6 @@ idmap_lookup(struct svc_rqst *rqstp,
>  		*item = lookup_fn(detail, key);
>  		if (*item != prev_item)
>  			goto retry;
> -		cache_put(&(*item)->h, detail);
>  	}
>  	return ret;
>  }
> -- 
> 2.35.3
> 
> 

Re: [PATCH] nfsd: remove unnecessary cache_put from idmap_lookup

[Guoqing Jiang]

Hi Neil,

On 8/7/24 07:58, NeilBrown wrote:
> On Mon, 05 Aug 2024, Guoqing Jiang wrote:
>> It is not needed given cache_check already calls cache_put on failure,
>> otherwise we call cache_put twice in case of -ETIMEDOUT.
> cache_check() was called on a different *item.
>
> This cache_put() puts the *item returned by lookup_fn() two lines
> earlier.
>
> The current code is correct.

Thanks for point it out! So this cache_put is paired with lookup_fn which
calls cache_get on a new item.

BTW, I checked idmap_id_to_name which didn't put cache if there is no
enough buffer, not sure if we need it here.

diff --git a/fs/nfsd/nfs4idmap.c b/fs/nfsd/nfs4idmap.c
index 7a806ac13e31..7abddf7d8f6d 100644
--- a/fs/nfsd/nfs4idmap.c
+++ b/fs/nfsd/nfs4idmap.c
@@ -594,8 +594,10 @@ static __be32 idmap_id_to_name(struct xdr_stream *xdr,
         ret = strlen(item->name);
         WARN_ON_ONCE(ret > IDMAP_NAMESZ);
         p = xdr_reserve_space(xdr, ret + 4);
-       if (!p)
+       if (!p) {
+               cache_put(&item->h, nn->idtoname_cache);
                 return nfserr_resource;
+       }
         p = xdr_encode_opaque(p, item->name, ret);
         cache_put(&item->h, nn->idtoname_cache);
         return 0;

Thanks,
Guoqing