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

[Chuck Lever]

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.


-- 
Chuck Lever