On 6/23/25 9:31 PM, Su Hui wrote:
> On 2025/6/24 08:19, NeilBrown wrote:
>> 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.
> Got it. I will focus on "Change the order ..." part in the next v2 patch.
>> 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
>>
> Really thanks for your suggestions!
> Yes, I'd like to do it in the next v2 patchset as soon as possible.
> I'm always searching some things I can participate in about linux kernel
> community, so it's happy for me to do this thing.
Hi Su -
Split the individual changes Neil suggested into separate patches. That
makes the changes easier to review.
>>> 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
>>>
>>>
--
Chuck Lever
