Received-SPF: pass (google.com: domain of linux-nfs+bounces-2009-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Sat, 17 Feb 2024 16:09:57 -0500
From: Chuck Lever <chuck.lever@oracle.com>
To: Dai Ngo <dai.ngo@oracle.com>
Cc: jlayton@kernel.org, linux-nfs@vger.kernel.org
Subject: Re: [PATCH 1/1] NFSD: send OP_CB_RECALL_ANY to clients when number
 of delegations reach a threshold
Message-ID: <ZdEgpStNxUc94j01@klimt.1015granger.net>
References: <1708192859-25002-1-git-send-email-dai.ngo@oracle.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1708192859-25002-1-git-send-email-dai.ngo@oracle.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?s0oiuWuQIRq9NI3XeqGcH9LkrUWarDpcpzxeDAscHECwNrxiUxFBOFB3tuHL?=
 =?us-ascii?Q?BneO3crr+P93Euz1M0m9jhNjQXlmQsh1OOkd6SIhJ3MjsJd4fFdZjTFAOxL4?=
 =?us-ascii?Q?muziU6PTKollXGqjeHB+g5G5QG9OtGh4N03UlqJMMmg4wdqfqzxfMmHpJJ9j?=
 =?us-ascii?Q?1m/Haa4aMhKHLZ+OJ2SlX8Fth09UjiWSnFA1/XHHRVKiObBUZF7KjIrwSNDo?=
 =?us-ascii?Q?HxL/ewUrm+siCYFNmYlvWn+embFj5iIvoWXzYCoS0t457k37NFO4DhRlL/ZN?=
 =?us-ascii?Q?2kksxRH4pRFbTHzHtuSCriFpmv1fvepcXoAILn4xj9r/zeskx0gEW3mQtQFR?=
 =?us-ascii?Q?n/DRSc5QBtD4tZI1i2x6OO04kEyvYB2wtU8lXfqNg2rIL+jljsOO4dmj6pMH?=
 =?us-ascii?Q?kqB314mky0HXYatC6YxWRs+Zg88dmtAPfH+lG4OcvLbqqZbYbxHesoBt9oVX?=
 =?us-ascii?Q?GxgFkB6Jmu0nYgBXhVHr58rcAuLPaKO5zVMiV/6BEZ1crESWtJt7mLGGb1Zx?=
 =?us-ascii?Q?S8iXaitXsJadgSXmwjzw+IYUXHjwxBgaANFLNfaPE93jaDR+pcutXs/OEwWh?=
 =?us-ascii?Q?1Kg3MNF6zF1DXzpiZME9snhXaXAYuaUCbmKGAZq6ijTvRzy48cJJ5aGZO4iX?=
 =?us-ascii?Q?WBskdM43CfF6D8bHsh928O+Ly7UYowDctlsZWcGVg4lrPs8Pq0Z2b54V6Q0Q?=
 =?us-ascii?Q?OOaD1ALflcuPq0Gq9rJdYwsnOh61gOB0veD+jy6TBUGqpSUIdFYmi9fgV3ts?=
 =?us-ascii?Q?GpxHzD4kwySG2TmfjtfJTVquAYZi0+nf+a5EL33BoX4OxKj5pOqXYgs0jFQa?=
 =?us-ascii?Q?TnzHQwkrXp42kDYxyWd3vg/p1pIvQjC15MQMG4O8+kUy0d2So2eU7Yf2D2gu?=
 =?us-ascii?Q?jradKxtQD1Xa5sQv2BLPB8CcR+3O3pQBrbvguW9aIskTJQHAUo+I1ngcr8QQ?=
 =?us-ascii?Q?5PezDMSPgPN9A0lXkRybJH0jIVZ2h/8OtRzXBEgIMqRtwLFiPqlGZzd7V9GZ?=
 =?us-ascii?Q?M59u8KQjiiXBlrdYgFMzHBK6cS0Jz0RGGAAe6Vi3DiX82lQcP0yXSeZeHhk0?=
 =?us-ascii?Q?0ugBWc30N9pSeisyltAyGHa7Rs6D1am2U1uqE9m6fAA/WyT0zw+w5XLp1GPF?=
 =?us-ascii?Q?SyJEg4KtNbN6DoYs+0+zRAYFtHRkPn7cTOvXCgyqbbtCOX2HrQG2Nm/IjxrQ?=
 =?us-ascii?Q?24972RzaMR9cki37smo7CVu5PTUjq862g4iCt4suDVt/QVnZQV+JlMGVOZ3r?=
 =?us-ascii?Q?AA/CSP2hs9BYGHxjpE8sUSD2BGU2VEomS5SaMxk/2Pz8DTedBfoOE3K9vBzK?=
 =?us-ascii?Q?cIAqL2YWJf/VmC8JIDhSxklf0yWe+5bUQsVv5wBXma1dxTMr1Zi5tFYGwIoj?=
 =?us-ascii?Q?IwnwDhbMB021e0QvM3g5j0dDRMb3upjTsGe0ffnlfkhNMfnWuuE+xqyrYh9A?=
 =?us-ascii?Q?alK/bfyIplBD0q/m1YjiCEF6FOsNdC71piB8YZyxExhAYdG05Yg2cmFCJ/kD?=
 =?us-ascii?Q?fPq++FMvjeMiT7NATJcBf+YTUSc1e4fdHZHrDpvua5jywvHFmsfbwGMCP/h4?=
 =?us-ascii?Q?z295URIB4ODvY+0UcAxtTIDfRZOSL6ppPAoqSPXswjY1sCBE6bNeKy/7TSFF?=
 =?us-ascii?Q?4w=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	EH3VfGLWEuijYTTGOiVjTbtvz9xtGrbABUVcubZl/Olh6EVwTW72gyqmczextXm+Ro5SRWjYrWImoK9/vhTh+NE0gToyCjiL8UXr7a1jqJgM4EzwG7VZZbhskOrb0aHTfrqdGUk5zfRxP1ul9epp0ehEM88o7wr9jXBMx0Dqg1KP3WeNqmyto1wt0bLCGci2HV4L6AGtq8USAt4IvrcDa2WEit6JPH5evMPMnSblghIXV/aKW6Gk55z+Sd+bBSCy37f5pARd+ecyJITa4SPKBeNHHU/s8ZPb35twG+dqJg0PxMfLwP7CNTbVq0/3vpIzElIanl+3LjmZbgYkyIVUDNJ9zEuO0LwYPTwwXxpmelMrlpssI1igykEmgqlFpI6lHuwjRp85piwKBcIs3MwGplA2M2tCoLGgf1/49Wdi0xUmVMG1teCfIYw2eEluGaK1HPlaLb+S8M4jzQIRlh9LMDr/30JCgQCcTq0rTP7iUqZZNREGCLhRjf6SuE0JvNsqwbPFu+PRfqLtJ5hhlrMBO1w5Iw2B8LurIF/uDChK4u45Rwp0uE+tdo6l9+wdUXOzGnnQy9aNPf4F2cLs7h4DpJJ8to7Gfop19358sMK1lpE=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 694103fe-26f3-4e93-7b9d-08dc2ffcd0f9
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Feb 2024 21:10:06.4931
 (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: OFo7bdfPrPqCeq5Oehoviakf+CAkVm8egmNXUcq9DUFxEmgjBI4r5bNZXA/d4494tbS94o/f8/FazOKJ79urmA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BLAPR10MB5059
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-02-17_20,2024-02-16_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 phishscore=0
 adultscore=0 mlxlogscore=999 mlxscore=0 bulkscore=0 malwarescore=0
 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311290000 definitions=main-2402170175
X-Proofpoint-GUID: csLInbO7lBN_RWpXaPb_6tdw9-aIH5YW
X-Proofpoint-ORIG-GUID: csLInbO7lBN_RWpXaPb_6tdw9-aIH5YW

On Sat, Feb 17, 2024 at 10:00:59AM -0800, Dai Ngo wrote:
> When the number of granted delegation becomes large, there are some
> undesire effects happen on the NFS server. Besides the consumption
> of system resources, the number of entries on the linked lists of the
> file cache can grow significantly.
> 
> When this condition happens, the NFS performace grounds to a halt as
> reported here [1].

That was a v5.15.131 kernel. The LRU problems were addressed in
v6.2. This doesn't seem like a clean rationale for adding this
reaper behavior in, say, v6.9.


> This patch attempts to alleviate this problem by asking the clients to
> voluntarily return any unused delegations when the number of delegation
> reaches 3/4 of the max_delegations by sending OP_CB_RECALL_ANY to all
> clients that hold delegations.

I don't have a strong sense of how big max_delegations can get. Is
there evidence that CB_RECALL_ANY has enough impact, reliably, to
reduce the size of the filecache?

More below.


> [1] https://lore.kernel.org/all/PH0PR14MB5493F59229B802B871407F9CAA442@PH0PR14MB5493.namprd14.prod.outlook.com
> 
> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
> ---
>  fs/nfsd/nfs4state.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index fdc95bfbfbb6..5fb83853533f 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -130,6 +130,7 @@ static const struct nfsd4_callback_ops nfsd4_cb_notify_lock_ops;
>  static const struct nfsd4_callback_ops nfsd4_cb_getattr_ops;
>  
>  static struct workqueue_struct *laundry_wq;
> +static void deleg_reaper(struct nfsd_net *nn);
>  
>  int nfsd4_create_laundry_wq(void)
>  {
> @@ -696,6 +697,9 @@ static struct nfsd_file *find_any_file_locked(struct nfs4_file *f)
>  static atomic_long_t num_delegations;
>  unsigned long max_delegations;
>  
> +/* threshold to trigger deleg_reaper */
> +static unsigned long delegations_soft_limit;
> +
>  /*
>   * Open owner state (share locks)
>   */
> @@ -6466,6 +6470,7 @@ nfs4_laundromat(struct nfsd_net *nn)
>  	struct nfs4_cpntf_state *cps;
>  	copy_stateid_t *cps_t;
>  	int i;
> +	long n;
>  
>  	if (clients_still_reclaiming(nn)) {
>  		lt.new_timeo = 0;
> @@ -6550,6 +6555,9 @@ nfs4_laundromat(struct nfsd_net *nn)
>  	/* service the server-to-server copy delayed unmount list */
>  	nfsd4_ssc_expire_umount(nn);
>  #endif
> +	n = atomic_long_inc_return(&num_delegations);

I don't think you want to modify the number of delegations here.
atomic_long_read() instead?


> +	if (n > delegations_soft_limit)

This introduces a mixed-sign comparison: n is a long, but
delegations_soft_limit is an unsigned long. I'm always suspicious
about whether an atomic counter can underflow, and then we have
real problems when there are mixed-sign comparisons.

But I'm also wondering if, instead, this logic should look directly
at the length of the filecache LRU.


> +		deleg_reaper(nn);
>  out:
>  	return max_t(time64_t, lt.new_timeo, NFSD_LAUNDROMAT_MINTIMEOUT);
>  }
> @@ -8482,6 +8490,7 @@ set_max_delegations(void)
>  	 * giving a worst case usage of about 6% of memory.
>  	 */
>  	max_delegations = nr_free_buffer_pages() >> (20 - 2 - PAGE_SHIFT);
> +	delegations_soft_limit = (max_delegations / 4) * 3;

I don't see a strong reason to keep delegations_soft_limit as a
separate variable.


>  }
>  
>  static int nfs4_state_create_net(struct net *net)
> -- 
> 2.39.3
> 

-- 
Chuck Lever