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

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.
-- 
Jeff Layton <[email protected]>