Re: [PATCH 13/26] dm: convert to blk_alloc_disk/blk_cleanup_disk

Hannes Reinecke
2021-05-23T08:10:39Z

On 5/21/21 7:51 AM, Christoph Hellwig wrote:
> Convert the dm driver to use the blk_alloc_disk and blk_cleanup_disk
> helpers to simplify gendisk and request_queue allocation.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/md/dm.c | 16 +++++++---------
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index ca2aedd8ee7d..3c7c2d257018 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -1801,13 +1801,13 @@ static void cleanup_mapped_device(struct mapped_device *md)
> md->disk->private_data = NULL;
> spin_unlock(&_minor_lock);
> del_gendisk(md->disk);
> - put_disk(md->disk);
> }
>
> - if (md->queue) {
> + if (md->queue)
> dm_queue_destroy_keyslot_manager(md->queue);
> - blk_cleanup_queue(md->queue);
> - }
> +
> + if (md->disk)
> + blk_cleanup_disk(md->disk);
>
> cleanup_srcu_struct(&md->io_barrier);
>

Can't these conditionals be merged into a single 'if (md->disk)'?
Eg like:

if (md->disk) {
spin_lock(&_minor_lock);
md->disk->private_data = NULL;
spin_unlock(&_minor_lock);
del_gendisk(md->disk);
dm_queue_destroy_keyslot_manager(md->queue);
blk_cleanup_disk(md->queue);
}

We're now always allocating 'md->disk' and 'md->queue' together,
so how can we end up in a situation where one is set without the other?

> @@ -1869,13 +1869,10 @@ static struct mapped_device *alloc_dev(int minor)
> * established. If request-based table is loaded: blk-mq will
> * override accordingly.
> */
> - md->queue = blk_alloc_queue(numa_node_id);
> - if (!md->queue)
> - goto bad;
> -
> - md->disk = alloc_disk_node(1, md->numa_node_id);
> + md->disk = blk_alloc_disk(md->numa_node_id);
> if (!md->disk)
> goto bad;
> + md->queue = md->disk->queue;
>
> init_waitqueue_head(&md->wait);
> INIT_WORK(&md->work, dm_wq_work);
> @@ -1888,6 +1885,7 @@ static struct mapped_device *alloc_dev(int minor)
>
> md->disk->major = _major;
> md->disk->first_minor = minor;
> + md->disk->minors = 1;
> md->disk->fops = &dm_blk_dops;
> md->disk->queue = md->queue;
> md->disk->private_data = md;
>
Cheers,

Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer