| Message ID | 20220718062928.335399-1-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] block: call blk_mq_exit_queue from disk_release for never added disks | expand |
On Mon, Jul 18, 2022 at 08:29:27AM +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. > > 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 eventual > but for now this isolated hack will do it. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > block/genhd.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/block/genhd.c b/block/genhd.c > index 44dfcf67ed96a..ad8a3678d4480 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); > + Request queue is allocated successfully, but disk allocation may fail, so blk_mq_exit_queue still may be missed in this case. thanks, Ming
On Mon, Jul 18, 2022 at 03:18:52PM +0800, Ming Lei wrote: > On Mon, Jul 18, 2022 at 08:29:27AM +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. > > > > 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 eventual > > but for now this isolated hack will do it. > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > --- > > block/genhd.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/block/genhd.c b/block/genhd.c > > index 44dfcf67ed96a..ad8a3678d4480 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); > > + > > Request queue is allocated successfully, but disk allocation may fail, > so blk_mq_exit_queue still may be missed in this case. Or we do have request queue uses without disk attached, such as nvme admin/connection queue. Thanks, Ming
On Mon, Jul 18, 2022 at 03:18:52PM +0800, Ming Lei wrote: > Request queue is allocated successfully, but disk allocation may fail, > so blk_mq_exit_queue still may be missed in this case. Yes. That's a separate issue, though and solved by this one liner: 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);
On Mon, Jul 18, 2022 at 04:33:33PM +0800, Ming Lei wrote: > > Request queue is allocated successfully, but disk allocation may fail, > > so blk_mq_exit_queue still may be missed in this case. > > Or we do have request queue uses without disk attached, such as nvme > admin/connection queue. All these need to call blk_mq_destroy_queue anyway, and as far as I can tell do that already.
On Mon, Jul 18, 2022 at 03:07:25PM +0200, Christoph Hellwig wrote: > On Mon, Jul 18, 2022 at 03:18:52PM +0800, Ming Lei wrote: > > Request queue is allocated successfully, but disk allocation may fail, > > so blk_mq_exit_queue still may be missed in this case. > > Yes. That's a separate issue, though and solved by this one liner: > > 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); OK, but it is still tricky, since there isn't the required order between calling blk_mq_free_tag_set and put_disk, but now put_disk has to be called before calling blk_mq_free_tag_set, so you may have to audit drivers first, and make sure that all put_disk is called before blk_mq_free_tag_set() in driver's error handling code path. Thanks, Ming
diff --git a/block/genhd.c b/block/genhd.c index 44dfcf67ed96a..ad8a3678d4480 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);
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. 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 eventual but for now this isolated hack will do it. Signed-off-by: Christoph Hellwig <hch@lst.de> --- block/genhd.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)