fs/nfsd/nfs4callback: Remove deprecated create_singlethread_workqueue

PATCH In review Bhaktipriya Shridhar 6 participants 15 messages Aug 30, 2016 – Nov 9, 2016

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

Bhaktipriya Shridhar

author

2016-08-30 20:53 UTC #0 lore raw

PATCH v2 1/1 fs/nfsd/nfs4callback: Remove deprecated create_singlethread_workqueue

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 <[email protected]>

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

fs/nfsd/nfs4callback.c +1 -1

index 7389cb1..a6611c6 100644
@@ -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

Jeff Layton

2016-08-30 21:07 UTC #1 lore raw

Quoted 37 lines · Bhaktipriya Shridhar

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 <[email protected]>
---
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(-)
fs/nfsd/nfs4callback.c +1 -1
index 7389cb1..a6611c6 100644
@@ -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)

Quoted 5 lines

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 <[email protected]>

Tejun Heo

2016-08-31 14:39 UTC #2 lore raw

Hello, Jeff.

Quoted 8 lines · Jeff Layton

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.

Signature · 1 line

tejun

Jeff Layton

Reviewed-by

2016-08-31 15:01 UTC #3 lore raw

Quoted 26 lines · Tejun Heo

Hello, Jeff.
Quoted 4 lines · Jeff Layton
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.
Quoted 9 lines
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.

Quoted 2 lines

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 <[email protected]>

J. Bruce Fields

2016-11-08 21:39 UTC #4 lore raw

In reply to Bhaktipriya Shridhar

Apologies, just cleaning out old mail and finding some I should have responded to long ago:

Quoted 2 lines · Bhaktipriya Shridhar

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.

Quoted 35 lines

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 <[email protected]>
---
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(-)
fs/nfsd/nfs4callback.c +1 -2
index 7389cb1..a6611c6 100644
@@ -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

Tejun Heo

2016-11-08 22:58 UTC #5 lore raw

Hello, Bruce.

Quoted 11 lines · J. Bruce Fields

Apologies, just cleaning out old mail and finding some I should have responded to long ago:
Quoted 2 lines · Bhaktipriya Shridhar
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.

Signature · 1 line

tejun

J. Bruce Fields

2016-11-09 01:27 UTC #6 lore raw

Quoted 10 lines · Tejun Heo

Hello, Bruce.
Quoted 7 lines · J. Bruce Fields
Apologies, just cleaning out old mail and finding some I should have responded to long ago:
Quoted 2 lines · Bhaktipriya Shridhar
The workqueue "callback_wq" queues a single work item &cb->cb_work per nfsd4_callback instance and thus, it doesn't require execution ordering.

Quoted 7 lines

Quoted 4 lines
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.

Jeff Layton

2016-11-09 13:18 UTC #7 lore raw

Quoted 9 lines · J. Bruce Fields

Quoted 8 lines · Tejun Heo
Hello, Bruce.
Quoted 4 lines · J. Bruce Fields
Apologies, just cleaning out old mail and finding some I should have responded to long ago:

Quoted 7 lines

Quoted 7 lines
Quoted 7 lines · Bhaktipriya Shridhar
Quoted 3 lines
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.

Quoted 3 lines

Quoted 3 lines
Quoted 2 lines
We definitely do depend on the fact that at most one of these is running at a time.

We do?

Quoted 3 lines

Quoted 2 lines
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.

Quoted 2 lines

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.

Signature · 1 line

Jeff Layton <[email protected]>

Trond Myklebust

2016-11-09 15:08 UTC #8 lore raw

T24gV2VkLCAyMDE2LTExLTA5IGF0IDA4OjE4IC0wNTAwLCBKZWZmIExheXRvbiB3cm90ZToNCj4g T24gVHVlLCAyMDE2LTExLTA4IGF0IDIwOjI3IC0wNTAwLCBKLiBCcnVjZSBGaWVsZHMgd3JvdGU6 DQo+ID4gDQo+ID4gT24gVHVlLCBOb3YgMDgsIDIwMTYgYXQgMDU6NTI6MjFQTSAtMDUwMCwgVGVq dW4gSGVvIHdyb3RlOg0KPiA+ID4gDQo+ID4gPiANCj4gPiA+IEhlbGxvLCBCcnVjZS4NCj4gPiA+ IA0KPiA+ID4gT24gVHVlLCBOb3YgMDgsIDIwMTYgYXQgMDQ6Mzk6MTFQTSAtMDUwMCwgSi4gQnJ1 Y2UgRmllbGRzIHdyb3RlOg0KPiA+ID4gPiANCj4gPiA+ID4gDQo+ID4gPiA+IEFwb2xvZ2llcywg anVzdCBjbGVhbmluZyBvdXQgb2xkIG1haWwgYW5kIGZpbmRpbmcgc29tZSBJIHNob3VsZA0KPiA+ ID4gPiBoYXZlDQo+ID4gPiA+IHJlc3BvbmRlZCB0byBsb25nIGFnbzoNCj4gPiA+ID4gDQo+ID4g PiA+IE9uIFdlZCwgQXVnIDMxLCAyMDE2IGF0IDAyOjIzOjQ4QU0gKzA1MzAsIEJoYWt0aXByaXlh IFNocmlkaGFyDQo+ID4gPiA+IHdyb3RlOg0KPiA+ID4gPiA+IA0KPiA+ID4gPiA+IA0KPiA+ID4g PiA+IFRoZSB3b3JrcXVldWUgImNhbGxiYWNrX3dxIiBxdWV1ZXMgYSBzaW5nbGUgd29yayBpdGVt ICZjYi0NCj4gPiA+ID4gPiA+Y2Jfd29yayBwZXINCj4gPiA+ID4gPiBuZnNkNF9jYWxsYmFjayBp bnN0YW5jZSBhbmQgdGh1cywgaXQgZG9lc24ndCByZXF1aXJlDQo+ID4gPiA+ID4gZXhlY3V0aW9u IG9yZGVyaW5nLg0KPiA+ID4gPiANCj4gPiA+ID4gV2hhdCdzICJleGVjdXRpb24gb3JkZXJpbmci Pw0KPiA+ID4gPiANCj4gDQo+IEFJVUksIGl0IG1lYW5zIHRoYXQgam9icyBhcmUgYWx3YXlzIHJ1 biBpbiB0aGUgb3JkZXIgcXVldWVkIGFuZCBhcmUNCj4gc2VyaWFsaXplZC4NCj4gDQo+ID4gDQo+ ID4gPiANCj4gPiA+ID4gDQo+ID4gPiA+IFdlIGRlZmluaXRlbHkgZG8gZGVwZW5kIG9uIHRoZSBm YWN0IHRoYXQgYXQgbW9zdCBvbmUgb2YgdGhlc2UNCj4gPiA+ID4gaXMgcnVubmluZw0KPiA+ID4g PiBhdCBhIHRpbWUuDQo+ID4gPiANCj4gDQo+IFdlIGRvPw0KPiANCj4gPiANCj4gPiA+IA0KPiA+ ID4gSWYgdGhlcmUgY2FuIGJlIG11bHRpcGxlIGNiJ3MgYW5kIHRodXMgY2ItPmNiX3dvcmsncyBw ZXINCj4gPiA+IGNhbGxiYWNrX3dxLA0KPiA+ID4gaXQnZCBuZWVkIGV4cGxpY2l0IG9yZGVyaW5n LsKgwqBJcyB0aGF0IHRoZSBjYXNlPw0KPiA+IA0KPiANCj4gVGhlc2UgYXJlIGJhc2ljYWxseSBj bGllbnQgUlBDIHRhc2tzLCBhbmQgdGhlIGNiX3dvcmsganVzdCBoYW5kbGVzDQo+IHRoZQ0KPiBz dWJtaXNzaW9uIGludG8gdGhlIGNsaWVudCBSUEMgc3RhdGUgbWFjaGluZS4gSnVzdCBiZWNhdXNl IHdlJ3JlDQo+IHJ1bm5pbmcNCj4gc2V2ZXJhbCBjYWxsYmFja3MgYXQgdGhlIHNhbWUgdGltZSBk b2Vzbid0IG1lYW4gdGhhdCB0aGV5IG5lZWQgdG8gYmUNCj4gc3RyaWN0bHkgb3JkZXJlZC4gVGhl IGNsaWVudCBzdGF0ZSBtYWNoaW5lIGNhbiBjZXJ0YWlubHkgaGFuZGxlDQo+IHJ1bm5pbmcNCj4g dGhlc2UgaW4gcGFyYWxsZWwuDQo+IA0KPiA+IA0KPiA+IFllcywgdGhlcmUgY2FuIGJlIG11bHRp cGxlIGNiX3dvcmsncy4NCj4gPiANCj4gDQo+IFllcywgYnV0IGVhY2ggaXMgZWZmZWN0aXZlbHkg YSBzZXBhcmF0ZSB3b3JrIHVuaXQuIEkgc2VlIG5vIHJlYXNvbg0KPiB3aHkNCj4gd2UnZCBuZWVk IHRvIG9yZGVyIHRoZW0gYXQgYWxsLg0KPiANCg0KVGhlcmUgbmVlZHMgdG8gYmUgc2VyaWFsaXNh dGlvbiBhdCB0aGUgc2Vzc2lvbiBsZXZlbCAoaS5lLiB0aGUNCmNhbGxiYWNrcyBoYXZlIHRvIHJl c3BlY3QgdGhlIHNsb3QgbGltaXRzIHNldCBieSB0aGUgY2xpZW50KSBob3dldmVyDQp0aGVyZSBz aG91bGRu4oCZdCBiZSBhIG5lZWQgZm9yIHNlcmlhbGlzYXRpb24gYXQgdGhlIFJQQyBsZXZlbC4N Cg0KQ2hlZXJzDQrCoCBUcm9uZA==

Jeff Layton

2016-11-09 15:17 UTC #9 lore raw

Quoted 77 lines · Trond Myklebust

Quoted 68 lines · Jeff Layton
Quoted 21 lines · J. Bruce Fields
Quoted 18 lines · Tejun Heo
Hello, Bruce.
Quoted 12 lines · J. Bruce Fields
Apologies, just cleaning out old mail and finding some I should have
responded to long ago:
Quoted 3 lines · Bhaktipriya Shridhar
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 5 lines
The workqueue "callback_wq" queues a single work item &cb-
Quoted 2 lines
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.
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
We definitely do depend on the fact that at most one of these is running
at a time.
We do?
Quoted 8 lines
Quoted 7 lines
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.
Quoted 2 lines
Quoted 7 lines
Quoted 2 lines
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.

Quoted 6 lines

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 <[email protected]>

J. Bruce Fields

2016-11-09 16:27 UTC #10 lore raw

In reply to Jeff Layton

Quoted 31 lines · Jeff Layton

Quoted 9 lines · J. Bruce Fields
Quoted 8 lines · Tejun Heo
Hello, Bruce.
Quoted 4 lines · J. Bruce Fields
Apologies, just cleaning out old mail and finding some I should have responded to long ago:
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines · Bhaktipriya Shridhar
Quoted 3 lines
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.
Quoted 3 lines
Quoted 3 lines
Quoted 2 lines
We definitely do depend on the fact that at most one of these is running at a time.
We do?
Quoted 3 lines
Quoted 2 lines
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?

Quoted 5 lines

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.

Quoted 8 lines

Quoted 2 lines
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.
Signature · 1 line
Jeff Layton <[email protected]>

Jeff Layton

2016-11-09 17:33 UTC #11 lore raw

Quoted 48 lines · J. Bruce Fields

Quoted 41 lines · Jeff Layton
Quoted 12 lines · J. Bruce Fields
Quoted 10 lines · Tejun Heo
Hello, Bruce.
Quoted 5 lines · J. Bruce Fields
Apologies, just cleaning out old mail and finding some I should have responded to long ago:
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines · Bhaktipriya Shridhar
Quoted 4 lines
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.
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
We definitely do depend on the fact that at most one of these is running at a time.
We do?
Quoted 5 lines
Quoted 4 lines
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?
Quoted 5 lines
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.

Quoted 9 lines

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?

Signature · 1 line

Jeff Layton <[email protected]>

J. Bruce Fields

2016-11-09 19:47 UTC #12 lore raw

Quoted 62 lines · Jeff Layton

Quoted 48 lines · J. Bruce Fields
Quoted 41 lines · Jeff Layton
Quoted 12 lines · J. Bruce Fields
Quoted 10 lines · Tejun Heo
Hello, Bruce.
Quoted 5 lines · J. Bruce Fields
Apologies, just cleaning out old mail and finding some I should have responded to long ago:
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines · Bhaktipriya Shridhar
Quoted 4 lines
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.
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
We definitely do depend on the fact that at most one of these is running at a time.
We do?
Quoted 5 lines
Quoted 4 lines
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?
Quoted 5 lines
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.
Quoted 9 lines
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.

Quoted 7 lines

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.

J. Bruce Fields

2016-11-09 20:24 UTC #13 lore raw

Quoted 3 lines · J. Bruce Fields

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.

fs/nfsd/nfs4callback.c +1 -1

index 211dc2aed8e1..eb78109d666c 100644
@@ -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;

Jeff Layton

Reviewed-by

2016-11-09 22:35 UTC #14 lore raw

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:

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.

fs/nfsd/nfs4callback.c +1 -1

index 211dc2aed8e1..eb78109d666c 100644
@@ -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 <[email protected]>