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;
Message-ID: <72b4e7e9-93c9-6f49-9e08-3fab14f8ca67@oracle.com>
Date:   Tue, 10 Jan 2023 09:25:49 -0800
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.6.1
Subject: Re: [PATCH 1/1] NFSD: register/unregister of nfsd-client shrinker at
 nfsd startup/shutdown time
Content-Language: en-US
To:     Jeff Layton <jlayton@kernel.org>, chuck.lever@oracle.com
Cc:     linux-nfs@vger.kernel.org
References: <1673332991-24406-1-git-send-email-dai.ngo@oracle.com>
 <8b456fe7a351b861a29186a421afdee57bd10309.camel@kernel.org>
From:   dai.ngo@oracle.com
In-Reply-To: <8b456fe7a351b861a29186a421afdee57bd10309.camel@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?TU5HRWpWWFA3dkMwMCtJbDBVSHZ3OFQvSkIwL2dMbVNZbGgvSVFERGRpS3ND?=
 =?utf-8?B?VnVhK0hQamRtYklPVFByRFJjd0F1eFlsTUp1UzQ3OWhSQmlHbldpb3NzVGRG?=
 =?utf-8?B?bEU0RmtvMkhvZzUvdHhLUnVPRHZjU29NZGMzVTl0cVE1NjlMUDhBZ1M1SlJQ?=
 =?utf-8?B?TVpuNUhDd2dnb1d2Qy9tKzFKQ0J1UytmbDFEc0Q3VWozZ0pSZWlxc0p0U0Fq?=
 =?utf-8?B?azJ0dktWNlFFVGtsYVc2NzJnNDhoeld1blhFYWdyY0RJN0xyZ1BJeDQ1M1p0?=
 =?utf-8?B?OW8vbG8zMmQyeUU3cDFwK01tUEo2VThLYk9CbU0xYU9lSlZ2RVcwWmpYbGg4?=
 =?utf-8?B?K2pBVVBkbVd6U3pnTFg4bFZGRTZRNVdQUURVK3Vod3E4amVWMFIyV0VQdDJR?=
 =?utf-8?B?RlhCcFlZNkt6Q2IzRXYydDBqSGt2a094K3VIeDZ1NVdNNmt4aVRLNVkrenBO?=
 =?utf-8?B?VGxDYXQ4SlJNOEpjdlAvZTFHeXlmWUlFUm1STThwMHFkaTg2U041NHV0ZzRC?=
 =?utf-8?B?OG8rQnczZEtjTEJIdDFzb0ViNXRQNUpjRjdrSHA5eWpSK2JyQXo5OW9VdGdr?=
 =?utf-8?B?ZC9EemhPdDdXdXdENDZ1WEloQnMzTENaM0xGVlpHVUNpRXBxOFl5THZqRjQ2?=
 =?utf-8?B?bEVKSzhFZXN2TzJyVTNPUGh1MVUxR3ZCWmZKKzR1TTl5aXJNdlkvbEZSUS94?=
 =?utf-8?B?QUdjWHZhU3hLZS9uOWtmWXB1V1pkVHRmMmZiczFZWUxvWFVjZjQ5VWpOVWxV?=
 =?utf-8?B?ZTJtZTVaRWNaZFBMMGxIZGNXdmFxOTFEL0E5Q2RZNmVaZk1LOHNzczBXRnRp?=
 =?utf-8?B?OW9sUGU3NG5KVnRzd1B3VEpUaC9WYUhrcWNOZjV1RWlIWXhtK2ZTS1dJNHQw?=
 =?utf-8?B?RjkyTmtNMG9BeVd4Q2ljYktFTnE2S1VZc1ZTWkZoQnhZRmVyWi9DSWhiOXFG?=
 =?utf-8?B?clV4ZGpvSTJMYW9XemNJcExWM3g1MExxSlJSZ0g4OVVJaUlaR1BMdStjNC9D?=
 =?utf-8?B?RVdNNUVCQ0dra3NDZmUrYUJXaWd6QWJRQjlUaW5HUGtMaW9pOGhieVlFR3J4?=
 =?utf-8?B?VlRnN3NmV2VsWTIyMUEzMVRpSk9oeVYzdlN1OGhId1ZlZHZ1dEJRa01NaTR4?=
 =?utf-8?B?T000RFUrd2t6RXNNdm9JY2o1Z0g1cS8wbHFSVTloL1Z3SjI4UUE1Sjk3SnZD?=
 =?utf-8?B?SUUySmNaT2NuLzlTNEhOSmE0K0R3RFZhL044VTRDb1ZoSGkwMWtRRTNvODU3?=
 =?utf-8?B?eHVWcjBIbGpPTHRYbm1EL3prSjZ1L2d0aFVSZ3ZKalFlb1RWbkVJNXBjZHkx?=
 =?utf-8?B?cjlyUVc1YU1qUWxLaXRWTzRIS3BCdjBNUEkzU3d0SVR4ZVgrY1g1MEdqZW1T?=
 =?utf-8?B?bUdJSjBkUmMyaG9GeXdpbXdCZ0hBUVhtTlRFMUhHZnVDdWZneURIWUFaS0dr?=
 =?utf-8?B?THh2T0lRbGp4V05GOTVuNVlicWFFdDJvamQweVR1TXgrRG5CTlhvektrTU1W?=
 =?utf-8?B?N2xtTGswK0FaRVgxTlBmSEdjUnh6bnFZaTJ6SURDYjV6RURLYnI0UFBKQW5Y?=
 =?utf-8?B?OEw5OXZtOVlmWFFrc3NMd0dhUHhxbm5EemdwN2cyVXI3VUM4RnVxdVliaTFz?=
 =?utf-8?B?enUrN0RYeVpicWZTTDM2WGxpdkhnSFlabmJVMmhqK3dJVDB2QUd2clAzSm1w?=
 =?utf-8?B?SjkwWnBNR1AybXdxQXRtdmNCSnFmNW9tbk52WGJZeE9TVnR2RkQ1SnI3d0t4?=
 =?utf-8?B?U1JIV2dSVVlDRTNnTGFFTUhrTlNSSHowT1Q1NnVXZDZxeVJEbTAzNVgyc2Yw?=
 =?utf-8?B?Mm9PS2NsMHNNcU1JN3A2WEp3d1NrT0R5WjNWdkNqZFNiMjE5cXFXenFrMmxR?=
 =?utf-8?B?SzI3UG1MSFd0UUhkd3hsUFdnY2llYUZxRk1BRmQvRWdjYVRybTZkQ21KOHFy?=
 =?utf-8?B?UVhNUXZLZUFrZU1LTFpGOW5LSjZkc051Z2s5S08xQ1lCZmRsT2lzMlZHcEhw?=
 =?utf-8?B?S0ovZE43R2JhaVc3Zm1yUU5PWHhUQWVra1F6clBEVUp0cWtOdm5yR3haZUlR?=
 =?utf-8?B?WHpsb05FVGQreFBMLytvblpWeGdrcG15VXVxS1FZdDdvSEZYMXpaZFZ2UXZW?=
 =?utf-8?Q?JVF8mYV/xB1eAOriUZRBdjKdI?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: Mav1pAdKFe7qrgDM+fbSLs6MjIuiVsX+P3TdPLpvBC+697/virMdZFzJ/fVm56Ec997WxyEgIzDGGS4MJ1W7qyYGYILpDFRUZjsnRxmia8bl7SDe1ipcoojfH3crzpgk439qxsAgjZkB77US2DfcBvSxYv5XzKZvl6hCnwT6AiKtUSYZeia7HHFyng62GyaZdzzezAMstT9mXnSZQDj15OB73ksHgpYWy6UrSgstuy5BjuArbg7bR5imRYPiCb3xzte/i1PDTOesIoZV0tecwJ/CV6cyKRkZss0D0Rabc07bdMCDMVhTYfO2fb3/p2zs2rN1WdZl1e0iLDISMWqtKU5Os5oAJEBRj5yJaR1vJtiUOqEYw/Cs1jKocnDew0efKcZnA0dfbPflr830ZZc8iVpxhL/kRaXY4LqecnQSy37q8VK7foBZJxA4MMo8AlBCtO5dAYsysssq+19I7RnHgEVaNqXlIVTv3OJ1sVZCrCbTtXVcaO3ECXLJ39FWVbAPa3gNtgVm8VKO9deelgQK3H9RHWckqWy5zZot5YSvsAqczBB0E6cRFPhJerc9u29+yyapj3tu4tuFMZm+56z/4wOjKAFlvOHvGqeXRjJJlJ8MpHU2onoqOWhibHqDosrR6li3e1icElALfeh7hgecMevwxvIBbCv0ZfEdgvCOGXvqANxa0VcYUdwU5ZInv9xEsibGi5OmOHuO3PSZ7e8AOToy6pUQ4nyJWy9vcENo+XN2RJJ1z2yUp6/QURc1ZrIESMNnyRH9D4bT3OYU6wGRz/AFy/oaVFd3Vbbbw5e7g0U=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9daec111-a3ed-4144-3d81-08daf32fb961
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4257.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Jan 2023 17:25:52.8809
 (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: sWf0NmhMYlHL/dIXIvqQi1MeKPuGYXu/uprguaoP51uwEVqTqXNDWO2JeG7haITz7mKnQ0C0z6QooBQaOBqAcQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR10MB6661
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.923,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2023-01-10_07,2023-01-10_03,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 bulkscore=0
 suspectscore=0 adultscore=0 spamscore=0 phishscore=0 malwarescore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2212070000 definitions=main-2301100110
X-Proofpoint-GUID: A-ocHAy_AbBGgwdon3kcYXTiBMMtxqI-
X-Proofpoint-ORIG-GUID: A-ocHAy_AbBGgwdon3kcYXTiBMMtxqI-
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE 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 1/10/23 2:24 AM, Jeff Layton wrote:
> On Mon, 2023-01-09 at 22:43 -0800, Dai Ngo wrote:
>> Currently the nfsd-client shrinker is registered and unregistered at
>> the time the nfsd module is loaded and unloaded. This means after the
>> nfsd service is shutdown, the nfsd-client shrinker is still registered
>> in the system. This causes the nfsd-client shrinker to be called when
>> memory is low even thought nfsd service is not running. This is also
>> true for the nfsd_reply_cache_shrinker.
>>
> But this patch doesn't move the reply cache shrinker. Do you intend to
> do that too?

yes, I plan to do that soon in a separate patch.

>
>> This patch moves the register/unregister of nfsd-client shrinker from
>> module load/unload time to nfsd startup/shutdown time.
>>
>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>> ---
>>   fs/nfsd/nfs4state.c | 18 ++++++------------
>>   fs/nfsd/nfsctl.c    |  7 +------
>>   fs/nfsd/nfsd.h      |  6 ++----
>>   3 files changed, 9 insertions(+), 22 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 7b2ee535ade8..ee56c9466304 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -4421,7 +4421,7 @@ nfsd4_state_shrinker_scan(struct shrinker *shrink, struct shrink_control *sc)
>>   	return SHRINK_STOP;
>>   }
>>   
>> -int
>> +void
>>   nfsd4_init_leases_net(struct nfsd_net *nn)
>>   {
>>   	struct sysinfo si;
>> @@ -4443,16 +4443,6 @@ nfsd4_init_leases_net(struct nfsd_net *nn)
>>   	nn->nfs4_max_clients = max_t(int, max_clients, NFS4_CLIENTS_PER_GB);
>>   
>>   	atomic_set(&nn->nfsd_courtesy_clients, 0);
>> -	nn->nfsd_client_shrinker.scan_objects = nfsd4_state_shrinker_scan;
>> -	nn->nfsd_client_shrinker.count_objects = nfsd4_state_shrinker_count;
>> -	nn->nfsd_client_shrinker.seeks = DEFAULT_SEEKS;
>> -	return register_shrinker(&nn->nfsd_client_shrinker, "nfsd-client");
>> -}
>> -
>> -void
>> -nfsd4_leases_net_shutdown(struct nfsd_net *nn)
>> -{
>> -	unregister_shrinker(&nn->nfsd_client_shrinker);
>>   }
>>   
>>   static void init_nfs4_replay(struct nfs4_replay *rp)
>> @@ -8077,7 +8067,10 @@ static int nfs4_state_create_net(struct net *net)
>>   	INIT_DELAYED_WORK(&nn->nfsd_shrinker_work, nfsd4_state_shrinker_worker);
>>   	get_net(net);
>>   
>> -	return 0;
>> +	nn->nfsd_client_shrinker.scan_objects = nfsd4_state_shrinker_scan;
>> +	nn->nfsd_client_shrinker.count_objects = nfsd4_state_shrinker_count;
>> +	nn->nfsd_client_shrinker.seeks = DEFAULT_SEEKS;
>> +	return register_shrinker(&nn->nfsd_client_shrinker, "nfsd-client");
>>   
>>   err_sessionid:
>>   	kfree(nn->unconf_id_hashtbl);
>> @@ -8171,6 +8164,7 @@ nfs4_state_shutdown_net(struct net *net)
>>   	struct list_head *pos, *next, reaplist;
>>   	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
>>   
>> +	unregister_shrinker(&nn->nfsd_client_shrinker);
>>   	cancel_delayed_work_sync(&nn->laundromat_work);
>>   	locks_end_grace(&nn->nfsd4_manager);
>>   
>> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
>> index d1e581a60480..c2577ee7ffb2 100644
>> --- a/fs/nfsd/nfsctl.c
>> +++ b/fs/nfsd/nfsctl.c
>> @@ -1457,9 +1457,7 @@ static __net_init int nfsd_init_net(struct net *net)
>>   		goto out_idmap_error;
>>   	nn->nfsd_versions = NULL;
>>   	nn->nfsd4_minorversions = NULL;
>> -	retval = nfsd4_init_leases_net(nn);
>> -	if (retval)
>> -		goto out_drc_error;
>> +	nfsd4_init_leases_net(nn);
>>   	retval = nfsd_reply_cache_init(nn);
>>   	if (retval)
>>   		goto out_cache_error;
>> @@ -1469,8 +1467,6 @@ static __net_init int nfsd_init_net(struct net *net)
>>   	return 0;
>>   
>>   out_cache_error:
>> -	nfsd4_leases_net_shutdown(nn);
>> -out_drc_error:
>>   	nfsd_idmap_shutdown(net);
>>   out_idmap_error:
>>   	nfsd_export_shutdown(net);
>> @@ -1486,7 +1482,6 @@ static __net_exit void nfsd_exit_net(struct net *net)
>>   	nfsd_idmap_shutdown(net);
>>   	nfsd_export_shutdown(net);
>>   	nfsd_netns_free_versions(net_generic(net, nfsd_net_id));
>> -	nfsd4_leases_net_shutdown(nn);
>>   }
>>   
>>   static struct pernet_operations nfsd_net_ops = {
>> diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
>> index 93b42ef9ed91..fa0144a74267 100644
>> --- a/fs/nfsd/nfsd.h
>> +++ b/fs/nfsd/nfsd.h
>> @@ -504,8 +504,7 @@ extern void unregister_cld_notifier(void);
>>   extern void nfsd4_ssc_init_umount_work(struct nfsd_net *nn);
>>   #endif
>>   
>> -extern int nfsd4_init_leases_net(struct nfsd_net *nn);
>> -extern void nfsd4_leases_net_shutdown(struct nfsd_net *nn);
>> +extern void nfsd4_init_leases_net(struct nfsd_net *nn);
>>   
>>   #else /* CONFIG_NFSD_V4 */
>>   static inline int nfsd4_is_junction(struct dentry *dentry)
>> @@ -513,8 +512,7 @@ static inline int nfsd4_is_junction(struct dentry *dentry)
>>   	return 0;
>>   }
>>   
>> -static inline int nfsd4_init_leases_net(struct nfsd_net *nn) { return 0; };
>> -static inline void nfsd4_leases_net_shutdown(struct nfsd_net *nn) {};
>> +static inline void nfsd4_init_leases_net(struct nfsd_net *nn) { };
>>   
>>   #define register_cld_notifier() 0
>>   #define unregister_cld_notifier() do { } while(0)
>
>
> Patch looks reasonable. It might be good to also move the reply cache
> init in a similar way (as you pointed out in the patch description).

Thank you for reviewing.

-Dai

>
> Reviewed-by: Jeff Layton <jlayton@kernel.org>