Re: [PATCH v6 3/6] nfsd: implement server-stats-get netlink handler

Hi,

The|wdeleg_getattr| counter tracks the number of times a|GETATTR|
operation conflicts with an outstanding write delegation. As such,
the name|wdeleg_getattr| does not accurately describe what the
counter represents.

Furthermore, this statistic probably does not belong in|nfsstats|.
Since there are no known user-space consumers of this counter, it
would be better to remove it altogether.

Thanks,
-Dai


On 7/16/26 7:28 AM, Chuck Lever wrote:
>
> On Thu, Jul 16, 2026, at 10:16 AM, Chuck Lever wrote:
>> On Thu, Jul 16, 2026, at 10:12 AM, Jeff Layton wrote:
>>> On Thu, 2026-07-16 at 10:04 -0400, Chuck Lever wrote:
>>>> On Thu, Jul 16, 2026, at 9:53 AM, Jeff Layton wrote:
>>>>> On Fri, 2026-06-19 at 16:40 -0400, Chuck Lever wrote:
>>>>>> On Fri, Jun 19, 2026, at 11:26 AM, Jeff Layton wrote:
>>>>>>> +		/* Per-version procedure counts */
>>>>>>> +		if (statp->vs_count) {
>>>>>>> +			static const int proc_attrs[] = {
>>>>>>> +				[2] = NFSD_A_SERVER_STATS_PROC2_OPS,
>>>>>>> +				[3] = NFSD_A_SERVER_STATS_PROC3_OPS,
>>>>>>> +				[4] = NFSD_A_SERVER_STATS_PROC4_OPS,
>>>>>>> +			};
>>>>>>> +			unsigned int i;
>>>>>>> +
>>>>>>> +			for (i = 0; i < prog->pg_nvers &&
>>>>>>> +			     i < ARRAY_SIZE(proc_attrs); i++) {
>>>>>>> +				if (!prog->pg_vers[i] ||
>>>>>>> +				    !statp->vs_count[i])
>>>>>>> +					continue;
>>>>>>> +				if (!proc_attrs[i])
>>>>>>> +					continue;
>>>>>>> +				if (nfsd_nl_fill_proc_ops(skb,
>>>>>>> +						proc_attrs[i],
>>>>>>> +						statp->vs_count[i],
>>>>>>> +						prog->pg_vers[i]->vs_nproc))
>>>>>>> +					goto err_cancel;
>>>>>>> +			}
>>>>>>> +		}
>>>>>>> +
>>>>>>> +#ifdef CONFIG_NFSD_V4
>>>>>>> +		/* NFSv4 individual operation counts */
>>>>>>> +		for (int i = 0; i <= LAST_NFS4_OP; i++) {
>>>>>>> +			struct nlattr *nest;
>>>>>>> +			u64 cnt;
>>>>>>> +
>>>>>>> +			cnt = percpu_counter_sum_positive(
>>>>>>> +				&nn->counter[NFSD_STATS_NFS4_OP(i)]);
>>>>>>> +
>>>>>>> +			nest = nla_nest_start(skb,
>>>>>>> +					NFSD_A_SERVER_STATS_PROC4OPS_OPS);
>>>>>>> +			if (!nest)
>>>>>>> +				goto err_cancel;
>>>>>>> +			if (nla_put_u32(skb, NFSD_A_SERVER_PROC_ENTRY_OP, i) ||
>>>>>>> +			    nla_put_u64_64bit(skb, NFSD_A_SERVER_PROC_ENTRY_COUNT,
>>>>>>> +					      cnt, NFSD_A_SERVER_PROC_ENTRY_PAD)) {
>>>>>>> +				nla_nest_cancel(skb, nest);
>>>>>>> +				goto err_cancel;
>>>>>>> +			}
>>>>>>> +			nla_nest_end(skb, nest);
>>>>>>> +		}
>>>>>>> +#endif
>>>>>> This loop open-codes the same nest that nfsd_nl_fill_proc_ops() builds just
>>>>>> above -- nla_nest_start(), nla_put_u32(PROC_ENTRY_OP),
>>>>>> nla_put_u64_64bit(PROC_ENTRY_COUNT), nla_nest_end() -- into the same
>>>>>> NFSD_A_SERVER_STATS_PROC4OPS_OPS attribute.  Could the helper be generalized
>>>>>> to take the per-op counter source so this is not a second copy of the same
>>>>>> code?
>>>>>>
>>>>>> The per-version block above skips empty versions:
>>>>>>
>>>>>> 	if (!prog->pg_vers[i] || !statp->vs_count[i])
>>>>>> 		continue;
>>>>>>
>>>>>> but this loop emits an entry for every op 0..LAST_NFS4_OP, zero-count ops
>>>>>> included.  Is that difference intentional?  Skipping zero counts here would
>>>>>> also trim the worst-case message size above.
>>>>>>
>>>>>> There is also a counter that this dump does not emit.  /proc/net/rpc/nfsd
>>>>>> prints a wdeleg_getattr line after proc4ops:
>>>>>>
>>>>>> 	seq_printf(seq, "\nwdeleg_getattr %lld",
>>>>>> 		percpu_counter_sum_positive(&nn->counter[NFSD_STATS_WDELEG_GETATTR]));
>>>>>>
>>>>>> incremented by nfsd_stats_wdeleg_getattr_inc().  Since the goal is to expose
>>>>>> the statistics currently available via /proc/net/rpc/nfsd, should
>>>>>> wdeleg_getattr get an attribute here too, so nfsstat over netlink does not
>>>>>> drop it relative to the procfs path?
>>>>>>
>>>>> The only thing in this review that is not addressed in my current
>>>>> series is the above comment, and I think we had agreed in an earlier
>>>>> email thread that this was approximately the same value as the counter
>>>>> for CB_GETATTR calls and so it's good enough for this purpose.
>>>>>
>>>>> Are you still ok with this approach?
>>>> In spite of the email quoting, I’ve still forgotten most of the context
>>>> of that conversation.
>>>>
>>>> Looking at this now, I think someone later could get an urge to add the
>>>> wdeleg_getattr statistic to netlink… for symmetry, because it smells like
>>>> technical debt, or it might actually be useful for something.
>>>>
>>>> IMHO we should include it now.
>>>>
>>> IDGI: what's the point of collecting that stat separately when we have
>>> stats for CB_GETATTR already? I'd prefer to avoid perpetuating
>>> wdeleg_getattr as a stat here, since it's not directly tied to the
>>> protocol in the same way as the others are.
>>>
>>> The only difference between the two is that if another client does a
>>> GETATTR for the fh while there is already a CB_GETATTR in flight,
>>> wdeleg_getattr gets bumped today, whereas CB_GETATTR counter does not
>>> (since there is only a single CB_GETATTR for that).
>>>
>>> Is that difference relevant? I think Dai added wdeleg_getattr
>>> originally, so I'd really like to hear his opinion on this.
>> OK, so you are actually re-litigating whether the stat should exist
>> at all. I confess I had forgotten that context.
>>
>> I’m arguing only that the netlink and procfs APIs should remain
>> equivalent, at least initially. Deleting the stat entirely probably
>> has no consequences right now, but let’s hear from Dai when the US
>> west coast wakes up.
> Another option is to keep the stat but report it some other way.
> It is arguable whether nfsstat is the proper mechanism to report
> the different reasons for sending a CB_GETATTR.
>
>