block: blk-throttle should be drained regardless of q->elevator

PATCH In review Tejun Heo 3 participants 7 messages Feb 13, 2012 – Feb 15, 2012 Block layer

Share & tools

Tejun Heo

author

2012-02-13 22:52 UTC #0 lore raw

PATCH 1/1 block: blk-throttle should be drained regardless of q->elevator

Currently, blk_cleanup_queue() doesn't call elv_drain_elevator() if q->elevator doesn't exist; however, bio based drivers don't have elevator initialized but can still use blk-throttle. This patch moves q->elevator test inside blk_drain_queue() such that only elv_drain_elevator() is skipped if !q->elevator.

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

Reported-by Vivek Goyal <[email protected]>

---
 block/blk-core.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

block/blk-core.c +9 -8

Index: work/block/blk-core.c
===================================================================
@@ -365,7 +365,13 @@ void blk_drain_queue(struct request_queu
 
 		spin_lock_irq(q->queue_lock);
 
-		elv_drain_elevator(q);
+		/*
+		 * The caller might be trying to drain @q before its
+		 * elevator is initialized.
+		 */
+		if (q->elevator)
+			elv_drain_elevator(q);
+
 		if (drain_all)
 			blk_throtl_drain(q);

@@ -428,13 +434,8 @@ void blk_cleanup_queue(struct request_qu
 	spin_unlock_irq(lock);
 	mutex_unlock(&q->sysfs_lock);
 
-	/*
-	 * Drain all requests queued before DEAD marking.  The caller might
-	 * be trying to tear down @q before its elevator is initialized, in
-	 * which case we don't want to call into draining.
-	 */
-	if (q->elevator)
-		blk_drain_queue(q, true);
+	/* drain all requests queued before DEAD marking */
+	blk_drain_queue(q, true);
 
 	/* @q won't process any more request, flush async actions */
 	del_timer_sync(&q->backing_dev_info.laptop_mode_wb_timer);

Vivek Goyal

Acked-by on v1 1/1

2012-02-13 23:27 UTC #1 lore raw

Quoted 8 lines · Tejun Heo

Currently, blk_cleanup_queue() doesn't call elv_drain_elevator() if q->elevator doesn't exist; however, bio based drivers don't have elevator initialized but can still use blk-throttle. This patch moves q->elevator test inside blk_drain_queue() such that only elv_drain_elevator() is skipped if !q->elevator.
Signed-off-by Tejun Heo <[email protected]>
Reported-by Vivek Goyal <[email protected]>

Thanks Tejun for fixing this. Looks good to me. Just a minor nit below.

Acked-by Vivek Goyal <[email protected]>

Vivek

[..]

block/blk-core.c void blk_cleanup_queue(struct request_qu · line 437

Quoted 12 lines

block/blk-core.c +1 -7

@@ -428,13 +434,8 @@ void blk_cleanup_queue(struct request_qu
 	spin_unlock_irq(lock);
 	mutex_unlock(&q->sysfs_lock);
 
-	/*
-	 * Drain all requests queued before DEAD marking.  The caller might
-	 * be trying to tear down @q before its elevator is initialized, in
-	 * which case we don't want to call into draining.
-	 */
-	if (q->elevator)
-		blk_drain_queue(q, true);
+	/* drain all requests queued before DEAD marking */

We have already marked the queue DEAD before we start draining the queue. May be we need to fix the comment.

Quoted 1 line

block/blk-core.c +1 -0
+	blk_drain_queue(q, true);

Tejun Heo

author

2012-02-13 23:41 UTC #2 lore raw

Hello,

Quoted 11 lines · Vivek Goyal

block/blk-core.c void blk_cleanup_queue(struct request_qu · line 437
Quoted 8 lines
block/blk-core.c +1 -7
-	/*
-	 * Drain all requests queued before DEAD marking.  The caller might
-	 * be trying to tear down @q before its elevator is initialized, in
-	 * which case we don't want to call into draining.
-	 */
-	if (q->elevator)
-		blk_drain_queue(q, true);
+	/* drain all requests queued before DEAD marking */
We have already marked the queue DEAD before we start draining the queue. May be we need to fix the comment.

Hmmm... it actually is correct. It drains all requests which were queued before the preceding DEAD marking. ie... it's describing the following.

1. requests queued
2. mark q DEAD
3. drain requests which were queued before #2. We don't care about requests queued after #2.

Thanks.

Signature · 1 line

tejun

Tejun Heo

author

2012-02-14 01:14 UTC #4 lore raw

PATCH 1/1 block: blk-throttle should be drained regardless of q->elevator

In reply to Tejun Heo

-v2: loop can have registered queue which has NULL request_fn. Make
sure we don't call into __blk_run_queue() in such cases.

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

Reported-by Vivek Goyal <[email protected]>

Acked-by Vivek Goyal <[email protected]>

---
Added check against NULL q->request_fn for loop driver. Thanks.

 block/blk-core.c |   26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

block/blk-core.c +14 ~~-12~~

Index: work/block/blk-core.c
===================================================================
@@ -365,17 +365,24 @@ void blk_drain_queue(struct request_queu
 
 		spin_lock_irq(q->queue_lock);
 
-		elv_drain_elevator(q);
+		/*
+		 * The caller might be trying to drain @q before its
+		 * elevator is initialized.
+		 */
+		if (q->elevator)
+			elv_drain_elevator(q);
+
 		if (drain_all)
 			blk_throtl_drain(q);
 
 		/*
 		 * This function might be called on a queue which failed
-		 * driver init after queue creation.  Some drivers
-		 * (e.g. fd) get unhappy in such cases.  Kick queue iff
-		 * dispatch queue has something on it.
+		 * driver init after queue creation or is not yet fully
+		 * active yet.  Some drivers (e.g. fd and loop) get unhappy
+		 * in such cases.  Kick queue iff dispatch queue has
+		 * something on it and @q has request_fn set.
 		 */
-		if (!list_empty(&q->queue_head))
+		if (!list_empty(&q->queue_head) && q->request_fn)
 			__blk_run_queue(q);
 
 		drain |= q->rq.elvpriv;

@@ -428,13 +435,8 @@ void blk_cleanup_queue(struct request_qu
 	spin_unlock_irq(lock);
 	mutex_unlock(&q->sysfs_lock);
 
-	/*
-	 * Drain all requests queued before DEAD marking.  The caller might
-	 * be trying to tear down @q before its elevator is initialized, in
-	 * which case we don't want to call into draining.
-	 */
-	if (q->elevator)
-		blk_drain_queue(q, true);
+	/* drain all requests queued before DEAD marking */
+	blk_drain_queue(q, true);
 
 	/* @q won't process any more request, flush async actions */
 	del_timer_sync(&q->backing_dev_info.laptop_mode_wb_timer);

Vivek Goyal

2012-02-15 15:53 UTC #3 lore raw

In reply to Tejun Heo

Quoted 15 lines · Tejun Heo

Hello,
Quoted 11 lines · Vivek Goyal
block/blk-core.c void blk_cleanup_queue(struct request_qu · line 437
Quoted 8 lines
block/blk-core.c +1 -7
-	/*
-	 * Drain all requests queued before DEAD marking.  The caller might
-	 * be trying to tear down @q before its elevator is initialized, in
-	 * which case we don't want to call into draining.
-	 */
-	if (q->elevator)
-		blk_drain_queue(q, true);
+	/* drain all requests queued before DEAD marking */
We have already marked the queue DEAD before we start draining the queue. May be we need to fix the comment.

Quoted 8 lines

Hmmm... it actually is correct. It drains all requests which were queued before the preceding DEAD marking. ie... it's describing the following.
1. requests queued
2. mark q DEAD
3. drain requests which were queued before #2. We don't care about requests queued after #2.

Ok, thanks. I read the comment wrong. I thought of it as "drain all requests before marking queue DEAD". :-)

Thanks
Vivek

Vivek Goyal

2012-02-15 15:57 UTC #5 lore raw

In reply to Tejun Heo

On Mon, Feb 13, 2012 at 05:14:27PM -0800, Tejun Heo wrote:

[..]

block/blk-core.c void blk_drain_queue(struct request_queu · line 386

Quoted 13 lines

block/blk-core.c +5 -4

 		/*
 		 * This function might be called on a queue which failed
-		 * driver init after queue creation.  Some drivers
-		 * (e.g. fd) get unhappy in such cases.  Kick queue iff
-		 * dispatch queue has something on it.
+		 * driver init after queue creation or is not yet fully
+		 * active yet.  Some drivers (e.g. fd and loop) get unhappy
+		 * in such cases.  Kick queue iff dispatch queue has
+		 * something on it and @q has request_fn set.
 		 */
-		if (!list_empty(&q->queue_head))
+		if (!list_empty(&q->queue_head) && q->request_fn)
 			__blk_run_queue(q);

Is it not a BUG() condition. We have a queue which has some requests in it and we don't have q->request_fn?

Thanks
Vivek

Jens Axboe

maintainer Applied

2012-02-15 15:58 UTC #6 lore raw

In reply to Tejun Heo

Quoted 6 lines · Tejun Heo

Currently, blk_cleanup_queue() doesn't call elv_drain_elevator() if q->elevator doesn't exist; however, bio based drivers don't have elevator initialized but can still use blk-throttle. This patch moves q->elevator test inside blk_drain_queue() such that only elv_drain_elevator() is skipped if !q->elevator.

Quoted 8 lines

-v2: loop can have registered queue which has NULL request_fn. Make
sure we don't call into __blk_run_queue() in such cases.
Signed-off-by Tejun Heo <[email protected]>
Reported-by Vivek Goyal <[email protected]>
Acked-by Vivek Goyal <[email protected]>
---
Added check against NULL q->request_fn for loop driver. Thanks.

Applied, thanks.

Signature · 1 line

Jens Axboe