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 v4 2/2] NFSD: add shrinker to reap courtesy clients on low
 memory condition
Thread-Topic: [PATCH v4 2/2] NFSD: add shrinker to reap courtesy clients on
 low memory condition
Thread-Index: AQHYvLpF3IXRGSf70UK3afu6RNBIZ63JEv+AgAJR5wCAACuHAA==
Date:   Fri, 2 Sep 2022 04:32:01 +0000
Message-ID: <7041D47D-ECB3-497E-9174-96E9E36FFBDE@oracle.com>
References: <1661896113-8013-1-git-send-email-dai.ngo@oracle.com>
 <1661896113-8013-3-git-send-email-dai.ngo@oracle.com>
 <FA83E721-C874-4A47-87BA-54B13E0B12A3@oracle.com>
 <2df6f1fe-c8eb-d5a1-0a11-2fd965555a33@oracle.com>
In-Reply-To: <2df6f1fe-c8eb-d5a1-0a11-2fd965555a33@oracle.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?yfc0mLXHXPUqYV0eAmE9flcow7CTHqC3Ci8mWglKfUYU5jhceERD8qsUy4RR?=
 =?us-ascii?Q?sBHyb6V1RoqKhqEPMHy6xctkF4fAVPRaARg545zh+ZC6EaiomZQEIkO1IhvW?=
 =?us-ascii?Q?lWIvsTx00tcl9m7GXqxsnd5cRanWkTc+siWrgLy/lit2BUIzkuWeoqOg5FZF?=
 =?us-ascii?Q?RnSdrEErwZiNfXruppPnWB0F5xoc5xD9NKaa6adGMoJVdz0AZD2QKSmHdBte?=
 =?us-ascii?Q?aiJXI3LmCt89PcT7nFizvSWHqd9sh92QAhsD5gjlaQlnEhqlw6hTR8pGPr8h?=
 =?us-ascii?Q?nXyPFCGtwPQghOGC1sgWHndIOkAZTRq0X9JWdqsMquyFFegA7E78515Ef3dq?=
 =?us-ascii?Q?+TzMLtfKc7I6R3oqhajEWqPaDmk23GIb4uDru3tZRV38eqrAMJ6lfK7fVitc?=
 =?us-ascii?Q?mWdFnr1P7KJoK2RbeIIR4+aNYfMGxuvnPeMwPVJCktqe7yPSSbqdSDS4U7CY?=
 =?us-ascii?Q?ZQJsrraICLwH9AIKKieybeZoQ3qaNH+BHv1vr2rR9m7yialwt/TLygtnqjnJ?=
 =?us-ascii?Q?vaKZq9XAT0lOHsOMoGVSOtkVzvcmfdqvr3Yul5S5bH4rwEzZY227Hwqq7k3w?=
 =?us-ascii?Q?7zMEHIpInzbWn5kwRLh9ENam/k/yXsZNQbTwEbq9mZbVVnFT22/ZeiPpoMzv?=
 =?us-ascii?Q?TG4zqihC7ZDPiSQfYPpseSXxN7UWC68D8Ip9T2+kplk66o5OKBExP7U6gnzA?=
 =?us-ascii?Q?s3cg2GJoiB2Z7HWCyKJN3C29aUm/KaqfgXMqly+BYx/B7iJVXgSjORqzGmUq?=
 =?us-ascii?Q?Zm/wbHR5OFkF2SnCrueuWwPsiteZw5NPqyOjqT1sZHpRuM7srmN7T8cbyVBm?=
 =?us-ascii?Q?XQsZAGWslO3GZPjMRAaQ9qpEQ/cVrJoVXwgcYkD9NpwgqsP5WwEd2+k4spOP?=
 =?us-ascii?Q?7Nse6vkgtXs7jLUvUfezWkd9FVuxxeDcOJ5beUVLygdQJ04eu4Ne11VaRLZ4?=
 =?us-ascii?Q?lFa4c8nkdCX+umG9B0dfOQ/3051RxNbm3eV0Kb8ILsqffbO7OTfjGtiOktRQ?=
 =?us-ascii?Q?lwnDymXYLdyW6DwaWKh5tMUiC4R8IgRb4GhPWt34zup1j/NTUjnEfDniqm3L?=
 =?us-ascii?Q?kO/shGTfaOBHN8VI4EUv0ucoIShDM9xlyuTgruHsdMKXQ5SUtG91B+D+Xa+9?=
 =?us-ascii?Q?urY53kELqU6AIn5Wk6xobtrvagZNAyThWZaSHUF2QW+vumft2DPiKgBu8PkM?=
 =?us-ascii?Q?GKY86/m7igiDQunL0yrM9aCzQCwiMWQkK7irdxqC9czYCR85v0I0Bk/WB+Lq?=
 =?us-ascii?Q?b0PwLD3CBlQKh60kJIb3qBKx2cJesAJfHI5NSVafq3IUbnlmyqjj/2M8ZFGK?=
 =?us-ascii?Q?qXTrlBqNFm6lUwRauD97DdGbarfGRHGUg7yAiisB5R/wl0VKdRxLeH+kQGWm?=
 =?us-ascii?Q?EV6+FddcAbMCC1cILNcSWf+g+yLNIhF834zHPhDHeX4Thj0DaWGbjFJ0HwVh?=
 =?us-ascii?Q?I+4VaMQWyiKsEAU41NRVJi8g7zaxNtN5yxvBk3odj+o0b3V4QEusJiF52HKH?=
 =?us-ascii?Q?z8IgdPnIUYl3K8mKNHHbU8a9JmMHggZi+SvNh5G9ikoLx0jczaZUFyw65MbK?=
 =?us-ascii?Q?4tK+BGUe5m0YWs8085TT9zJKlwWhEcdVVyHqnlreRI1124SnBqPkrO5t031O?=
 =?us-ascii?Q?Jw=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <220F6451065F0A49BC928FACCBB767B4@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: 2a2aa895-b51c-4202-42e7-08da8c9c14ad
X-MS-Exchange-CrossTenant-originalarrivaltime: 02 Sep 2022 04:32:01.5408
 (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: yIdSCo+379e/vT69dnujcQOWQWUl9GNfsbriwp+h5bV8AVTGih9rfGP50Yaycosbjy1FAIOPc23VM5uiRxXoeQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR10MB4593
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-09-01_12,2022-08-31_03,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 mlxlogscore=999
 phishscore=0 suspectscore=0 malwarescore=0 adultscore=0 spamscore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2207270000 definitions=main-2209020019
X-Proofpoint-ORIG-GUID: 0KUqDyr1XHzJXEXxUauCYRvD4IDNson-
X-Proofpoint-GUID: 0KUqDyr1XHzJXEXxUauCYRvD4IDNson-
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 Sep 1, 2022, at 9:56 PM, Dai Ngo <dai.ngo@oracle.com> wrote:
>=20
> Hi Chuck,
>=20
> On 8/31/22 7:30 AM, Chuck Lever III wrote:
>>> 	struct list_head *pos, *next;
>>> 	struct nfs4_client *clp;
>>>=20
>>> -	maxreap =3D (atomic_read(&nn->nfs4_client_count) >=3D nn->nfs4_max_cl=
ients) ?
>>> -			NFSD_CLIENT_MAX_TRIM_PER_RUN : 0;
>>> +	cb_cnt =3D atomic_read(&nn->nfsd_client_shrinker_cb_count);
>>> +	if (atomic_read(&nn->nfs4_client_count) >=3D nn->nfs4_max_clients ||
>>> +							cb_cnt) {
>>> +		maxreap =3D NFSD_CLIENT_MAX_TRIM_PER_RUN;
>>> +		atomic_set(&nn->nfsd_client_shrinker_cb_count, 0);
>>> +	}
>> I'm not terribly happy with this, but I don't have a better suggestion
>> at the moment. Let me think about it.
>=20
> Do you have any suggestion to improve this, I want to incorporate it
> before sending out v5?

Let's consider some broad outlines...

With regards to parametrizing the reaplist behavior, you want
a normal laundromat run to reap zero or more courtesy clients.
You want a shrinker laundromat run to reap more than zero. I
think you want a minreap variable as well as a maxreap variable
in there to control how the reaplist is built. Making @minreap
a function parameter rather than a global atomic would be a
plus for me, but maybe that's not practical.

But I would prefer a more straightforward approach overall. The
proposed approach seems tricky and brittle, and needs a lot of
explaining to understand. Other subsystems seem to get away with
something simpler.

Can nfsd_courtesy_client_count() simply reduce
nn->nfs4_max_clients, kick the laundromat, and then return 0?
Then get rid of nfsd_courtesy_client_scan().

Or, nfsd_courtesy_client_count() could return
nfsd_couresy_client_count. nfsd_courtesy_client_scan() could
then look something like this:

	if ((sc->gfp_mask & GFP_KERNEL) !=3D GFP_KERNEL)
		return SHRINK_STOP;

	nfsd_get_client_reaplist(nn, reaplist, lt);
	list_for_each_safe(pos, next, &reaplist) {
		clp =3D list_entry(pos, struct nfs4_client, cl_lru);
		trace_nfsd_clid_purged(&clp->cl_clientid);
		list_del_init(&clp->cl_lru);
		expire_client(clp);
		count++;
	}
	return count;

Obviously you would need to refactor common code into helper
functions.

--
Chuck Lever