Re: [PATCH V2 1/1] nvme: introduce generic per-namespace chardev

[Minwoo Im <minwoo.im.dev@gmail.com>]

On 21-04-08 00:35:36, Minwoo Im wrote:
> On 21-04-07 16:21:52, Christoph Hellwig wrote:
> > On Wed, Apr 07, 2021 at 11:11:28PM +0900, Minwoo Im wrote:
> > > Then can we have like this ?  The following diff is just considering the
> > > ns_head only, but just conceptually, not hanging out with bdev, it's
> > > just get the reference.
> > 
> > FYI, this is what I did today.  Not really tested yet and at least
> > one known issue.  Busy with calls for now, but I hope to have something
> > ready tonight:
> > 
> > http://git.infradead.org/users/hch/block.git/shortlog/refs/heads/nvme-generic
> 
> Here's an additional patch based on the branch above:
> 
> 1. During the `nvme list` command, controller ioctl for the ns_head has
>    been not coming out from the mutex_lock_killable(&nvme_subsystems_lock)
>    because it just gets the controller reference and return it without
>    unlocking it.  So the first change point of this patch is to unlock the
>    mutex right before the return.  But, Is this a real issue? because
>    this changes are not from this series though.....
> 
> 2. Can we have the check whether the ns_head has disk allocated or not
>    by getting `disk` pointer out of the #ifdef CONFIG_NVME_MULTIPATH?
>    If it's not allocated due to some reasons (e.g., !multipath, or CMIC
>    does not support multiple controllers, or some failures during the
>    allocations), disk will never be allocated.  So, I tried to pull the
>    `disk` pointer out of the #ifdef from the nvme_ns_head, but maybe
>    this is not what you have intended....  It would be great if you can
>    give some feedback on this.
> 
> I had a quick tests based on the branch with the following patch:
>   - !CONFIG_NVME_MULTIPATH
>   - CONFIG_NVME_MULTIPATH && !multipath

Forgot to mention a case:
    - CONFIG_NVME_MULTIPATH && multipath

>   - Basic I/O for /dev/ng* chardev through `nvme io-passthru` command.
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 2b04fc451f09..f05eff5b7c30 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2016,6 +2016,7 @@ struct nvme_ctrl *nvme_find_get_live_ctrl(struct nvme_subsystem *subsys)
>  		if (ctrl->state != NVME_CTRL_LIVE)
>  			continue;
>  		nvme_get_ctrl(ctrl);
> +		mutex_unlock(&nvme_subsystems_lock);
>  		return ctrl;
>  	}
>  
> @@ -3687,8 +3688,8 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
>  	nvme_get_ctrl(ctrl);
>  
>  	device_add_disk(ctrl->device, ns->disk, nvme_ns_id_attr_groups);
> -	// XXX: only for the !multipath case
> -	nvme_add_ns_cdev(ns);
> +	if (!nvme_ns_head_multipath(ns->head))
> +		nvme_add_ns_cdev(ns);
>  
>  	nvme_mpath_add_disk(ns, id);
>  	nvme_fault_inject_init(&ns->fault_inject, ns->disk->disk_name);
> @@ -3733,8 +3734,8 @@ static void nvme_ns_remove(struct nvme_ns *ns)
>  	synchronize_srcu(&ns->head->srcu); /* wait for concurrent submissions */
>  
>  	if (ns->disk->flags & GENHD_FL_UP) {
> -		// XXX: only for !multipath
> -		cdev_device_del(&ns->cdev, &ns->cdev_device);
> +		if (!nvme_ns_head_multipath(ns->head))
> +			cdev_device_del(&ns->cdev, &ns->cdev_device);
>  		del_gendisk(ns->disk);
>  		blk_cleanup_queue(ns->queue);
>  		if (blk_get_integrity(ns->disk))
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index dc1846d3e4f2..91ff75b41ed6 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -417,8 +417,9 @@ struct nvme_ns_head {
>  	struct cdev		cdev;
>  	struct device		cdev_device;
>  
> -#ifdef CONFIG_NVME_MULTIPATH
>  	struct gendisk		*disk;
> +
> +#ifdef CONFIG_NVME_MULTIPATH
>  	struct bio_list		requeue_list;
>  	spinlock_t		requeue_lock;
>  	struct work_struct	requeue_work;
> @@ -429,6 +430,11 @@ struct nvme_ns_head {
>  #endif
>  };
>  
> +static inline bool nvme_ns_head_multipath(struct nvme_ns_head *head)
> +{
> +	return !!head->disk;
> +}
> +
>  enum nvme_ns_features {
>  	NVME_NS_EXT_LBAS = 1 << 0, /* support extended LBA format */
>  	NVME_NS_METADATA_SUPPORTED = 1 << 1, /* support getting generated md */

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme