Received-SPF: pass (google.com: domain of linux-nfs+bounces-2967-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: Tue, 23 Apr 2024 14:08:07 -0400
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 v2 1/3] NFSD: mark cl_cb_state as NFSD4_CB_DOWN if
 cl_cb_client is NULL
Message-ID: <Zif5B3fT5JA8GvEt@tissot.1015granger.net>
References: <1713841953-19594-1-git-send-email-dai.ngo@oracle.com>
 <1713841953-19594-2-git-send-email-dai.ngo@oracle.com>
 <Zie6ochDV9VjumK4@tissot.1015granger.net>
 <d18e8b50-2eb1-419d-a937-9314ea39e08e@oracle.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <d18e8b50-2eb1-419d-a937-9314ea39e08e@oracle.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?4lbasnDQek8HPrjhzu0NEbnPLGxO5DzQr7/asHaiIlmymEAYglYiQMUrKtkq?=
 =?us-ascii?Q?YhgwxCAqlvMi6n/IBkjLj/fHL4FTfhpJcTsBR66Q7WVwVOe/9ofOGAGdy0VF?=
 =?us-ascii?Q?L7mhESVthrC73+VKLKMy2/Kxa3SJa8Ukl+YVSR2cZXpyM17YcWkpKXwZfiJ9?=
 =?us-ascii?Q?f9UQ89nCZWUu2N0CaZOpSz2GwRFEoGgAx7MeeNHmCnf8MhRms/VWyBpj1Xhr?=
 =?us-ascii?Q?tLEb/uZFIdbGmBwEz/qThAAaN56g2D00sD1bikU/sOg+f+VZtF8sF6AhRxLK?=
 =?us-ascii?Q?sElg1pwj7XwxE8vp37XEguQf55px/1sE+a7NYmwYcBgFoa4s3OR93wQtuNIQ?=
 =?us-ascii?Q?Y5dRlkVWR32YHVX0rR62NN9rn3li/P/HYDKtbWpkiGy737Z+TXAXlZX013Wz?=
 =?us-ascii?Q?ZRBZr/Y3WjWxkJMg74BPvpOLaAtPVCWRZeHN5N89bMqmOEP8J42W3btt87xB?=
 =?us-ascii?Q?ON1tHWjKRR4vkcjVLXLFoosulnjroNfbEoTuFhk4CG4mJXE+vpcqO02xrt6l?=
 =?us-ascii?Q?azuk1ic/yFzYsWKnE4eukDwmS8mtreisH/zcPLJ2KMSAVng7leOAnhMR/osw?=
 =?us-ascii?Q?eyyQA/kBoO0VTvL1CmytqZ1tgzNB6H8yFXGkaCfc26aN4/6zYTOZ30P98vLZ?=
 =?us-ascii?Q?mJNg6d3SZ3eYS4AfBQqbeJXLyOIW98UrgmxLzbT143nFPy1EvqGV4gIkHi60?=
 =?us-ascii?Q?98Z6JRoCy1NcAdqTJ9QwR1YWizihQu0aMSaL0GB62Y5kX+D6FnDFQ06qjYtK?=
 =?us-ascii?Q?nvnBldxchCbqIMRg2TXqmYqy/JJ3n6rM5uAMOsTbI/kcdrn1xI7fIX77WUOR?=
 =?us-ascii?Q?aZhrqJCbfXr7zjUXIXaZbqJi15cvQj3vQat8IcpH3gveFyGkxVuODI9oHln7?=
 =?us-ascii?Q?pAsf19kPqpwMPDIi0qj7wPrLr3ZGj527Lj4kVy/53+lbhnKLG322Ia1cQR7u?=
 =?us-ascii?Q?n1QAdu671SgLuZ+nqG6o/rI0RPHxjDAaEJk9kLE9omPJ1hlj9cwPn+fkPfVj?=
 =?us-ascii?Q?/mEpHbfZ7tIjmFFAuJtjl+9I3VBcJMSZXQZURgtLJ0tzjc494W+0lBTEdvo3?=
 =?us-ascii?Q?8Tone/+8pwkiGu6Xg4E4V7KTjNPONke/aiMC+MFUxsbYoALAragNtPIo7epx?=
 =?us-ascii?Q?lbDf0cAO3/9TH8h33Ii6xMlqzrsUz5Kc1eT/LarwghMKt6dZMBcNB93DmwTy?=
 =?us-ascii?Q?prtZ7vQXV1EwfH8nFvHtcCMZUztXMjIJcdejpDvpNKgcEwVHWa2eGmPBphVu?=
 =?us-ascii?Q?yLLUUhJLOd7BDL+MB52CYtRhLGdQWNbt6KujXIFq5mo/EP5OQWIbKyFyV6rH?=
 =?us-ascii?Q?4E4GJt9xk931wMYvtB5m/Wf8B/2el1HveRxtXKIICQ8puVhN29EweQotg99o?=
 =?us-ascii?Q?PAZFi/rRBwIaTc9+v2sVnHp5mP7K1QEEBYyhkkSNFs2E3rCrCac8D/45Yk10?=
 =?us-ascii?Q?gIFGj+tdo+0LEARxTHrWQJLpwU7yYeB9UV6AiF3sqiRNeueXCS+1dfyJ5sq5?=
 =?us-ascii?Q?FLEKIvXYhAyjfsUvpNstYHC0OV2AQs5AVVA3vIPtYRKRp54LCYk4i9uZo45a?=
 =?us-ascii?Q?t7BYUjQdM7YWWRLhT2RmZ4jtHD//LV3dUlIRNiyWjukaBpegEUkxTkSqAsXu?=
 =?us-ascii?Q?6w=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	6wkLq0V4cCuBsPtyPiJNlzG13C2Ol00ok4Gw5wBk32SCBbJDCjfgMaCuvQkWPo2Sv1fKoYSlvjnvqQr8lFkimWwwoGu4E1vHgoIiyS0rz/oZQuMra8ZOg4yD3CvP89JoW4Lu8LyS8jKJN4zu0P8RpFVmR+4wVqazxFxiP9qDK7EBw5Raxt1UeBBHUNvQxE+2aY2F+bvlgZ1F8lyaeXSRQ31SvNwHBRg9iZGRaZgu/hCIx+JJKP/SwST5jtWEW0gn49tbZn4/11rSM4sULuyY6xKSEA3GWeWRDWGlCi7uMmVwoghFqUHcZbYmEn0H715W18Jk+9QtTy5hTMTKvYAlwD96L4E+0E8dQpMWeU4wFit1+wZC07MojawEUrZmNidIyQygRggXW3HkGLZZemFJxJJtKeVAzv6B/HVFaP9DzAdBYHenDJoHCnovohJ8r3wHpjPZ1FjLtAxZhvGcrFSDDeGLKzmMVb9VHcHSAHF4CCsK2yAZhSw9xj6WbH4UBf4DlTBO02SdRxAunlljZWxRW9GGXOj7DaWvdlYpaEnfxTiWAP4feen/TcrE2r4Kv+JTxFCa1wL4zk2ENqWcwkO5x+Ie5G0wAzM4vXG7fjAYLHI=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 282b7c46-dcfc-426e-e003-08dc63c05544
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Apr 2024 18:08:09.6760
 (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: 2aLO1PQgw1QZnxN4oLykCRQPdWG2M64H8NS4YrYtaO48c+NQIykipqjszWoMbxJo5a7MLqLMXJxX8HQsd7il9w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM3PR10MB7945
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1011,Hydra:6.0.650,FMLib:17.11.176.26
 definitions=2024-04-23_14,2024-04-23_02,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 mlxscore=0 spamscore=0
 bulkscore=0 suspectscore=0 mlxlogscore=999 adultscore=0 phishscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2404010000
 definitions=main-2404230041
X-Proofpoint-GUID: 60wP-q694zIW0vLj_K0ExPZ157EMc7LS
X-Proofpoint-ORIG-GUID: 60wP-q694zIW0vLj_K0ExPZ157EMc7LS

On Tue, Apr 23, 2024 at 10:49:25AM -0700, Dai Ngo wrote:
> 
> On 4/23/24 6:41 AM, Chuck Lever wrote:
> > On Mon, Apr 22, 2024 at 08:12:31PM -0700, Dai Ngo wrote:
> > > In nfsd4_run_cb_work if the rpc_clnt for the back channel is no longer
> > > exists, the callback state in nfs4_client should be marked as NFSD4_CB_DOWN
> > > so the server can notify the client to establish a new back channel
> > > connection.
> > > 
> > > Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
> > > ---
> > >   fs/nfsd/nfs4callback.c | 9 +++++++--
> > >   1 file changed, 7 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> > > index cf87ace7a1b0..f8bb5ff2e9ac 100644
> > > --- a/fs/nfsd/nfs4callback.c
> > > +++ b/fs/nfsd/nfs4callback.c
> > > @@ -1491,9 +1491,14 @@ nfsd4_run_cb_work(struct work_struct *work)
> > >   	clnt = clp->cl_cb_client;
> > >   	if (!clnt) {
> > > -		if (test_bit(NFSD4_CLIENT_CB_KILL, &clp->cl_flags))
> > > +		if (test_bit(NFSD4_CLIENT_CB_KILL, &clp->cl_flags)) {
> > >   			nfsd41_destroy_cb(cb);
> > > -		else {
> > > +			clear_bit(NFSD4_CLIENT_CB_KILL, &clp->cl_flags);
> > > +
> > > +			/* let client knows BC is down when it reconnects */
> > > +			clear_bit(NFSD4_CLIENT_CB_UPDATE, &clp->cl_flags);
> > > +			nfsd4_mark_cb_down(clp);
> > > +		} else {
> > >   			/*
> > >   			 * XXX: Ideally, we could wait for the client to
> > >   			 *	reconnect, but I haven't figured out how
> > NFSD4_CLIENT_CB_KILL is for when the lease is getting expunged. It's
> > not supposed to be used when only the transport is closed.
> 
> The reason NFSD4_CLIENT_CB_KILL needs to be set when the transport is
> closed is because of commit c1ccfcf1a9bf3.
> 
> When the transport is closed, nfsd4_conn_lost is called which then calls
> nfsd4_probe_callback to set NFSD4_CLIENT_CB_UPDATE and schedule cl_cb_null
> work to activate the callback worker (nfsd4_run_cb_work) to do the update.
> 
> Callback worker calls nfsd4_process_cb_update to do rpc_shutdown_client
> then clear cl_cb_client.
> 
> When nfsd4_process_cb_update returns to nfsd4_run_cb_work, if cl_cb_client
> is NULL and NFSD4_CLIENT_CB_KILL not set then it re-queues the callback,
> causing an infinite loop.

That's the way it is supposed to work today. The callback is
re-queued until the client reconnects, at which point the loop is
broken.


> > Thus, shouldn't you mark_cb_down in this arm, instead?
> 
> I'm not clear what you mean here, the callback worker calls
> nfsd4_mark_cb_down after destroying the callback.

No, I mean in the re-queue case.


> > Even so, isn't the
> > backchannel already marked down when we get here?
> 
> No, according to my testing. Without marking the back channel down the
> client does not re-establish the back channel when it reconnects.

I didn't expect that closing the transport on the server side would
need any changes in fs/nfsd/nfs4callback.c. Let me get the
backchannel retransmit behavior sorted first. I'm still working on
setting up a test rig here.


-- 
Chuck Lever