Received-SPF: pass (google.com: domain of linux-nfs+bounces-1430-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Thu, 25 Jan 2024 15:29:46 -0500
From: Chuck Lever <chuck.lever@oracle.com>
To: Jeff Layton <jlayton@kernel.org>
Cc: Chuck Lever <cel@kernel.org>, linux-nfs@vger.kernel.org
Subject: Re: [PATCH RFC 02/13] NFSD: Reschedule CB operations when
 backchannel rpc_clnt is shut down
Message-ID: <ZbLEul2xcYFge2ga@tissot.1015granger.net>
References: <170619984210.2833.7173004255003914651.stgit@manet.1015granger.net>
 <170620013252.2833.10156142379669175540.stgit@manet.1015granger.net>
 <736b0c878f228e28e0ae18974efbddca17c1919f.camel@kernel.org>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <736b0c878f228e28e0ae18974efbddca17c1919f.camel@kernel.org>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?PND55GiCTCtDw+8FJ10vaNhudw8NTXP/xwMtJzAAmL8i06V2o4JYMP13dl5Y?=
 =?us-ascii?Q?pFca071mJtC3mw4z9IqtvO1DKnJKdN3jcYSRxYDkbe2woa2KW1x0eZnwlh08?=
 =?us-ascii?Q?W085f8lmo/PWAD6rhG3vZFMO3Kg3Eg8Zk8xfyU2rh1z0RDM9Jmj06iZM9w6K?=
 =?us-ascii?Q?nYzjDHF9G0CzNdsSLS64UC1Txw1w16feo9nw7f2IAAo8EsbYRLY9F7dYx+ga?=
 =?us-ascii?Q?T5NjHzbE0QHkNKrCDCvAJSIxaNl7QspUQyBBakqBXnPmJ5IE5ZLtPjSqlB2w?=
 =?us-ascii?Q?JGOXJ+/uLgdPRqi4qyUQvtOG70xGpcMTWkiu6oSWcFMVG5lzriCTXBWEh0yP?=
 =?us-ascii?Q?6QDnJVgHjWXLqoEnV97xxOp+8UaOMC/Z/Wi62cDXgw+JVLYeVtIs2eEseYUQ?=
 =?us-ascii?Q?FRaZvBJryeWzuE94bImKYCPvNU6xdTbiU6E/LNciTwKBidVpwbHshNqdXyTx?=
 =?us-ascii?Q?KMWcrE+0A3l3IhmYcymAIiG7dYdoLzME6kuvD+1rHsQf2QDU1MUrnAfSD/Hx?=
 =?us-ascii?Q?Qtj/m8F0XFxyilSg6lpoZctFoEQtd5NocIYWYDFx1pT5+PU0GF0WkhSoazve?=
 =?us-ascii?Q?mBmfTKvNQCnlmECmUaIJnJyA0lqH0S0EAo06btSg+4AbqOsFshQOg27MpAKq?=
 =?us-ascii?Q?4gO4Ci/QPDyoz9f9HDse3Jdhk7sRpr7dBH8oaQRBHPdpoy5QpeumxI2H28ce?=
 =?us-ascii?Q?uHTDDv+mqPJqzy03zxQ4qyg4hd7hOEgoLkZE4cCQQz+OZrunJbV28Pdr0pcd?=
 =?us-ascii?Q?9IExLYj6cBAIwAHLDYNJU6lBveKHdJqdRdTDVRJkTCNp4V2IOixe9mG/ZdtY?=
 =?us-ascii?Q?7+BXiUj7in86jMEDmyVs9y3EFaTmVVuNPer2CdY2Oy/Pj10jO/5pi355Ikzh?=
 =?us-ascii?Q?/yqPmKcpJOhJFbyt1OrnWI/+7vb0NdHZxUQ4CRy87SKsrsbRaj/jyN64fCdi?=
 =?us-ascii?Q?HtpAAD78U4WUia2RDrHY5VvYBedtsHVNf0pgaZM24iTBWJi+XHs+zdfzE39g?=
 =?us-ascii?Q?v2iMZU3dyHFZgUQV0cTzlKlMoBnbaYrb+y2gpGuDwRHqUCVQeRH4VbEvJJqm?=
 =?us-ascii?Q?4+1yKsXAnAbp39sMPuyyXVI9xTDIRLZax3/oDWqqaY/kOBtsbbuAcklwhRhk?=
 =?us-ascii?Q?/lO33K54Sq56ZHUf3M1KdDlUNmtNa/NnJFzTeCKGf3FA6vteRC68JorYcvXD?=
 =?us-ascii?Q?sXSnv8hNCbxtCVA6HrcgEfY9QLolVgu8nwiVtoCXDvB7ZQctm+EyqcaComVT?=
 =?us-ascii?Q?c2GtjWBC/HHklYsoM5mHlh8HuiSLyB2JFUjUYrhQL6xPlF+IteAYp9epBt/7?=
 =?us-ascii?Q?r+v18Eh7ry0Ky5Z253XxE1NtPUDrjausIigW+gqhSM/kLcDMhjc3BGC0FhqQ?=
 =?us-ascii?Q?v3Hj1tXyq4YuddvX+Y0AHK4ruC43x+XozFjPMoxL1XPiEybXd1L98dVs3W+M?=
 =?us-ascii?Q?gUMDCGRuMAyf6/HDbk6vQsYkj/Mn/mP/SzRgMKNU9J+XSUwRc7xFXAYrqJ7O?=
 =?us-ascii?Q?PDBzSAYtoAsKtfF36wcEOtyIehTTysx/qjrT19HYZpCPO/vIK/ZM7biAMM76?=
 =?us-ascii?Q?M+V9bdXEi96/MHHHXpWKoAlXRv3buPpXJf1tDnBBFZq1oIuXUQwA+UocLknM?=
 =?us-ascii?Q?Tw=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	/rJL4tbFVx9wJRG8uJm9zGC7cXn7Akcx9MM5f1gBWALg57HC/StqsTOQtcjIQAg7RaGcFk8I4t9wMOONYQgdnVf6l2RWtYPWncQ788hR2QeDjrdWMcAyJLVrWwNh6bmdfQXccUqc4gejhnVK62DXsUtjxaP2xYCrQWeoJSgZwOuvbx6GYiJkv/uX3B3nF0uJOAmVawu+zy5Mh/mm27A50jP8ixAQujCycr8CuPjgBLauZeeYkQ1yC2N/JNC1zqtWzBaSKS3S54Ve2cHQTMnu0fpTvYhi+vZ8u6aKo+BBNajcgyTctdhibYCjKaKICY5L4PMhImMRlo8aTmZ8QMk3Kn0chFBQgSRsF08TWgOT8z9wfahHNOEJq1YwW7MJrK/wq5dtnI8FhegA33+fiax3o3xIG0/p/QHFgS0UJ0uXB+l9yezH3m5HRg6Dh0ksmPcU6tejlCM9OYBNXtPnNpHxiuTH/WihEsqFfGWfdGk/UOWHEPUfzJ35P8ykHoHQTtFyNXm5ybDTmXtk6YVkZZHuxpjX/jb3WHwudMeUN6vEMosnnZCGPjZoUNjUIy4xWOiAHf8EfJTxHZ+4W+bn2INN6qrP3BChyStUT9vGy4kCsTI=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ee6c00ad-e147-4e49-ca25-08dc1de460a9
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Jan 2024 20:29:49.2761
 (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: SdN/wLM2NrMtWwHk/odejyIQWVR5aCkeOiCa8Upzxs4eAaXWsEcfYel60aaDcbgVPA4XygtXHewVjGw89fWviA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR10MB6271
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-01-25_12,2024-01-25_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 mlxlogscore=999
 malwarescore=0 mlxscore=0 phishscore=0 bulkscore=0 suspectscore=0
 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311290000 definitions=main-2401250147
X-Proofpoint-ORIG-GUID: WCWvIZU1-fDaKFs-Q0I_I8xajBKX1U9s
X-Proofpoint-GUID: WCWvIZU1-fDaKFs-Q0I_I8xajBKX1U9s

On Thu, Jan 25, 2024 at 03:19:41PM -0500, Jeff Layton wrote:
> On Thu, 2024-01-25 at 11:28 -0500, Chuck Lever wrote:
> > From: Chuck Lever <chuck.lever@oracle.com>
> > 
> > As part of managing a client disconnect, NFSD closes down and
> > replaces the backchannel rpc_clnt.
> > 
> > If a callback operation is pending when the backchannel rpc_clnt is
> > shut down, currently nfsd4_run_cb_work() just discards that
> > callback. But there are multiple cases to deal with here:
> > 
> >  o The client's lease is getting destroyed. Throw the CB away.
> > 
> >  o The client disconnected. It might be forcing a retransmit of
> >    CB operations, or it could have disconnected for other reasons.
> >    Reschedule the CB so it is retransmitted when the client
> >    reconnects.
> > 
> > Since callback operations can now be rescheduled, ensure that
> > cb_ops->prepare can be called only once by moving the
> > cb_ops->prepare paragraph down to just before the rpc_call_async()
> > call.
> > 
> > Fixes: 2bbfed98a4d8 ("nfsd: Fix races between nfsd4_cb_release() and nfsd4_shutdown_callback()")
> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > ---
> >  fs/nfsd/nfs4callback.c |   26 +++++++++++++++++---------
> >  1 file changed, 17 insertions(+), 9 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> > index 43b0a34a5d5b..b2844abcb51f 100644
> > --- a/fs/nfsd/nfs4callback.c
> > +++ b/fs/nfsd/nfs4callback.c
> > @@ -1375,20 +1375,22 @@ nfsd4_run_cb_work(struct work_struct *work)
> >  	struct rpc_clnt *clnt;
> >  	int flags;
> >  
> > -	if (cb->cb_need_restart) {
> > -		cb->cb_need_restart = false;
> > -	} else {
> > -		if (cb->cb_ops && cb->cb_ops->prepare)
> > -			cb->cb_ops->prepare(cb);
> > -	}
> > -
> >  	if (clp->cl_flags & NFSD4_CLIENT_CB_FLAG_MASK)
> >  		nfsd4_process_cb_update(cb);
> >  
> >  	clnt = clp->cl_cb_client;
> >  	if (!clnt) {
> > -		/* Callback channel broken, or client killed; give up: */
> > -		nfsd41_destroy_cb(cb);
> > +		if (test_bit(NFSD4_CLIENT_CB_KILL, &clp->cl_flags))
> > +			nfsd41_destroy_cb(cb);
> > +		else {
> > +			/*
> > +			 * XXX: Ideally, we would wait for the client to
> > +			 *	reconnect, but I haven't figured out how
> > +			 *	to do that yet.
> > +			 */
> > +			msleep(30);
> > +			nfsd4_queue_cb(cb);
> 
> It would probably be better to just queue the cb as delayed_work here,
> so you don't squat on the workqueue thread.

You found my placeholder :-)


> That'll mean changing
> cb_work to struct delayed_work, but that should be NBD.

I've looked at that. I wanted to be sure, before going that route,
that there is no obvious way to implement a "wait for client to
reconnect". msleep (or delayed_work) is basically a slow busy wait.


> > +		}
> >  		return;
> >  	}
> >  
> > @@ -1401,6 +1403,12 @@ nfsd4_run_cb_work(struct work_struct *work)
> >  		return;
> >  	}
> >  
> > +	if (cb->cb_need_restart) {
> > +		cb->cb_need_restart = false;
> > +	} else {
> > +		if (cb->cb_ops && cb->cb_ops->prepare)
> > +			cb->cb_ops->prepare(cb);
> > +	}
> >  	cb->cb_msg.rpc_cred = clp->cl_cb_cred;
> >  	flags = clp->cl_minorversion ? RPC_TASK_NOCONNECT : RPC_TASK_SOFTCONN;
> >  	rpc_call_async(clnt, &cb->cb_msg, RPC_TASK_SOFT | flags,
> > 
> > 
> > 
> 
> -- 
> Jeff Layton <jlayton@kernel.org>

-- 
Chuck Lever