diff mbox series

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

Message ID 20210521055116.1053587-14-hch@lst.de (mailing list archive)
State New
Headers show
Series [01/26] block: refactor device number setup in __device_add_disk | expand

Commit Message

Christoph Hellwig May 21, 2021, 5:51 a.m. UTC
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(-)

Comments

Hannes Reinecke May 23, 2021, 8:10 a.m. UTC | #1
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
Christoph Hellwig May 24, 2021, 7:25 a.m. UTC | #2
On Sun, May 23, 2021 at 10:10:34AM +0200, Hannes Reinecke wrote:
> 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?

I guess we could do that, not sure it is worth the churn, though.
diff mbox series

Patch

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);
 
@@ -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;