| Message ID | 1443875882-12089-1-git-send-email-jeff.layton@primarydata.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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(-)