On Mon, Jan 4, 2021 at 11:55 PM J . Bruce Fields <bfields@xxxxxxxxxxxx> wrote:
>
> Thanks for looking at this, it's long overdue!
>
> On Mon, Dec 28, 2020 at 07:03:43PM +0200, Amir Goldstein wrote:
> > nfsd stats counters can be updated by concurrent nfsd threads without any
> > protection.
> >
> > Convert some nfsd_stats and nfsd_net struct members to use percpu counters.
> >
> > There are several members of struct nfsd_stats that are reported in file
> > /proc/net/rpc/nfsd by never updated. Those have been left untouched.
>
> Looking through the history, the code that updated fh_lookup, for
> example, was removed in 2002.
>
> I'd be OK with removing those entirely, maybe just leave a /* deprecated
> field */ comment where we printk the hard-coded 0's. If somebody wants
> to know more they can still find the answers in git.
>
Sure. I can send a followup patch.
> > The longest_chain* members of struct nfsd_net remain unprotected.
> >
> > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> > ---
> > fs/nfsd/netns.h | 20 +++++++----
> > fs/nfsd/nfs4proc.c | 2 +-
> > fs/nfsd/nfscache.c | 52 +++++++++++++++++++--------
> > fs/nfsd/nfsctl.c | 5 ++-
> > fs/nfsd/nfsfh.c | 2 +-
> > fs/nfsd/stats.c | 87 ++++++++++++++++++++++++++++++++++++----------
> > fs/nfsd/stats.h | 42 +++++++++++++++-------
> > fs/nfsd/vfs.c | 4 +--
> > 8 files changed, 156 insertions(+), 58 deletions(-)
> >
> > diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> > index 7346acda9d76..080c5389b2e7 100644
> > --- a/fs/nfsd/netns.h
> > +++ b/fs/nfsd/netns.h
> > @@ -10,6 +10,7 @@
> >
> > #include <net/net_namespace.h>
> > #include <net/netns/generic.h>
> > +#include <linux/percpu_counter.h>
> >
> > /* Hash tables for nfs4_clientid state */
> > #define CLIENT_HASH_BITS 4
> > @@ -149,20 +150,25 @@ struct nfsd_net {
> >
> > /*
> > * Stats and other tracking of on the duplicate reply cache.
> > - * These fields and the "rc" fields in nfsdstats are modified
> > - * with only the per-bucket cache lock, which isn't really safe
> > - * and should be fixed if we want the statistics to be
> > - * completely accurate.
> > + * The longest_chain* fields are modified with only the per-bucket
> > + * cache lock, which isn't really safe and should be fixed if we want
> > + * these statistics to be completely accurate.
> > */
> >
> > /* total number of entries */
> > atomic_t num_drc_entries;
> >
> > + /* Reference to below counters as array for init/destroy */
> > + struct percpu_counter counters[0];
>
> This feels slightly too clever for its own good, but.... OK, I see
> there's a bunch of initializations to do in the nfsdstats case, and you
> don't want to open code all that (and its error handling). I guess I
Yeh, look at ceph_metric_init() and imagine what nfsdstats init
would look like.
> don't have a better idea. Is this a common pattern elsewhere?
>
Sort of. Inspired by xfsstats and related macros (fs/xfs/xfs_stats.h).
I have tried several approaches and this one ended up being the
cleanest and smallest patch.
The cleaner way would be an actual percpu_counter array and
convert all callers to use enum index to array like the dqstats counters
(include/linux/quota.h), but IMO current patch is enough.
Thanks,
Amir.
