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

On Thu, 2026-07-16 at 10:28 -0400, 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.
> > 

More or less. IIRC:

We had decided that wdeleg_getattr was sort of an oddity that didn't
really belong in this /proc file. The values in that file are mostly
counting other very specific protocol operations, but wdeleg_getattr is
basically counting GETATTR operations from other clients while there is
an outstanding write delegation on a file.

Since CB_GETATTR counts should approximate wdeleg_getattr, we decided
to start counting those instead (along with all of the other callback
ops). wdeleg_getattr in this series is changed to just report
CB_GETATTR counts.

> > 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.
> 

Sure. We could add some way to keep reporting it (or just keep using
/proc for this). AFAIK though, there is no (public) consumer of this
stat. nfs-utils doesn't look at it.

To wit: I'd like to head a clear use-case for this stat before we
decide to we need to count it like we did before.
-- 
Jeff Layton <[email protected]>