Received-SPF: pass (google.com: domain of linux-nfs+bounces-2677-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Fri, 5 Apr 2024 14:07:24 -0400
From: Chuck Lever <chuck.lever@oracle.com>
To: Jeff Layton <jlayton@kernel.org>
Cc: Neil Brown <neilb@suse.de>, Olga Kornievskaia <kolga@netapp.com>,
        Dai Ngo <Dai.Ngo@oracle.com>, Tom Talpey <tom@talpey.com>,
        Vladimir Benes <vbenes@redhat.com>, linux-nfs@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] nfsd: hold a lighter-weight client reference over
 CB_RECALL_ANY
Message-ID: <ZhA93BQSxkJqmqaw@tissot.1015granger.net>
References: <20240405-rhel-31513-v2-1-b0f6c10be929@kernel.org>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240405-rhel-31513-v2-1-b0f6c10be929@kernel.org>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?/2clvrE89kZSROCqRgwOjHqAYRsJL28FsGyJLeWrN2qneIppgaXfqMqwmfVb?=
 =?us-ascii?Q?/0E6BTSwb9AiQs8JDFcPNmjMwTu8l7b7DV9GpyhCGaFQIS729yEEqFIxRwbu?=
 =?us-ascii?Q?DTOPol77bZ9CqE/JPkLrq5D5DfnF6aBLHHqAyllUhftQfyCU61pey2q/1m69?=
 =?us-ascii?Q?mVYrCW6eUi/ln2LWEDhDWDx0NAQrb976H1gvpa04c+CzNgpUk9yqk3vlszXa?=
 =?us-ascii?Q?GsPm9VODsg434KNK/MK5MhL9ha8y586vtK6jWW0/JEwTqffhTDvBwz1/CShU?=
 =?us-ascii?Q?UMZtqHKt/y6qxZBRflNeLHqxvKzIbJ2AE6ljF0iKDKNYvKurug4VcWro4KQu?=
 =?us-ascii?Q?pAgAJOUIuUWghLiH5TnqmYsyuc1FGk4TWUDYOz1OAz7ZG2TDg/x5kqcoQaOX?=
 =?us-ascii?Q?l3dIldioazndtoV+Ox/HAVwj7VZz2+Ad62vqkqgVt3Nb3katng+vODN5e3ik?=
 =?us-ascii?Q?booFYtvSd823xHtfyUqg/ogfkER375aeUFJjuChmtNPYB6pRxKLS4OmlYvR0?=
 =?us-ascii?Q?bQ5z//fytSlOajiRtIQsR0yOIQaFkmKRfpHBaNngFOxBpM+LYcd58JH3kTXy?=
 =?us-ascii?Q?brsM7VzHpVc+nRJ63ToNfd1nkBjymR1Q5sG4+sK5RWowH4KV+ojFroGmAsDz?=
 =?us-ascii?Q?NCt0KNJdvwmgV6wLVoWg5Ym3iYZaNyflwmeJ6JURX7vpAyG2M16u79jEPnYm?=
 =?us-ascii?Q?p1NRFxHlRCEGlQVSKl2BfLbRRvIQxiIogn6qsksii6fJ2vQ2cG2iQCOybDbZ?=
 =?us-ascii?Q?nZMknL0KEfRbULviCJMFiRxtjaBMdacs36U7d73t4GSyIvWi51LAKSLHbXGr?=
 =?us-ascii?Q?s+HP14oaJs5eiZhpIUgBpEF0McKyqEoM1W74QUC/sJajCi2nuE/5Cq3UTeML?=
 =?us-ascii?Q?HPPEW4C6fzU8Gc2LdKU7Z2G2s/3BcFpzCi/v7B6d/sa5M1/EeYzglCS9GyOW?=
 =?us-ascii?Q?eh7oq9xUbnUBmxtLXEkH+zY37ARSDv/soliE4TL2eMZVugw2UDNcRTbtgAd7?=
 =?us-ascii?Q?ZoXq9PeoJ30ASJ41KtQ9+0x7wAH39RQXsmKD0u7Srl03rifs3KNCO91W6Hoz?=
 =?us-ascii?Q?U0qVavi5rLM1yPofvJ39epzW3t0gdREmNFtRhxPXY4vVXzmx7HM/OKeizoX/?=
 =?us-ascii?Q?nEU91tTs7EyVsK8cDzMw2QTtD/Qm1+WQ5cy5Tyygoqzi9vVGfD7z11vavz0W?=
 =?us-ascii?Q?jJWJy+H9A7M1jYro2l41ysjyHFWZiqwM+AZA+qLtnAwXdvPKh9DfC3+NTTEy?=
 =?us-ascii?Q?OpgHgVWoYqEMbHRqpx22jl2mLkPkwqTWmbcOoDtbVK6rc6D0UHiQwGsIeVGH?=
 =?us-ascii?Q?7Trio53OpzuLTANXMOf4itNr6xJJ3rB5PjfR46vEDz9ghysLreMSWsTtY0X3?=
 =?us-ascii?Q?rbjhAP0yCJOiod3VgiY7KHHjUpuf6EgSk0ONuhZ+1+EoJ7TvcHgCaQK1be3x?=
 =?us-ascii?Q?0fCV5nrfOKpX2UzM/9abAV4V/pp8r2ULry9EUXLoDUMpSvcS1XjcQ5Upif/l?=
 =?us-ascii?Q?4LU/SPVdRVSyeAK8/eXqstGGwUCIvfZMvNS4rPGh5I0hvkEuHDNwe4tjtsmb?=
 =?us-ascii?Q?fuk5JKwY3H1l2RbEIR46GZeuPBqrI1HvXhXHdn3eESfcVUiOZrMXKexItjFq?=
 =?us-ascii?Q?RQ=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	UflHXB3uqAZGBkvdMgXWX9x3uW3bXlV160fZvNgl5GuhRZ0lrAiYo3VoZQxzG2yATKdk95A9u44TC5iPbCbZAxpEPPM1mrlbCKoRpf68jXQZkMA2Q6OsggbUMkYYx+1UHII80S/UuYDE7ewpF+zDfGot2Hq2sdnQnEdzAwh4adgMJ91DRKgwLks26pMei1jaZOxbHWtefyZiUBXt6gD/XRmUnMYz5TRCJtEvX+pwoRJCd0Asfl3fy0neSTvrLA3bGKS2Pp5TwF/6y/lCaaACeHR4PYQJACjavn+7Xx4OKfgYryEa/65L/lddjTKVTug3/P/ve3lFubGq+/jYtRdQ/cnWTmYyTP33lns8hX0L1QIJA2s9K7vUBfK2LUWtjocFW36v06lmFkQp3J4nTxsQEJgjVUfyHrHW/OCOE7aRj+KYJvRrJbfn0Qxto9IM1Ln5BDZmmls7sppKT58t3hm+qXv0kPfZ0t+W6mlAlLW0/tUJy/SG+9ZUeYWewsThpVyQYC/mRzlQ6gpGWEF61PrA8L3eH8xAbQpeoRUcbQEetcU7HrehzrNJvMTFa10RydBaY2B2F1+qfg8eVpp0IRh5ncZ8xVgJtzJU32Owl9j4C7Y=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c72358ce-75a7-4601-4067-08dc559b4097
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Apr 2024 18:07:27.2824
 (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: hPHNiOVyKAE6uxx7nPupd2A4Cd9SzX6qXOIw27LzELGCfUx/i+2THAXowL2T9aLsTRunvKHrhDv9mOMby5F45g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR10MB4847
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-04-05_19,2024-04-05_02,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 spamscore=0 bulkscore=0
 mlxscore=0 phishscore=0 malwarescore=0 adultscore=0 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2404010000
 definitions=main-2404050127
X-Proofpoint-GUID: XIW3FjVYdEN9D4S25k4EY6DLp2kl1hcU
X-Proofpoint-ORIG-GUID: XIW3FjVYdEN9D4S25k4EY6DLp2kl1hcU

On Fri, Apr 05, 2024 at 01:56:18PM -0400, Jeff Layton wrote:
> Currently the CB_RECALL_ANY job takes a cl_rpc_users reference to the
> client. While a callback job is technically an RPC that counter is
> really more for client-driven RPCs, and this has the effect of
> preventing the client from being unhashed until the callback completes.
> 
> If nfsd decides to send a CB_RECALL_ANY just as the client reboots, we
> can end up in a situation where the callback can't complete on the (now
> dead) callback channel, but the new client can't connect because the old
> client can't be unhashed. This usually manifests as a NFS4ERR_DELAY
> return on the CREATE_SESSION operation.
> 
> The job is only holding a reference to the client so it can clear a flag
> in the after the RPC completes. Fix this by having CB_RECALL_ANY instead
> hold a reference to the cl_nfsdfs.cl_ref. Typically we only take that
> sort of reference when dealing with the nfsdfs info files, but it should
> work appropriately here to ensure that the nfs4_client doesn't
> disappear.
> 
> Fixes: 44df6f439a17 ("NFSD: add delegation reaper to react to low memory condition")
> Reported-by: Vladimir Benes <vbenes@redhat.com>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>

Applied to nfsd-fixes while waiting for review and testing. Thanks!


> ---
> Changes in v2:
> - Clean up the changelog
> - Add Fixes: tag
> - Use kref_get instead of kref_get_unless_zero
> ---
>  fs/nfsd/nfs4state.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 5fcd93f7cb8c..3cef81e196c6 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3042,12 +3042,9 @@ static void
>  nfsd4_cb_recall_any_release(struct nfsd4_callback *cb)
>  {
>  	struct nfs4_client *clp = cb->cb_clp;
> -	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
>  
> -	spin_lock(&nn->client_lock);
>  	clear_bit(NFSD4_CLIENT_CB_RECALL_ANY, &clp->cl_flags);
> -	put_client_renew_locked(clp);
> -	spin_unlock(&nn->client_lock);
> +	drop_client(clp);
>  }
>  
>  static int
> @@ -6616,7 +6613,7 @@ deleg_reaper(struct nfsd_net *nn)
>  		list_add(&clp->cl_ra_cblist, &cblist);
>  
>  		/* release in nfsd4_cb_recall_any_release */
> -		atomic_inc(&clp->cl_rpc_users);
> +		kref_get(&clp->cl_nfsdfs.cl_ref);
>  		set_bit(NFSD4_CLIENT_CB_RECALL_ANY, &clp->cl_flags);
>  		clp->cl_ra_time = ktime_get_boottime_seconds();
>  	}
> 
> ---
> base-commit: 05258a0a69b3c5d2c003f818702c0a52b6fea861
> change-id: 20240405-rhel-31513-028ab6f14252
> 
> Best regards,
> -- 
> Jeff Layton <jlayton@kernel.org>
> 
> 

-- 
Chuck Lever