[PATCH] nfsd: use a multithreaded workqueue for nfsd4_callbacks

[PATCH] nfsd: use a multithreaded workqueue for nfsd4_callbacks

[Jeff Layton]

I don't see any need to order callbacks with respect to one another.
Also, these are generally not involved in memory reclaim, so I don't see
the need for a rescuer thread here either.

Signed-off-by: Jeff Layton <jeff.layton@primarydata.com>
---
 fs/nfsd/nfs4callback.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index e7f50c4081d6..7dabbb436290 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -1017,7 +1017,7 @@ static const struct rpc_call_ops nfsd4_cb_ops = {
 
 int nfsd4_create_callback_queue(void)
 {
-	callback_wq = create_singlethread_workqueue("nfsd4_callbacks");
+	callback_wq = alloc_workqueue("%s", WQ_UNBOUND, 0, "nfsd4_callbacks");
 	if (!callback_wq)
 		return -ENOMEM;
 	return 0;
-- 
2.4.3

Re: [PATCH] nfsd: use a multithreaded workqueue for nfsd4_callbacks

[J. Bruce Fields]

On Sat, Oct 03, 2015 at 08:38:02AM -0400, Jeff Layton wrote:
> I don't see any need to order callbacks with respect to one another.

I thought the code in nfsd4_process_cb_update really depended on this.
The locking it has is against nfsd threads, it probably assumes it's
only run from a cb thread and that it's the only one running at a time.

But I haven't reviewed it lately.

--b.

> Also, these are generally not involved in memory reclaim, so I don't see
> the need for a rescuer thread here either.
> 
> Signed-off-by: Jeff Layton <jeff.layton@primarydata.com>
> ---
>  fs/nfsd/nfs4callback.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> index e7f50c4081d6..7dabbb436290 100644
> --- a/fs/nfsd/nfs4callback.c
> +++ b/fs/nfsd/nfs4callback.c
> @@ -1017,7 +1017,7 @@ static const struct rpc_call_ops nfsd4_cb_ops = {
>  
>  int nfsd4_create_callback_queue(void)
>  {
> -	callback_wq = create_singlethread_workqueue("nfsd4_callbacks");
> +	callback_wq = alloc_workqueue("%s", WQ_UNBOUND, 0, "nfsd4_callbacks");
>  	if (!callback_wq)
>  		return -ENOMEM;
>  	return 0;
> -- 
> 2.4.3

Re: [PATCH] nfsd: use a multithreaded workqueue for nfsd4_callbacks

[Jeff Layton]

On Fri, 9 Oct 2015 17:24:59 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Sat, Oct 03, 2015 at 08:38:02AM -0400, Jeff Layton wrote:
> > I don't see any need to order callbacks with respect to one another.
> 
> I thought the code in nfsd4_process_cb_update really depended on this.
> The locking it has is against nfsd threads, it probably assumes it's
> only run from a cb thread and that it's the only one running at a time.
> 
> But I haven't reviewed it lately.
> 
> --b.
> 

Yikes -- ok. That's not at all obvious. That should prob be documented
if so.

Yeah, ok...I guess you could end up with multiple threads racing to
tear down the cb_client and xprt and create a new one, and it looks
like the cl_cb_client and cl_cred pointers could get clobbered by new
ones in that case.

Shouldn't be too hard to fix protecting those pointers with the
cl_lock. That said, I prob won't be able to spend time on it right now.
You can go ahead and drop that patch and I'll resend if/when I get
around to fixing that.

Thanks for having a look...

> > Also, these are generally not involved in memory reclaim, so I don't see
> > the need for a rescuer thread here either.
> > 
> > Signed-off-by: Jeff Layton <jeff.layton@primarydata.com>
> > ---
> >  fs/nfsd/nfs4callback.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> > index e7f50c4081d6..7dabbb436290 100644
> > --- a/fs/nfsd/nfs4callback.c
> > +++ b/fs/nfsd/nfs4callback.c
> > @@ -1017,7 +1017,7 @@ static const struct rpc_call_ops nfsd4_cb_ops = {
> >  
> >  int nfsd4_create_callback_queue(void)
> >  {
> > -	callback_wq = create_singlethread_workqueue("nfsd4_callbacks");
> > +	callback_wq = alloc_workqueue("%s", WQ_UNBOUND, 0, "nfsd4_callbacks");
> >  	if (!callback_wq)
> >  		return -ENOMEM;
> >  	return 0;
> > -- 
> > 2.4.3


-- 
Jeff Layton <jlayton@poochiereds.net>

Re: [PATCH] nfsd: use a multithreaded workqueue for nfsd4_callbacks

[J. Bruce Fields]

On Fri, Oct 09, 2015 at 05:39:23PM -0400, Jeff Layton wrote:
> On Fri, 9 Oct 2015 17:24:59 -0400
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > On Sat, Oct 03, 2015 at 08:38:02AM -0400, Jeff Layton wrote:
> > > I don't see any need to order callbacks with respect to one another.
> > 
> > I thought the code in nfsd4_process_cb_update really depended on this.
> > The locking it has is against nfsd threads, it probably assumes it's
> > only run from a cb thread and that it's the only one running at a time.
> > 
> > But I haven't reviewed it lately.
> > 
> > --b.
> > 
> 
> Yikes -- ok. That's not at all obvious. That should prob be documented
> if so.

Yes, my bad.

> Yeah, ok...I guess you could end up with multiple threads racing to
> tear down the cb_client and xprt and create a new one, and it looks
> like the cl_cb_client and cl_cred pointers could get clobbered by new
> ones in that case.
> 
> Shouldn't be too hard to fix protecting those pointers with the
> cl_lock. That said, I prob won't be able to spend time on it right now.
> You can go ahead and drop that patch and I'll resend if/when I get
> around to fixing that.

What's the problem that this fixes exactly?

--b.

Re: [PATCH] nfsd: use a multithreaded workqueue for nfsd4_callbacks

[Jeff Layton]

On Mon, 12 Oct 2015 14:11:17 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Fri, Oct 09, 2015 at 05:39:23PM -0400, Jeff Layton wrote:
> > On Fri, 9 Oct 2015 17:24:59 -0400
> > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > 
> > > On Sat, Oct 03, 2015 at 08:38:02AM -0400, Jeff Layton wrote:
> > > > I don't see any need to order callbacks with respect to one another.
> > > 
> > > I thought the code in nfsd4_process_cb_update really depended on this.
> > > The locking it has is against nfsd threads, it probably assumes it's
> > > only run from a cb thread and that it's the only one running at a time.
> > > 
> > > But I haven't reviewed it lately.
> > > 
> > > --b.
> > > 
> > 
> > Yikes -- ok. That's not at all obvious. That should prob be documented
> > if so.
> 
> Yes, my bad.
> 
> > Yeah, ok...I guess you could end up with multiple threads racing to
> > tear down the cb_client and xprt and create a new one, and it looks
> > like the cl_cb_client and cl_cred pointers could get clobbered by new
> > ones in that case.
> > 
> > Shouldn't be too hard to fix protecting those pointers with the
> > cl_lock. That said, I prob won't be able to spend time on it right now.
> > You can go ahead and drop that patch and I'll resend if/when I get
> > around to fixing that.
> 
> What's the problem that this fixes exactly?
> 

Unnecessary serialization of callbacks?

Not that that's necessarily a problem, but with the newer workqueue
implementation there is more overhead in running a single threaded
workqueue since it implies a rescuer thread and has to do extra work
to ensure that the jobs are run in sequential order.

If that serialization is not actually needed, then it's better to move
it to a multithreaded workqueue.

-- 
Jeff Layton <jlayton@poochiereds.net>

Re: [PATCH] nfsd: use a multithreaded workqueue for nfsd4_callbacks

[J. Bruce Fields]

On Mon, Oct 12, 2015 at 04:14:21PM -0400, Jeff Layton wrote:
> On Mon, 12 Oct 2015 14:11:17 -0400
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > On Fri, Oct 09, 2015 at 05:39:23PM -0400, Jeff Layton wrote:
> > > On Fri, 9 Oct 2015 17:24:59 -0400
> > > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > > 
> > > > On Sat, Oct 03, 2015 at 08:38:02AM -0400, Jeff Layton wrote:
> > > > > I don't see any need to order callbacks with respect to one another.
> > > > 
> > > > I thought the code in nfsd4_process_cb_update really depended on this.
> > > > The locking it has is against nfsd threads, it probably assumes it's
> > > > only run from a cb thread and that it's the only one running at a time.
> > > > 
> > > > But I haven't reviewed it lately.
> > > > 
> > > > --b.
> > > > 
> > > 
> > > Yikes -- ok. That's not at all obvious. That should prob be documented
> > > if so.
> > 
> > Yes, my bad.
> > 
> > > Yeah, ok...I guess you could end up with multiple threads racing to
> > > tear down the cb_client and xprt and create a new one, and it looks
> > > like the cl_cb_client and cl_cred pointers could get clobbered by new
> > > ones in that case.
> > > 
> > > Shouldn't be too hard to fix protecting those pointers with the
> > > cl_lock. That said, I prob won't be able to spend time on it right now.
> > > You can go ahead and drop that patch and I'll resend if/when I get
> > > around to fixing that.
> > 
> > What's the problem that this fixes exactly?
> > 
> 
> Unnecessary serialization of callbacks?
> 
> Not that that's necessarily a problem, but with the newer workqueue
> implementation there is more overhead in running a single threaded
> workqueue since it implies a rescuer thread and has to do extra work
> to ensure that the jobs are run in sequential order.
> 
> If that serialization is not actually needed, then it's better to move
> it to a multithreaded workqueue.

We'd still be serializing a lot of the same code, just using locks
instead, wouldn't we?

--b.

Re: [PATCH] nfsd: use a multithreaded workqueue for nfsd4_callbacks

[Jeff Layton]

On Mon, 12 Oct 2015 16:20:44 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Mon, Oct 12, 2015 at 04:14:21PM -0400, Jeff Layton wrote:
> > On Mon, 12 Oct 2015 14:11:17 -0400
> > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > 
> > > On Fri, Oct 09, 2015 at 05:39:23PM -0400, Jeff Layton wrote:
> > > > On Fri, 9 Oct 2015 17:24:59 -0400
> > > > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > > > 
> > > > > On Sat, Oct 03, 2015 at 08:38:02AM -0400, Jeff Layton wrote:
> > > > > > I don't see any need to order callbacks with respect to one another.
> > > > > 
> > > > > I thought the code in nfsd4_process_cb_update really depended on this.
> > > > > The locking it has is against nfsd threads, it probably assumes it's
> > > > > only run from a cb thread and that it's the only one running at a time.
> > > > > 
> > > > > But I haven't reviewed it lately.
> > > > > 
> > > > > --b.
> > > > > 
> > > > 
> > > > Yikes -- ok. That's not at all obvious. That should prob be documented
> > > > if so.
> > > 
> > > Yes, my bad.
> > > 
> > > > Yeah, ok...I guess you could end up with multiple threads racing to
> > > > tear down the cb_client and xprt and create a new one, and it looks
> > > > like the cl_cb_client and cl_cred pointers could get clobbered by new
> > > > ones in that case.
> > > > 
> > > > Shouldn't be too hard to fix protecting those pointers with the
> > > > cl_lock. That said, I prob won't be able to spend time on it right now.
> > > > You can go ahead and drop that patch and I'll resend if/when I get
> > > > around to fixing that.
> > > 
> > > What's the problem that this fixes exactly?
> > > 
> > 
> > Unnecessary serialization of callbacks?
> > 
> > Not that that's necessarily a problem, but with the newer workqueue
> > implementation there is more overhead in running a single threaded
> > workqueue since it implies a rescuer thread and has to do extra work
> > to ensure that the jobs are run in sequential order.
> > 
> > If that serialization is not actually needed, then it's better to move
> > it to a multithreaded workqueue.
> 
> We'd still be serializing a lot of the same code, just using locks
> instead, wouldn't we?
> 
> --b.

Any serialization would be per-clnt instead of global like it is today.

Also, we'd likely only serialize changes to some of the pointers
instead of the entire operation.

-- 
Jeff Layton <jlayton@poochiereds.net>

Re: [PATCH] nfsd: use a multithreaded workqueue for nfsd4_callbacks

[J. Bruce Fields]

On Mon, Oct 12, 2015 at 05:02:19PM -0400, Jeff Layton wrote:
> On Mon, 12 Oct 2015 16:20:44 -0400
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > On Mon, Oct 12, 2015 at 04:14:21PM -0400, Jeff Layton wrote:
> > > On Mon, 12 Oct 2015 14:11:17 -0400
> > > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > > 
> > > > On Fri, Oct 09, 2015 at 05:39:23PM -0400, Jeff Layton wrote:
> > > > > On Fri, 9 Oct 2015 17:24:59 -0400
> > > > > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > > > > 
> > > > > > On Sat, Oct 03, 2015 at 08:38:02AM -0400, Jeff Layton wrote:
> > > > > > > I don't see any need to order callbacks with respect to one another.
> > > > > > 
> > > > > > I thought the code in nfsd4_process_cb_update really depended on this.
> > > > > > The locking it has is against nfsd threads, it probably assumes it's
> > > > > > only run from a cb thread and that it's the only one running at a time.
> > > > > > 
> > > > > > But I haven't reviewed it lately.
> > > > > > 
> > > > > > --b.
> > > > > > 
> > > > > 
> > > > > Yikes -- ok. That's not at all obvious. That should prob be documented
> > > > > if so.
> > > > 
> > > > Yes, my bad.
> > > > 
> > > > > Yeah, ok...I guess you could end up with multiple threads racing to
> > > > > tear down the cb_client and xprt and create a new one, and it looks
> > > > > like the cl_cb_client and cl_cred pointers could get clobbered by new
> > > > > ones in that case.
> > > > > 
> > > > > Shouldn't be too hard to fix protecting those pointers with the
> > > > > cl_lock. That said, I prob won't be able to spend time on it right now.
> > > > > You can go ahead and drop that patch and I'll resend if/when I get
> > > > > around to fixing that.
> > > > 
> > > > What's the problem that this fixes exactly?
> > > > 
> > > 
> > > Unnecessary serialization of callbacks?
> > > 
> > > Not that that's necessarily a problem, but with the newer workqueue
> > > implementation there is more overhead in running a single threaded
> > > workqueue since it implies a rescuer thread and has to do extra work
> > > to ensure that the jobs are run in sequential order.
> > > 
> > > If that serialization is not actually needed, then it's better to move
> > > it to a multithreaded workqueue.
> > 
> > We'd still be serializing a lot of the same code, just using locks
> > instead, wouldn't we?
> > 
> > --b.
> 
> Any serialization would be per-clnt instead of global like it is today.
> 
> Also, we'd likely only serialize changes to some of the pointers
> instead of the entire operation.

OK, makes sense.  Probably not a priority for now, though.

--b.