Received-SPF: pass (google.com: domain of linux-nfs+bounces-2584-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <a30b343f-b6cf-4566-9565-28a5fd5ca851@oracle.com>
Date: Mon, 1 Apr 2024 09:46:25 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/1] NFSD: cancel CB_RECALL_ANY call when nfs4_client is
 about to be destroyed
From: Dai Ngo <dai.ngo@oracle.com>
To: Chuck Lever <chuck.lever@oracle.com>, Jeff Layton <jlayton@kernel.org>
Cc: linux-nfs@vger.kernel.org
References: <ZgV5zwR0q/vjBAtI@tissot.1015granger.net>
 <88fac8af-c194-452b-94eb-7658b9056246@oracle.com>
 <c97be8b9-c0ba-4f2d-9340-78368008ba4b@oracle.com>
 <ZgbWevtNp8Vust4A@tissot.1015granger.net>
 <97387ec5-bcb4-4c5e-81af-a0038f7fc311@oracle.com>
 <ZgdR48dhdSoUj+VM@tissot.1015granger.net>
 <09da882f-4bad-4398-a2d0-ef8daefce374@oracle.com>
 <ZghZzfIi5RkWDh9K@tissot.1015granger.net>
 <84d6311e-a0a3-4fc6-a87e-e09857c765b3@oracle.com>
 <039c7e38b70287541849ab03d92818740fdf5d43.camel@kernel.org>
 <Zgq365RJ9M5qsgWY@tissot.1015granger.net>
 <5108ca5a-b626-4ae9-a809-ae3fffb50cab@oracle.com>
Content-Language: en-US
In-Reply-To: <5108ca5a-b626-4ae9-a809-ae3fffb50cab@oracle.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?RGx1UXJQbkExYWo5N3A2Y2lvWXlNRnZRMTVGcjN2Ujg5WTI1UUgyRnhwUUtQ?=
 =?utf-8?B?Q2JCbkk2VUF1SDlzTjNRVVVNbGdFZTJFbEl0T1l1OW9EYnFQQm9YaE5TSTJr?=
 =?utf-8?B?VE1OR2xabGkyTDlzTk5FYS8xVmVlTXEyUGUwMmZCYkd6clJwRDRONk5SMVRz?=
 =?utf-8?B?c1djbnZNdTBMRy9uYXpRUVVmVWdQeXZ5WVZ3d2EvWVI5RWxZdFJ2MGduMGNv?=
 =?utf-8?B?clAxeXhiNEFWamZlT3FPRkRmY25jMjlYTUtMbGNKYjM5WWZ1eWhidHpDaGlr?=
 =?utf-8?B?Z1J2WFNXWlNzT3MxS3hQdkdwZnFjMFlueGg0aG56RU1RemxzamtxM1lweXYr?=
 =?utf-8?B?NzZudHZjaHArRFUzS1EwYmJjNUkvei9ScUdKeldLeHZQcm90Zi9JakdhVlh1?=
 =?utf-8?B?eXcxVXJ3aWxlRGk1T05VQ3N3RGtHQ3dXRURaTUZVTHFjNlhjdENZY1RMQk1u?=
 =?utf-8?B?UGt4cmRYQkJpKzc0dW5tdmVUYzZSLzVtdVg2Y0xLTnVYMnYwby9xZGdreGtL?=
 =?utf-8?B?eEV0dkVUK1QyVXFlMkdsL1ZxYmQrY0IzdUJHaGk1a0xnNUdncVpncUx5RFAw?=
 =?utf-8?B?UExhNU5sWU1rZkFTZjE2RDlHRE9FVGJjWHFJWXllYkdDNEdCYXFoUk1VOG9w?=
 =?utf-8?B?R2FrTm5wUDU2dGR3bDV3K3kwMUM5bjNabVdieVhsOUtKOHd5d0UxUTJVOXA1?=
 =?utf-8?B?RWdPSFgvRmlmY3dDRGJlSS9RVTl4VFFDdDM5V2ZlYVVycE8zWXdwdk4wZG4x?=
 =?utf-8?B?MHdFSlVyOTRiWGg0WGRxRHZ2UEVOUU5Nb2lzc3l1MjQ5Vzc1QVd1VzNMQmo5?=
 =?utf-8?B?MmtDY0RVNjRycHQwTTIwOVRzSERWRkJPUHllN1dhZlE0c24waG1JUlhVV2dp?=
 =?utf-8?B?OU16Q2JqU0h5dXoxWmlwN2w4UXkrdGlLMGVYMkNkTWlianlDZGt5SVkzcmM1?=
 =?utf-8?B?SG4wUXVTWW5Fb3VsTHpJN0pDV2lDNHF4b0tnV3M2RXNubXRFazk0NWhDK0Vh?=
 =?utf-8?B?b20vM0NGYnNBSEc0c1Q4T3RBeFpoK0JBalpzenVLejNYcDJibXFENWs2YUR6?=
 =?utf-8?B?Z1U4RUVjQmFHaDJnNFNZUCtMSzNCajZtSGhDY3cwR0FjREVpN0Y4YXJWUkEz?=
 =?utf-8?B?d2RGOEtzckJZN3RMVW9DR3RNZFo5RW54YkkwdTh0dVRVSTJGbTRnRWdCdE9j?=
 =?utf-8?B?b1pGdndJOUFuVnVTN005ZG1FcGZ0bGhXRlNETGY3MUZERE1lekZ0NS8zYXBY?=
 =?utf-8?B?SVN5MVd3azl1YVdzNUtrd2hvUTQ3TlBBeEpFWm1MVTR2QUFUOGVGRS9NQW43?=
 =?utf-8?B?dGpGaXpydC95aVJZOGhIWlBVSEFFVE0rUDcrMVVwcFNPYTQ2M3o2N05MQTZW?=
 =?utf-8?B?cWxWRWxPWUdpVkpYNVFhbXlmZkhLUzFDSXRSSGhtTEVUMlpTZmNmYWM5UU1l?=
 =?utf-8?B?bjRnaEVzVUJhMjRKZnRoQWc0Y0FOdkgrTmVvV0l1aG1seVJRVU9xR1JGbFI3?=
 =?utf-8?B?MTEwTkd5TVhJbU84aWJnc3B1eGtoTUpBZGdlNUFSODJRU2FVaUM2VEtCS2tB?=
 =?utf-8?B?dUJaanpLaVNERmdIcGRRT05KNWNuSkV2cmpJVUM3cGdEUjMzWDBKd1NNWWJZ?=
 =?utf-8?B?RzVEd3VOelM3WExWV2xQVDFGaVE2MmFiQ3ZIV1ZnM21ISDVEa253K2NyYmo2?=
 =?utf-8?B?UVNVN0U3UW43Q3B3ejVCL0xnRWc0OG5udzQ0TDUrQlNxU3psQ0cyWDlOWGJn?=
 =?utf-8?B?TUNRSi8rWjU0RjhhK1piS0FibVdnL0ZiUmlMSk1oK2txZEw3ZWtFbUhzK1Rm?=
 =?utf-8?B?bmtWNTRhZW15RTU1OHFWRk9VZmx1K0JPNldBa0ZackRBWFEzZytVV3dBWkVV?=
 =?utf-8?B?TjgvV2Mvb0RPOXpmUUw4dEh4MjZwWlFSczc5WVhXTWlSQWlVZFJhNFRpeVRR?=
 =?utf-8?B?R3JxQkFSdUVPWVc0YmdnN2xxRHRiZExLL2dCWGhFaWFZaTlDOHRWb1gwRkFq?=
 =?utf-8?B?UDlaejNHbEdEbkU3emtlUVY2R24ram9yNmxUQ3lKa1NSc0VxQ2N2Q09kaWNY?=
 =?utf-8?B?V0VnaVFlSDhpNTJBVkdOODhNQXJXZmQzMVFaU0JkZExuWnVUVFRmVXFORExq?=
 =?utf-8?Q?68Q+dwXTK8Cg6h+AyPOA+hX1l?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	hwHChkJkYIlpGbqTpYZk4CV8u0jxI3aynuBclxnQ5bCldLJzw39mxuwQ2xJI2mgHVcIfeLIJ+gctS/MO31fH+QuyJ00uQ7sTmOEf0t6bIVBocdNIzfEOieeKQKK1XHKhd3WDMySmgRiCL2vCMl7/jlBcRZyP45yVyesLN0OdNvCB4lapnD05cy/fJRbJdQrDlZ1aVaumzyuvraQpb4ecRjiwkwPNOKxC0yVRklDdUl0lU+8oeEKgqAGJdQVKOY5Qv6e3hspk6cDiGFSJTP3Ddf6Q4iXPyuMs7a8U0lJoQwxj/MjtF3U8wfNXpWdylL+f8+HFMbDXT/i8tnqkUYFhH2DEzPoRCqippXaucGW4lnZGltx5zByDHdmkyh4kcstw+fISBWJU4ybIc9/oiLAPXL2y0cnp5wyvRHdYsmBvilToER0uOhRZ5fhytGaPhRSebhdSrJQlboYm4UeLpBDO+98Mp1M0HhQB8b1TTfTWDh/8WK1ho6dF3SM3wHKuqymJICdC5DLVtbz5Oc3Tv1Zaouwc0JiAfIxiV7Y9jZj+IaNUkmmnK84upc0AjOLY7WU+kmhEz+CsjnsQGw5kjkJvnkYI0CJEr7A0b3Jj2UYeEM8=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 7b5b3986-f840-4d7b-3f79-08dc526b469e
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4257.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Apr 2024 16:46:28.1375
 (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: nabfecNBovsyRfJBxyqbl4kb9f1ItHFkxcb3PFqM7sqRrLhrNmlel/52fPeWbODiMFqi7F4H4COxm9Mf+h0aqQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR10MB4331
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-01_11,2024-04-01_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 bulkscore=0
 mlxlogscore=999 adultscore=0 malwarescore=0 spamscore=0 mlxscore=0
 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2403210000 definitions=main-2404010117
X-Proofpoint-ORIG-GUID: 586Uy0ISs8TXH8X4Y195621hGu8oqOBJ
X-Proofpoint-GUID: 586Uy0ISs8TXH8X4Y195621hGu8oqOBJ


On 4/1/24 9:00 AM, Dai Ngo wrote:
>
> On 4/1/24 6:34 AM, Chuck Lever wrote:
>> On Mon, Apr 01, 2024 at 08:49:49AM -0400, Jeff Layton wrote:
>>> On Sat, 2024-03-30 at 16:30 -0700, Dai Ngo wrote:
>>>> On 3/30/24 11:28 AM, Chuck Lever wrote:
>>>>> On Sat, Mar 30, 2024 at 10:46:08AM -0700, Dai Ngo wrote:
>>>>>> On 3/29/24 4:42 PM, Chuck Lever wrote:
>>>>>>> On Fri, Mar 29, 2024 at 10:57:22AM -0700, Dai Ngo wrote:
>>>>>>>> On 3/29/24 7:55 AM, Chuck Lever wrote:
>>>>>>> It could be straightforward, however, to move the callback_wq into
>>>>>>> struct nfs4_client so that each client can have its own workqueue.
>>>>>>> Then we can take some time and design something less brittle and
>>>>>>> more scalable (and maybe come up with some test infrastructure so
>>>>>>> this stuff doesn't break as often).
>>>>>> IMHO I don't see why the callback workqueue has to be different
>>>>>> from the laundry_wq or nfsd_filecache_wq used by nfsd.
>>>>> You mean, you don't see why callback_wq has to be ordered, while
>>>>> the others are not so constrained? Quite possibly it does not have
>>>>> to be ordered.
>>>> Yes, I looked at the all the nfsd4_callback_ops on nfsd and they
>>>> seems to take into account of concurrency and use locks appropriately.
>>>> For each type of work I don't see why one work has to wait for
>>>> the previous work to complete before proceed.
>>>>
>>>>> But we might have lost the bit of history that explains why, so
>>>>> let's be cautious about making broad changes here until we have a
>>>>> good operational understanding of the code and some robust test
>>>>> cases to check any changes we make.
>>>> Understand, you make the call.
>>> commit 88382036674770173128417e4c09e9e549f82d54
>>> Author: J. Bruce Fields <bfields@fieldses.org>
>>> Date:   Mon Nov 14 11:13:43 2016 -0500
>>>
>>>      nfsd: update workqueue creation
>>>           No real change in functionality, but the old interface 
>>> seems to be
>>>      deprecated.
>>>           We don't actually care about ordering necessarily, but we 
>>> do depend on
>>>      running at most one work item at a time: nfsd4_process_cb_update()
>>>      assumes that no other thread is running it, and that no new 
>>> callbacks
>>>      are starting while it's running.
>>>           Reviewed-by: Jeff Layton <jlayton@redhat.com>
>>>      Signed-off-by: J. Bruce Fields <bfields@redhat.com>
>>>
>>>
>>> ...so it may be as simple as just fixing up nfsd4_process_cb_update().
>>> Allowing parallel recalls would certainly be a good thing.
>
> Thank you Jeff for pointing this out.
>
>> Thanks for the research. I was about to do the same.
>>
>> I think we do allow parallel recalls -- IIUC, callback_wq
>> single-threads only the dispatch of RPC calls, not their
>> processing. Note the use of rpc_call_async().
>>
>> So nfsd4_process_cb_update() is protecting modifications of
>> cl_cb_client and the backchannel transport. We might wrap that in
>> a mutex, for example. But I don't see strong evidence (yet) that
>> this design is a bottleneck when it is working properly.
>>
>> However, if for some reason, a work item sleeps, that would
>> block forward progress of the work queue, and would be Bad (tm).
>>
>>
>>> That said, a workqueue per client would be a great place to start. I
>>> don't see any reason to serialize callbacks to different clients.
>> I volunteer to take care of that for v6.10.

Since you're going to make callback workqueue per client, do we still need
a fix in nfsd to shut down the callback when a client is about to enter
courtesy state and there is pending RPC calls.

With callback workqueue per client, it fixes the problem of all callbacks
hang when a job get stuck in the workqueue. The fix in nfsd prevents a
stuck job to loop until the client reconnects which might be a very long
time or never if that client is no longer used.

-Dai

>
> Thank you Chuck!
>
> -Dai
>
>>
>>
>