* [PATCH] nfsd: use a multithreaded workqueue for nfsd4_callbacks @ 2015-10-03 12:38 Jeff Layton 2015-10-09 21:24 ` J. Bruce Fields 0 siblings, 1 reply; 8+ messages in thread From: Jeff Layton @ 2015-10-03 12:38 UTC (permalink / raw) To: bfields; +Cc: linux-nfs I don't see any need to order callbacks with respect to one another. Also, these are generally not involved in memory reclaim, so I don't see the need for a rescuer thread here either. Signed-off-by: Jeff Layton <jeff.layton@primarydata.com> --- fs/nfsd/nfs4callback.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c index e7f50c4081d6..7dabbb436290 100644 --- a/fs/nfsd/nfs4callback.c +++ b/fs/nfsd/nfs4callback.c @@ -1017,7 +1017,7 @@ static const struct rpc_call_ops nfsd4_cb_ops = { int nfsd4_create_callback_queue(void) { - callback_wq = create_singlethread_workqueue("nfsd4_callbacks"); + callback_wq = alloc_workqueue("%s", WQ_UNBOUND, 0, "nfsd4_callbacks"); if (!callback_wq) return -ENOMEM; return 0; -- 2.4.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] nfsd: use a multithreaded workqueue for nfsd4_callbacks 2015-10-03 12:38 [PATCH] nfsd: use a multithreaded workqueue for nfsd4_callbacks Jeff Layton @ 2015-10-09 21:24 ` J. Bruce Fields 2015-10-09 21:39 ` Jeff Layton 0 siblings, 1 reply; 8+ messages in thread From: J. Bruce Fields @ 2015-10-09 21:24 UTC (permalink / raw) To: Jeff Layton; +Cc: linux-nfs On Sat, Oct 03, 2015 at 08:38:02AM -0400, Jeff Layton wrote: > I don't see any need to order callbacks with respect to one another. I thought the code in nfsd4_process_cb_update really depended on this. The locking it has is against nfsd threads, it probably assumes it's only run from a cb thread and that it's the only one running at a time. But I haven't reviewed it lately. --b. > Also, these are generally not involved in memory reclaim, so I don't see > the need for a rescuer thread here either. > > Signed-off-by: Jeff Layton <jeff.layton@primarydata.com> > --- > fs/nfsd/nfs4callback.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c > index e7f50c4081d6..7dabbb436290 100644 > --- a/fs/nfsd/nfs4callback.c > +++ b/fs/nfsd/nfs4callback.c > @@ -1017,7 +1017,7 @@ static const struct rpc_call_ops nfsd4_cb_ops = { > > int nfsd4_create_callback_queue(void) > { > - callback_wq = create_singlethread_workqueue("nfsd4_callbacks"); > + callback_wq = alloc_workqueue("%s", WQ_UNBOUND, 0, "nfsd4_callbacks"); > if (!callback_wq) > return -ENOMEM; > return 0; > -- > 2.4.3 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] nfsd: use a multithreaded workqueue for nfsd4_callbacks 2015-10-09 21:24 ` J. Bruce Fields @ 2015-10-09 21:39 ` Jeff Layton 2015-10-12 18:11 ` J. Bruce Fields 0 siblings, 1 reply; 8+ messages in thread From: Jeff Layton @ 2015-10-09 21:39 UTC (permalink / raw) To: J. Bruce Fields; +Cc: linux-nfs On Fri, 9 Oct 2015 17:24:59 -0400 "J. Bruce Fields" <bfields@fieldses.org> wrote: > On Sat, Oct 03, 2015 at 08:38:02AM -0400, Jeff Layton wrote: > > I don't see any need to order callbacks with respect to one another. > > I thought the code in nfsd4_process_cb_update really depended on this. > The locking it has is against nfsd threads, it probably assumes it's > only run from a cb thread and that it's the only one running at a time. > > But I haven't reviewed it lately. > > --b. > Yikes -- ok. That's not at all obvious. That should prob be documented if so. Yeah, ok...I guess you could end up with multiple threads racing to tear down the cb_client and xprt and create a new one, and it looks like the cl_cb_client and cl_cred pointers could get clobbered by new ones in that case. Shouldn't be too hard to fix protecting those pointers with the cl_lock. That said, I prob won't be able to spend time on it right now. You can go ahead and drop that patch and I'll resend if/when I get around to fixing that. Thanks for having a look... > > Also, these are generally not involved in memory reclaim, so I don't see > > the need for a rescuer thread here either. > > > > Signed-off-by: Jeff Layton <jeff.layton@primarydata.com> > > --- > > fs/nfsd/nfs4callback.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c > > index e7f50c4081d6..7dabbb436290 100644 > > --- a/fs/nfsd/nfs4callback.c > > +++ b/fs/nfsd/nfs4callback.c > > @@ -1017,7 +1017,7 @@ static const struct rpc_call_ops nfsd4_cb_ops = { > > > > int nfsd4_create_callback_queue(void) > > { > > - callback_wq = create_singlethread_workqueue("nfsd4_callbacks"); > > + callback_wq = alloc_workqueue("%s", WQ_UNBOUND, 0, "nfsd4_callbacks"); > > if (!callback_wq) > > return -ENOMEM; > > return 0; > > -- > > 2.4.3 -- Jeff Layton <jlayton@poochiereds.net> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] nfsd: use a multithreaded workqueue for nfsd4_callbacks 2015-10-09 21:39 ` Jeff Layton @ 2015-10-12 18:11 ` J. Bruce Fields 2015-10-12 20:14 ` Jeff Layton 0 siblings, 1 reply; 8+ messages in thread From: J. Bruce Fields @ 2015-10-12 18:11 UTC (permalink / raw) To: Jeff Layton; +Cc: linux-nfs On Fri, Oct 09, 2015 at 05:39:23PM -0400, Jeff Layton wrote: > On Fri, 9 Oct 2015 17:24:59 -0400 > "J. Bruce Fields" <bfields@fieldses.org> wrote: > > > On Sat, Oct 03, 2015 at 08:38:02AM -0400, Jeff Layton wrote: > > > I don't see any need to order callbacks with respect to one another. > > > > I thought the code in nfsd4_process_cb_update really depended on this. > > The locking it has is against nfsd threads, it probably assumes it's > > only run from a cb thread and that it's the only one running at a time. > > > > But I haven't reviewed it lately. > > > > --b. > > > > Yikes -- ok. That's not at all obvious. That should prob be documented > if so. Yes, my bad. > Yeah, ok...I guess you could end up with multiple threads racing to > tear down the cb_client and xprt and create a new one, and it looks > like the cl_cb_client and cl_cred pointers could get clobbered by new > ones in that case. > > Shouldn't be too hard to fix protecting those pointers with the > cl_lock. That said, I prob won't be able to spend time on it right now. > You can go ahead and drop that patch and I'll resend if/when I get > around to fixing that. What's the problem that this fixes exactly? --b. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] nfsd: use a multithreaded workqueue for nfsd4_callbacks 2015-10-12 18:11 ` J. Bruce Fields @ 2015-10-12 20:14 ` Jeff Layton 2015-10-12 20:20 ` J. Bruce Fields 0 siblings, 1 reply; 8+ messages in thread From: Jeff Layton @ 2015-10-12 20:14 UTC (permalink / raw) To: J. Bruce Fields; +Cc: linux-nfs On Mon, 12 Oct 2015 14:11:17 -0400 "J. Bruce Fields" <bfields@fieldses.org> wrote: > On Fri, Oct 09, 2015 at 05:39:23PM -0400, Jeff Layton wrote: > > On Fri, 9 Oct 2015 17:24:59 -0400 > > "J. Bruce Fields" <bfields@fieldses.org> wrote: > > > > > On Sat, Oct 03, 2015 at 08:38:02AM -0400, Jeff Layton wrote: > > > > I don't see any need to order callbacks with respect to one another. > > > > > > I thought the code in nfsd4_process_cb_update really depended on this. > > > The locking it has is against nfsd threads, it probably assumes it's > > > only run from a cb thread and that it's the only one running at a time. > > > > > > But I haven't reviewed it lately. > > > > > > --b. > > > > > > > Yikes -- ok. That's not at all obvious. That should prob be documented > > if so. > > Yes, my bad. > > > Yeah, ok...I guess you could end up with multiple threads racing to > > tear down the cb_client and xprt and create a new one, and it looks > > like the cl_cb_client and cl_cred pointers could get clobbered by new > > ones in that case. > > > > Shouldn't be too hard to fix protecting those pointers with the > > cl_lock. That said, I prob won't be able to spend time on it right now. > > You can go ahead and drop that patch and I'll resend if/when I get > > around to fixing that. > > What's the problem that this fixes exactly? > Unnecessary serialization of callbacks? Not that that's necessarily a problem, but with the newer workqueue implementation there is more overhead in running a single threaded workqueue since it implies a rescuer thread and has to do extra work to ensure that the jobs are run in sequential order. If that serialization is not actually needed, then it's better to move it to a multithreaded workqueue. -- Jeff Layton <jlayton@poochiereds.net> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] nfsd: use a multithreaded workqueue for nfsd4_callbacks 2015-10-12 20:14 ` Jeff Layton @ 2015-10-12 20:20 ` J. Bruce Fields 2015-10-12 21:02 ` Jeff Layton 0 siblings, 1 reply; 8+ messages in thread From: J. Bruce Fields @ 2015-10-12 20:20 UTC (permalink / raw) To: Jeff Layton; +Cc: linux-nfs On Mon, Oct 12, 2015 at 04:14:21PM -0400, Jeff Layton wrote: > On Mon, 12 Oct 2015 14:11:17 -0400 > "J. Bruce Fields" <bfields@fieldses.org> wrote: > > > On Fri, Oct 09, 2015 at 05:39:23PM -0400, Jeff Layton wrote: > > > On Fri, 9 Oct 2015 17:24:59 -0400 > > > "J. Bruce Fields" <bfields@fieldses.org> wrote: > > > > > > > On Sat, Oct 03, 2015 at 08:38:02AM -0400, Jeff Layton wrote: > > > > > I don't see any need to order callbacks with respect to one another. > > > > > > > > I thought the code in nfsd4_process_cb_update really depended on this. > > > > The locking it has is against nfsd threads, it probably assumes it's > > > > only run from a cb thread and that it's the only one running at a time. > > > > > > > > But I haven't reviewed it lately. > > > > > > > > --b. > > > > > > > > > > Yikes -- ok. That's not at all obvious. That should prob be documented > > > if so. > > > > Yes, my bad. > > > > > Yeah, ok...I guess you could end up with multiple threads racing to > > > tear down the cb_client and xprt and create a new one, and it looks > > > like the cl_cb_client and cl_cred pointers could get clobbered by new > > > ones in that case. > > > > > > Shouldn't be too hard to fix protecting those pointers with the > > > cl_lock. That said, I prob won't be able to spend time on it right now. > > > You can go ahead and drop that patch and I'll resend if/when I get > > > around to fixing that. > > > > What's the problem that this fixes exactly? > > > > Unnecessary serialization of callbacks? > > Not that that's necessarily a problem, but with the newer workqueue > implementation there is more overhead in running a single threaded > workqueue since it implies a rescuer thread and has to do extra work > to ensure that the jobs are run in sequential order. > > If that serialization is not actually needed, then it's better to move > it to a multithreaded workqueue. We'd still be serializing a lot of the same code, just using locks instead, wouldn't we? --b. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] nfsd: use a multithreaded workqueue for nfsd4_callbacks 2015-10-12 20:20 ` J. Bruce Fields @ 2015-10-12 21:02 ` Jeff Layton 2015-10-12 21:07 ` J. Bruce Fields 0 siblings, 1 reply; 8+ messages in thread From: Jeff Layton @ 2015-10-12 21:02 UTC (permalink / raw) To: J. Bruce Fields; +Cc: linux-nfs On Mon, 12 Oct 2015 16:20:44 -0400 "J. Bruce Fields" <bfields@fieldses.org> wrote: > On Mon, Oct 12, 2015 at 04:14:21PM -0400, Jeff Layton wrote: > > On Mon, 12 Oct 2015 14:11:17 -0400 > > "J. Bruce Fields" <bfields@fieldses.org> wrote: > > > > > On Fri, Oct 09, 2015 at 05:39:23PM -0400, Jeff Layton wrote: > > > > On Fri, 9 Oct 2015 17:24:59 -0400 > > > > "J. Bruce Fields" <bfields@fieldses.org> wrote: > > > > > > > > > On Sat, Oct 03, 2015 at 08:38:02AM -0400, Jeff Layton wrote: > > > > > > I don't see any need to order callbacks with respect to one another. > > > > > > > > > > I thought the code in nfsd4_process_cb_update really depended on this. > > > > > The locking it has is against nfsd threads, it probably assumes it's > > > > > only run from a cb thread and that it's the only one running at a time. > > > > > > > > > > But I haven't reviewed it lately. > > > > > > > > > > --b. > > > > > > > > > > > > > Yikes -- ok. That's not at all obvious. That should prob be documented > > > > if so. > > > > > > Yes, my bad. > > > > > > > Yeah, ok...I guess you could end up with multiple threads racing to > > > > tear down the cb_client and xprt and create a new one, and it looks > > > > like the cl_cb_client and cl_cred pointers could get clobbered by new > > > > ones in that case. > > > > > > > > Shouldn't be too hard to fix protecting those pointers with the > > > > cl_lock. That said, I prob won't be able to spend time on it right now. > > > > You can go ahead and drop that patch and I'll resend if/when I get > > > > around to fixing that. > > > > > > What's the problem that this fixes exactly? > > > > > > > Unnecessary serialization of callbacks? > > > > Not that that's necessarily a problem, but with the newer workqueue > > implementation there is more overhead in running a single threaded > > workqueue since it implies a rescuer thread and has to do extra work > > to ensure that the jobs are run in sequential order. > > > > If that serialization is not actually needed, then it's better to move > > it to a multithreaded workqueue. > > We'd still be serializing a lot of the same code, just using locks > instead, wouldn't we? > > --b. Any serialization would be per-clnt instead of global like it is today. Also, we'd likely only serialize changes to some of the pointers instead of the entire operation. -- Jeff Layton <jlayton@poochiereds.net> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] nfsd: use a multithreaded workqueue for nfsd4_callbacks 2015-10-12 21:02 ` Jeff Layton @ 2015-10-12 21:07 ` J. Bruce Fields 0 siblings, 0 replies; 8+ messages in thread From: J. Bruce Fields @ 2015-10-12 21:07 UTC (permalink / raw) To: Jeff Layton; +Cc: linux-nfs On Mon, Oct 12, 2015 at 05:02:19PM -0400, Jeff Layton wrote: > On Mon, 12 Oct 2015 16:20:44 -0400 > "J. Bruce Fields" <bfields@fieldses.org> wrote: > > > On Mon, Oct 12, 2015 at 04:14:21PM -0400, Jeff Layton wrote: > > > On Mon, 12 Oct 2015 14:11:17 -0400 > > > "J. Bruce Fields" <bfields@fieldses.org> wrote: > > > > > > > On Fri, Oct 09, 2015 at 05:39:23PM -0400, Jeff Layton wrote: > > > > > On Fri, 9 Oct 2015 17:24:59 -0400 > > > > > "J. Bruce Fields" <bfields@fieldses.org> wrote: > > > > > > > > > > > On Sat, Oct 03, 2015 at 08:38:02AM -0400, Jeff Layton wrote: > > > > > > > I don't see any need to order callbacks with respect to one another. > > > > > > > > > > > > I thought the code in nfsd4_process_cb_update really depended on this. > > > > > > The locking it has is against nfsd threads, it probably assumes it's > > > > > > only run from a cb thread and that it's the only one running at a time. > > > > > > > > > > > > But I haven't reviewed it lately. > > > > > > > > > > > > --b. > > > > > > > > > > > > > > > > Yikes -- ok. That's not at all obvious. That should prob be documented > > > > > if so. > > > > > > > > Yes, my bad. > > > > > > > > > Yeah, ok...I guess you could end up with multiple threads racing to > > > > > tear down the cb_client and xprt and create a new one, and it looks > > > > > like the cl_cb_client and cl_cred pointers could get clobbered by new > > > > > ones in that case. > > > > > > > > > > Shouldn't be too hard to fix protecting those pointers with the > > > > > cl_lock. That said, I prob won't be able to spend time on it right now. > > > > > You can go ahead and drop that patch and I'll resend if/when I get > > > > > around to fixing that. > > > > > > > > What's the problem that this fixes exactly? > > > > > > > > > > Unnecessary serialization of callbacks? > > > > > > Not that that's necessarily a problem, but with the newer workqueue > > > implementation there is more overhead in running a single threaded > > > workqueue since it implies a rescuer thread and has to do extra work > > > to ensure that the jobs are run in sequential order. > > > > > > If that serialization is not actually needed, then it's better to move > > > it to a multithreaded workqueue. > > > > We'd still be serializing a lot of the same code, just using locks > > instead, wouldn't we? > > > > --b. > > Any serialization would be per-clnt instead of global like it is today. > > Also, we'd likely only serialize changes to some of the pointers > instead of the entire operation. OK, makes sense. Probably not a priority for now, though. --b. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-10-12 21:07 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-10-03 12:38 [PATCH] nfsd: use a multithreaded workqueue for nfsd4_callbacks Jeff Layton 2015-10-09 21:24 ` J. Bruce Fields 2015-10-09 21:39 ` Jeff Layton 2015-10-12 18:11 ` J. Bruce Fields 2015-10-12 20:14 ` Jeff Layton 2015-10-12 20:20 ` J. Bruce Fields 2015-10-12 21:02 ` Jeff Layton 2015-10-12 21:07 ` J. Bruce Fields
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.