[PATCH 1/2] blk-mq: fix error handling in __blk_mq_alloc_disk

[PATCH 1/2] blk-mq: fix error handling in __blk_mq_alloc_disk

[Christoph Hellwig]

To fully clean up the queue if the disk allocation fails we need to
call blk_mq_destroy_queue and not just blk_put_queue.

Fixes: 6f8191fdf41d ("block: simplify disk shutdown")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-mq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index d716b7f3763f3..70177ee74295b 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -3960,7 +3960,7 @@ struct gendisk *__blk_mq_alloc_disk(struct blk_mq_tag_set *set, void *queuedata,
 
 	disk = __alloc_disk_node(q, set->numa_node, lkclass);
 	if (!disk) {
-		blk_put_queue(q);
+		blk_mq_destroy_queue(q);
 		return ERR_PTR(-ENOMEM);
 	}
 	set_bit(GD_OWNS_QUEUE, &disk->state);
-- 
2.30.2

[PATCH 2/2] block: call blk_mq_exit_queue from disk_release for never added disks

[Christoph Hellwig]

To undo the all initialization from blk_mq_init_allocated_queue in case
of a probe failure where add_disk is never called we have to call
blk_mq_exit_queue from put_disk.

This relies on the fact that drivers always call blk_mq_free_tag_set
after calling put_disk in the probe error path if they have a gendisk
at all.

We should be doing this in general, but can't do it for the normal
teardown case (yet) as the tagset can be gone by the time the disk is
released once it was added.  I hope to sort this out properly eventually
but for now this isolated hack will do it.

Fixes: 6f8191fdf41d ("block: simplify disk shutdown")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/genhd.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/block/genhd.c b/block/genhd.c
index 44dfcf67ed96a..e1d5b10ac1931 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -1138,6 +1138,18 @@ static void disk_release(struct device *dev)
 	might_sleep();
 	WARN_ON_ONCE(disk_live(disk));
 
+	/*
+	 * To undo the all initialization from blk_mq_init_allocated_queue in
+	 * case of a probe failure where add_disk is never called we have to
+	 * call blk_mq_exit_queue here. We can't do this for the more common
+	 * teardown case (yet) as the tagset can be gone by the time the disk
+	 * is released once it was added.
+	 */
+	if (queue_is_mq(disk->queue) &&
+	    test_bit(GD_OWNS_QUEUE, &disk->state) &&
+	    !test_bit(GD_ADDED, &disk->state))
+		blk_mq_exit_queue(disk->queue);
+
 	blkcg_exit_queue(disk->queue);
 
 	disk_release_events(disk);
@@ -1403,6 +1415,9 @@ EXPORT_SYMBOL(__blk_alloc_disk);
  * This decrements the refcount for the struct gendisk. When this reaches 0
  * we'll have disk_release() called.
  *
+ * Note: for blk-mq disk put_disk must be called before freeing the tag_set
+ * when handling probe errors (that is before add_disk() is called).
+ *
  * Context: Any context, but the last reference must not be dropped from
  *          atomic context.
  */
-- 
2.30.2

Re: [PATCH 2/2] block: call blk_mq_exit_queue from disk_release for never added disks

[Ming Lei]

On Wed, Jul 20, 2022 at 03:05:41PM +0200, Christoph Hellwig wrote:
> To undo the all initialization from blk_mq_init_allocated_queue in case
> of a probe failure where add_disk is never called we have to call
> blk_mq_exit_queue from put_disk.
> 
> This relies on the fact that drivers always call blk_mq_free_tag_set
> after calling put_disk in the probe error path if they have a gendisk
> at all.
> 
> We should be doing this in general, but can't do it for the normal
> teardown case (yet) as the tagset can be gone by the time the disk is
> released once it was added.  I hope to sort this out properly eventually
> but for now this isolated hack will do it.
> 
> Fixes: 6f8191fdf41d ("block: simplify disk shutdown")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Ming Lei <ming.lei@redhat.com>

-- 
Ming

Re: [PATCH 1/2] blk-mq: fix error handling in __blk_mq_alloc_disk

[Ming Lei]

On Wed, Jul 20, 2022 at 03:05:40PM +0200, Christoph Hellwig wrote:
> To fully clean up the queue if the disk allocation fails we need to
> call blk_mq_destroy_queue and not just blk_put_queue.
> 
> Fixes: 6f8191fdf41d ("block: simplify disk shutdown")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/blk-mq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index d716b7f3763f3..70177ee74295b 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -3960,7 +3960,7 @@ struct gendisk *__blk_mq_alloc_disk(struct blk_mq_tag_set *set, void *queuedata,
>  
>  	disk = __alloc_disk_node(q, set->numa_node, lkclass);
>  	if (!disk) {
> -		blk_put_queue(q);
> +		blk_mq_destroy_queue(q);
>  		return ERR_PTR(-ENOMEM);
 
The same change is needed in case of blk_mq_init_allocated_queue() failure too.

Thanks,
Ming

Re: [PATCH 1/2] blk-mq: fix error handling in __blk_mq_alloc_disk

[Christoph Hellwig]

On Thu, Jul 21, 2022 at 07:55:03AM +0800, Ming Lei wrote:
> > diff --git a/block/blk-mq.c b/block/blk-mq.c
> > index d716b7f3763f3..70177ee74295b 100644
> > --- a/block/blk-mq.c
> > +++ b/block/blk-mq.c
> > @@ -3960,7 +3960,7 @@ struct gendisk *__blk_mq_alloc_disk(struct blk_mq_tag_set *set, void *queuedata,
> >  
> >  	disk = __alloc_disk_node(q, set->numa_node, lkclass);
> >  	if (!disk) {
> > -		blk_put_queue(q);
> > +		blk_mq_destroy_queue(q);
> >  		return ERR_PTR(-ENOMEM);
>  
> The same change is needed in case of blk_mq_init_allocated_queue() failure too.

I don't think so.  blk_mq_init_allocated_queue only calls
blk_mq_add_queue_tag_set at the very end, after any failure point,
and the last failure point is blk_mq_realloc_hw_ctxs not mapping
any queues.  So what would we clean up when
blk_mq_init_allocated_queue fails?

Re: [PATCH 1/2] blk-mq: fix error handling in __blk_mq_alloc_disk

[Ming Lei]

On Thu, Jul 21, 2022 at 07:03:32AM +0200, Christoph Hellwig wrote:
> On Thu, Jul 21, 2022 at 07:55:03AM +0800, Ming Lei wrote:
> > > diff --git a/block/blk-mq.c b/block/blk-mq.c
> > > index d716b7f3763f3..70177ee74295b 100644
> > > --- a/block/blk-mq.c
> > > +++ b/block/blk-mq.c
> > > @@ -3960,7 +3960,7 @@ struct gendisk *__blk_mq_alloc_disk(struct blk_mq_tag_set *set, void *queuedata,
> > >  
> > >  	disk = __alloc_disk_node(q, set->numa_node, lkclass);
> > >  	if (!disk) {
> > > -		blk_put_queue(q);
> > > +		blk_mq_destroy_queue(q);
> > >  		return ERR_PTR(-ENOMEM);
> >  
> > The same change is needed in case of blk_mq_init_allocated_queue() failure too.
> 
> I don't think so.  blk_mq_init_allocated_queue only calls
> blk_mq_add_queue_tag_set at the very end, after any failure point,
> and the last failure point is blk_mq_realloc_hw_ctxs not mapping
> any queues.  So what would we clean up when
> blk_mq_init_allocated_queue fails?
 
OK, miss that, so looks fine:

Reviewed-by: Ming Lei <ming.lei@redhat.com>


Thanks,
Ming

Re: [PATCH 1/2] blk-mq: fix error handling in __blk_mq_alloc_disk

[Jens Axboe]

On Wed, 20 Jul 2022 15:05:40 +0200, Christoph Hellwig wrote:
> To fully clean up the queue if the disk allocation fails we need to
> call blk_mq_destroy_queue and not just blk_put_queue.
> 
> 

Applied, thanks!

[1/2] blk-mq: fix error handling in __blk_mq_alloc_disk
      commit: 0a3e5cc7bbfcd571a2e53779ef7d7aa3c57d5432
[2/2] block: call blk_mq_exit_queue from disk_release for never added disks
      commit: c5db2cfc6274692d821d33b59acb6ff615e350c1

Best regards,
-- 
Jens Axboe