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

[Chuck Lever]

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.


-- 
Chuck Lever