| Message ID | 20240805105715.11660-1-guoqing.jiang@linux.dev (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | nfsd: remove unnecessary cache_put from idmap_lookup | expand |
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 > >
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
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; }
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(-)