Received-SPF: pass (google.com: domain of linux-nfs+bounces-2586-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: Mon, 1 Apr 2024 13:49:53 -0400
From: Chuck Lever <chuck.lever@oracle.com>
To: Dai Ngo <dai.ngo@oracle.com>
Cc: Jeff Layton <jlayton@kernel.org>, linux-nfs@vger.kernel.org
Subject: Re: [PATCH 1/1] NFSD: cancel CB_RECALL_ANY call when nfs4_client is
 about to be destroyed
Message-ID: <ZgrzwVp4GrbmZGWt@tissot.1015granger.net>
References: <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>
 <a30b343f-b6cf-4566-9565-28a5fd5ca851@oracle.com>
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <a30b343f-b6cf-4566-9565-28a5fd5ca851@oracle.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?iso-8859-1?Q?7m4v0R2mDVdPz4TJQG0K7kwcisTUzuHtxxbeQolwpvocxj9xru+GzBa5pe?=
 =?iso-8859-1?Q?rt9OLLoMdjNLPwqYXAYfJNDNeVNfdJx+NVvbLaSIkSdU5uB7YxEvOr7Mat?=
 =?iso-8859-1?Q?bGm7MixufiXTF4AEWvZDqeueXewnU89E9l5RjGvwBFKZxfYNGQnzc8eXKH?=
 =?iso-8859-1?Q?G4OdJa1MzLMiC3ca0amYm367e7qX0hwCJDVpci73PLj/c7fVDMjcAWwUTw?=
 =?iso-8859-1?Q?VDtfYlInuQWU12Oka6fwowCX6sHDaw1jWFli+bhcJQ5xUxbXy24a8LC4Sc?=
 =?iso-8859-1?Q?kkZ6oPxUh5JZWJ8WVQhT9p659OZ5ccpecdm7exIaDoENUzpjfJXGDwlIUD?=
 =?iso-8859-1?Q?K0sdFOgoPVMZmnbCofDj2n4FsZS3cTovS/lwkH1/t2IRMpYv8OvFF3yBOr?=
 =?iso-8859-1?Q?ukjXfPqrB2o6cheeSFH2INAZXZbnPPE9LJYuJPY+L7qdiJVbjg2gTMCkbr?=
 =?iso-8859-1?Q?F/7LRqEtKBPK/b7465p0eDSEzSZArM0gMg9A/m2mLLRuMeW0lRv60xkhsf?=
 =?iso-8859-1?Q?TegPRTSalNoualyhyfmoN8PB4pm6+BQcpWp5DH/ou6uLqgLSVzIYlo6o9I?=
 =?iso-8859-1?Q?KrrFwFTipZ3gCi8D5BQvpPewLP67nf+Q2AqxtB0GFq/mIVuCIgVc5PZP3G?=
 =?iso-8859-1?Q?eswUtHh8rjpfGGPsWSyiC3sTiAemt3joIgF+zdwrZZChwbLiBjL0epoMQu?=
 =?iso-8859-1?Q?4PzIftj/qKmMhhqzccXUWCgoH/mGaSPQmpWjxcHD/s7GB210h1tq0Xal5K?=
 =?iso-8859-1?Q?X4GM8onyhAFq4ybOBDbvd7c/sblIL12egq8QsZQBcpuWbJBynk7yuzmE2o?=
 =?iso-8859-1?Q?u+kX3Os8NrXcUnpMReVEn6g724ZufVBfi9vaKQtAEmdlfq8NPfeulTl4dV?=
 =?iso-8859-1?Q?RdpSEID+maN8yPXice0QrZpf0JoauCpC/1kDWGGzC1f3xU+EjNDzLqM17m?=
 =?iso-8859-1?Q?29UUqKcUYAfs/sgBkZX3791E/Bytuy/Ouw6GaUz91+ofvQQrNndK9Wp3id?=
 =?iso-8859-1?Q?6mKcc564cAOYdz1xCGzBPwNBAb/iuqxoBIRRXbygjqMNr5VQHLjEuYB3VJ?=
 =?iso-8859-1?Q?NOcEHg8SMKAFwM7J0tgxBedNYWGqtOLjBTaiLbJeSTONwGeC3LCpMzjVJ6?=
 =?iso-8859-1?Q?iYo01823QsEELrKsxZa/qBUYFKGE4gokhs/7IeiTUTVnlkDGC7PBMIQTOb?=
 =?iso-8859-1?Q?C084ccmBEwd+aiULJT4yjp+d442LT2dV9IlQHBKmo2YOSzgMLk7r9jWN7k?=
 =?iso-8859-1?Q?FPUdgnp2hAARmnMPx1B0k0UM10YTn7IkLJE44hzTboTGMYdfyz9lgjzA8A?=
 =?iso-8859-1?Q?YGeuDI02oZLFcJkmtiGldBwR/4kkltV/dKRIAxcyBCsol8G8rNWOIJHQAJ?=
 =?iso-8859-1?Q?YDJ7NOS8e7+M8Xh9jWf3iGKZd8evwsHsWBL+CZEr2SCeNGuG5O0UHd0kNL?=
 =?iso-8859-1?Q?8nDYnBFemLpBbdVDrNCIdwJnQZuoZ1NksFC2s9hYlvxiTuAni3d4tYfRkS?=
 =?iso-8859-1?Q?xIuGuNRSV3lGtBSlkhZd83B1Zr72W6NATnJxsQNqN8Mt2knORhzB9eMwoA?=
 =?iso-8859-1?Q?jB52ZrP49O14PfBTtHsRH+T0kyx/H9x9P7DD+G4XBOJYdogUYwrhLZ4vfB?=
 =?iso-8859-1?Q?uvRyXEagMG0kCfygS/QJ1IJfye/BtoxhHoBW2FHvoQB+H83n9kNOpHhA?=
 =?iso-8859-1?Q?=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	bcBWvJMepbiX9WGtpR8bOrOuPH53YF6OkMsAwW9tpjN8uGZOBoC9FKHQwvPwOCQWspAfo2svP9b/plG8oty75cErO6MNm/Yd8Gv5QHFewavIwDy3CDEPDPLAGSiugFP2FikxpoKEn05qD3ZgnlAqJhrJ5phIydwQZFF1tkOHHMyy3vtZvFNyfbj+EwXsK2j7fcmpDPfMq1pWJBGcDAd2TwlCTKzvpLdgQ1UTR0RB+pYnDj4nQ9dDSBdakXuwUO0xxh98+02o9KHpOn1B4WAu+XbJI6Pt1jaDB2I98bRnjq10bHYn6WUsmWEaSqcCPqCoTeb8L3GV8/G0NspNaZc9DmtICfimqqEZZ2/aQWjuRuvTkVJMmuIdJhps54etIuneUz9ppwhfbw4hrvtv6blzMInLu6i6LCiPvdASEnBgQ1ZOfgLC2p8HBJ3odLwCEnFKEyYEGA/f5dwxNdAi7Fg6VD7E96OT0O1Yavv2hdu2jn+Ot3WXDrVD1Ud3jc2NRwdm7Bc/g6CcY3ujHUsiYaUfgmmYaqJYcDqwTgqmIoEL43F7QP4XqLT9aaz3itfhH20jWq1H7P2t4R/v2uGIHCAy2WUFvdQNXdkbLQZlWS17M/g=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 96c67983-c385-4380-99d8-08dc52742493
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Apr 2024 17:49:56.6703
 (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: 3jZiqKIBWNOZ0bT/u1mOWhCwahX3IYh+SPfF2vLw34yoWGU1YzSRo5e7ODG857qXT3FLU5hm4xUVfPNYRyeI7w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH8PR10MB6598
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_12,2024-04-01_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 mlxlogscore=999
 bulkscore=0 spamscore=0 suspectscore=0 phishscore=0 adultscore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2403210000 definitions=main-2404010126
X-Proofpoint-ORIG-GUID: -C2Bc94C0tNk2dCnq8Z4EqvZ2GPryLPX
X-Proofpoint-GUID: -C2Bc94C0tNk2dCnq8Z4EqvZ2GPryLPX

On Mon, Apr 01, 2024 at 09:46:25AM -0700, Dai Ngo wrote:
> 
> 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.

I would rather just close down the transports for courtesy clients.
But that doesn't seem to be the root cause, so let's put this aside
for a bit.


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

The question I have is will this unresponsive client cause other
issues, such as:

 - a hang when the server tries to unexport or shutdown
 - CPU or memory consumption for each retried callback

That is an ongoing concern.

-- 
Chuck Lever