Re: [PATCH v2 2/3] nfsd: protect concurrent access to nfsd stats counters

From: "J . Bruce Fields" <bfields@fieldses.org>
To: Amir Goldstein <amir73il@gmail.com>
Cc: Chuck Lever <chuck.lever@oracle.com>,
	Jeff Layton <jlayton@poochiereds.net>,
	Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v2 2/3] nfsd: protect concurrent access to nfsd stats counters
Date: Thu, 7 Jan 2021 09:25:29 -0500	[thread overview]
Message-ID: <20210107142529.GA5730@fieldses.org> (raw)
In-Reply-To: <CAOQ4uxgh=f7fQ6Zf6ry8tC-WXMEoZzTo50y+K56paLTyadV7yw@mail.gmail.com>

On Thu, Jan 07, 2021 at 09:17:42AM +0200, Amir Goldstein wrote:
> On Wed, Jan 6, 2021 at 10:50 PM J . Bruce Fields <bfields@fieldses.org> wrote:
> >
> > These three patches seem fine.  To bikeshed just a bit more:
> >
> > On Wed, Jan 06, 2021 at 09:52:35AM +0200, Amir Goldstein wrote:
> > >       /* We found a matching entry which is either in progress or done. */
> > > -     nfsdstats.rchits++;
> > > +     nfsd_stats_rc_hits_inc();
> >
> > Maybe make that something like
> >
> >         nfsd_stats_inc(NFSD_STATS_RC_HITS);
> >
> > and then we could avoid boilerplate like the below?
> >
> 
> It looks like boilerplate, but every helper is a bit different,
> so we would have to introduce several variants like:
> 
>          nfsd_net_stats_inc(nn, NFSD_NET_PAYLOAD_MISSES);
> 
> Which is the way I started, but realized it opens a big hole for
> human mistakes in the form of:
> 
>          nfsd_net_stats_inc(nn, NFSD_STATS_RC_HITS);
>          nfsd_stats_inc(NFSD_NET_PAYLOAD_MISSES);
> 
> And we have no way to detect those errors at compile time
> because enum types are not strict types.
> 
> Also, in the next patch, three more helpers become unique
> as they are made into helpers to account for both global and per-export
> stats (fh_stale, io_read, io_write).
> Making those helpers generic would require aligning the start of global
> stats enum values with the per-export enum values and that is a source
> of more human errors.
> 
> See the end result of stats.h. All the helpers are unique and the only ones
> that remain generic are nfsd_stats_rc_*.
> Making those generic would also require checking the range of the index
> value is in the NFSD_STATS_RC_* range.
> 
> I also tried a version with boilerplate defining macros, i.e.:
> 
> NFSD_STATS_FUNCS(rc_hits, RC_HITS)
> NFSD_STATS_FUNCS(fh_stale, FH_STALE)
> NFSD_STATS_FUNCS(io_read, IO_READ)
> 
> But when I realized in the end it can only serve the rc_* counters,
> I dropped it.

Yeah, OK.  I'm not a fan of that kind of macro use, either.  It's made
me waste time when "grep" doesn't turn up a function definition, for
example.

> Thoughts?

I've got no clever alternative, and you've tried a lot of alternatives,
so I'll trust your judgement.  Thanks for the explanation.

--b.