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

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

[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 <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

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

[Jeff Layton]

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>

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

[Tejun Heo]

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.

-- 
tejun

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

[Jeff Layton]

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>

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

[J. Bruce Fields]

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

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

[Tejun Heo]

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

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

[J. Bruce Fields]

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.

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

[Jeff Layton]

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>

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

[Trond Myklebust]

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==

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

[Jeff Layton]

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>

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

[J. Bruce Fields]

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>

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

[Jeff Layton]

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>

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

[J. Bruce Fields]

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.

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

[J. Bruce Fields]

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;

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

[Jeff Layton]

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>