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

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

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
  - 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