On Tue, Jan 05, 2021 at 12:22:27AM +0200, Amir Goldstein wrote:
> 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.
OK, I can live with that.
--b.
