[PATCH] block: drain queue before waiting for q_usage_counter becoming zero

[PATCH] block: drain queue before waiting for q_usage_counter becoming zero

[Ming Lei]

Now we track legacy requests with .q_usage_counter in commit 055f6e18e08f
("block: Make q_usage_counter also track legacy requests"), but that
commit never runs and drains legacy queue before waiting for this counter
becoming zero, then IO hang is caused in the test of pulling disk during IO.

This patch fixes the issue by draining requests before waiting for
q_usage_counter becoming zero.

Fixes: 055f6e18e08f("block: Make q_usage_counter also track legacy requests")
Cc: Wen Xiong <wenxiong@us.ibm.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-core.c | 9 +++++++--
 block/blk-mq.c   | 2 ++
 block/blk.h      | 2 ++
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 1038706edd87..5ae5ea0dca4c 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -562,6 +562,13 @@ static void __blk_drain_queue(struct request_queue *q, bool drain_all)
 	}
 }
 
+void blk_drain_queue(struct request_queue *q)
+{
+	spin_lock_irq(q->queue_lock);
+	__blk_drain_queue(q, true);
+	spin_unlock_irq(q->queue_lock);
+}
+
 /**
  * blk_queue_bypass_start - enter queue bypass mode
  * @q: queue of interest
@@ -689,8 +696,6 @@ void blk_cleanup_queue(struct request_queue *q)
 	 */
 	blk_freeze_queue(q);
 	spin_lock_irq(lock);
-	if (!q->mq_ops)
-		__blk_drain_queue(q, true);
 	queue_flag_set(QUEUE_FLAG_DEAD, q);
 	spin_unlock_irq(lock);
 
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 11097477eeab..3d3797327491 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -161,6 +161,8 @@ void blk_freeze_queue(struct request_queue *q)
 	 * exported to drivers as the only user for unfreeze is blk_mq.
 	 */
 	blk_freeze_queue_start(q);
+	if (!q->mq_ops)
+		blk_drain_queue(q);
 	blk_mq_freeze_queue_wait(q);
 }
 
diff --git a/block/blk.h b/block/blk.h
index 3f1446937aec..442098aa9463 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -330,4 +330,6 @@ static inline void blk_queue_bounce(struct request_queue *q, struct bio **bio)
 }
 #endif /* CONFIG_BOUNCE */
 
+extern void blk_drain_queue(struct request_queue *q);
+
 #endif /* BLK_INTERNAL_H */
-- 
2.9.5

Re: [PATCH] block: drain queue before waiting for q_usage_counter becoming zero

[Hannes Reinecke]

On 11/22/2017 06:11 AM, Ming Lei wrote:
> Now we track legacy requests with .q_usage_counter in commit 055f6e18e08f
> ("block: Make q_usage_counter also track legacy requests"), but that
> commit never runs and drains legacy queue before waiting for this counter
> becoming zero, then IO hang is caused in the test of pulling disk during IO.
> 
> This patch fixes the issue by draining requests before waiting for
> q_usage_counter becoming zero.
> 
> Fixes: 055f6e18e08f("block: Make q_usage_counter also track legacy requests")
> Cc: Wen Xiong <wenxiong@us.ibm.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  block/blk-core.c | 9 +++++++--
>  block/blk-mq.c   | 2 ++
>  block/blk.h      | 2 ++
>  3 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 1038706edd87..5ae5ea0dca4c 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -562,6 +562,13 @@ static void __blk_drain_queue(struct request_queue *q, bool drain_all)
>  	}
>  }
>  
> +void blk_drain_queue(struct request_queue *q)
> +{
> +	spin_lock_irq(q->queue_lock);
> +	__blk_drain_queue(q, true);
> +	spin_unlock_irq(q->queue_lock);
> +}
> +
>  /**
>   * blk_queue_bypass_start - enter queue bypass mode
>   * @q: queue of interest
> @@ -689,8 +696,6 @@ void blk_cleanup_queue(struct request_queue *q)
>  	 */
>  	blk_freeze_queue(q);
>  	spin_lock_irq(lock);
> -	if (!q->mq_ops)
> -		__blk_drain_queue(q, true);
>  	queue_flag_set(QUEUE_FLAG_DEAD, q);
>  	spin_unlock_irq(lock);
>  
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 11097477eeab..3d3797327491 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -161,6 +161,8 @@ void blk_freeze_queue(struct request_queue *q)
>  	 * exported to drivers as the only user for unfreeze is blk_mq.
>  	 */
>  	blk_freeze_queue_start(q);
> +	if (!q->mq_ops)
> +		blk_drain_queue(q);
>  	blk_mq_freeze_queue_wait(q);
>  }
>  
> diff --git a/block/blk.h b/block/blk.h
> index 3f1446937aec..442098aa9463 100644
> --- a/block/blk.h
> +++ b/block/blk.h
> @@ -330,4 +330,6 @@ static inline void blk_queue_bounce(struct request_queue *q, struct bio **bio)
>  }
>  #endif /* CONFIG_BOUNCE */
>  
> +extern void blk_drain_queue(struct request_queue *q);
> +
>  #endif /* BLK_INTERNAL_H */
> 
I seem to have missed something.
Is blk_drain_queue() ever used?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH] block: drain queue before waiting for q_usage_counter becoming zero

[Ming Lei]

On Wed, Nov 22, 2017 at 08:04:13AM +0100, Hannes Reinecke wrote:
> On 11/22/2017 06:11 AM, Ming Lei wrote:
> > Now we track legacy requests with .q_usage_counter in commit 055f6e18e08f
> > ("block: Make q_usage_counter also track legacy requests"), but that
> > commit never runs and drains legacy queue before waiting for this counter
> > becoming zero, then IO hang is caused in the test of pulling disk during IO.
> > 
> > This patch fixes the issue by draining requests before waiting for
> > q_usage_counter becoming zero.
> > 
> > Fixes: 055f6e18e08f("block: Make q_usage_counter also track legacy requests")
> > Cc: Wen Xiong <wenxiong@us.ibm.com>
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  block/blk-core.c | 9 +++++++--
> >  block/blk-mq.c   | 2 ++
> >  block/blk.h      | 2 ++
> >  3 files changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block/blk-core.c b/block/blk-core.c
> > index 1038706edd87..5ae5ea0dca4c 100644
> > --- a/block/blk-core.c
> > +++ b/block/blk-core.c
> > @@ -562,6 +562,13 @@ static void __blk_drain_queue(struct request_queue *q, bool drain_all)
> >  	}
> >  }
> >  
> > +void blk_drain_queue(struct request_queue *q)
> > +{
> > +	spin_lock_irq(q->queue_lock);
> > +	__blk_drain_queue(q, true);
> > +	spin_unlock_irq(q->queue_lock);
> > +}
> > +
> >  /**
> >   * blk_queue_bypass_start - enter queue bypass mode
> >   * @q: queue of interest
> > @@ -689,8 +696,6 @@ void blk_cleanup_queue(struct request_queue *q)
> >  	 */
> >  	blk_freeze_queue(q);
> >  	spin_lock_irq(lock);
> > -	if (!q->mq_ops)
> > -		__blk_drain_queue(q, true);
> >  	queue_flag_set(QUEUE_FLAG_DEAD, q);
> >  	spin_unlock_irq(lock);
> >  
> > diff --git a/block/blk-mq.c b/block/blk-mq.c
> > index 11097477eeab..3d3797327491 100644
> > --- a/block/blk-mq.c
> > +++ b/block/blk-mq.c
> > @@ -161,6 +161,8 @@ void blk_freeze_queue(struct request_queue *q)
> >  	 * exported to drivers as the only user for unfreeze is blk_mq.
> >  	 */
> >  	blk_freeze_queue_start(q);
> > +	if (!q->mq_ops)
> > +		blk_drain_queue(q);
> >  	blk_mq_freeze_queue_wait(q);
> >  }
> >  
> > diff --git a/block/blk.h b/block/blk.h
> > index 3f1446937aec..442098aa9463 100644
> > --- a/block/blk.h
> > +++ b/block/blk.h
> > @@ -330,4 +330,6 @@ static inline void blk_queue_bounce(struct request_queue *q, struct bio **bio)
> >  }
> >  #endif /* CONFIG_BOUNCE */
> >  
> > +extern void blk_drain_queue(struct request_queue *q);
> > +
> >  #endif /* BLK_INTERNAL_H */
> > 
> I seem to have missed something.
> Is blk_drain_queue() ever used?

It is always called in blk_cleanup_queue().

Before 055f6e18e08f("block: Make q_usage_counter also track legacy requests",
the q_usage_counter just works for sync IO(such as dax) because we only
hold the refcount when calling q->make_request_fn(), so blk_drain_queue()
does do the job always.

Thanks,
Ming

Re: [PATCH] block: drain queue before waiting for q_usage_counter becoming zero

[Bart Van Assche]

T24gV2VkLCAyMDE3LTExLTIyIGF0IDEzOjExICswODAwLCBNaW5nIExlaSB3cm90ZToNCj4gZGlm
ZiAtLWdpdCBhL2Jsb2NrL2Jsay1tcS5jIGIvYmxvY2svYmxrLW1xLmMNCj4gaW5kZXggMTEwOTc0
NzdlZWFiLi4zZDM3OTczMjc0OTEgMTAwNjQ0DQo+IC0tLSBhL2Jsb2NrL2Jsay1tcS5jDQo+ICsr
KyBiL2Jsb2NrL2Jsay1tcS5jDQo+IEBAIC0xNjEsNiArMTYxLDggQEAgdm9pZCBibGtfZnJlZXpl
X3F1ZXVlKHN0cnVjdCByZXF1ZXN0X3F1ZXVlICpxKQ0KPiAgCSAqIGV4cG9ydGVkIHRvIGRyaXZl
cnMgYXMgdGhlIG9ubHkgdXNlciBmb3IgdW5mcmVlemUgaXMgYmxrX21xLg0KPiAgCSAqLw0KPiAg
CWJsa19mcmVlemVfcXVldWVfc3RhcnQocSk7DQo+ICsJaWYgKCFxLT5tcV9vcHMpDQo+ICsJCWJs
a19kcmFpbl9xdWV1ZShxKTsNCj4gIAlibGtfbXFfZnJlZXplX3F1ZXVlX3dhaXQocSk7DQo+ICB9
DQoNClNpbmNlIHFfdXNhZ2VfY291bnRlciBub3cgdHJhY2tzIGxlZ2FjeSByZXF1ZXN0cywgaXMg
dGhlcmUgYW55IHJlYXNvbiB3aHkgd2UNCnN0aWxsIG5lZWQgX19ibGtfZHJhaW5fcXVldWUoKT8g
SGF2ZSB5b3UgY29uc2lkZXJlZCB0byBlbGltaW5hdGUNCl9fYmxrX2RyYWluX3F1ZXVlKCkgYW5k
IHRvIGNhbGwgYmxrX3J1bl9xdWV1ZSgpIGZyb20gaW5zaWRlIGJsa19mcmVlemVfcXVldWUoKQ0K
aW5zdGVhZCBvZiBjYWxsaW5nIGJsa19kcmFpbl9xdWV1ZSgpPyBJJ20gYXNraW5nIHRoaXMgYmVj
YXVzZQ0KYmxrX21xX2ZyZWV6ZV9xdWV1ZV93YWl0KCkgdXNlcyBhIG1vcmUgZWZmaWNpZW50IG1l
Y2hhbmlzbSAod2FpdF9ldmVudCgpKQ0KdGhhbiBfX2Jsa19kcmFpbl9xdWV1ZSgpICh3aGlsZSAo
Li4uKSBtc2xlZXAoMTApKS4NCg0KQmFydC4=

Re: [PATCH] block: drain queue before waiting for q_usage_counter becoming zero

[Ming Lei]

On Wed, Nov 22, 2017 at 04:47:48PM +0000, Bart Van Assche wrote:
> On Wed, 2017-11-22 at 13:11 +0800, Ming Lei wrote:
> > diff --git a/block/blk-mq.c b/block/blk-mq.c
> > index 11097477eeab..3d3797327491 100644
> > --- a/block/blk-mq.c
> > +++ b/block/blk-mq.c
> > @@ -161,6 +161,8 @@ void blk_freeze_queue(struct request_queue *q)
> >  	 * exported to drivers as the only user for unfreeze is blk_mq.
> >  	 */
> >  	blk_freeze_queue_start(q);
> > +	if (!q->mq_ops)
> > +		blk_drain_queue(q);
> >  	blk_mq_freeze_queue_wait(q);
> >  }
> 
> Since q_usage_counter now tracks legacy requests, is there any reason why we
> still need __blk_drain_queue()? Have you considered to eliminate
> __blk_drain_queue() and to call blk_run_queue() from inside blk_freeze_queue()
> instead of calling blk_drain_queue()? I'm asking this because

Yeah, that looks better, I am thinking of that too, will do this way
in V2.

-- 
Ming