[Cluster-devel] GFS2: Use new workqueue scheme

[Cluster-devel] GFS2: Use new workqueue scheme

[Steven Whitehouse]

Hi,

Hopefully this is the final version :-)

Steve.

From 29e8eb2a793cbf7ccba398b93f362a17f8cb4e6a Mon Sep 17 00:00:00 2001
From: Steven Whitehouse <swhiteho@redhat.com>
Date: Mon, 13 Sep 2010 16:23:00 +0100
Subject: GFS2: Use new workqueue scheme

The recovery workqueue can be freezable since
we want it to finish what it is doing if the system is to
be frozen (although why you'd want to freeze a cluster node
is beyond me since it will result in it being ejected from
the cluster). It does still make sense for single node
GFS2 filesystems though.

The glock workqueue will benefit from being able to run more
work items concurrently. A test running postmark shows
improved performance and multi-threaded workloads are likely
to benefit even more.

The delete workqueue is similar to the recovery workqueue in
that it must not get blocked by memory allocations, and may
run for a long time.

Potentially other GFS2 threads might also be converted to
workqueues, but I'll leave that for a later patch.

Signed-off-by: Steven Whitehouse <swhiteho@redhat.com>
Cc: Tejun Heo <tj@kernel.org>

diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index 8e478e2..c3f2a5c 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -1783,10 +1783,12 @@ int __init gfs2_glock_init(void)
 	}
 #endif
 
-	glock_workqueue = create_workqueue("glock_workqueue");
+	glock_workqueue = alloc_workqueue("glock_workqueue", WQ_RESCUER |
+					  WQ_HIGHPRI | WQ_FREEZEABLE, 0);
 	if (IS_ERR(glock_workqueue))
 		return PTR_ERR(glock_workqueue);
-	gfs2_delete_workqueue = create_workqueue("delete_workqueue");
+	gfs2_delete_workqueue = alloc_workqueue("delete_workqueue", WQ_RESCUER |
+						WQ_FREEZEABLE, 0);
 	if (IS_ERR(gfs2_delete_workqueue)) {
 		destroy_workqueue(glock_workqueue);
 		return PTR_ERR(gfs2_delete_workqueue);
diff --git a/fs/gfs2/main.c b/fs/gfs2/main.c
index b1e9630..1c8bbf2 100644
--- a/fs/gfs2/main.c
+++ b/fs/gfs2/main.c
@@ -140,7 +140,7 @@ static int __init init_gfs2_fs(void)
 
 	error = -ENOMEM;
 	gfs_recovery_wq = alloc_workqueue("gfs_recovery",
-					  WQ_NON_REENTRANT | WQ_RESCUER, 0);
+					  WQ_RESCUER | WQ_FREEZEABLE, 0);
 	if (!gfs_recovery_wq)
 		goto fail_wq;
 
-- 
1.7.1.1

[Cluster-devel] GFS2: Use new workqueue scheme

[Steven Whitehouse]

Hi,

On Tue, 2010-09-14 at 09:52 +0200, Tejun Heo wrote:
> Hello,
> 
> On 09/13/2010 08:26 PM, Steven Whitehouse wrote:
> >>From 29e8eb2a793cbf7ccba398b93f362a17f8cb4e6a Mon Sep 17 00:00:00 2001
> > From: Steven Whitehouse <swhiteho@redhat.com>
> > Date: Mon, 13 Sep 2010 16:23:00 +0100
> > Subject: GFS2: Use new workqueue scheme
> 
> The changes look good to me.
> 
>   Acked-by: Tejun Heo <tj@kernel.org>
> 
> > -	glock_workqueue = create_workqueue("glock_workqueue");
> > +	glock_workqueue = alloc_workqueue("glock_workqueue", WQ_RESCUER |
> > +					  WQ_HIGHPRI | WQ_FREEZEABLE, 0);
> 
> Maybe explaning why HIGHPRI is necessary here would be a good idea?
> 
Thanks for the review, I've updated that in the version now pushed into
my -nmw GFS2 tree.

> > diff --git a/fs/gfs2/main.c b/fs/gfs2/main.c
> > index b1e9630..1c8bbf2 100644
> > --- a/fs/gfs2/main.c
> > +++ b/fs/gfs2/main.c
> > @@ -140,7 +140,7 @@ static int __init init_gfs2_fs(void)
> >  
> >  	error = -ENOMEM;
> >  	gfs_recovery_wq = alloc_workqueue("gfs_recovery",
> > -					  WQ_NON_REENTRANT | WQ_RESCUER, 0);
> > +					  WQ_RESCUER | WQ_FREEZEABLE, 0);
> 
> GFS2 was already using alloc_workqueue() before this patch?
> 
> Thanks.
> 
Yes, due to this patch:

commit 6ecd7c2dd9f5dd4f6e8f65c8027159f9c73b0e4c
Author: Tejun Heo <tj@kernel.org>
Date:   Tue Jul 20 22:09:02 2010 +0200

    gfs2: use workqueue instead of slow-work

:-)

Steve.

[Cluster-devel] GFS2: Use new workqueue scheme

[Steven Whitehouse]

The recovery workqueue can be unbound and also freezable since
we want it to finish what it is doing if the system is to
be frozen (although why you'd want to freeze a cluster node
is beyond me since it will result in it being ejected from
the cluster). It does still make sense for single node
GFS2 filesystems though.

The glock workqueue will benefit from being able to run more
work items concurrently. A test running postmark shows
improved performance and multi-threaded workloads a likely
to benefit even more.

The delete workqueue is similar to the recovery workqueue in
that it must not get blocked by memory allocations, may
run for a long time, and each individual work should only
be scheduled once anywhere in the system.

Potentially other GFS2 threads might also be converted to
workqueues, but I'll leave that for a later patch.

Signed-off-by: Steven Whitehouse <swhiteho@redhat.com>
Cc: Tejun Heo <tj@kernel.org>

diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index 8e478e2..fffc1bf 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -1783,10 +1783,14 @@ int __init gfs2_glock_init(void)
 	}
 #endif
 
-	glock_workqueue = create_workqueue("glock_workqueue");
+	glock_workqueue = alloc_workqueue("glock_workqueue", WQ_RESCUER |
+					  WQ_HIGHPRI | WQ_CPU_INTENSIVE |
+					  WQ_FREEZEABLE, 0);
 	if (IS_ERR(glock_workqueue))
 		return PTR_ERR(glock_workqueue);
-	gfs2_delete_workqueue = create_workqueue("delete_workqueue");
+	gfs2_delete_workqueue = alloc_workqueue("delete_workqueue", WQ_RESCUER |
+						WQ_UNBOUND | WQ_NON_REENTRANT |
+						WQ_FREEZEABLE, 0);
 	if (IS_ERR(gfs2_delete_workqueue)) {
 		destroy_workqueue(glock_workqueue);
 		return PTR_ERR(gfs2_delete_workqueue);
diff --git a/fs/gfs2/main.c b/fs/gfs2/main.c
index b1e9630..60f3465 100644
--- a/fs/gfs2/main.c
+++ b/fs/gfs2/main.c
@@ -140,7 +140,8 @@ static int __init init_gfs2_fs(void)
 
 	error = -ENOMEM;
 	gfs_recovery_wq = alloc_workqueue("gfs_recovery",
-					  WQ_NON_REENTRANT | WQ_RESCUER, 0);
+					  WQ_NON_REENTRANT | WQ_RESCUER |
+					  WQ_UNBOUND | WQ_FREEZEABLE, 0);
 	if (!gfs_recovery_wq)
 		goto fail_wq;
 

[Cluster-devel] GFS2: Use new workqueue scheme

[Steven Whitehouse]

Hi,

On Thu, 2010-09-09 at 15:18 +0200, Tejun Heo wrote:
> Hello, Steven.
> 
> Thanks for working on this.
> 
I think it will be a big win for GFS2, particularly as the number of cpu
cores increases

> On 09/09/2010 02:36 PM, Steven Whitehouse wrote:
> > diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
> > index 8e478e2..fffc1bf 100644
> > --- a/fs/gfs2/glock.c
> > +++ b/fs/gfs2/glock.c
> > @@ -1783,10 +1783,14 @@ int __init gfs2_glock_init(void)
> >  	}
> >  #endif
> >  
> > -	glock_workqueue = create_workqueue("glock_workqueue");
> > +	glock_workqueue = alloc_workqueue("glock_workqueue", WQ_RESCUER |
> > +					  WQ_HIGHPRI | WQ_CPU_INTENSIVE |
> > +					  WQ_FREEZEABLE, 0);
> 
> Does this really need WQ_HIGHRPI and WQ_CPU_INTENSIVE?
> 
This would be a tasklet were it not for the fact that it needs to be
able to submit block I/O from time to time. It does need to be as fast
as possible since it directly affects the latency of operations using
large numbers of inodes.

I read your latest set of docs before assigning the flags, so I hope
I've understood it correctly.

The glock workqueue is involved in sending requests to the DLM and
processing the results of those requests, waking up waiting processes as
quickly as possible.

> >  	if (IS_ERR(glock_workqueue))
> >  		return PTR_ERR(glock_workqueue);
> > -	gfs2_delete_workqueue = create_workqueue("delete_workqueue");
> > +	gfs2_delete_workqueue = alloc_workqueue("delete_workqueue", WQ_RESCUER |
> > +						WQ_UNBOUND | WQ_NON_REENTRANT |
> > +						WQ_FREEZEABLE, 0);
> 
> >  	if (IS_ERR(gfs2_delete_workqueue)) {
> >  		destroy_workqueue(glock_workqueue);
> >  		return PTR_ERR(gfs2_delete_workqueue);
> > diff --git a/fs/gfs2/main.c b/fs/gfs2/main.c
> > index b1e9630..60f3465 100644
> > --- a/fs/gfs2/main.c
> > +++ b/fs/gfs2/main.c
> > @@ -140,7 +140,8 @@ static int __init init_gfs2_fs(void)
> >  
> >  	error = -ENOMEM;
> >  	gfs_recovery_wq = alloc_workqueue("gfs_recovery",
> > -					  WQ_NON_REENTRANT | WQ_RESCUER, 0);
> > +					  WQ_NON_REENTRANT | WQ_RESCUER |
> > +					  WQ_UNBOUND | WQ_FREEZEABLE, 0);
> 
> And do these need to be WQ_UNBOUND?  Unless the flags are specifically
> needed, I think it would be better to stick with the default.  I'm
> currently working on the documentation.  It's still not complete but
> please take a look for more information the behaviors of each flag.
> 
> Thanks.
> 
I wouldn't say that it was 100% a requirement, but they are long running
(potentially a few seconds, or even as far as a minute or two in extreme
cases). The recovery workqueue seems to meet this criteria:

>	* Long running CPU intensive workloads which can be better
> 	  managed by the system scheduler.

and the delete_workqueue seems to meet this criteria:

> 	* Wide fluctuation in the concurrency level requirement is
> 	  expected and using bound wq may end up creating large number
> 	  of mostly unused workers across different CPUs as the issuer
> 	  hops through different CPUs.

It may be that I didn't understand the docs correctly, but I think I've
found the right flags. The delete_workqueue is usually unused during
normal fs operation, but occasionally it might have a lot to do. It was
made a separate workqueue because it needs to be able to manipulate
glocks and thus must never block the glock workqueue.

Steve.

[Cluster-devel] GFS2: Use new workqueue scheme

[Steven Whitehouse]

Hi,

On Thu, 2010-09-09 at 15:48 +0200, Tejun Heo wrote:
> Hello,
> 
> On 09/09/2010 03:45 PM, Steven Whitehouse wrote:
> > On Thu, 2010-09-09 at 15:18 +0200, Tejun Heo wrote:
> >> Hello, Steven.
> >>
> >> Thanks for working on this.
> >>
> > I think it will be a big win for GFS2, particularly as the number of cpu
> > cores increases
> 
> Awesome. :-)
> 
> >>> -	glock_workqueue = create_workqueue("glock_workqueue");
> >>> +	glock_workqueue = alloc_workqueue("glock_workqueue", WQ_RESCUER |
> >>> +					  WQ_HIGHPRI | WQ_CPU_INTENSIVE |
> >>> +					  WQ_FREEZEABLE, 0);
> >>
> >> Does this really need WQ_HIGHRPI and WQ_CPU_INTENSIVE?
> >>
> > This would be a tasklet were it not for the fact that it needs to be
> > able to submit block I/O from time to time. It does need to be as fast
> > as possible since it directly affects the latency of operations using
> > large numbers of inodes.
> > 
> > I read your latest set of docs before assigning the flags, so I hope
> > I've understood it correctly.
> > 
> > The glock workqueue is involved in sending requests to the DLM and
> > processing the results of those requests, waking up waiting processes as
> > quickly as possible.
> 
> I see but then wouldn't WQ_CPU_INTENSIVE be unnecessary?  It's high
> priority but doesn't sound like they're gonna hog huge amount of CPU
> cycles.  Also, please note that the high priority is global across all
> workqueues and thus _must_ be used judiciously.  Well, if you were
> gonna use tasklets for it, it probably is a good candidate tho.
> 
Ah, I see. Maybe I misunderstood. I read the bit about using both
WQ_HIGHPRO and WQ_CPU_INTENSIVE which says:

"Work items queued on a highpri CPU-intensive wq start execution as soon
as resources are available and don't affect execution of other work
items."

and assumed that was what I needed, but maybe I don't need to
WQ_CPU_INTENSIVE as you suggest.

> >>>  	gfs_recovery_wq = alloc_workqueue("gfs_recovery",
> >>> -					  WQ_NON_REENTRANT | WQ_RESCUER, 0);
> >>> +					  WQ_NON_REENTRANT | WQ_RESCUER |
> >>> +					  WQ_UNBOUND | WQ_FREEZEABLE, 0);
> >>
> >> And do these need to be WQ_UNBOUND?  Unless the flags are specifically
> >> needed, I think it would be better to stick with the default.  I'm
> >> currently working on the documentation.  It's still not complete but
> >> please take a look for more information the behaviors of each flag.
> >
> > I wouldn't say that it was 100% a requirement, but they are long running
> > (potentially a few seconds, or even as far as a minute or two in extreme
> > cases). The recovery workqueue seems to meet this criteria:
> 
> Long running doesn't matter.  Normal workqueues can handle them
> perfectly fine.  The only cases you would want to use unbound
> workqueues are long running CPU hogs and (very) high fluctuation in
> the number of concurrent work items.
> 
It sounds like maybe the delete workqueue needs that, but that the
recovery one certainly doesn't in that case.

> >> 	* Long running CPU intensive workloads which can be better
> >> 	  managed by the system scheduler.
> > 
> > and the delete_workqueue seems to meet this criteria:
> > 
> >> 	* Wide fluctuation in the concurrency level requirement is
> >> 	  expected and using bound wq may end up creating large number
> >> 	  of mostly unused workers across different CPUs as the issuer
> >> 	  hops through different CPUs.
> > 
> > It may be that I didn't understand the docs correctly, but I think I've
> > found the right flags. The delete_workqueue is usually unused during
> > normal fs operation, but occasionally it might have a lot to do. It was
> > made a separate workqueue because it needs to be able to manipulate
> > glocks and thus must never block the glock workqueue.
> 
> Heh, these being one of the first conversions, I just wanna make sure.
> Long running CPU-intensive tasks would be things like works running
> RAID checksums, crypto stuff, IOW, stuff which are actually gonna
> perform a long calculation.  If a work is just gonna be blocking on
> locks for long period of time, there's no need to use the unbound
> ones.  So, unless I'm misunderstanding, I don't really think
> WQ_UNBOUND is necessary for the latter two.
> 
> Thanks.
> 
Yes, I'll try it without and see if that is ok. I am also trying to be a
bit cautious about the flags in case I accidentally introduce some
dependency which was not there before.

I'll follow up with an updated patch shortly,

Steve.