On Mon, 23 Jun 2025, Su Hui wrote:
> Using guard() to replace *unlock* label. guard() makes lock/unlock code
> more clear. Change the order of the code to let all lock code in the
> same scope. No functional changes.
While I agree that this code could usefully be cleaned up and that you
have made some improvements, I think the use of guard() is a nearly
insignificant part of the change. You could easily do exactly the same
patch without using guard() but having and explicit spin_unlock() before
the new return. That doesn't mean you shouldn't use guard(), but it
does mean that the comment explaining the change could be more usefully
focused on the "Change the order ..." part, and maybe explain what that
is important.
I actually think there is room for other changes which would make the
code even better:
- Change nfsd_prune_bucket_locked() to nfsd_prune_bucket(). Have it
take the lock when needed, then drop it, then call
nfsd_cacherep_dispose() - and return the count.
- change nfsd_cache_insert to also skip updating the chain length stats
when it finds a match - in that case the "entries" isn't a chain
length. So just lru_put_end(), return. Have it return NULL if
no match was found
- after the found_entry label don't use nfsd_reply_cache_free_locked(),
just free rp. It has never been included in any rbtree or list, so it
doesn't need to be removed.
- I'd be tempted to have nfsd_cache_insert() take the spinlock itself
and call it under rcu_read_lock() - and use RCU to free the cached
items.
- put the chunk of code after the found_entry label into a separate
function and instead just return RC_REPLY (and maybe rename that
RC_CACHED). Then in nfsd_dispatch(), if RC_CACHED was returned, call
that function that has the found_entry code.
I think that would make the code a lot easier to follow. Would you like
to have a go at that - I suspect it would be several patches - or shall
I do it?
Thanks,
NeilBrown
>
> Signed-off-by: Su Hui <suhui@xxxxxxxxxxxx>
> ---
> fs/nfsd/nfscache.c | 99 ++++++++++++++++++++++------------------------
> 1 file changed, 48 insertions(+), 51 deletions(-)
>
> diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c
> index ba9d326b3de6..2d92adf3e6b0 100644
> --- a/fs/nfsd/nfscache.c
> +++ b/fs/nfsd/nfscache.c
> @@ -489,7 +489,7 @@ int nfsd_cache_lookup(struct svc_rqst *rqstp, unsigned int start,
>
> if (type == RC_NOCACHE) {
> nfsd_stats_rc_nocache_inc(nn);
> - goto out;
> + return rtn;
> }
>
> csum = nfsd_cache_csum(&rqstp->rq_arg, start, len);
> @@ -500,64 +500,61 @@ int nfsd_cache_lookup(struct svc_rqst *rqstp, unsigned int start,
> */
> rp = nfsd_cacherep_alloc(rqstp, csum, nn);
> if (!rp)
> - goto out;
> + return rtn;
>
> b = nfsd_cache_bucket_find(rqstp->rq_xid, nn);
> - spin_lock(&b->cache_lock);
> - found = nfsd_cache_insert(b, rp, nn);
> - if (found != rp)
> - goto found_entry;
> - *cacherep = rp;
> - rp->c_state = RC_INPROG;
> - nfsd_prune_bucket_locked(nn, b, 3, &dispose);
> - spin_unlock(&b->cache_lock);
> + scoped_guard(spinlock, &b->cache_lock) {
> + found = nfsd_cache_insert(b, rp, nn);
> + if (found == rp) {
> + *cacherep = rp;
> + rp->c_state = RC_INPROG;
> + nfsd_prune_bucket_locked(nn, b, 3, &dispose);
> + goto out;
> + }
> + /* We found a matching entry which is either in progress or done. */
> + nfsd_reply_cache_free_locked(NULL, rp, nn);
> + nfsd_stats_rc_hits_inc(nn);
> + rtn = RC_DROPIT;
> + rp = found;
> +
> + /* Request being processed */
> + if (rp->c_state == RC_INPROG)
> + goto out_trace;
> +
> + /* From the hall of fame of impractical attacks:
> + * Is this a user who tries to snoop on the cache?
> + */
> + rtn = RC_DOIT;
> + if (!test_bit(RQ_SECURE, &rqstp->rq_flags) && rp->c_secure)
> + goto out_trace;
>
> + /* Compose RPC reply header */
> + switch (rp->c_type) {
> + case RC_NOCACHE:
> + break;
> + case RC_REPLSTAT:
> + xdr_stream_encode_be32(&rqstp->rq_res_stream, rp->c_replstat);
> + rtn = RC_REPLY;
> + break;
> + case RC_REPLBUFF:
> + if (!nfsd_cache_append(rqstp, &rp->c_replvec))
> + return rtn; /* should not happen */
> + rtn = RC_REPLY;
> + break;
> + default:
> + WARN_ONCE(1, "nfsd: bad repcache type %d\n", rp->c_type);
> + }
> +
> +out_trace:
> + trace_nfsd_drc_found(nn, rqstp, rtn);
> + return rtn;
> + }
> +out:
> nfsd_cacherep_dispose(&dispose);
>
> nfsd_stats_rc_misses_inc(nn);
> atomic_inc(&nn->num_drc_entries);
> nfsd_stats_drc_mem_usage_add(nn, sizeof(*rp));
> - goto out;
> -
> -found_entry:
> - /* We found a matching entry which is either in progress or done. */
> - nfsd_reply_cache_free_locked(NULL, rp, nn);
> - nfsd_stats_rc_hits_inc(nn);
> - rtn = RC_DROPIT;
> - rp = found;
> -
> - /* Request being processed */
> - if (rp->c_state == RC_INPROG)
> - goto out_trace;
> -
> - /* From the hall of fame of impractical attacks:
> - * Is this a user who tries to snoop on the cache? */
> - rtn = RC_DOIT;
> - if (!test_bit(RQ_SECURE, &rqstp->rq_flags) && rp->c_secure)
> - goto out_trace;
> -
> - /* Compose RPC reply header */
> - switch (rp->c_type) {
> - case RC_NOCACHE:
> - break;
> - case RC_REPLSTAT:
> - xdr_stream_encode_be32(&rqstp->rq_res_stream, rp->c_replstat);
> - rtn = RC_REPLY;
> - break;
> - case RC_REPLBUFF:
> - if (!nfsd_cache_append(rqstp, &rp->c_replvec))
> - goto out_unlock; /* should not happen */
> - rtn = RC_REPLY;
> - break;
> - default:
> - WARN_ONCE(1, "nfsd: bad repcache type %d\n", rp->c_type);
> - }
> -
> -out_trace:
> - trace_nfsd_drc_found(nn, rqstp, rtn);
> -out_unlock:
> - spin_unlock(&b->cache_lock);
> -out:
> return rtn;
> }
>
> --
> 2.30.2
>
>
