Received-SPF: pass (google.com: domain of linux-nfs+bounces-2969-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <89688b35-f6dc-4103-8a13-ae4fe2865e19@oracle.com>
Date: Tue, 23 Apr 2024 13:13:47 -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>
 <d18e8b50-2eb1-419d-a937-9314ea39e08e@oracle.com>
 <Zif5B3fT5JA8GvEt@tissot.1015granger.net>
Content-Language: en-US
From: Dai Ngo <dai.ngo@oracle.com>
In-Reply-To: <Zif5B3fT5JA8GvEt@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?UW9waWU2a3gzNVNvbE1zWVQzcWV5enJEVS96OHh5MFlSNXMrYkU1WlVzS1di?=
 =?utf-8?B?N2xSTVkrWlgwaDRYWEloeld5WE1ZcXkyU3Q2VnlZMnMzVEdObmxjZ3lUR3M5?=
 =?utf-8?B?cmtTU1YxUkVuZUQxdHRSb2lHdm1iZUlidVJoejQ3OGdqcVIyVDNLbzY3ZkJj?=
 =?utf-8?B?YlVUZVBuZC9QL05BUUlXYXFWekVUdzFUaTVCdDVnSXF5Ylc5eXpkbEgzNU5N?=
 =?utf-8?B?Nlk4cVdPNVRGc0VTeUs0QmtnS3N3TE05RGJYYXY4Mk5Zb3hJK1RLUGpNdC9v?=
 =?utf-8?B?bW1aeENYN09vRFE2RW0rcWhTck1HekdXdjN3R01nK3dDdkREK1ZyTStkOG9k?=
 =?utf-8?B?WXd1YXM3Z0trbmZoRTBhNmhqNTZFemI2Qi84NHFLNStSRjlYWjQvcFJsWjNp?=
 =?utf-8?B?dC9PMFVWNlRLcDk2T25RSW1GMEdvakg3RG5lUkRZVGVkalV1RDlNeS9RenVr?=
 =?utf-8?B?RkMzMGNEYk5JSlhMTjJDMEVVU2hXUEhEcU9LZG1FaUhJYnBiQjlZSWE1RnJD?=
 =?utf-8?B?cjBhVXpCTU14cWR4b0l5OVAxVGo2Mk5Fcnh5RTN4NnFUWks2czBlQ2lMUUVw?=
 =?utf-8?B?TW5DUWJHMjUzeFh5M0R6RUU0ekMwbTg0YnluenErUjN2eis2Ty92cEpTRjF6?=
 =?utf-8?B?ZlhOS0VFWWk2S3dERGJpY3hmRWhHL2Q1TUxMS2pzbWF4OUpjYXliM1FYYUlL?=
 =?utf-8?B?dXdMZkZkK3JOc093K2FISFZqQjB0dDBSNGx1a3lKZ3ljc04yVE9vd1NXVmlH?=
 =?utf-8?B?czF5NHo3Z0ZVYzRpa0wrR2pPK01pYVFWcFlxOEx4Ly9oUnNWY21teGFDaDh6?=
 =?utf-8?B?WDhxSVA3L2VHaWU4TlRNSEJCd3MycHROcXJDbFRtWSsvVU5rSVVVS1ZNUWtI?=
 =?utf-8?B?SllXdDBLakxsUGNsaXRvVWJic2l4T1ZkYmdKL2N4eHlhT0l4TnZKRk1IeFhV?=
 =?utf-8?B?eVFtdHNhLzNmUkZrVDhYOXgzdEYzUW5STmhSSXVOSllXOS9NWTg3WDNmZFY5?=
 =?utf-8?B?VHY0RFlZeC96dDNvZlB0eGZhbXV1ZzcyRDh0ZXBtcW5GZVNENlc5RnM4NUpv?=
 =?utf-8?B?L3R5Tk5VOUFRY3h6SDcvRDRuenlpMmJ1U0dIcnpENHRyRzhWWkxNT1lyVjhH?=
 =?utf-8?B?TnFpaWRnSi9tMXYzcVN5YkdIUFRmdFZ0Wm85NTY5akJQNHg3QmxYZUxWemhw?=
 =?utf-8?B?Qk5RMzZlczM5dmJzYk9XNklKU3hUWGhZbHNRcFVBOHBIY0lnWm9ZMzlhQ1Vo?=
 =?utf-8?B?OFNiNXdObW1sQW5oZDh5QWdrclV5SmtBVEs3U05POWJHNlE4WUd5T0tvcmNT?=
 =?utf-8?B?U2k3WDg2czN6aktlVCt2NjhSR2tpRzZlZ3RzSWYvMmI1UFZjVmZ6eERBdU9N?=
 =?utf-8?B?TGJBOWVuNGlYNnNEa3BCQ2RDa2o5RVFXQTFRcDNnWDh3eDcxWko0YmRXUU1O?=
 =?utf-8?B?d3Y0TTA2NVJYdjYwUFdlK1FFYUxYRi9nVENXdEwyQnRDQVZVUmExSFIxSTFm?=
 =?utf-8?B?dU02cUEvanFTMElZSEpNQVlWWjRuT1ZZaHdyKzUyTXplOFhibExVVGhHNkpj?=
 =?utf-8?B?QnUzaitCalpVT3hNcjYvcTRRYXExTEViNElHRnRuZ2hSRkptNHdrSGd0SndM?=
 =?utf-8?B?RnRMN0xmY2hNU2ZIUUg5Z2p4ODF4TytLK0pXcXhFSWRSbUgvUlpnekhDdWUw?=
 =?utf-8?B?UXo2RU5GR2JkSFJSZ2lJckJQVVZvRkZiOUIzMStVY3RFMlJlVWFVVnNoNVIr?=
 =?utf-8?B?RXBJSVNvN3JqbmtGOERFUmF0Yk12dGEvVmtrMUhrUmtEekpTVHdjdG5LSFJs?=
 =?utf-8?B?SUhGb2ZXSHczMnJwR3RrZkw4WnBqSVFWeEQ1QXNjWXNhTS9IQUdOQUdrQm5i?=
 =?utf-8?B?dmZqb2RicVpiWTgxY2NXUkpsb1o0RVhwcVVaZ1RWY25obE5aY0tObkx0cnF2?=
 =?utf-8?B?T3ErWkNiczNKNlhRNnozcUJReEJmZTR4dXAyTHM4MkxvZDljRVk4SmRIbnJW?=
 =?utf-8?B?Z0JRT1phOW1MUVkwVEVOa2FTOUJVbkZ3L3c1cm1jV1dFY0Ntb2E0Tm1JWm0r?=
 =?utf-8?B?ZFVCNHhpVFVpOFFuUjBPWk1HRnhWT1BOZ2RtZnl1dTFWeWgwS2lFQ25jRm1Q?=
 =?utf-8?Q?KItNOwB81RjuA37BEOty91tLC?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	40SS2wOXrdBC3q4+DFxXNSKIF7rbbSslMDXJodEDCYwrq3E9xuKbKZpHihcA3MI5VK6PJ2BRppCmM9G+XMLQLVxIji4/eJcbyLgO05HoXZ+VYWW+/3k+FA/prZsMFQhF2ai6UyanNOggn0J38/YOVjQtNLAjB2T6hs5g+AAeB6PAxlybMdx8urVM8cGtUqG7RUEmTw8JpUALgpFQ8JhfQCIwn/DvGo3T6dOfWJjGba7VmPpStXIIw0pxfW4oayKr8qPTeCUp7Mq4nmL9PfqN+PFxG8Ht1ySLdkuibx7sX3+SsZSESoGAHp9n1hTGe6wjpztaKx1zKCMqvAcLeEoLWH+qzGx+dKu8KrZGZh7sYrLaW4Nj5Oq67FYTqXXIzFUWnECKOMeElzd+ntGKBeTZoci7/BA8wZ3Gu80U01S9bVPzqKIoscP36BwFpqclhQOj2Gh4QMVq9PiQje+cOVOGjolEy5vP1yPA7Pb3wQNTxVUNm6R/NgKqU1eqT0y1bFC0DLFyhfMGisbl2wRaks+X066VBELd2NLO4bkjlLbP82Qpe3jqkWW0ut9ieZlwmPblnc6BTIG4204Z+Osxbg7pfF4n4DK1SCFGNerXeZeHuBw=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 75627b48-757e-4155-34dc-08dc63d1e5e9
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4257.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Apr 2024 20:13:53.8985
 (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: RIs6L+PBIwCpvBErh2d/v8o3Q5QDh6mcCUkjKpIXz0EKeKFQiHTfCcDMyU/R47WoRa/F7LRZyUf6lbuT8xHlZQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR10MB7027
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_16,2024-04-23_02,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 mlxlogscore=999
 mlxscore=0 phishscore=0 spamscore=0 malwarescore=0 adultscore=0
 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2404010000 definitions=main-2404230048
X-Proofpoint-GUID: H2M5iUZo4vRhqTYtdPtnO2y7FBOUttNH
X-Proofpoint-ORIG-GUID: H2M5iUZo4vRhqTYtdPtnO2y7FBOUttNH


On 4/23/24 11:08 AM, Chuck Lever wrote:
> 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.

As you mentioned below, this needs to be reworked.

What if the client never comes back, decommissioned or student hibernates
the laptop and opens it up few days later. Even when the client comes back,
it might have been rebooted so the callback does not mean anything to it.

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

In the case of re-queue, the back channel is already marked as NFSD4_CB_DOWN
and cl_flags is NFSD4_CLIENT_STABLE|NFSD4_CLIENT_RECLAIM_COMPLETE|NFSD4_CLIENT_CONFIRMED:

Apr 23 08:07:23 nfsvmc14 kernel: nfsd4_run_cb_work: NULL cl_cb_client REQUEUE CB cb[ffff888126e8a728] clp[ffff888126e8a430] cl_cb_state[2] cl_flags[0x1c]

but that does not stop the loop.

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

Thanks, I will wait until you sort this out.

-Dai

>
>