On Tue, Jan 5, 2021 at 4:12 AM Chuck Lever <chuck.lever@xxxxxxxxxx> wrote:
>
>
>
> > On Jan 4, 2021, at 5:22 PM, Amir Goldstein <amir73il@xxxxxxxxx> 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.
>
> I'd prefer the "array with enum indices" approach. The current
> patch risks breaking C aliasing rules, IMHO -- with undefined
> consequences. I don't see a compelling reason we have to take
> that risk, however small it might be.
>
> At the very least, you could make the counters array and the
> set of counters into a union, but I'd prefer always accessing
> the underlying memory using the same high-level language
> constructs. That way, humans and compilers agree on what's
> going on here.
>
No problem. I just cannot resist a good shortcut when I see one ;-)
enum indices it shall be.
Thanks,
Amir.
