Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Dai Ngo <dai.ngo@oracle.com>
CC:     Jeff Layton <jlayton@kernel.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v3 1/2] NFSD: keep track of the number of courtesy clients
 in the system
Thread-Topic: [PATCH v3 1/2] NFSD: keep track of the number of courtesy
 clients in the system
Thread-Index: AQHYvKTIVKcnz2SABUymlSPMPaQmba3H12SAgAABrAA=
Date:   Tue, 30 Aug 2022 19:46:20 +0000
Message-ID: <D11AD125-5623-4DAA-BCF3-5F8BE9BE28F5@oracle.com>
References: <1661886865-30304-1-git-send-email-dai.ngo@oracle.com>
 <1661886865-30304-2-git-send-email-dai.ngo@oracle.com>
 <e32e725974ca5f5f5ee4e16192d4c4cce80e9248.camel@kernel.org>
In-Reply-To: <e32e725974ca5f5f5ee4e16192d4c4cce80e9248.camel@kernel.org>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?QvZBHMVqlWicPqR/V33nwBDYxr6Z4n6RFpmafNaE4Bzl63UpmBQChDC6X78t?=
 =?us-ascii?Q?SuSZ/6yjycSC8mDamXSvhdRegA9ClZnF6CuZM3Xg3OIVJhbhlqMo77wiYP0j?=
 =?us-ascii?Q?OsQGcXuuK/ZvVVJiBKABRc4aVykBTQ7x3oAqDxFIWeCK8IlmDhyEyRg8AgP5?=
 =?us-ascii?Q?wnckyQ2E/r4SPJZwPge/Zf8IILHQZ+uhgjvHXm3MMj0C4dKf9jG+yaM4KVNo?=
 =?us-ascii?Q?c4XJoduV/M7IC00jRU1xiSho3kLFDOM1HMsinMCbVgVybH+VQsyx/kggs0/c?=
 =?us-ascii?Q?R1FPo2cjIOlpp9WGDnvPtyPChk6R5VrXGEMBr/d2f38DEHnsfI0RbjbJon2w?=
 =?us-ascii?Q?Y0YixoS7tUbl+zkudQ//vH5xuWX4epZarGqrvB8auEz8bP6EVc/ynZ2iJysI?=
 =?us-ascii?Q?mBQGXOnN1kE7Bk+P4gDDITMkQC3KPLBMNSsDBcIltJFJRryu68k4Y1fbPQsn?=
 =?us-ascii?Q?SYBKFElDuDLjKUNE3J1VdcZyROpkR1FsK95fYU5/8qLkOYDve+TPnZTLsctk?=
 =?us-ascii?Q?YYaPo0refgHETNAahjUwvYmE7C3f86oU3nRd7uavKH0Uz/vSJ6CIQPJ2Ynd5?=
 =?us-ascii?Q?618GqeLdJi++Yat5RCkDh9g/P1ta5yAojthFNgn2Cg7JfQVx26stDWtorcx4?=
 =?us-ascii?Q?KHsngu19W74jlmZiKZM/2Dqjm/JFnxbR4skrDkcMAQtNnG3kE7g9BWOpuAQJ?=
 =?us-ascii?Q?QwqyLZVYZdifLGcpOgukw3BbkWH5YWbek5RNMbgHHUcf8du+tMvQ7OlUrHaG?=
 =?us-ascii?Q?5krpoPPr5nJgYe34gJFKkzaKt/+WNnsjpD2Xe0IRVa4eltYEkq3doMktG91T?=
 =?us-ascii?Q?lsweygb5JfLzG+EHCyRnr1nSEJM5K3f0K0E9uB0UhSZtQrIW7qFRwz1kasSb?=
 =?us-ascii?Q?osNz696q6m9NkoEy+sRM1YbQ0YVtU8tpnZcTUczUJ3D1/bDSaW7TgjfcNykz?=
 =?us-ascii?Q?B9BbLfW3LL0xpTIzkF2pfy722BHKkrMNeOyEA+fAiC36Oji7omWhMv+ke1Vm?=
 =?us-ascii?Q?X1cIXL9DTChyAkJ8nFQvgHl0knyT+bbUd1CAQO0tbplDmTs/12NQFTaPFdts?=
 =?us-ascii?Q?MtgLGyxEwqiH7aDXWsuq2vgZwcs3u4jonvvU6tQORHdNX52k7qgFmPAy7Or5?=
 =?us-ascii?Q?SKpLXh1aAEEDGSyPP4Y6DI8HP5gb/6sjprwPMzbbiNA/qypw4Q/RiOJxNd79?=
 =?us-ascii?Q?Gz1X4jjqsK4nF/9na8ftQzObgEs5ClIjdnQ6Azx3CTikOfVTwLM2uSVQtc00?=
 =?us-ascii?Q?wYojibriRUVK97aoy1EkLmcg/aUKGbetFhkLIfivSBpcWoR4IQ7jPha7taAt?=
 =?us-ascii?Q?CGaerqmhLEPd8rJ2qXW7an85wuJWJflvCpiVqwo7PRTK5DmcCH+q4avzAR+a?=
 =?us-ascii?Q?pV69RC7RyLdTa16ZjXKKJv0+Hyokpdhv4fRVkbcSl6QnmKnZYbxV8WEULpQ4?=
 =?us-ascii?Q?mtGhRhTztfGMYxj8I6CUmNP5o5QNdoHtOvAxsZQEbiIeQDGT6K0m1VScVA3T?=
 =?us-ascii?Q?diY79ulnRIR4ttABzYO5kFqf9VxOR0gFZj074j4fV5A6hVXitDpCF7/weatA?=
 =?us-ascii?Q?5AZOiAw+No+HkOUR7irHOlt/RQwGfGLAaKD4A67zHEbnXO5BhEzsagHSCC7D?=
 =?us-ascii?Q?hA=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <CC57E8F305D5274BBFF58C4292D9DB89@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2473850c-e964-4895-99f2-08da8ac0502e
X-MS-Exchange-CrossTenant-originalarrivaltime: 30 Aug 2022 19:46:20.9306
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: Nur0Ppn9K0zwIBn1jft3yMCBXzDcvK2AjETM8sBckPMkYlwM8vKErp/n72qEa757kJYSISuG9gqiv2YTBRU1kA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR10MB6442
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.517,FMLib:17.11.122.1
 definitions=2022-08-30_10,2022-08-30_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 mlxscore=0 malwarescore=0
 suspectscore=0 spamscore=0 adultscore=0 mlxlogscore=999 bulkscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2207270000
 definitions=main-2208300089
X-Proofpoint-ORIG-GUID: arfFHoDMK6PeyiVjLfc6MrT6fNuS7gqq
X-Proofpoint-GUID: arfFHoDMK6PeyiVjLfc6MrT6fNuS7gqq
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Aug 30, 2022, at 3:40 PM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> On Tue, 2022-08-30 at 12:14 -0700, Dai Ngo wrote:
>> Add counter nfs4_courtesy_client_count to nfsd_net to keep track
>> of the number of courtesy clients in the system.
>>=20
>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>> ---
>> fs/nfsd/netns.h     |  2 ++
>> fs/nfsd/nfs4state.c | 14 +++++++++++---
>> 2 files changed, 13 insertions(+), 3 deletions(-)
>>=20
>> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
>> index ffe17743cc74..2695dff1378a 100644
>> --- a/fs/nfsd/netns.h
>> +++ b/fs/nfsd/netns.h
>> @@ -192,6 +192,8 @@ struct nfsd_net {
>>=20
>> 	atomic_t		nfs4_client_count;
>> 	int			nfs4_max_clients;
>> +
>> +	atomic_t		nfsd_courtesy_client_count;
>> };
>>=20
>> /* Simple check to find out if a given net was properly initialized */
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index c5d199d7e6b4..9675b5d8f408 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -169,7 +169,8 @@ static __be32 get_client_locked(struct nfs4_client *=
clp)
>> 	if (is_client_expired(clp))
>> 		return nfserr_expired;
>> 	atomic_inc(&clp->cl_rpc_users);
>> -	clp->cl_state =3D NFSD4_ACTIVE;
>> +	if (xchg(&clp->cl_state, NFSD4_ACTIVE) !=3D NFSD4_ACTIVE)
>> +		atomic_add_unless(&nn->nfsd_courtesy_client_count, -1, 0);
>=20
> I'd prefer this:
>=20
> 	if (clp->cl_state !=3D NFSD4_ACTIVE)
> 		atomic_add_unless(...
> 	clp->cl_state =3D NFSD4_ACTIVE;
>=20
> It's more lines, but it's easier to read. It's may also be more
> efficient since you're protected by a spinlock anyway.
>=20
> I think it's also less deceptive when reading the code. When I see
> xchg() operations being used like that, my immediate thought is that
> they must be needed for synchronization purposes. It's not in this case.

Agree: generally speaking, primitives like xchg() are used only when
hardware-assisted synchronization is needed. These are expensive
operations, to be used sparingly.

Let's replace these with plain vanilla "=3D", as Jeff suggested.

Likewise, I'm not totally convinced that nfsd_courtesy_client_count
has to be an atomic.


>> 	return nfs_ok;
>> }
>>=20
>> @@ -190,7 +191,8 @@ renew_client_locked(struct nfs4_client *clp)
>>=20
>> 	list_move_tail(&clp->cl_lru, &nn->client_lru);
>> 	clp->cl_time =3D ktime_get_boottime_seconds();
>> -	clp->cl_state =3D NFSD4_ACTIVE;
>> +	if (xchg(&clp->cl_state, NFSD4_ACTIVE) !=3D NFSD4_ACTIVE)
>> +		atomic_add_unless(&nn->nfsd_courtesy_client_count, -1, 0);
>> }
>>=20
>> static void put_client_renew_locked(struct nfs4_client *clp)
>> @@ -2233,6 +2235,8 @@ __destroy_client(struct nfs4_client *clp)
>> 	if (clp->cl_cb_conn.cb_xprt)
>> 		svc_xprt_put(clp->cl_cb_conn.cb_xprt);
>> 	atomic_add_unless(&nn->nfs4_client_count, -1, 0);
>> +	if (clp->cl_state !=3D NFSD4_ACTIVE)
>> +		atomic_add_unless(&nn->nfsd_courtesy_client_count, -1, 0);
>> 	free_client(clp);
>> 	wake_up_all(&expiry_wq);
>> }
>> @@ -4356,6 +4360,8 @@ void nfsd4_init_leases_net(struct nfsd_net *nn)
>> 	max_clients =3D (u64)si.totalram * si.mem_unit / (1024 * 1024 * 1024);
>> 	max_clients *=3D NFS4_CLIENTS_PER_GB;
>> 	nn->nfs4_max_clients =3D max_t(int, max_clients, NFS4_CLIENTS_PER_GB);
>> +
>> +	atomic_set(&nn->nfsd_courtesy_client_count, 0);
>> }
>>=20
>> static void init_nfs4_replay(struct nfs4_replay *rp)
>> @@ -5879,7 +5885,9 @@ nfs4_get_client_reaplist(struct nfsd_net *nn, stru=
ct list_head *reaplist,
>> 		if (!state_expired(lt, clp->cl_time))
>> 			break;
>> 		if (!atomic_read(&clp->cl_rpc_users))
>> -			clp->cl_state =3D NFSD4_COURTESY;
>> +			if (xchg(&clp->cl_state, NFSD4_COURTESY) =3D=3D
>> +							NFSD4_ACTIVE)
>> +				atomic_inc(&nn->nfsd_courtesy_client_count);
>> 		if (!client_has_state(clp))
>> 			goto exp_client;
>> 		if (!nfs4_anylock_blockers(clp))
>=20
> --=20
> Jeff Layton <jlayton@kernel.org>

--
Chuck Lever