Received-SPF: pass (google.com: domain of linux-nfs+bounces-2577-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 1 Apr 2024 09:34:35 -0400
From: Chuck Lever <chuck.lever@oracle.com>
To: Jeff Layton <jlayton@kernel.org>
Cc: Dai Ngo <dai.ngo@oracle.com>, 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: <Zgq365RJ9M5qsgWY@tissot.1015granger.net>
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>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <039c7e38b70287541849ab03d92818740fdf5d43.camel@kernel.org>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?+OdjCICsFZfKXRhBbZ9RHhE+bsrtAg7+wL7fa54HBQI7tVVVrCS1pkHm0aeQ?=
 =?us-ascii?Q?tA9L1y26njnM/imiDLuTeCdzcpbpboxZkAMLIeuqP6lMBqlQCgxTjyol8Qn/?=
 =?us-ascii?Q?tYNuSyjWWhuhJfCSbt7fxb8vR8yF7Iwn+0/sIUO8xqRlnf1kM1jpwIog6+0c?=
 =?us-ascii?Q?CdYQ4O5pt9cJddV9kGmXelE260xxaVAzsI9nzp7hmNHf1j3/Xu6i1a6O6YWV?=
 =?us-ascii?Q?4vzIdIWv7wWkscBsIGjykVAGW/nlQ7aFzvMSDimrzC8CMpJvd3tCM6UA1e3m?=
 =?us-ascii?Q?DT6pu6huC/UYS1XmfHCZZOjxcpd9CGfKvQNENkOwkrQvFO6Lrqv4C25q/l8P?=
 =?us-ascii?Q?Xk1+9/t5V0KFuleBooMA4/67cOa3gLv8RrSzF7Rd9VhIFcm6UxyjVy04KBJm?=
 =?us-ascii?Q?7S4SWr5P+Tm25bSH0KKB+8KLAum20dMGgYknTdrqVDF3ea2rIybB4j0ao6eS?=
 =?us-ascii?Q?vVR8SVXPe3lAawBTriF1M8t2+GGSz2NXeeiY9kUXK6yvyMx2XLKgFQktVF8D?=
 =?us-ascii?Q?A46lKUJbJqK6xFG/0wE4yOlHPF0OqtJV5sBWhPULo7sWqI1lmeK/9Cf4nsYl?=
 =?us-ascii?Q?Sa6HT1il4wgziBUvpSg4XGp+0ojbNysBPjuM75m13Za4x8aKhUtCyJvDW96m?=
 =?us-ascii?Q?yIhetET3GI5OUbO+vdQnFZ8qpbyidI8BhXIoVCGjJLGWgS5qqOBno4OQm4Gw?=
 =?us-ascii?Q?yp+7OZG51otrW6uo2Ny2R5+hVlDDlEawieYgRXagjiKvT4v6y8q1QZ+O1SFa?=
 =?us-ascii?Q?4NazRvYKqsciBcYOv1KIDSxRlkjUpDkKCJTeCR6o/z0v6LzcVvcKDo7VZwzN?=
 =?us-ascii?Q?7yrFRSG1ZHa18aIedDUF0WvNwS6VPFRRPUDAX8XwzknOuevwjjWiO3EM5JLg?=
 =?us-ascii?Q?55ISV5xrcxbKox3cWk28uUhcSOUREKD6X5AbCL//PoLryca1+pdZq71OeZ3A?=
 =?us-ascii?Q?3X7RdWI58zH7D4OejKfRe+v+RzjvGlDikPSrAzvWyCdSQomKpz5kZW9Zb8mB?=
 =?us-ascii?Q?Cs77ibAxRAtKVr2Bmuz7mgAL0ggphY11du8F1Rl+5leDafmbTIDgYHZpNqsd?=
 =?us-ascii?Q?TojGxApgNxnpjaiDOQxjfCLhBZiUapI7x1FTJhstRze/xkH/oOhRnPBpO6UI?=
 =?us-ascii?Q?OL9tTE8uyPNy+A0kyL5jCop9iMYIlmXFeMKfl0oSGHzqXfNWvxxTqUsRcVnm?=
 =?us-ascii?Q?JCPIbwcq8LC9F7DhKLuxkG7PDeRnTM0+iBIFjJPV6oU3k1iLNpETEM8dSjZV?=
 =?us-ascii?Q?T2iT2icavpP7APeaGw1hOJsodQ5mWjCiNY421j0VgeYud2qOBdFMLHOZ3Tl8?=
 =?us-ascii?Q?0Jh5M+4ExzPY15VhetRDAq1kQbI4ZPIHlxvJqbRet9wwGNS7ZuwvgCdC1fCg?=
 =?us-ascii?Q?bfQFAfLeB31pK7mf7ezH/NCYZGstqCOZTUFPR4aq7zUfAygRSNRmcU2H52ne?=
 =?us-ascii?Q?7sZ+8iIDWFjAsq4o/D9jen1+UtR3xUOUkLAfGabROpC6xkRDnEctJ3xQ7BEZ?=
 =?us-ascii?Q?Ybb2mMaOsTQxthIVyeLg5Biy8c0J5BQkIVjoSrSZ3ceftVck2szaiI6VXUaq?=
 =?us-ascii?Q?uQOOp51pHYoiYT53fW+rqbNHXtCNTU6g83lAjruPb7nRgZQlOtdiLns1rO2I?=
 =?us-ascii?Q?aw=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	OuEhFCCxCnyjYqTDecP8BxbSMAxd3IYBxFbkZeFIOEMkYnnFGXg9gaKHa3LZDcg0TmSZrLNWySqj7479G9N7UlyfdhJSoZq6deOMmwdJxCA09DiS6dU7rHZ7EiBFiELwKoeL/wXNW6DvQ5hByuu//lJGY1OgLNtzzvTzmTmdy+FySE0NNJvCraIcrIC6SpYCtVE5ELkz0AsC1M0BnDehAQ9SetnZl87/DXGVj46Th8iICuRVC3pr4t0Sun7Y4h/xC7VPjCCBk637Os9s7Wx3vMz4y4Jef3KJ3fgy4U7fghQk9JSLmnejlB4L+BGl8Pen2Z/hmQKT8xav6S3IQfeVAJ/wwfQaXTvCdtXZ6Hl0CRR78p5HHmq+hU0tj8wsVMAWQUoVe7QwmchPOHj6OlfOxEZDezTOKnpkU051UaCYc08v11yQ6xXBxpbqNYcPC7jlxoUiJY8xndKeFoN3kA4Z4k22XYz/XyRTujNUFybgUdj5EExA9V80N9srcwvrz4Hz1faFqglQqsQwr8qZtBLvFaPMZ1AFK24bhrwiZ90RWQAWr7Nv30h2PnV3+AjU3Y5ddq0Zk9/b8GSB9UcEz/siExj0BYLR39of6m/CBVlqNUw=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: df62681b-05f9-4d4d-c0e0-08dc52507a0a
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Apr 2024 13:34:38.0114
 (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: LX0GW4RRPF9bv2vJRcs4uXmY28eJ+O0NVY1Zb0PCGFiaJ7NApEu6X8Mga/q8p0AlOKJNyMbDOC7QPTx/DpmdTg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB5859
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_09,2024-04-01_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 mlxscore=0 adultscore=0
 spamscore=0 phishscore=0 bulkscore=0 suspectscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2403210000
 definitions=main-2404010096
X-Proofpoint-GUID: hS9V8ipQgoBOKynAhZJWSleXfJigRvcJ
X-Proofpoint-ORIG-GUID: hS9V8ipQgoBOKynAhZJWSleXfJigRvcJ

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.

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.


-- 
Chuck Lever