Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\))
Subject: Re: [PATCH 1/2] nfsd: protect concurrent access to nfsd stats
 counters
From:   Chuck Lever <chuck.lever@oracle.com>
In-Reply-To: <CAOQ4uxg_ZMPp_Huh3RS=bmFzoNtLicFdkoOQfZni9g_o+CBhDg@mail.gmail.com>
Date:   Mon, 4 Jan 2021 21:12:40 -0500
Cc:     Bruce Fields <bfields@fieldses.org>,
        Jeff Layton <jlayton@poochiereds.net>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <AF5EDF2D-6164-42A6-96E9-72E75D3F226A@oracle.com>
References: <20201228170344.22867-1-amir73il@gmail.com>
 <20201228170344.22867-2-amir73il@gmail.com>
 <20210104215528.GA27763@fieldses.org>
 <CAOQ4uxg_ZMPp_Huh3RS=bmFzoNtLicFdkoOQfZni9g_o+CBhDg@mail.gmail.com>
To:     Amir Goldstein <amir73il@gmail.com>
Precedence: bulk


> On Jan 4, 2021, at 5:22 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>=20
> On Mon, Jan 4, 2021 at 11:55 PM J . Bruce Fields =
<bfields@fieldses.org> wrote:
>>=20
>> Thanks for looking at this, it's long overdue!
>>=20
>> 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.
>>>=20
>>> Convert some nfsd_stats and nfsd_net struct members to use percpu =
counters.
>>>=20
>>> 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.
>>=20
>> Looking through the history, the code that updated fh_lookup, for
>> example, was removed in 2002.
>>=20
>> 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.
>>=20
>=20
> Sure. I can send a followup patch.
>=20
>>> The longest_chain* members of struct nfsd_net remain unprotected.
>>>=20
>>> 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(-)
>>>=20
>>> 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 @@
>>>=20
>>> #include <net/net_namespace.h>
>>> #include <net/netns/generic.h>
>>> +#include <linux/percpu_counter.h>
>>>=20
>>> /* Hash tables for nfs4_clientid state */
>>> #define CLIENT_HASH_BITS                 4
>>> @@ -149,20 +150,25 @@ struct nfsd_net {
>>>=20
>>>      /*
>>>       * 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.
>>>       */
>>>=20
>>>      /* total number of entries */
>>>      atomic_t                 num_drc_entries;
>>>=20
>>> +     /* Reference to below counters as array for init/destroy */
>>> +     struct percpu_counter    counters[0];
>>=20
>> 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
>=20
> Yeh, look at ceph_metric_init() and imagine what nfsdstats init
> would look like.
>=20
>> don't have a better idea.  Is this a common pattern elsewhere?
>>=20
>=20
> 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.
>=20
> 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