> 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.
--
Chuck Lever
