Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1517601 > unrolled thread

Re: [PATCH v2] fs/nfsd/nfs4callback: Remove deprecated create_singlethread_workqueue

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] fs/nfsd/nfs4callback: Remove deprecated  create_singlethread_workqueue "J. Bruce Fields" <bfields@fieldses.org> - 2016-11-08 22:40 +0100
    Re: [PATCH v2] fs/nfsd/nfs4callback: Remove deprecated  create_singlethread_workqueue Tejun Heo <tj@kernel.org> - 2016-11-09 00:00 +0100
      Re: [PATCH v2] fs/nfsd/nfs4callback: Remove deprecated  create_singlethread_workqueue "J. Bruce Fields" <bfields@fieldses.org> - 2016-11-09 02:30 +0100
        Re: [PATCH v2] fs/nfsd/nfs4callback: Remove deprecated  create_singlethread_workqueue Jeff Layton <jlayton@poochiereds.net> - 2016-11-09 14:20 +0100
          Re: [PATCH v2] fs/nfsd/nfs4callback: Remove deprecated  create_singlethread_workqueue Trond Myklebust <trondmy@primarydata.com> - 2016-11-09 16:10 +0100
            Re: [PATCH v2] fs/nfsd/nfs4callback: Remove deprecated  create_singlethread_workqueue Jeff Layton <jlayton@poochiereds.net> - 2016-11-09 16:20 +0100
          Re: [PATCH v2] fs/nfsd/nfs4callback: Remove deprecated  create_singlethread_workqueue "J. Bruce Fields" <bfields@fieldses.org> - 2016-11-09 17:30 +0100
            Re: [PATCH v2] fs/nfsd/nfs4callback: Remove deprecated  create_singlethread_workqueue Jeff Layton <jlayton@poochiereds.net> - 2016-11-09 18:40 +0100
              Re: [PATCH v2] fs/nfsd/nfs4callback: Remove deprecated  create_singlethread_workqueue "J. Bruce Fields" <bfields@fieldses.org> - 2016-11-09 20:50 +0100
                Re: [PATCH v2] fs/nfsd/nfs4callback: Remove deprecated  create_singlethread_workqueue "J. Bruce Fields" <bfields@fieldses.org> - 2016-11-09 21:30 +0100
                  Re: [PATCH v2] fs/nfsd/nfs4callback: Remove deprecated  create_singlethread_workqueue Jeff Layton <jlayton@poochiereds.net> - 2016-11-09 23:40 +0100

#1517601 — Re: [PATCH v2] fs/nfsd/nfs4callback: Remove deprecated create_singlethread_workqueue

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

[toc] | [next] | [standalone]

#1517631

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.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1517713

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.

[toc] | [prev] | [next] | [standalone]

#1518142

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.

-- 
Jeff Layton <jlayton@poochiereds.net>

[toc] | [prev] | [next] | [standalone]

#1518248

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

[toc] | [prev] | [next] | [standalone]

#1518255

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>

[toc] | [prev] | [next] | [standalone]

#1518291

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>

[toc] | [prev] | [next] | [standalone]

#1518334

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?

-- 
Jeff Layton <jlayton@poochiereds.net>

[toc] | [prev] | [next] | [standalone]

#1518436

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.

[toc] | [prev] | [next] | [standalone]

#1518456

On Wed, Nov 09, 2016 at 02:47:24PM -0500, J. Bruce Fields wrote:
> 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()?

Oh, wait, I missed the _ordered_ in:

	#define create_singlethread_workqueue(name) \
	        alloc_ordered_workqueue("%s", __WQ_LEGACY | WQ_MEM_RECLAIM, name)

So our existing create_singlethread_workqueue is fine--but we probably
don't need those flags:

--b.

diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 211dc2aed8e1..eb78109d666c 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -1061,7 +1061,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_ordered_workqueue("nfsd4_callbacks", 0);
 	if (!callback_wq)
 		return -ENOMEM;
 	return 0;

[toc] | [prev] | [next] | [standalone]

#1518531

On Wed, 2016-11-09 at 15:23 -0500, J. Bruce Fields wrote:
On Wed, Nov 09, 2016 at 02:47:24PM -0500, J. Bruce Fields wrote:

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()?

Oh, wait, I missed the _ordered_ in:

	#define create_singlethread_workqueue(name) \
	        alloc_ordered_workqueue("%s", __WQ_LEGACY | WQ_MEM_RECLAIM, name)

So our existing create_singlethread_workqueue is fine--but we probably
don't need those flags:

--b.

diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 211dc2aed8e1..eb78109d666c 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -1061,7 +1061,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_ordered_workqueue("nfsd4_callbacks", 0);
 	if (!callback_wq)
 		return -ENOMEM;
 	return 0;

That looks good to me. Eventually we could better serialize the handling
of the callback code and move to an unordered workqueue, but for now I
think Bruce is right that we have to keep it.


Reviewed-by: Jeff Layton <jlayton@redhat.com>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web