[PATCH block/for-linus] block: blk-throttle should be drained regardless of q->elevator

[PATCH block/for-linus] block: blk-throttle should be drained regardless of q->elevator

[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 <tj@kernel.org>
Reported-by: Vivek Goyal <vgoyal@redhat.com>
---
 block/blk-core.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

Index: work/block/blk-core.c
===================================================================
--- work.orig/block/blk-core.c
+++ 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);

Re: [PATCH block/for-linus] block: blk-throttle should be drained regardless of q->elevator

[Vivek Goyal]

On Mon, Feb 13, 2012 at 02:52:48PM -0800, Tejun Heo wrote:
> 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 <tj@kernel.org>
> Reported-by: Vivek Goyal <vgoyal@redhat.com>

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

Acked-by: Vivek Goyal <vgoyal@redhat.com>

Vivek

[..]
> @@ -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.

> +	blk_drain_queue(q, true);

Re: [PATCH block/for-linus] block: blk-throttle should be drained regardless of q->elevator

[Tejun Heo]

Hello,

On Mon, Feb 13, 2012 at 06:27:42PM -0500, Vivek Goyal wrote:
> > -	/*
> > -	 * 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.

-- 
tejun

[PATCH UPDATED block/for-linus] block: blk-throttle should be drained regardless of q->elevator

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

-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 <tj@kernel.org>
Reported-by: Vivek Goyal <vgoyal@redhat.com>
Acked-by: Vivek Goyal <vgoyal@redhat.com>
---
Added check against NULL q->request_fn for loop driver.  Thanks.

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

Index: work/block/blk-core.c
===================================================================
--- work.orig/block/blk-core.c
+++ 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);

Re: [PATCH block/for-linus] block: blk-throttle should be drained regardless of q->elevator

[Vivek Goyal]

On Mon, Feb 13, 2012 at 03:40:57PM -0800, Tejun Heo wrote:
> Hello,
> 
> On Mon, Feb 13, 2012 at 06:27:42PM -0500, Vivek Goyal wrote:
> > > -	/*
> > > -	 * 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.

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

Thanks
Vivek

Re: [PATCH UPDATED block/for-linus] block: blk-throttle should be drained regardless of q->elevator

[Vivek Goyal]

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

[..]
>  		/*
>  		 * 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

Re: [PATCH UPDATED block/for-linus] block: blk-throttle should be drained regardless of q->elevator

[Jens Axboe]

On 2012-02-14 02:14, Tejun Heo wrote:
> 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.
> 
> -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 <tj@kernel.org>
> Reported-by: Vivek Goyal <vgoyal@redhat.com>
> Acked-by: Vivek Goyal <vgoyal@redhat.com>
> ---
> Added check against NULL q->request_fn for loop driver.  Thanks.

Applied, thanks.

-- 
Jens Axboe