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