Received-SPF: pass (google.com: domain of linux-nfs+bounces-2965-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <d18e8b50-2eb1-419d-a937-9314ea39e08e@oracle.com>
Date: Tue, 23 Apr 2024 10:49:25 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 1/3] NFSD: mark cl_cb_state as NFSD4_CB_DOWN if
 cl_cb_client is NULL
To: Chuck Lever <chuck.lever@oracle.com>
Cc: jlayton@kernel.org, linux-nfs@vger.kernel.org
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>
Content-Language: en-US
From: Dai Ngo <dai.ngo@oracle.com>
In-Reply-To: <Zie6ochDV9VjumK4@tissot.1015granger.net>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?TWNzaHIxSldZb3cyOVRlRDJHZWFJSWw2Q2pCWGdvK0kvdFZGa1R1aWRKRkg0?=
 =?utf-8?B?OFVLclo2emtMZWZkbkVJMTQvK3l5ZVdmN01oU3FSRGo5QW9MMkl1cXk1b0d6?=
 =?utf-8?B?eFlKZ3JYb1EwOHp1b1BUZmU2TVJtb2xET0VaczV4SmJaOWV4NFdzSnlFVEV6?=
 =?utf-8?B?KzBSeXFzUlYvek53M04vM0tNUlpoV1pwRG5qRytBbGtkT3o1VVRKOW5lVEt0?=
 =?utf-8?B?d3V3UnlwM3loRnE0enN4QzIyNUNZcFV3QWFIUnJ6djFlem5RTjdEcnM2c01y?=
 =?utf-8?B?QVVBaWhKM1cxUWtSSHBXZHFCbWM3cGgwL0Y3WVNUalhCZ2xObm96WElqNEpI?=
 =?utf-8?B?LzhJMnJxUWpTK2NiRFJkcWJuSFQwR280NUw5bFFROUZ4OFRxRHdvQVhPcnpv?=
 =?utf-8?B?Zk5JdjdGcERkd2IxRjhOVzJlWUZTL0hWSGJoYVFHNk92WTBwRkhXN01BNXk3?=
 =?utf-8?B?UGhLaXlRYXJ4dDBoQUt2RDlHd0RNaS9xbmRFVkdmcENOWjQvNTR0S0s0czNU?=
 =?utf-8?B?ZHBIY3E0TXM3YXdyWEwwNUk1S0pkUjhub2liOU1xK1Q0ZGdrN05qbnM0cnpu?=
 =?utf-8?B?S01uNVpKT1phU3llMkVkMVhOTmlKVTZUSmVaZTYxMXV4OGcrS0l4cFZUalNi?=
 =?utf-8?B?aVlLdElPT0p4RFU4dnFvWW9iODJCMmhBRUYzelRNZFJSVlh4OXR6bjByZGp6?=
 =?utf-8?B?c1dXVUEyV2RkLzl4UEIyUXdpamhEMWRzV3B1U3VacEtQSjZnREEvNTczQXk5?=
 =?utf-8?B?SjBRRjNzUkNGNEhIUFdBVVdId0xnS2kzcGliOEpyb292ZlpLWjVXNWxXSjk0?=
 =?utf-8?B?SkVNT1Fvb1F4WTNsRFp6REFlZ0JyVjRQajZKb2hKWWw3TkhGVDFRZWhraGlT?=
 =?utf-8?B?UEhNSTg3V1FjWFdFbWswWnI0S1VhUTc3UC9YT1dBdWtzY3A3NXB2S2U0WmVI?=
 =?utf-8?B?eDhZRFV1aSsxVGNSQ3NFdWh3ZlRnWFRQNVR5NC8rb0FqL1V5bmRlRGsvMWtC?=
 =?utf-8?B?YkFGNllQWm1sZjdQd2xRSi8vRFJsSFcvTEFnRlpmNlJwUXBGTmpWak5lQnBo?=
 =?utf-8?B?TXFTYmhteDQ2RzZMR1plWFBmcVpESG5lQkxwRXdBL05hc1RGS1RISDR3anQ0?=
 =?utf-8?B?TXdIQnpycG1Sd3ZldnF5RHVmYi9YWmU4TzdlU1BzWHNKT3VlSi82d2FvVjRW?=
 =?utf-8?B?TWI5Ly9jcWR2a1NXbTh0OC93WDNoUHBSTWIvNkd6aFpCbm5IWjVXV3EwTG5n?=
 =?utf-8?B?bGlqL2dPM2ZXY2w0QXpMTXZydExyb0FvVVZncHFzbUM5RWRxMHU5MXR0ZWZh?=
 =?utf-8?B?QVBUcTRJNjZnbU5NREtzNlNtc3ZiNnQ3bHZYWk5aK29mVWdiQkhCVGg3YTBY?=
 =?utf-8?B?dVdFeVQ2SW1nWWlOeURQRW95Wk5tdE9GQ3J1aHBGZmJFblJ5cDlReUVNVSta?=
 =?utf-8?B?ZCs4NjAxRktlelhPNVU5K0dPZ0drZzFJSXFqOVczSEJnNldiOEFpSUNCaUJK?=
 =?utf-8?B?RmFVRlJNV0swcEQxbzdFSHhFYUx5SE9ESm9lTEphZEdrSXlnU2Z6N1ZPRGQr?=
 =?utf-8?B?amR4VU10UStmZzBEUkJ5RndDUDlEdUVZeno5Ti9zQUxFWTE5Qjl2VEd5Ny96?=
 =?utf-8?B?aERhSm16UUpFZjdDVy9GTjdmYjluTXZmdHZYeW9sd3dBdTdjY2QyMTBGdTNF?=
 =?utf-8?B?MTVkOCtjM1N2SEZXYjFtOUY5V3IvUjRWSUZXUHhwdm5YL1VjcWlEWENONGN3?=
 =?utf-8?B?aWdKR29tek5rTWpMT3UvTkVSOG8rcjZHUVRMaHd2MlRSQjhnNUhqT3I3NmZn?=
 =?utf-8?B?ZTVzMnFHdDJNK1YwNVQzdVh1NE9vdE5DNXQzejQzYndGVnBGRnVET2Z2RlN4?=
 =?utf-8?B?MEl5d1hlakhYM3hxeWMxWk9sMFBTUkl5dXJ0K2NJZmZrbSt5T01tTVNWVDdB?=
 =?utf-8?B?WUNDMDl1ek1EZUFuQjFwUm5qaGdqTFVTMGw4QnVSckNRQWdLekVyOW8za1o0?=
 =?utf-8?B?ZjA4V0w4c21qbE1nMWpudHNnWmREMENMMExUTWIrMUs5VkhxUXJQZ2VJdWV0?=
 =?utf-8?B?b3NFa0VMVXdVWk1kWG45d0Nrb0Ntc1pXcUREZ1gzbXlERExwVjdNaUhjV1hX?=
 =?utf-8?Q?SLnxRzOnu6r8pSiz5P8nV+/NB?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	nOzA7HZouImG9cLL1Fu3G+FIhvVEGjWeTlcVwJ9LCoqTD0xU/42oU+9VIYidIw2GXTyXKBVxX1dVWOdF5feBOFf/YPQ7MWi5JgZ2jfmSNUXHFH/fPpo0+Je55JsDtGHpDa3fPi6Qmja+9TLQBM7kooPBZph5aotomgg+P06ttY9aufv0zW+U5ScXVlNLcHsYKGPfp5esbIKC5PWEEGZ0c0GFInDQ8xbYHzjlwlExPum6VolIgQOzvgrVtFRgrV4yh+IYRndKP/L/n7CyIg17kzCNf0R0Uy9zJqoFvP+2amhMhVlp0KNLkPttP3pzubU6qapk7l+UTkDiWp5URvSiaJV+3IXEdPgzlkvdl0arP01ngaRDzhA3AUBBvKOnUl2kJe1imgTFKVKQ3AVhBYWYpqogadCh3lCgc2dF/jFB9F1pbiC7uIj1PK+wAoSYj3lC/iT17b4SJqgba2eOcYu/Ad9QTbr2KrR8G4zYADGxgwQvh4FDhQG4ND37lHaHTI/qvga3aKqU5u/1YpgMLqpFDPQK6kgWk/OFmbw79ZKo9HGFwwEEFbQuHHLM2UNsUvNN7scQgaKWNW4F960Qa1IwOn04sBVR8kEPsOkauPDBal4=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f09d0fbe-7c3e-4f01-b443-08dc63bdb9d3
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4257.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Apr 2024 17:49:29.9208
 (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: PUxFJMvJOgXbps0HUo2BBGKm39HJ/AkbJmWntUnyMkgfzhjSjrPLb1hVs/Yk6DNEtAKdp79tC84bCc1q5hT9FQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN0PR10MB6005
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 phishscore=0 mlxlogscore=999
 bulkscore=0 mlxscore=0 suspectscore=0 adultscore=0 malwarescore=0
 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2404010000 definitions=main-2404230040
X-Proofpoint-ORIG-GUID: SjAAYtY5I6b2KnPrBh_09cqO-9bzU7o3
X-Proofpoint-GUID: SjAAYtY5I6b2KnPrBh_09cqO-9bzU7o3


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.


>   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.

>   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.

-Dai

>
>