| Message ID | 20160830205348.GA31915@Karyakshetra (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
On Wed, 2016-08-31 at 02:23 +0530, Bhaktipriya Shridhar wrote: > The workqueue "callback_wq" queues a single work item &cb->cb_work > per > nfsd4_callback instance and thus, it doesn't require execution > ordering. > Hence, alloc_workqueue has been used to replace the > deprecated create_singlethread_workqueue instance. > > The WQ_MEM_RECLAIM flag has not been set since this is an in-kernel > nfs > server and isn't involved in memory reclaim operations on the local > host. > > Since there are fixed number of work items, explicit concurrency > limit is unnecessary here. > > Signed-off-by: Bhaktipriya Shridhar <bhaktipriya96@gmail.com> > --- > Changes in v2: > - No change. Made this a separate patch (categorised based on > directories). > > 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 7389cb1..a6611c6 100644 > --- a/fs/nfsd/nfs4callback.c > +++ b/fs/nfsd/nfs4callback.c > @@ -1021,7 +1021,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("nfsd4_callbacks", 0, 0); > if (!callback_wq) > return -ENOMEM; > return 0; > -- > 2.1.4 > Hah! I have almost exactly the same patch in my tree. I've only not sent it because I haven't had the chance to test it well. The only difference in mine is that it passes in WQ_UNBOUND. ISTM that we don't really need a bound workqueue here since we only use this to kick off callbacks to the client. I doubt we'd get much out of strictly maintaining cache locality here, and we're better off just sending it the callback as quickly as possible. Thoughts? -- Jeff Layton <jlayton@poochiereds.net> -- 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
Hello, Jeff. On Tue, Aug 30, 2016 at 05:07:23PM -0400, Jeff Layton wrote: > Hah! I have almost exactly the same patch in my tree. I've only not > sent it because I haven't had the chance to test it well. > > The only difference in mine is that it passes in WQ_UNBOUND. ISTM that > we don't really need a bound workqueue here since we only use this to > kick off callbacks to the client. I doubt we'd get much out of strictly > maintaining cache locality here, and we're better off just sending it > the callback as quickly as possible. We recently broke strong locality guarantee for users which don't use queue_work_on(), so the default locality is now only for optimization instead of correctness anyway. Unless there are actual benefits to using WQ_UNBOUND, I think in general it's better to stick with as little attributes as possible so that we have more maneuvering room down the line. But, yeah, if this can impact performance in subtle ways, it could be best to just do an identity conversion at least for now. Thanks.
On Wed, 2016-08-31 at 10:39 -0400, Tejun Heo wrote: > Hello, Jeff. > > On Tue, Aug 30, 2016 at 05:07:23PM -0400, Jeff Layton wrote: > > > > Hah! I have almost exactly the same patch in my tree. I've only not > > sent it because I haven't had the chance to test it well. > > > > The only difference in mine is that it passes in WQ_UNBOUND. ISTM > > that > > we don't really need a bound workqueue here since we only use this > > to > > kick off callbacks to the client. I doubt we'd get much out of > > strictly > > maintaining cache locality here, and we're better off just sending > > it > > the callback as quickly as possible. > > We recently broke strong locality guarantee for users which don't use > queue_work_on(), so the default locality is now only for optimization > instead of correctness anyway. Unless there are actual benefits to > using WQ_UNBOUND, I think in general it's better to stick with as > little attributes as possible so that we have more maneuvering room > down the line. But, yeah, if this can impact performance in subtle > ways, it could be best to just do an identity conversion at least for > now. > > Thanks. > Ahh ok, thanks...that's good to know. If you think we don't need WQ_UNBOUND then this is fine with me. Reviewed-by: Jeff Layton <jlayton@redhat.com> -- 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
Apologies, just cleaning out old mail and finding some I should have responded to long ago: On Wed, Aug 31, 2016 at 02:23:48AM +0530, Bhaktipriya Shridhar wrote: > The workqueue "callback_wq" queues a single work item &cb->cb_work per > nfsd4_callback instance and thus, it doesn't require execution ordering. What's "execution ordering"? We definitely do depend on the fact that at most one of these is running at a time. --b. > Hence, alloc_workqueue has been used to replace the > deprecated create_singlethread_workqueue instance. > > The WQ_MEM_RECLAIM flag has not been set since this is an in-kernel nfs > server and isn't involved in memory reclaim operations on the local > host. > > Since there are fixed number of work items, explicit concurrency > limit is unnecessary here. > > Signed-off-by: Bhaktipriya Shridhar <bhaktipriya96@gmail.com> > --- > Changes in v2: > - No change. Made this a separate patch (categorised based on > directories). > > 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 7389cb1..a6611c6 100644 > --- a/fs/nfsd/nfs4callback.c > +++ b/fs/nfsd/nfs4callback.c > @@ -1021,7 +1021,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("nfsd4_callbacks", 0, 0); > if (!callback_wq) > return -ENOMEM; > return 0; > -- > 2.1.4 -- 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
Hello, Bruce. On Tue, Nov 08, 2016 at 04:39:11PM -0500, J. Bruce Fields wrote: > Apologies, just cleaning out old mail and finding some I should have > responded to long ago: > > On Wed, Aug 31, 2016 at 02:23:48AM +0530, Bhaktipriya Shridhar wrote: > > The workqueue "callback_wq" queues a single work item &cb->cb_work per > > nfsd4_callback instance and thus, it doesn't require execution ordering. > > What's "execution ordering"? > > We definitely do depend on the fact that at most one of these is running > at a time. If there can be multiple cb's and thus cb->cb_work's per callback_wq, it'd need explicit ordering. Is that the case? Thanks.
On Tue, Nov 08, 2016 at 05:52:21PM -0500, Tejun Heo wrote: > Hello, Bruce. > > On Tue, Nov 08, 2016 at 04:39:11PM -0500, J. Bruce Fields wrote: > > Apologies, just cleaning out old mail and finding some I should have > > responded to long ago: > > > > On Wed, Aug 31, 2016 at 02:23:48AM +0530, Bhaktipriya Shridhar wrote: > > > The workqueue "callback_wq" queues a single work item &cb->cb_work per > > > nfsd4_callback instance and thus, it doesn't require execution ordering. > > > > What's "execution ordering"? > > > > We definitely do depend on the fact that at most one of these is running > > at a time. > > If there can be multiple cb's and thus cb->cb_work's per callback_wq, > it'd need explicit ordering. Is that the case? Yes, there can be multiple cb_work's. --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 Tue, 2016-11-08 at 20:27 -0500, J. Bruce Fields wrote: > On Tue, Nov 08, 2016 at 05:52:21PM -0500, Tejun Heo wrote: > > > > Hello, Bruce. > > > > On Tue, Nov 08, 2016 at 04:39:11PM -0500, J. Bruce Fields wrote: > > > > > > Apologies, just cleaning out old mail and finding some I should have > > > responded to long ago: > > > > > > On Wed, Aug 31, 2016 at 02:23:48AM +0530, Bhaktipriya Shridhar wrote: > > > > > > > > The workqueue "callback_wq" queues a single work item &cb->cb_work per > > > > nfsd4_callback instance and thus, it doesn't require execution ordering. > > > > > > What's "execution ordering"? > > > AIUI, it means that jobs are always run in the order queued and are serialized. > > > We definitely do depend on the fact that at most one of these is running > > > at a time. > > We do? > > If there can be multiple cb's and thus cb->cb_work's per callback_wq, > > it'd need explicit ordering. Is that the case? > These are basically client RPC tasks, and the cb_work just handles the submission into the client RPC state machine. Just because we're running several callbacks at the same time doesn't mean that they need to be strictly ordered. The client state machine can certainly handle running these in parallel. > Yes, there can be multiple cb_work's. > Yes, but each is effectively a separate work unit. I see no reason why we'd need to order them at all.
On Wed, 2016-11-09 at 08:18 -0500, Jeff Layton wrote: > On Tue, 2016-11-08 at 20:27 -0500, J. Bruce Fields wrote: > > > > On Tue, Nov 08, 2016 at 05:52:21PM -0500, Tejun Heo wrote: > > > > > > > > > Hello, Bruce. > > > > > > On Tue, Nov 08, 2016 at 04:39:11PM -0500, J. Bruce Fields wrote: > > > > > > > > > > > > Apologies, just cleaning out old mail and finding some I should > > > > have > > > > responded to long ago: > > > > > > > > On Wed, Aug 31, 2016 at 02:23:48AM +0530, Bhaktipriya Shridhar > > > > wrote: > > > > > > > > > > > > > > > The workqueue "callback_wq" queues a single work item &cb- > > > > > >cb_work per > > > > > nfsd4_callback instance and thus, it doesn't require > > > > > execution ordering. > > > > > > > > What's "execution ordering"? > > > > > > AIUI, it means that jobs are always run in the order queued and are > serialized. > > > > > > > > > > > > > > We definitely do depend on the fact that at most one of these > > > > is running > > > > at a time. > > > > > We do? > > > > > > > > > If there can be multiple cb's and thus cb->cb_work's per > > > callback_wq, > > > it'd need explicit ordering. Is that the case? > > > > These are basically client RPC tasks, and the cb_work just handles > the > submission into the client RPC state machine. Just because we're > running > several callbacks at the same time doesn't mean that they need to be > strictly ordered. The client state machine can certainly handle > running > these in parallel. > > > > > Yes, there can be multiple cb_work's. > > > > Yes, but each is effectively a separate work unit. I see no reason > why > we'd need to order them at all. > There needs to be serialisation at the session level (i.e. the callbacks have to respect the slot limits set by the client) however there shouldn’t be a need for serialisation at the RPC level. Cheers Trond
On Wed, 2016-11-09 at 15:08 +0000, Trond Myklebust wrote: > On Wed, 2016-11-09 at 08:18 -0500, Jeff Layton wrote: > > > > On Tue, 2016-11-08 at 20:27 -0500, J. Bruce Fields wrote: > > > > > > > > > On Tue, Nov 08, 2016 at 05:52:21PM -0500, Tejun Heo wrote: > > > > > > > > > > > > > > > > Hello, Bruce. > > > > > > > > On Tue, Nov 08, 2016 at 04:39:11PM -0500, J. Bruce Fields wrote: > > > > > > > > > > > > > > > > > > > > Apologies, just cleaning out old mail and finding some I should > > > > > have > > > > > responded to long ago: > > > > > > > > > > On Wed, Aug 31, 2016 at 02:23:48AM +0530, Bhaktipriya Shridhar > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > The workqueue "callback_wq" queues a single work item &cb- > > > > > > > > > > > > > > cb_work per > > > > > > nfsd4_callback instance and thus, it doesn't require > > > > > > execution ordering. > > > > > > > > > > What's "execution ordering"? > > > > > > > > > AIUI, it means that jobs are always run in the order queued and are > > serialized. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > We definitely do depend on the fact that at most one of these > > > > > is running > > > > > at a time. > > > > > > > > We do? > > > > > > > > > > > > > > > > > > > > If there can be multiple cb's and thus cb->cb_work's per > > > > callback_wq, > > > > it'd need explicit ordering. Is that the case? > > > > > > > These are basically client RPC tasks, and the cb_work just handles > > the > > submission into the client RPC state machine. Just because we're > > running > > several callbacks at the same time doesn't mean that they need to be > > strictly ordered. The client state machine can certainly handle > > running > > these in parallel. > > > > > > > > > > > Yes, there can be multiple cb_work's. > > > > > > > Yes, but each is effectively a separate work unit. I see no reason > > why > > we'd need to order them at all. > > > > There needs to be serialisation at the session level (i.e. the > callbacks have to respect the slot limits set by the client) however > there shouldn’t be a need for serialisation at the RPC level. > > Cheers > Trond Yes, that all happens in nfsd4_cb_prepare, which is the rpc_call_prepare operation for the callback. That gets run by the rpc state machine in the context of the rpciod workqueues. None of that happens in the context of the cb_work here. If you have a look at nfsd4_run_cb_work, you can see that it just does a cb_ops->prepare and then submits it to the client rpc engine with rpc_call_async. None of that should require singlethreaded workqueue semantics. -- Jeff Layton <jlayton@poochiereds.net> -- 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 Wed, Nov 09, 2016 at 08:18:08AM -0500, Jeff Layton wrote: > On Tue, 2016-11-08 at 20:27 -0500, J. Bruce Fields wrote: > > On Tue, Nov 08, 2016 at 05:52:21PM -0500, Tejun Heo wrote: > > > > > > Hello, Bruce. > > > > > > On Tue, Nov 08, 2016 at 04:39:11PM -0500, J. Bruce Fields wrote: > > > > > > > > Apologies, just cleaning out old mail and finding some I should have > > > > responded to long ago: > > > > > > > > On Wed, Aug 31, 2016 at 02:23:48AM +0530, Bhaktipriya Shridhar wrote: > > > > > > > > > > The workqueue "callback_wq" queues a single work item &cb->cb_work per > > > > > nfsd4_callback instance and thus, it doesn't require execution ordering. > > > > > > > > What's "execution ordering"? > > > > > > AIUI, it means that jobs are always run in the order queued and are > serialized. > > > > > We definitely do depend on the fact that at most one of these is running > > > > at a time. > > > > > We do? > > > > If there can be multiple cb's and thus cb->cb_work's per callback_wq, > > > it'd need explicit ordering. Is that the case? > > > > These are basically client RPC tasks, and the cb_work just handles the > submission into the client RPC state machine. Just because we're running > several callbacks at the same time doesn't mean that they need to be > strictly ordered. The client state machine can certainly handle running > these in parallel. I'm not worried about the rpc calls themselves, I'm worried about the other stuff in nfsd4_run_cb_work(), especially nfsd4_process_cb_update(). It's been a while since I thought about it and maybe it'd be OK with a little bit of extra locking. --b. > > Yes, there can be multiple cb_work's. > > > > Yes, but each is effectively a separate work unit. I see no reason why > we'd need to order them at all. > > -- > Jeff Layton <jlayton@poochiereds.net> -- 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 Wed, 2016-11-09 at 11:27 -0500, J. Bruce Fields wrote: > On Wed, Nov 09, 2016 at 08:18:08AM -0500, Jeff Layton wrote: > > > > On Tue, 2016-11-08 at 20:27 -0500, J. Bruce Fields wrote: > > > > > > On Tue, Nov 08, 2016 at 05:52:21PM -0500, Tejun Heo wrote: > > > > > > > > > > > > Hello, Bruce. > > > > > > > > On Tue, Nov 08, 2016 at 04:39:11PM -0500, J. Bruce Fields wrote: > > > > > > > > > > > > > > > Apologies, just cleaning out old mail and finding some I should have > > > > > responded to long ago: > > > > > > > > > > On Wed, Aug 31, 2016 at 02:23:48AM +0530, Bhaktipriya Shridhar wrote: > > > > > > > > > > > > > > > > > > The workqueue "callback_wq" queues a single work item &cb->cb_work per > > > > > > nfsd4_callback instance and thus, it doesn't require execution ordering. > > > > > > > > > > What's "execution ordering"? > > > > > > > > > AIUI, it means that jobs are always run in the order queued and are > > serialized. > > > > > > > > > > > > > > > > > > > We definitely do depend on the fact that at most one of these is running > > > > > at a time. > > > > > > > > We do? > > > > > > > > > > > > > If there can be multiple cb's and thus cb->cb_work's per callback_wq, > > > > it'd need explicit ordering. Is that the case? > > > > > > > These are basically client RPC tasks, and the cb_work just handles the > > submission into the client RPC state machine. Just because we're running > > several callbacks at the same time doesn't mean that they need to be > > strictly ordered. The client state machine can certainly handle running > > these in parallel. > > I'm not worried about the rpc calls themselves, I'm worried about the > other stuff in nfsd4_run_cb_work(), especially > nfsd4_process_cb_update(). > > It's been a while since I thought about it and maybe it'd be OK with a > little bit of extra locking. > > --b. > Ahh good point there. nfsd4_process_cb_update is a bit of a special case, and I hadn't considered that. I think we could use the cl_lock to protect most of the fields that are affected there. I'm not sure how to handle setup_callback_client though. Should we serialize those calls so that we're only constructing one at a time and have other threads wait on it? We could use a cl_flags bit for a NFSD4_CLIENT_CB_CONSTRUCTING flag and serialize on those?
On Wed, Nov 09, 2016 at 12:33:35PM -0500, Jeff Layton wrote: > On Wed, 2016-11-09 at 11:27 -0500, J. Bruce Fields wrote: > > On Wed, Nov 09, 2016 at 08:18:08AM -0500, Jeff Layton wrote: > > > > > > On Tue, 2016-11-08 at 20:27 -0500, J. Bruce Fields wrote: > > > > > > > > On Tue, Nov 08, 2016 at 05:52:21PM -0500, Tejun Heo wrote: > > > > > > > > > > > > > > > Hello, Bruce. > > > > > > > > > > On Tue, Nov 08, 2016 at 04:39:11PM -0500, J. Bruce Fields wrote: > > > > > > > > > > > > > > > > > > Apologies, just cleaning out old mail and finding some I should have > > > > > > responded to long ago: > > > > > > > > > > > > On Wed, Aug 31, 2016 at 02:23:48AM +0530, Bhaktipriya Shridhar wrote: > > > > > > > > > > > > > > > > > > > > > The workqueue "callback_wq" queues a single work item &cb->cb_work per > > > > > > > nfsd4_callback instance and thus, it doesn't require execution ordering. > > > > > > > > > > > > What's "execution ordering"? > > > > > > > > > > > > AIUI, it means that jobs are always run in the order queued and are > > > serialized. > > > > > > > > > > > > > > > > > > > > > > > > We definitely do depend on the fact that at most one of these is running > > > > > > at a time. > > > > > > > > > > > We do? > > > > > > > > > > > > > > > > > If there can be multiple cb's and thus cb->cb_work's per callback_wq, > > > > > it'd need explicit ordering. Is that the case? > > > > > > > > > > These are basically client RPC tasks, and the cb_work just handles the > > > submission into the client RPC state machine. Just because we're running > > > several callbacks at the same time doesn't mean that they need to be > > > strictly ordered. The client state machine can certainly handle running > > > these in parallel. > > > > I'm not worried about the rpc calls themselves, I'm worried about the > > other stuff in nfsd4_run_cb_work(), especially > > nfsd4_process_cb_update(). > > > > It's been a while since I thought about it and maybe it'd be OK with a > > little bit of extra locking. > > > > --b. > > > > Ahh good point there. nfsd4_process_cb_update is a bit of a special > case, and I hadn't considered that. > > I think we could use the cl_lock to protect most of the fields that are > affected there. > > I'm not sure how to handle setup_callback_client though. Should we > serialize those calls so that we're only constructing one at a time and > have other threads wait on it? We could use a cl_flags bit for a > NFSD4_CLIENT_CB_CONSTRUCTING flag and serialize on those? For now I wish we could just like to continue assuming the workqueue processes only one item at a time. Do we have that now, or do we need to switch to (looking at workqueue.h...) alloc_ordered workqueue()? --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 7389cb1..a6611c6 100644 --- a/fs/nfsd/nfs4callback.c +++ b/fs/nfsd/nfs4callback.c @@ -1021,7 +1021,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("nfsd4_callbacks", 0, 0); if (!callback_wq) return -ENOMEM; return 0;
The workqueue "callback_wq" queues a single work item &cb->cb_work per nfsd4_callback instance and thus, it doesn't require execution ordering. Hence, alloc_workqueue has been used to replace the deprecated create_singlethread_workqueue instance. The WQ_MEM_RECLAIM flag has not been set since this is an in-kernel nfs server and isn't involved in memory reclaim operations on the local host. Since there are fixed number of work items, explicit concurrency limit is unnecessary here. Signed-off-by: Bhaktipriya Shridhar <bhaktipriya96@gmail.com> --- Changes in v2: - No change. Made this a separate patch (categorised based on directories). fs/nfsd/nfs4callback.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.1.4 -- 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