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

["J . Bruce Fields" <bfields@fieldses.org>]

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@fieldses.org> 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@gmail.com>
> > > ---
> > >  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.