Re: [PATCH 18/26] nvme-multipath: convert to blk_alloc_disk/blk_cleanup_disk

[Hannes Reinecke <hare@suse.de>]

On 5/21/21 7:51 AM, Christoph Hellwig wrote:
> Convert the nvme-multipath 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/nvdimm/pmem.c         |  1 -
>   drivers/nvme/host/multipath.c | 45 ++++++++++-------------------------
>   2 files changed, 13 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> index 9fcd05084564..31f3c4bd6f72 100644
> --- a/drivers/nvdimm/pmem.c
> +++ b/drivers/nvdimm/pmem.c
> @@ -472,7 +472,6 @@ static int pmem_attach_disk(struct device *dev,
>   		blk_queue_flag_set(QUEUE_FLAG_DAX, q);
>   
>   	disk->fops		= &pmem_fops;
> -	disk->queue		= q;
>   	disk->private_data	= pmem;
>   	nvdimm_namespace_disk_name(ndns, disk->disk_name);
>   	set_capacity(disk, (pmem->size - pmem->pfn_pad - pmem->data_offset)
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index a5d02f236cca..b5fbdb416022 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -427,7 +427,6 @@ static void nvme_requeue_work(struct work_struct *work)
>   
>   int nvme_mpath_alloc_disk(struct nvme_ctrl *ctrl, struct nvme_ns_head *head)
>   {
> -	struct request_queue *q;
>   	bool vwc = false;
>   
>   	mutex_init(&head->lock);
> @@ -443,33 +442,24 @@ int nvme_mpath_alloc_disk(struct nvme_ctrl *ctrl, struct nvme_ns_head *head)
>   	if (!(ctrl->subsys->cmic & NVME_CTRL_CMIC_MULTI_CTRL) || !multipath)
>   		return 0;
>   
> -	q = blk_alloc_queue(ctrl->numa_node);
> -	if (!q)
> -		goto out;
> -	blk_queue_flag_set(QUEUE_FLAG_NONROT, q);
> -	/* set to a default value for 512 until disk is validated */
> -	blk_queue_logical_block_size(q, 512);
> -	blk_set_stacking_limits(&q->limits);
> -
> -	/* we need to propagate up the VMC settings */
> -	if (ctrl->vwc & NVME_CTRL_VWC_PRESENT)
> -		vwc = true;
> -	blk_queue_write_cache(q, vwc, vwc);
> -
> -	head->disk = alloc_disk(0);
> +	head->disk = blk_alloc_disk(ctrl->numa_node);
>   	if (!head->disk)
> -		goto out_cleanup_queue;
> +		return -ENOMEM;
>   	head->disk->fops = &nvme_ns_head_ops;
>   	head->disk->private_data = head;
> -	head->disk->queue = q;
>   	sprintf(head->disk->disk_name, "nvme%dn%d",
>   			ctrl->subsys->instance, head->instance);
> -	return 0;
>   
> -out_cleanup_queue:
> -	blk_cleanup_queue(q);
> -out:
> -	return -ENOMEM;
> +	blk_queue_flag_set(QUEUE_FLAG_NONROT, head->disk->queue);
> +	/* set to a default value of 512 until the disk is validated */
> +	blk_queue_logical_block_size(head->disk->queue, 512);
> +	blk_set_stacking_limits(&head->disk->queue->limits);
> +
> +	/* we need to propagate up the VMC settings */
> +	if (ctrl->vwc & NVME_CTRL_VWC_PRESENT)
> +		vwc = true;
> +	blk_queue_write_cache(head->disk->queue, vwc, vwc);
> +	return 0;
>   }
>   
>   static void nvme_mpath_set_live(struct nvme_ns *ns)
> @@ -768,16 +758,7 @@ void nvme_mpath_remove_disk(struct nvme_ns_head *head)
>   	/* make sure all pending bios are cleaned up */
>   	kblockd_schedule_work(&head->requeue_work);
>   	flush_work(&head->requeue_work);
> -	blk_cleanup_queue(head->disk->queue);
> -	if (!test_bit(NVME_NSHEAD_DISK_LIVE, &head->flags)) {
> -		/*
> -		 * if device_add_disk wasn't called, prevent
> -		 * disk release to put a bogus reference on the
> -		 * request queue
> -		 */
> -		head->disk->queue = NULL;
> -	}
> -	put_disk(head->disk);
> +	blk_cleanup_disk(head->disk);
>   }
>   
>   void nvme_mpath_init_ctrl(struct nvme_ctrl *ctrl)
> 
What about the check for GENHD_FL_UP a bit further up in line 766?
Can this still happen with the new allocation scheme, ie is there still 
a difference in lifetime between ->disk and ->disk->queue?

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