3.6-rc5 cgroups blkio throttle + md regression

Hi,

When using cgroups blkio controller with bio throttling my shell hangs attempting to write limits for md devices.
Standard sd devices work as expected but md devices cause the shell to hang on io_wait.

For example:

echo 9:0 0 > blkio.throttle.read_bps_device

Where 9:0 => /dev/md0

I haven't started trying to narrow down when this occured yet, it works fine on 3.2 and I will try out 3.5 etc shortly to see if I can bisect where it stopped working.

Joseph.

--
CTO | Orion Virtualisation Solutions | www.orionvm.com.au
Phone: 1300 56 99 52 | Mobile: 0428 754 846

Hi,

I booted the machine under bare metal to continue bisecting. Thankfully this allowed me to locate the commit that causes the problem.

git bisect reports this as the offending commit:
commit b82d4b197c782ced82a8b7b76664125d2d3c156c

Author: Tejun Heo <[email protected]>
Date:   Fri Apr 13 13:11:31 2012 -0700

blkcg: make request_queue bypassing on allocation

With the previous change to guarantee bypass visiblity for RCU read lock regions, entering bypass mode involves non-trivial overhead and future changes are scheduled to make use of bypass mode during init path. Combined it may end up adding noticeable delay during boot.

This patch makes request_queue start its life in bypass mode, which is ended on queue init completion at the end of blk_init_allocated_queue(), and updates blk_queue_bypass_start() such that draining and RCU synchronization are performed only when the queue actually enters bypass mode.

This avoids unnecessarily switching in and out of bypass mode during init avoiding the overhead and any nasty surprises which may step from leaving bypass mode on half-initialized queues.

The boot time overhead was pointed out by Vivek.

Signed-off-by Tejun Heo <[email protected]>

Cc Vivek Goyal <[email protected]>

Signed-off-by Jens Axboe <axboe-tSWWG44O7X1aa/[email protected]>

block/blk-core.c +25 -12

index f2db628..3b02ba3 100644
@@ -421,14 +421,18 @@ void blk_drain_queue(struct request_queue *q,
 bool drain_all)
  */
 void blk_queue_bypass_start(struct request_queue *q)
 {
+	bool drain;
+
 	spin_lock_irq(q->queue_lock);
-	q->bypass_depth++;
+	drain = !q->bypass_depth++;
 	queue_flag_set(QUEUE_FLAG_BYPASS, q);
 	spin_unlock_irq(q->queue_lock);
 
-	blk_drain_queue(q, false);
-	/* ensure blk_queue_bypass() is %true inside RCU read lock */
-	synchronize_rcu();
+	if (drain) {
+		blk_drain_queue(q, false);
+		/* ensure blk_queue_bypass() is %true inside RCU read lock */
+		synchronize_rcu();
+	}
 }
 EXPORT_SYMBOL_GPL(blk_queue_bypass_start);

@@ -577,6 +581,15 @@ struct request_queue *blk_alloc_queue_node(gfp_t
 gfp_mask, int node_id)
 	 */
 	q->queue_lock = &q->__queue_lock;
 
+	/*
+	 * A queue starts its life with bypass turned on to avoid
+	 * unnecessary bypass on/off overhead and nasty surprises during
+	 * init.  The initial bypass will be finished at the end of
+	 * blk_init_allocated_queue().
+	 */
+	q->bypass_depth = 1;
+	__set_bit(QUEUE_FLAG_BYPASS, &q->queue_flags);
+
 	if (blkcg_init_queue(q))
 		goto fail_id;

@@ -672,15 +685,15 @@ blk_init_allocated_queue(struct request_queue
 *q, request_fn_proc *rfn,
 
 	q->sg_reserved_size = INT_MAX;
 
-	/*
-	 * all done
-	 */
-	if (!elevator_init(q, NULL)) {
-		blk_queue_congestion_threshold(q);
-		return q;
-	}
+	/* init elevator */
+	if (elevator_init(q, NULL))
+		return NULL;
 
-	return NULL;
+	blk_queue_congestion_threshold(q);
+
+	/* all done, end the initial bypass */
+	blk_queue_bypass_end(q);
+	return q;
 }
 EXPORT_SYMBOL(blk_init_allocated_queue);

Reverting this commit fixes the regression. :)

Joseph.

Signature · 2 lines

CTO | Orion Virtualisation Solutions | www.orionvm.com.au
Phone: 1300 56 99 52 | Mobile: 0428 754 846

Quoted 6 lines · Joseph Glanville

Hi,

I booted the machine under bare metal to continue bisecting. Thankfully this allowed me to locate the commit that causes the problem.

I tested it and I am also noticing the hang. I can see this hang on dm devices also.

I suspect this issue is related to bio based drivers. We exit the bypass mode in blk_init_allocated_queue() and that will be called only for request based drivers. So for bio based drivers may be we never exit the bypass mode and this issue is somehow side affect of that.

Hello,

Quoted 11 lines · Vivek Goyal

Quoted 6 lines · Joseph Glanville

Hi,

I booted the machine under bare metal to continue bisecting. Thankfully this allowed me to locate the commit that causes the problem.

I tested it and I am also noticing the hang. I can see this hang on dm devices also.

Quoted 5 lines

I suspect this issue is related to bio based drivers. We exit the bypass mode in blk_init_allocated_queue() and that will be called only for request based drivers. So for bio based drivers may be we never exit the bypass mode and this issue is somehow side affect of that.

Can you please trigger sysrq-t and post the result?

Quoted 14 lines · Tejun Heo

Hello,

Quoted 11 lines · Vivek Goyal

Quoted 6 lines · Joseph Glanville

Hi,

I booted the machine under bare metal to continue bisecting. Thankfully this allowed me to locate the commit that causes the problem.

I tested it and I am also noticing the hang. I can see this hang on dm devices also.

Quoted 7 lines

Quoted 5 lines

I suspect this issue is related to bio based drivers. We exit the bypass mode in blk_init_allocated_queue() and that will be called only for request based drivers. So for bio based drivers may be we never exit the bypass mode and this issue is somehow side affect of that.

Can you please trigger sysrq-t and post the result?

Sorry, I had taken the sysrq-t output yesterday itself. Got distracted in other things and could never look through the code. Here it is.

[  418.685015] bash            D ffff880037aa4e88  4720  2898   2847
0x00000080
[  418.685015]  ffff88007c777cd8 0000000000000082 ffff880037aa4b00
ffff88007c777fd8
[  418.685015]  ffff88007c777fd8 ffff88007c777fd8 ffffffff81c13440
ffff880037aa4b00
[  418.685015]  ffff88007c777ce8 ffffffff81e05e40 ffff88007c777d18
000000010001d5e4
[  418.685015] Call Trace:
[  418.685015]  [<ffffffff817a2fa9>] schedule+0x29/0x70
[  418.685015]  [<ffffffff817a1570>] schedule_timeout+0x130/0x250
[  418.685015]  [<ffffffff8140bbcb>] ? kobj_lookup+0x10b/0x160
[  418.685015]  [<ffffffff8104a9b0>] ? usleep_range+0x50/0x50
[  418.685015]  [<ffffffff817a16ae>] schedule_timeout_uninterruptible+0x1e/0x20
[  418.685015]  [<ffffffff8104c310>] msleep+0x20/0x30
[  418.685015]  [<ffffffff812ccaa8>] blkg_conf_prep+0x118/0x140
[  418.685015]  [<ffffffff812cd400>] ? tg_set_conf_uint+0x20/0x20

[  418.685015]  [<ffffffff812cd33a>] tg_set_conf.isra.20+0x2a/0xd0
[  418.685015]  [<ffffffff810021ff>] ? do_signal+0x3f/0x610
[  418.685015]  [<ffffffff812cd417>] tg_set_conf_u64+0x17/0x20
[  418.685015]  [<ffffffff8109de6f>] cgroup_file_write+0x1bf/0x2c0
[  418.685015]  [<ffffffff812864bc>] ? security_file_permission+0x2c/0xb0
[  418.685015]  [<ffffffff811325cc>] vfs_write+0xac/0x180
[  418.685015]  [<ffffffff811328fa>] sys_write+0x4a/0x90
[  418.685015]  [<ffffffff817ab8d2>] system_call_fastpath+0x16/0x1b

I suspect we are looping in retry code because bio based queues never come out of bypass mode.

                /*
                 * If queue was bypassing, we should retry.  Do so after a
                 * short msleep().  It isn't strictly necessary but queue
                 * can be bypassing for some time and it's always nice to
                 * avoid busy looping.
                 */
                if (ret == -EBUSY) {
                        msleep(10);
                        ret = restart_syscall();
                }

Quoted 13 lines · Vivek Goyal

I suspect we are looping in retry code because bio based queues never come out of bypass mode.

                /*
                 * If queue was bypassing, we should retry.  Do so after a
                 * short msleep().  It isn't strictly necessary but queue
                 * can be bypassing for some time and it's always nice to
                 * avoid busy looping.
                 */
                if (ret == -EBUSY) {
                        msleep(10);
                        ret = restart_syscall();
                }

Yeah, I incorrectly assumed that bio based drivers would call blk_init_allocated_queue(). I think we need to move the initial bypass_end call to blk_register_queue(). Like the following. Not completely sure yet tho.

block/blk-core.c +0 -3

index 4b4dbdf..cbb019a 100644
@@ -714,9 +714,6 @@ blk_init_allocated_queue(struct request_queue *q, request_fn_proc *rfn,
 		return NULL;
 
 	blk_queue_congestion_threshold(q);
-
-	/* all done, end the initial bypass */
-	blk_queue_bypass_end(q);
 	return q;
 }
 EXPORT_SYMBOL(blk_init_allocated_queue);

block/blk-sysfs.c +2 -0

index 9628b29..f53802e 100644
@@ -527,6 +527,8 @@ int blk_register_queue(struct gendisk *disk)
 	if (WARN_ON(!q))
 		return -ENXIO;
 
+	blk_queue_bypass_end(q);
+
 	ret = blk_trace_init_sysfs(dev);
 	if (ret)
 		return ret;

Quoted 4 lines

Yeah, I incorrectly assumed that bio based drivers would call blk_init_allocated_queue(). I think we need to move the initial bypass_end call to blk_register_queue(). Like the following. Not completely sure yet tho.

Hmm.., I am just trying to remember the details hence thinking loud.

So issue was that we did not want to call synchronize_rcu() in queue exit path (ex. cfq_exit_queue()) because some driver could call create/destroy queues very frequently during device/disk discovery.

So we started queue in bypass mode so that if it is destroyed early, it will be destroyed in bypass mode and we shall never have to call synchronize_rcu().

So assumption here is that blk_register_queue() will be called by add_disk() and hopefully add_disk() will be called by driver only if it is not planning to tear down the queue soon. If this assumption is right, then this patch should work, I think.

Quoted 13 lines · Vivek Goyal

I suspect we are looping in retry code because bio based queues never come out of bypass mode.

                /*
                 * If queue was bypassing, we should retry.  Do so after a
                 * short msleep().  It isn't strictly necessary but queue
                 * can be bypassing for some time and it's always nice to
                 * avoid busy looping.
                 */
                if (ret == -EBUSY) {
                        msleep(10);
                        ret = restart_syscall();
                }

Yeah, I incorrectly assumed that bio based drivers would call blk_init_allocated_queue(). I think we need to move the initial bypass_end call to blk_register_queue(). Like the following. Not completely sure yet tho.

block/blk-core.c +0 -3

index 4b4dbdf..cbb019a 100644
@@ -714,9 +714,6 @@ blk_init_allocated_queue(struct request_queue *q, request_fn_proc *rfn,
 		return NULL;
 
 	blk_queue_congestion_threshold(q);
-
-	/* all done, end the initial bypass */
-	blk_queue_bypass_end(q);
 	return q;
 }
 EXPORT_SYMBOL(blk_init_allocated_queue);

block/blk-sysfs.c +2 -0

index 9628b29..f53802e 100644
@@ -527,6 +527,8 @@ int blk_register_queue(struct gendisk *disk)
 	if (WARN_ON(!q))
 		return -ENXIO;
 
+	blk_queue_bypass_end(q);
+
 	ret = blk_trace_init_sysfs(dev);
 	if (ret)
 		return ret;

Quoted 4 lines

Yeah, I incorrectly assumed that bio based drivers would call blk_init_allocated_queue(). I think we need to move the initial bypass_end call to blk_register_queue(). Like the following. Not completely sure yet tho.

I can confirm that with this patch, hang issue is gone. I tried it on a dm device.

A queue newly allocated with blk_alloc_queue_node() has only QUEUE_FLAG_BYPASS set. For request-based drivers, blk_init_allocated_queue() is called and q->queue_flags is overwritten with QUEUE_FLAG_DEFAULT which doesn't include BYPASS even though the initial bypass is still in effect.

In blk_init_allocated_queue(), or QUEUE_FLAG_DEFAULT to q->queue_flags instead of overwriting.

Signed-off-by Tejun Heo <[email protected]>

Cc Vivek Goyal <[email protected]>

Cc [email protected]

---
 block/blk-core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

block/blk-core.c +1 -1

@@ -696,7 +696,7 @@ blk_init_allocated_queue(struct request_
 	q->request_fn		= rfn;
 	q->prep_rq_fn		= NULL;
 	q->unprep_rq_fn		= NULL;
-	q->queue_flags		= QUEUE_FLAG_DEFAULT;
+	q->queue_flags		|= QUEUE_FLAG_DEFAULT;
 
 	/* Override internal queue lock with supplied lock pointer */
 	if (lock)

b82d4b197c ("blkcg: make request_queue bypassing on allocation") made request_queues bypassed on allocation to avoid switching on and off bypass mode on a queue being initialized. Some drivers allocate and then destroy a lot of queues without fully initializing them and incurring bypass latency overhead on each of them could add upto significant overhead.

Unfortunately, blk_init_allocated_queue() is never used by queues of bio-based drivers, which means that all bio-based driver queues are in bypass mode even after initialization and registration complete successfully.

Due to the limited way request_queues are used by bio drivers, this problem is hidden pretty well but it shows up when blk-throttle is used in combination with a bio-based driver. Trying to configure (echoing to cgroupfs file) blk-throttle for a bio-based driver hangs indefinitely in blkg_conf_prep() waiting for bypass mode to end.

This patch moves the initial blk_queue_bypass_end() call from blk_init_allocated_queue() to blk_register_queue() which is called for any userland-visible queues regardless of its type.

I believe this is correct because I don't think there is any block driver which needs or wants working elevator and blk-cgroup on a queue which isn't visible to userland. If there are such users, we need a different solution.

Signed-off-by Tejun Heo <[email protected]>

Reported-by Joseph Glanville <[email protected]>

Cc Vivek Goyal <[email protected]>

Cc [email protected]

---
Jens, while these are fixes, I think it isn't extremely urgent and routing these through 3.7-rc1 should be enough.

Thanks.

 block/blk-core.c  |    7 ++-----
 block/blk-sysfs.c |    6 ++++++
 2 files changed, 8 insertions(+), 5 deletions(-)

block/blk-core.c +2 -5

@@ -608,8 +608,8 @@ struct request_queue *blk_alloc_queue_no
 	/*
 	 * A queue starts its life with bypass turned on to avoid
 	 * unnecessary bypass on/off overhead and nasty surprises during
-	 * init.  The initial bypass will be finished at the end of
-	 * blk_init_allocated_queue().
+	 * init.  The initial bypass will be finished when the queue is
+	 * registered by blk_register_queue().
 	 */
 	q->bypass_depth = 1;
 	__set_bit(QUEUE_FLAG_BYPASS, &q->queue_flags);

@@ -714,9 +714,6 @@ blk_init_allocated_queue(struct request_
 		return NULL;
 
 	blk_queue_congestion_threshold(q);
-
-	/* all done, end the initial bypass */
-	blk_queue_bypass_end(q);
 	return q;
 }
 EXPORT_SYMBOL(blk_init_allocated_queue);

block/blk-sysfs.c +6 -0

@@ -527,6 +527,12 @@ int blk_register_queue(struct gendisk *d
 	if (WARN_ON(!q))
 		return -ENXIO;
 
+	/*
+	 * Initialization must be complete by now.  Finish the initial
+	 * bypass from queue allocation.
+	 */
+	blk_queue_bypass_end(q);
+
 	ret = blk_trace_init_sysfs(dev);
 	if (ret)
 		return ret;

b82d4b197c ("blkcg: make request_queue bypassing on allocation") made request_queues bypassed on allocation to avoid switching on and off bypass mode on a queue being initialized. Some drivers allocate and then destroy a lot of queues without fully initializing them and incurring bypass latency overhead on each of them could add upto significant overhead.

Unfortunately, blk_init_allocated_queue() is never used by queues of bio-based drivers, which means that all bio-based driver queues are in bypass mode even after initialization and registration complete successfully.

Due to the limited way request_queues are used by bio drivers, this problem is hidden pretty well but it shows up when blk-throttle is used in combination with a bio-based driver. Trying to configure (echoing to cgroupfs file) blk-throttle for a bio-based driver hangs indefinitely in blkg_conf_prep() waiting for bypass mode to end.

This patch moves the initial blk_queue_bypass_end() call from blk_init_allocated_queue() to blk_register_queue() which is called for any userland-visible queues regardless of its type.

I believe this is correct because I don't think there is any block driver which needs or wants working elevator and blk-cgroup on a queue which isn't visible to userland. If there are such users, we need a different solution.

Signed-off-by Tejun Heo <[email protected]>

Reported-by Joseph Glanville <[email protected]>

Cc Vivek Goyal <[email protected]>

Cc [email protected]

---
Jens, while these are fixes, I think it isn't extremely urgent and routing these through 3.7-rc1 should be enough.

Quoted 27 lines · Tejun Heo

b82d4b197c ("blkcg: make request_queue bypassing on allocation") made request_queues bypassed on allocation to avoid switching on and off bypass mode on a queue being initialized. Some drivers allocate and then destroy a lot of queues without fully initializing them and incurring bypass latency overhead on each of them could add upto significant overhead.

Unfortunately, blk_init_allocated_queue() is never used by queues of bio-based drivers, which means that all bio-based driver queues are in bypass mode even after initialization and registration complete successfully.

Due to the limited way request_queues are used by bio drivers, this problem is hidden pretty well but it shows up when blk-throttle is used in combination with a bio-based driver. Trying to configure (echoing to cgroupfs file) blk-throttle for a bio-based driver hangs indefinitely in blkg_conf_prep() waiting for bypass mode to end.

This patch moves the initial blk_queue_bypass_end() call from blk_init_allocated_queue() to blk_register_queue() which is called for any userland-visible queues regardless of its type.

I believe this is correct because I don't think there is any block driver which needs or wants working elevator and blk-cgroup on a queue which isn't visible to userland. If there are such users, we need a different solution.

Quoted 7 lines

Signed-off-by Tejun Heo <[email protected]>

Reported-by Joseph Glanville <[email protected]>

Cc Vivek Goyal <[email protected]>

Cc [email protected]

---
Jens, while these are fixes, I think it isn't extremely urgent and routing these through 3.7-rc1 should be enough.

Agree, I'll shove them into for-3.7/core

--
Jens Axboe

Has this patch been marked for stable?