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@xxxxxxxxxxxx>
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@xxxxxxxxxx>
Signed-off-by: J. Bruce Fields <bfields@xxxxxxxxxx>
...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.
Thank you Chuck!
-Dai
