diff mbox series

[V2,2/3] block: don't drain in-progress dispatch in blk_cleanup_queue()

Message ID 20190401044247.29881-3-ming.lei@redhat.com (mailing list archive)
State Superseded
Headers show
Series blk-mq: allow to run queue if queue refcount is held | expand

Commit Message

Ming Lei April 1, 2019, 4:42 a.m. UTC
Now freeing hw queue resource is moved to hctx's release handler,
we don't need to worry about the race between blk_cleanup_queue and
run queue any more.

So don't drain in-progress dispatch in blk_cleanup_queue().

This is basically revert of c2856ae2f315 ("blk-mq: quiesce queue before
freeing queue").

Cc: Dongli Zhang <dongli.zhang@oracle.com>
Cc: James Smart <james.smart@broadcom.com>
Cc: Bart Van Assche <bart.vanassche@wdc.com>
Cc: linux-scsi@vger.kernel.org,
Cc: Martin K . Petersen <martin.petersen@oracle.com>,
Cc: Christoph Hellwig <hch@lst.de>,
Cc: James E . J . Bottomley <jejb@linux.vnet.ibm.com>,
Cc: jianchao wang <jianchao.w.wang@oracle.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-core.c | 12 ------------
 1 file changed, 12 deletions(-)

Comments

Bart Van Assche April 1, 2019, 3:14 p.m. UTC | #1
On Mon, 2019-04-01 at 12:42 +0800, Ming Lei wrote:
> Now freeing hw queue resource is moved to hctx's release handler,
> we don't need to worry about the race between blk_cleanup_queue and
> run queue any more.
> 
> So don't drain in-progress dispatch in blk_cleanup_queue().
> 
> This is basically revert of c2856ae2f315 ("blk-mq: quiesce queue before
> freeing queue").
> 
> Cc: Dongli Zhang <dongli.zhang@oracle.com>
> Cc: James Smart <james.smart@broadcom.com>
> Cc: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: linux-scsi@vger.kernel.org,
> Cc: Martin K . Petersen <martin.petersen@oracle.com>,
> Cc: Christoph Hellwig <hch@lst.de>,
> Cc: James E . J . Bottomley <jejb@linux.vnet.ibm.com>,
> Cc: jianchao wang <jianchao.w.wang@oracle.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  block/blk-core.c | 12 ------------
>  1 file changed, 12 deletions(-)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index b3bbf8a5110d..491dc0295778 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -347,18 +347,6 @@ void blk_cleanup_queue(struct request_queue *q)
>  
>         blk_queue_flag_set(QUEUE_FLAG_DEAD, q);
>  
> -       /*
> -        * make sure all in-progress dispatch are completed because
> -        * blk_freeze_queue() can only complete all requests, and
> -        * dispatch may still be in-progress since we dispatch requests
> -        * from more than one contexts.
> -        *
> -        * We rely on driver to deal with the race in case that queue
> -        * initialization isn't done.
> -        */
> -       if (queue_is_mq(q) && blk_queue_init_done(q))
> -               blk_mq_quiesce_queue(q);
> -
>         /* for synchronous bio-based driver finish in-flight integrity i/o */
>         blk_flush_integrity();

Many block drivers clean up resources immediately after blk_cleanup_queue()
returns. Not waiting for ongoing .queue_rq() calls to finish is wrong because
it can cause block drivers to destroy resources that are in use by a concurrent
.queue_rq() call.

Bart.
Ming Lei April 2, 2019, 12:38 a.m. UTC | #2
On Mon, Apr 01, 2019 at 08:14:59AM -0700, Bart Van Assche wrote:
> On Mon, 2019-04-01 at 12:42 +0800, Ming Lei wrote:
> > Now freeing hw queue resource is moved to hctx's release handler,
> > we don't need to worry about the race between blk_cleanup_queue and
> > run queue any more.
> > 
> > So don't drain in-progress dispatch in blk_cleanup_queue().
> > 
> > This is basically revert of c2856ae2f315 ("blk-mq: quiesce queue before
> > freeing queue").
> > 
> > Cc: Dongli Zhang <dongli.zhang@oracle.com>
> > Cc: James Smart <james.smart@broadcom.com>
> > Cc: Bart Van Assche <bart.vanassche@wdc.com>
> > Cc: linux-scsi@vger.kernel.org,
> > Cc: Martin K . Petersen <martin.petersen@oracle.com>,
> > Cc: Christoph Hellwig <hch@lst.de>,
> > Cc: James E . J . Bottomley <jejb@linux.vnet.ibm.com>,
> > Cc: jianchao wang <jianchao.w.wang@oracle.com>
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  block/blk-core.c | 12 ------------
> >  1 file changed, 12 deletions(-)
> >
> > diff --git a/block/blk-core.c b/block/blk-core.c
> > index b3bbf8a5110d..491dc0295778 100644
> > --- a/block/blk-core.c
> > +++ b/block/blk-core.c
> > @@ -347,18 +347,6 @@ void blk_cleanup_queue(struct request_queue *q)
> >  
> >         blk_queue_flag_set(QUEUE_FLAG_DEAD, q);
> >  
> > -       /*
> > -        * make sure all in-progress dispatch are completed because
> > -        * blk_freeze_queue() can only complete all requests, and
> > -        * dispatch may still be in-progress since we dispatch requests
> > -        * from more than one contexts.
> > -        *
> > -        * We rely on driver to deal with the race in case that queue
> > -        * initialization isn't done.
> > -        */
> > -       if (queue_is_mq(q) && blk_queue_init_done(q))
> > -               blk_mq_quiesce_queue(q);
> > -
> >         /* for synchronous bio-based driver finish in-flight integrity i/o */
> >         blk_flush_integrity();
> 
> Many block drivers clean up resources immediately after blk_cleanup_queue()
> returns. Not waiting for ongoing .queue_rq() calls to finish is wrong because
> it can cause block drivers to destroy resources that are in use by a concurrent
> .queue_rq() call.

blk_freeze_queue() has returned, so there can't be any in-flight IOs and .queue_rq(),
but there might be run queue activities, which do not call into driver.


Thanks,
Ming
diff mbox series

Patch

diff --git a/block/blk-core.c b/block/blk-core.c
index b3bbf8a5110d..491dc0295778 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -347,18 +347,6 @@  void blk_cleanup_queue(struct request_queue *q)
 
 	blk_queue_flag_set(QUEUE_FLAG_DEAD, q);
 
-	/*
-	 * make sure all in-progress dispatch are completed because
-	 * blk_freeze_queue() can only complete all requests, and
-	 * dispatch may still be in-progress since we dispatch requests
-	 * from more than one contexts.
-	 *
-	 * We rely on driver to deal with the race in case that queue
-	 * initialization isn't done.
-	 */
-	if (queue_is_mq(q) && blk_queue_init_done(q))
-		blk_mq_quiesce_queue(q);
-
 	/* for synchronous bio-based driver finish in-flight integrity i/o */
 	blk_flush_integrity();