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

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

[Minwoo Im]

Hello,

This is the second patch series to support generic ns character device
to expose per-namespace instance to the userspace.  This version fixed
code mis-ordered reported by Kanchan.

This patch introduces per-namespace character device to I/O in case that
blkdev is not initialized properly.  Userspace applications are able to
I/O to the generic namespace chardev even there's no blkdev properly
initialized.  Because we don't allow nvme controller device to I/O with
a specified nsid, this generic device will provide a way to I/O.

This patch is derived from Javier's patch series [1].  Javier and I have
re-coded this series again and it starts with new version tag.  Changes
from the previous series are:

  - Update naming convention for the chardev exactly the same with the
    blkdev:
        /dev/nvme-generic-XcYnZ  to  /dev/nvme-generic-XnY

  - Re-consider the multipath support.  Rather than exposing all the
    path-specific namespace path with controller(e.g., nvmeXcYnZ), only
    expose head namespace instance in case of multipath.  Otherwise, it
    will expose all the blkdevs created.

[1] https://lore.kernel.org/linux-nvme/20210301192452.16770-1-javier.gonz@samsung.com/

Since V1:
  - Fix code mis-ordered issue about nvme-subsystem class destroy.
  - Fix for case !multipath, but path-specified namespaces.  Eariler, it
    just created the generic device when the ns_head is initialized, but
    even it's a same namespace and path is different, then generic
    device will be shown as usual in case of !multipath.

Minwoo Im (1):
  nvme: introduce generic per-namespace chardev

 drivers/nvme/host/core.c      | 172 +++++++++++++++++++++++++++++++++-
 drivers/nvme/host/multipath.c |  17 ++++
 drivers/nvme/host/nvme.h      |  25 +++++
 3 files changed, 212 insertions(+), 2 deletions(-)

-- 
2.27.0


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

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

[Minwoo Im]

Userspace has not been allowed to I/O to device that's failed to
be initialized.  This patch introduces generic per-namespace character
device to allow userspace to I/O regardless the block device is there or
not.

The chardev naming convention will exactly be the same with the existing
blkdev's one.  nvme_set_generic_ns_name() introduced in this patch may
look like a duplication of nvme_set_disk_name(), but it's not allowing
controller path specific.  It just for per-namespace path specific.  So,
the naming will be:

	- /dev/nvme-generic-XnY

It also supports multipath which means it will not expose chardev for
the hidden namespace blkdevs (e.g., nvmeXcYnZ).  If
/dev/nvme-generic-XnY is created for the ns_head, then I/O request will
be routed to the controller-specified path by the iopolicy in the
subsystem.

This can be controlled by the module parameter `generic_ns` which is
turned on by default.

Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
Signed-off-by: Javier González <javier.gonz@samsung.com>
---
 drivers/nvme/host/core.c      | 172 +++++++++++++++++++++++++++++++++-
 drivers/nvme/host/multipath.c |  17 ++++
 drivers/nvme/host/nvme.h      |  25 +++++
 3 files changed, 212 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 83dc9403fbf4..a06634e7bad1 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -61,6 +61,10 @@ static bool streams;
 module_param(streams, bool, 0644);
 MODULE_PARM_DESC(streams, "turn on support for Streams write directives");
 
+static bool generic_ns = true;
+module_param(generic_ns, bool, 0644);
+MODULE_PARM_DESC(generic_ns, "support generic namespace character device");
+
 /*
  * nvme_wq - hosts nvme related works that are not reset or delete
  * nvme_reset_wq - hosts nvme reset works
@@ -85,8 +89,11 @@ static LIST_HEAD(nvme_subsystems);
 static DEFINE_MUTEX(nvme_subsystems_lock);
 
 static DEFINE_IDA(nvme_instance_ida);
+static DEFINE_IDA(nvme_generic_ns_minor_ida);
 static dev_t nvme_ctrl_base_chr_devt;
+static dev_t nvme_generic_ns_devt;
 static struct class *nvme_class;
+static struct class *nvme_generic_ns_class;
 static struct class *nvme_subsys_class;
 
 static void nvme_put_subsystem(struct nvme_subsystem *subsys);
@@ -542,6 +549,7 @@ static void nvme_free_ns_head(struct kref *ref)
 	struct nvme_ns_head *head =
 		container_of(ref, struct nvme_ns_head, ref);
 
+	cdev_device_del(&head->generic_ns->cdev, &head->generic_ns->device);
 	nvme_mpath_remove_disk(head);
 	ida_simple_remove(&head->subsys->ns_ida, head->instance);
 	cleanup_srcu_struct(&head->srcu);
@@ -3859,6 +3867,126 @@ static int __nvme_check_ids(struct nvme_subsystem *subsys,
 	return 0;
 }
 
+#ifdef CONFIG_NVME_MULTIPATH
+static int nvme_generic_ns_open(struct inode *inode, struct file *file)
+{
+	struct nvme_generic_ns *generic_ns = container_of(inode->i_cdev,
+			struct nvme_generic_ns, cdev);
+
+	if (!generic_ns->head->disk)
+		return -ENODEV;
+
+	file->private_data = generic_ns;
+	return nvme_ns_head_open(generic_ns->head->disk->part0, file->f_mode);
+}
+
+static int nvme_generic_ns_release(struct inode *inode, struct file *file)
+{
+	struct nvme_generic_ns *generic_ns = container_of(inode->i_cdev,
+			struct nvme_generic_ns, cdev);
+
+	nvme_ns_head_release(generic_ns->head->disk, file->f_mode);
+
+	return 0;
+}
+
+static long nvme_generic_ns_ioctl(struct file *file, unsigned int cmd,
+		unsigned long arg)
+{
+	struct nvme_generic_ns *generic_ns = file->private_data;
+
+	return nvme_ioctl(generic_ns->head->disk->part0, file->f_mode, cmd, arg);
+}
+#else
+static int nvme_generic_ns_open(struct inode *inode, struct file *file)
+{
+	struct nvme_generic_ns *generic_ns = container_of(inode->i_cdev,
+			struct nvme_generic_ns, cdev);
+
+	if (!generic_ns->ns)
+		return -ENODEV;
+
+	file->private_data = generic_ns;
+	return nvme_open(generic_ns->ns->disk->part0, file->f_mode);
+}
+
+static int nvme_generic_ns_release(struct inode *inode, struct file *file)
+{
+	struct nvme_generic_ns *generic_ns = container_of(inode->i_cdev,
+			struct nvme_generic_ns, cdev);
+
+	nvme_release(generic_ns->ns->disk, file->f_mode);
+
+	return 0;
+}
+
+static long nvme_generic_ns_ioctl(struct file *file, unsigned int cmd,
+		unsigned long arg)
+{
+	struct nvme_generic_ns *generic_ns = file->private_data;
+
+	return nvme_ioctl(generic_ns->ns->disk->part0, file->f_mode, cmd, arg);
+}
+#endif
+
+static const struct file_operations nvme_generic_ns_fops = {
+	.owner		= THIS_MODULE,
+	.open		= nvme_generic_ns_open,
+	.release	= nvme_generic_ns_release,
+	.unlocked_ioctl	= nvme_generic_ns_ioctl,
+	.compat_ioctl	= compat_ptr_ioctl,
+};
+
+static int nvme_alloc_generic_ns(struct nvme_ctrl *ctrl,
+		struct nvme_ns_head *head, struct nvme_ns *ns)
+{
+	struct nvme_generic_ns *generic_ns;
+	char name[DISK_NAME_LEN];
+	int minor;
+	int ret;
+
+	generic_ns = kzalloc_node(sizeof(*generic_ns), GFP_KERNEL,
+			ctrl->numa_node);
+	if (!generic_ns)
+		return -ENOMEM;
+
+	ret = ida_simple_get(&nvme_generic_ns_minor_ida, 0, 0, GFP_KERNEL);
+	if (ret < 0)
+		goto free_ns;
+	minor = ret;
+
+	device_initialize(&generic_ns->device);
+	generic_ns->device.devt = MKDEV(MAJOR(nvme_generic_ns_devt), minor);
+	generic_ns->device.class = nvme_generic_ns_class;
+	generic_ns->device.parent = ctrl->device;
+	dev_set_drvdata(&generic_ns->device, generic_ns);
+
+	nvme_set_generic_ns_name(name, head, ctrl);
+	ret = dev_set_name(&generic_ns->device, "%s", name);
+	if (ret)
+		goto put_ida;
+
+	cdev_init(&generic_ns->cdev, &nvme_generic_ns_fops);
+	generic_ns->cdev.owner = ctrl->ops->module;
+
+	ret = cdev_device_add(&generic_ns->cdev, &generic_ns->device);
+	if (ret)
+		goto free_kobj;
+
+	head->generic_ns = generic_ns;
+	generic_ns->head = head;
+	generic_ns->ns = ns;
+	return 0;
+
+free_kobj:
+	kfree_const(generic_ns->device.kobj.name);
+put_ida:
+	ida_simple_remove(&nvme_generic_ns_minor_ida, minor);
+free_ns:
+	kfree(generic_ns);
+	return ret;
+}
+
 static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
 		unsigned nsid, struct nvme_ns_ids *ids)
 {
@@ -3952,6 +4080,13 @@ static int nvme_init_ns_head(struct nvme_ns *ns, unsigned nsid,
 		}
 	}
 
+	if (generic_ns && nvme_alloc_generic_ns(ctrl, head, ns)) {
+		dev_err(ctrl->device,
+			"Failed to initialize generic namespace %d\n",
+			nsid);
+		goto out_put_ns_head;
+	}
+
 	list_add_tail_rcu(&ns->siblings, &head->list);
 	ns->head = head;
 	mutex_unlock(&ctrl->subsys->lock);
@@ -4038,8 +4173,17 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
 	memcpy(disk->disk_name, disk_name, DISK_NAME_LEN);
 	ns->disk = disk;
 
-	if (nvme_update_ns_info(ns, id))
-		goto out_put_disk;
+	/*
+	 * If the namespace update fails in a graceful manner, hide the block
+	 * device, but still allow for the generic namespae device to be
+	 * craeted.
+	 */
+	if (nvme_update_ns_info(ns, id)) {
+		if (generic_ns)
+			ns->disk->flags |= GENHD_FL_HIDDEN;
+		else
+			goto out_put_disk;
+	}
 
 	if ((ctrl->quirks & NVME_QUIRK_LIGHTNVM) && id->vs[0] == 0x1) {
 		if (nvme_nvm_register(ns, disk_name, node)) {
@@ -4058,6 +4202,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
 
 	nvme_mpath_add_disk(ns, id);
 	nvme_fault_inject_init(&ns->fault_inject, ns->disk->disk_name);
+
 	kfree(id);
 
 	return;
@@ -4889,8 +5034,27 @@ static int __init nvme_core_init(void)
 		result = PTR_ERR(nvme_subsys_class);
 		goto destroy_class;
 	}
+
+	if (!generic_ns)
+		return 0;
+
+	result = alloc_chrdev_region(&nvme_generic_ns_devt, 0,
+			NVME_MINORS, "nvme-generic-ns");
+	if (result < 0)
+		goto destroy_subsys_class;
+
+	nvme_generic_ns_class = class_create(THIS_MODULE, "nvme-generic-ns");
+	if (IS_ERR(nvme_generic_ns_class)) {
+		result = PTR_ERR(nvme_subsys_class);
+		goto unregister_generic_ns;
+	}
+
 	return 0;
 
+unregister_generic_ns:
+	unregister_chrdev_region(nvme_generic_ns_devt, NVME_MINORS);
+destroy_subsys_class:
+	class_destroy(nvme_subsys_class);
 destroy_class:
 	class_destroy(nvme_class);
 unregister_chrdev:
@@ -4907,12 +5071,16 @@ static int __init nvme_core_init(void)
 
 static void __exit nvme_core_exit(void)
 {
+	if (generic_ns)
+		class_destroy(nvme_generic_ns_class);
 	class_destroy(nvme_subsys_class);
 	class_destroy(nvme_class);
+	unregister_chrdev_region(nvme_generic_ns_devt, NVME_MINORS);
 	unregister_chrdev_region(nvme_ctrl_base_chr_devt, NVME_MINORS);
 	destroy_workqueue(nvme_delete_wq);
 	destroy_workqueue(nvme_reset_wq);
 	destroy_workqueue(nvme_wq);
+	ida_destroy(&nvme_generic_ns_minor_ida);
 	ida_destroy(&nvme_instance_ida);
 }
 
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index f2d0ce0f4d38..6dad52871e93 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -65,6 +65,23 @@ void nvme_set_disk_name(char *disk_name, struct nvme_ns *ns,
 	}
 }
 
+/*
+ * It sets generic namespace chardev name based on the given namespace head.
+ * The naming convention follows the existing namespace disk name from
+ * nvme_set_disk_name(), but not allowing controller path specific.
+ */
+void nvme_set_generic_ns_name(char *dev_name, struct nvme_ns_head *head,
+			      struct nvme_ctrl *ctrl)
+{
+	if (!multipath) {
+		sprintf(dev_name, "nvme-generic-%dn%d",
+				ctrl->instance, head->instance);
+	} else {
+		sprintf(dev_name, "nvme-generic-%dn%d",
+				ctrl->subsys->instance, head->instance);
+	}
+}
+
 void nvme_failover_req(struct request *req)
 {
 	struct nvme_ns *ns = req->q->queuedata;
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 815c032a190e..f6cc47316d0f 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -413,6 +413,8 @@ struct nvme_ns_head {
 	bool			shared;
 	int			instance;
 	struct nvme_effects_log *effects;
+	struct nvme_generic_ns	*generic_ns;
+
 #ifdef CONFIG_NVME_MULTIPATH
 	struct gendisk		*disk;
 	struct bio_list		requeue_list;
@@ -464,6 +466,20 @@ struct nvme_ns {
 
 };
 
+/*
+ * Generic character device for a namespace.  This device is to support I/O to
+ * namespace even it's failed to initialize due to some reasons (e.g.,
+ * unsupported protection information, etc).
+ */
+struct nvme_generic_ns {
+	struct device		device;
+	struct cdev		cdev;
+	struct nvme_ns_head	*head;
+
+	/* targted namespace instance.  only valid in non-multipath */
+	struct nvme_ns		*ns;
+};
+
 /* NVMe ns supports metadata actions by the controller (generate/strip) */
 static inline bool nvme_ns_has_pi(struct nvme_ns *ns)
 {
@@ -665,6 +681,8 @@ void nvme_mpath_wait_freeze(struct nvme_subsystem *subsys);
 void nvme_mpath_start_freeze(struct nvme_subsystem *subsys);
 void nvme_set_disk_name(char *disk_name, struct nvme_ns *ns,
 			struct nvme_ctrl *ctrl, int *flags);
+void nvme_set_generic_ns_name(char *dev_name, struct nvme_ns_head *head,
+			      struct nvme_ctrl *ctrl);
 void nvme_failover_req(struct request *req);
 void nvme_kick_requeue_lists(struct nvme_ctrl *ctrl);
 int nvme_mpath_alloc_disk(struct nvme_ctrl *ctrl,struct nvme_ns_head *head);
@@ -713,6 +731,13 @@ static inline void nvme_set_disk_name(char *disk_name, struct nvme_ns *ns,
 	sprintf(disk_name, "nvme%dn%d", ctrl->instance, ns->head->instance);
 }
 
+static inline void nvme_set_generic_ns_name(char *dev_name,
+		struct nvme_ns_head *head, struct nvme_ctrl *ctrl)
+{
+	sprintf(dev_name, "nvme-generic-%dn%d", ctrl->instance,
+			head->instance);
+}
+
 static inline void nvme_failover_req(struct request *req)
 {
 }
-- 
2.27.0


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

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

[Niklas Cassel]

On Tue, Apr 06, 2021 at 03:48:40PM +0900, Minwoo Im wrote:
> Hello,
> 
> This is the second patch series to support generic ns character device
> to expose per-namespace instance to the userspace.  This version fixed
> code mis-ordered reported by Kanchan.
> 
> This patch introduces per-namespace character device to I/O in case that
> blkdev is not initialized properly.  Userspace applications are able to
> I/O to the generic namespace chardev even there's no blkdev properly
> initialized.  Because we don't allow nvme controller device to I/O with
> a specified nsid, this generic device will provide a way to I/O.
> 
> This patch is derived from Javier's patch series [1].  Javier and I have
> re-coded this series again and it starts with new version tag.  Changes
> from the previous series are:
> 
>   - Update naming convention for the chardev exactly the same with the
>     blkdev:
>         /dev/nvme-generic-XcYnZ  to  /dev/nvme-generic-XnY

Hello Minwoo,

The current proposal puts these new per-ns char devs in directly in
/dev/ (I assume since Christoph didn't like the /dev/nvme/ subdir idea.
Keith seemed to like the subdir idea, since he had suggested the same.)

For the absolute majority of cases, the namespace will not be rejected,
so the user will be able to use the per-ns block dev to perform IOCTLs.

For the small minority of cases, Linux might reject the ns, so no block
dev will be created.

I'm slightly worried that adding all these new per-ns char devs, in the
same directory as the regular per-ns block devs, will lead to confusion
from regular Linux users.

Imagine the potential confusion about what device the user should use
with e.g. fdisk, mkfs, mount, in fstab, what to specify in fstab, etc.

I think that there is value in reducing the confusion for regular users.



I don't know the best way of reducing this potential confusion, but here
are some suggestions (suggestions are mutually exclusive):

1) Put the new per-ns char devs in a directory different from where
the regular per-ns block devs are located.

2) Only create the new per-ns char dev for namespaces that were rejected.

3) There is already a new module parameter for this, default it to false.

4) Introduce a sysfs /sys/class/nvme/nvme0/export_unsupported_namespace
that you can echo the nsid to, if you want to create the new per-ns char
dev for a certain ns.


I'm certain that someone can come up with an even better suggestion.


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

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

[Minwoo Im]

On 21-04-06 09:01:41, Niklas Cassel wrote:
> On Tue, Apr 06, 2021 at 03:48:40PM +0900, Minwoo Im wrote:
> > Hello,
> > 
> > This is the second patch series to support generic ns character device
> > to expose per-namespace instance to the userspace.  This version fixed
> > code mis-ordered reported by Kanchan.
> > 
> > This patch introduces per-namespace character device to I/O in case that
> > blkdev is not initialized properly.  Userspace applications are able to
> > I/O to the generic namespace chardev even there's no blkdev properly
> > initialized.  Because we don't allow nvme controller device to I/O with
> > a specified nsid, this generic device will provide a way to I/O.
> > 
> > This patch is derived from Javier's patch series [1].  Javier and I have
> > re-coded this series again and it starts with new version tag.  Changes
> > from the previous series are:
> > 
> >   - Update naming convention for the chardev exactly the same with the
> >     blkdev:
> >         /dev/nvme-generic-XcYnZ  to  /dev/nvme-generic-XnY
> 
> Hello Minwoo,
> 
> The current proposal puts these new per-ns char devs in directly in
> /dev/ (I assume since Christoph didn't like the /dev/nvme/ subdir idea.
> Keith seemed to like the subdir idea, since he had suggested the same.)
> 
> For the absolute majority of cases, the namespace will not be rejected,
> so the user will be able to use the per-ns block dev to perform IOCTLs.
> 
> For the small minority of cases, Linux might reject the ns, so no block
> dev will be created.
> 
> I'm slightly worried that adding all these new per-ns char devs, in the
> same directory as the regular per-ns block devs, will lead to confusion
> from regular Linux users.
>
> Imagine the potential confusion about what device the user should use
> with e.g. fdisk, mkfs, mount, in fstab, what to specify in fstab, etc.
> 
> I think that there is value in reducing the confusion for regular users.

Agreed on this point.  We might have thousands of namespaces and it
might be making confusions to users.

> I don't know the best way of reducing this potential confusion, but here
> are some suggestions (suggestions are mutually exclusive):
> 
> 1) Put the new per-ns char devs in a directory different from where
> the regular per-ns block devs are located.
> 
> 2) Only create the new per-ns char dev for namespaces that were rejected.

I prefer this one which is the major reason of this patch series being
posted.

> 3) There is already a new module parameter for this, default it to false.
> 
> 4) Introduce a sysfs /sys/class/nvme/nvme0/export_unsupported_namespace
> that you can echo the nsid to, if you want to create the new per-ns char
> dev for a certain ns.
> 
> 
> I'm certain that someone can come up with an even better suggestion.

Thanks for your suggestion.  It sounds like it totally worth to be
discussed with people.  I'd also like to hear from other's opinions :)

Thanks!

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

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

[Kanchan Joshi]

On Tue, Apr 6, 2021 at 2:33 PM Niklas Cassel <Niklas.Cassel@wdc.com> wrote:
>
> On Tue, Apr 06, 2021 at 03:48:40PM +0900, Minwoo Im wrote:
> > Hello,
> >
> > This is the second patch series to support generic ns character device
> > to expose per-namespace instance to the userspace.  This version fixed
> > code mis-ordered reported by Kanchan.
> >
> > This patch introduces per-namespace character device to I/O in case that
> > blkdev is not initialized properly.  Userspace applications are able to
> > I/O to the generic namespace chardev even there's no blkdev properly
> > initialized.  Because we don't allow nvme controller device to I/O with
> > a specified nsid, this generic device will provide a way to I/O.
> >
> > This patch is derived from Javier's patch series [1].  Javier and I have
> > re-coded this series again and it starts with new version tag.  Changes
> > from the previous series are:
> >
> >   - Update naming convention for the chardev exactly the same with the
> >     blkdev:
> >         /dev/nvme-generic-XcYnZ  to  /dev/nvme-generic-XnY
>
> Hello Minwoo,
>
> The current proposal puts these new per-ns char devs in directly in
> /dev/ (I assume since Christoph didn't like the /dev/nvme/ subdir idea.
> Keith seemed to like the subdir idea, since he had suggested the same.)
>
> For the absolute majority of cases, the namespace will not be rejected,
> so the user will be able to use the per-ns block dev to perform IOCTLs.
>
> For the small minority of cases, Linux might reject the ns, so no block
> dev will be created.
>
> I'm slightly worried that adding all these new per-ns char devs, in the
> same directory as the regular per-ns block devs, will lead to confusion
> from regular Linux users.
>
> Imagine the potential confusion about what device the user should use
> with e.g. fdisk, mkfs, mount, in fstab, what to specify in fstab, etc.
>
> I think that there is value in reducing the confusion for regular users.
>
>
>
> I don't know the best way of reducing this potential confusion, but here
> are some suggestions (suggestions are mutually exclusive):
>
> 1) Put the new per-ns char devs in a directory different from where
> the regular per-ns block devs are located.
>
> 2) Only create the new per-ns char dev for namespaces that were rejected.
>
> 3) There is already a new module parameter for this, default it to false.
>
> 4) Introduce a sysfs /sys/class/nvme/nvme0/export_unsupported_namespace
> that you can echo the nsid to, if you want to create the new per-ns char
> dev for a certain ns.
>
>
> I'm certain that someone can come up with an even better suggestion.

While 1 sounds good, the problem with 2, 3 and 4 is that it will make
things difficult for applications which want to use the new interface
(i.e. char device).
When char-device is always available/visible, it is more
automatic......applications can use it all the time without resorting
to any extra administrative tunable.

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

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

[Christoph Hellwig]

On Tue, Apr 06, 2021 at 10:35:33PM +0900, Minwoo Im wrote:
> > with e.g. fdisk, mkfs, mount, in fstab, what to specify in fstab, etc.
> > 
> > I think that there is value in reducing the confusion for regular users.
> 
> Agreed on this point.  We might have thousands of namespaces and it
> might be making confusions to users.

How does this create a confusion that it doesn't for the existing NVMe
block devices and the SCSI disk and generic devices?

Morover: why would anyone want to expose these huge numbers of
namespaces to a single host?  While larger LUN counts in SCSI are
sometimes needed for scalability reasons they aren't in NVMe.  I haven't
actually seen 4 digit namespace counts in NVMe except in synthetic
test setups yet.

> > 2) Only create the new per-ns char dev for namespaces that were rejected.
> 
> I prefer this one which is the major reason of this patch series being
> posted.

Which doesn't allow us to write portable programs just using the
char node, as now a kernel upgrade that supports a new namespace type
or feature will remove the char dev.  It also is very different from
what people expect from their SCSI and ATA setups.

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

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

[Minwoo Im]

On 21-04-06 16:59:48, Christoph Hellwig wrote:
> On Tue, Apr 06, 2021 at 10:35:33PM +0900, Minwoo Im wrote:
> > > with e.g. fdisk, mkfs, mount, in fstab, what to specify in fstab, etc.
> > > 
> > > I think that there is value in reducing the confusion for regular users.
> > 
> > Agreed on this point.  We might have thousands of namespaces and it
> > might be making confusions to users.
> 
> How does this create a confusion that it doesn't for the existing NVMe
> block devices and the SCSI disk and generic devices?

Okay.  As there's sg device for SCSI, nvme-generic makes sense not to be
confusions.

> Morover: why would anyone want to expose these huge numbers of
> namespaces to a single host?  While larger LUN counts in SCSI are
> sometimes needed for scalability reasons they aren't in NVMe.  I haven't
> actually seen 4 digit namespace counts in NVMe except in synthetic
> test setups yet.

Sorry for talking about what I have not really seen ever.  Thought about
this one in case that might be. :-)

> > > 2) Only create the new per-ns char dev for namespaces that were rejected.
> > 
> > I prefer this one which is the major reason of this patch series being
> > posted.
> 
> Which doesn't allow us to write portable programs just using the
> char node, as now a kernel upgrade that supports a new namespace type
> or feature will remove the char dev.  It also is very different from
> what people expect from their SCSI and ATA setups.

Got your point.

Thanks for your feedback on this!

And here's another questions for your opinions.

In this new series, we have created nvme generic chardev along with the
block device(e.g., nvme0n1) which is not path-specific (e..g,
nvme0c0n1) in case of multipath.  I think now we can say that it
supports multi-path.  What do you say about this :) ?

Thanks!

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

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

[Christoph Hellwig]

On Wed, Apr 07, 2021 at 01:23:09AM +0900, Minwoo Im wrote:
> In this new series, we have created nvme generic chardev along with the
> block device(e.g., nvme0n1) which is not path-specific (e..g,
> nvme0c0n1) in case of multipath.  I think now we can say that it
> supports multi-path.  What do you say about this :) ?

Yes, that is the right thing to do.

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

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

[Minwoo Im]

On 21-04-07 08:00:16, Christoph Hellwig wrote:
> On Wed, Apr 07, 2021 at 01:23:09AM +0900, Minwoo Im wrote:
> > In this new series, we have created nvme generic chardev along with the
> > block device(e.g., nvme0n1) which is not path-specific (e..g,
> > nvme0c0n1) in case of multipath.  I think now we can say that it
> > supports multi-path.  What do you say about this :) ?
> 
> Yes, that is the right thing to do.

Thanks for your feedback!

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

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

[Niklas Cassel]

On Tue, Apr 06, 2021 at 04:59:48PM +0200, Christoph Hellwig wrote:
> On Tue, Apr 06, 2021 at 10:35:33PM +0900, Minwoo Im wrote:
> > > with e.g. fdisk, mkfs, mount, in fstab, what to specify in fstab, etc.
> > > 
> > > I think that there is value in reducing the confusion for regular users.
> > 
> > Agreed on this point.  We might have thousands of namespaces and it
> > might be making confusions to users.
> 
> How does this create a confusion that it doesn't for the existing NVMe
> block devices and the SCSI disk and generic devices?

The SCSI generic devices are named /dev/sgX while the SCSI block devices
are named /dev/sd*, so the names are more distinct compared to /dev/nvme*
and /dev/nvme-generic-*.

A "regular" Linux user who writes /dev/sd<tab> will not see the SCSI
generic devices, but with the current proposal, if he writes
/dev/nvme<tab>, he will see the NVMe generic devices.

I just had a gut feeling that this could cause confusion in the future.
However, I trust your experience and judgement more than my gut feeling.


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

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

[Christoph Hellwig]

On Wed, Apr 07, 2021 at 07:36:22AM +0000, Niklas Cassel wrote:
> The SCSI generic devices are named /dev/sgX while the SCSI block devices
> are named /dev/sd*, so the names are more distinct compared to /dev/nvme*
> and /dev/nvme-generic-*.
> 
> A "regular" Linux user who writes /dev/sd<tab> will not see the SCSI
> generic devices, but with the current proposal, if he writes
> /dev/nvme<tab>, he will see the NVMe generic devices.
> 
> I just had a gut feeling that this could cause confusion in the future.
> However, I trust your experience and judgement more than my gut feeling.

Should we call the nodes /dev/ng* ?

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

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

[Niklas Cassel]

On Wed, Apr 07, 2021 at 11:39:23AM +0200, Christoph Hellwig wrote:
> On Wed, Apr 07, 2021 at 07:36:22AM +0000, Niklas Cassel wrote:
> > The SCSI generic devices are named /dev/sgX while the SCSI block devices
> > are named /dev/sd*, so the names are more distinct compared to /dev/nvme*
> > and /dev/nvme-generic-*.
> > 
> > A "regular" Linux user who writes /dev/sd<tab> will not see the SCSI
> > generic devices, but with the current proposal, if he writes
> > /dev/nvme<tab>, he will see the NVMe generic devices.
> > 
> > I just had a gut feeling that this could cause confusion in the future.
> > However, I trust your experience and judgement more than my gut feeling.
> 
> Should we call the nodes /dev/ng* ?

Man... naming things is the hardest thing about software development ;)

It would solve the problem of them being too similarly named.
And if we document this somewhere, it makes sense that SCSI generic
is /dev/sg and NVMe generic is /dev/ng...

The only "bad" thing is that we take up a very generic (pun intended)
name in the /dev/ namespace. The acronym might conflict with something
else in the future. But trying to predict the future seems silly,
so I can't come up with any argument against your naming proposal :)


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

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

[Damien Le Moal]

On 2021/04/07 18:47, Christoph Hellwig wrote:
> On Wed, Apr 07, 2021 at 07:36:22AM +0000, Niklas Cassel wrote:
>> The SCSI generic devices are named /dev/sgX while the SCSI block devices
>> are named /dev/sd*, so the names are more distinct compared to /dev/nvme*
>> and /dev/nvme-generic-*.
>>
>> A "regular" Linux user who writes /dev/sd<tab> will not see the SCSI
>> generic devices, but with the current proposal, if he writes
>> /dev/nvme<tab>, he will see the NVMe generic devices.
>>
>> I just had a gut feeling that this could cause confusion in the future.
>> However, I trust your experience and judgement more than my gut feeling.
> 
> Should we call the nodes /dev/ng* ?

That would be a nice symmetry with scsi / sg. I am all for it.
This would also make it easier to read a "ls" output, avoiding a lot of nvme
device names that differ only by 1 character ("c").

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


-- 
Damien Le Moal
Western Digital Research

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

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

[Javier González]

On 07.04.2021 10:34, Damien Le Moal wrote:
>On 2021/04/07 18:47, Christoph Hellwig wrote:
>> On Wed, Apr 07, 2021 at 07:36:22AM +0000, Niklas Cassel wrote:
>>> The SCSI generic devices are named /dev/sgX while the SCSI block devices
>>> are named /dev/sd*, so the names are more distinct compared to /dev/nvme*
>>> and /dev/nvme-generic-*.
>>>
>>> A "regular" Linux user who writes /dev/sd<tab> will not see the SCSI
>>> generic devices, but with the current proposal, if he writes
>>> /dev/nvme<tab>, he will see the NVMe generic devices.
>>>
>>> I just had a gut feeling that this could cause confusion in the future.
>>> However, I trust your experience and judgement more than my gut feeling.
>>
>> Should we call the nodes /dev/ng* ?
>
>That would be a nice symmetry with scsi / sg. I am all for it.
>This would also make it easier to read a "ls" output, avoiding a lot of nvme
>device names that differ only by 1 character ("c").
>

+1 to this naming convention

We will let the V2 hanging for a couple days and add this in V3 unless
any good argument against it comes.

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

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

[Christoph Hellwig]

On Tue, Apr 06, 2021 at 03:48:41PM +0900, Minwoo Im wrote:
> +static bool generic_ns = true;
> +module_param(generic_ns, bool, 0644);
> +MODULE_PARM_DESC(generic_ns, "support generic namespace character device");

I do not think the module option is a good idea.

> +#ifdef CONFIG_NVME_MULTIPATH
> +static int nvme_generic_ns_open(struct inode *inode, struct file *file)
> +{

The ifdef here means that the cases of either a subsystem that can't
have multiple controllers or the multipath=N option can't be handled
properly.  We really need two different code paths:  one with the
cdev in the namespace, and one with the cdev in the ns_head.  Just
like the gendisk.

> +	return nvme_ns_head_open(generic_ns->head->disk->part0, file->f_mode);

Calling this based on the bdev is a little silly when we can just
use the trivial underlying functionality.

> +	device_initialize(&generic_ns->device);
> +	generic_ns->device.devt = MKDEV(MAJOR(nvme_generic_ns_devt), minor);
> +	generic_ns->device.class = nvme_generic_ns_class;
> +	generic_ns->device.parent = ctrl->device;

We can't reference the controller for something hanging off the ns_head.

> +	/*
> +	 * If the namespace update fails in a graceful manner, hide the block
> +	 * device, but still allow for the generic namespae device to be
> +	 * craeted.
> +	 */
> +	if (nvme_update_ns_info(ns, id)) {
> +		if (generic_ns)
> +			ns->disk->flags |= GENHD_FL_HIDDEN;
> +		else
> +			goto out_put_disk;
> +	}

We must still fail when the error is one that does not just indicate
a not supported feature.

> +struct nvme_generic_ns {
> +	struct device		device;
> +	struct cdev		cdev;
> +	struct nvme_ns_head	*head;
> +
> +	/* targted namespace instance.  only valid in non-multipath */
> +	struct nvme_ns		*ns;
> +};

I don't really see point of this extra structure.



FYI, I've looked into addressing some of these points and will send
out an updated patch that sits on top of an ioctl and multipath
refatoring series I planned a while ago that I resurrected for this.

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

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

[Minwoo Im]

On 21-04-07 15:15:27, Christoph Hellwig wrote:
> On Tue, Apr 06, 2021 at 03:48:41PM +0900, Minwoo Im wrote:
> > +static bool generic_ns = true;
> > +module_param(generic_ns, bool, 0644);
> > +MODULE_PARM_DESC(generic_ns, "support generic namespace character device");
> 
> I do not think the module option is a good idea.

Okay. Will make generic device by default.

> > +#ifdef CONFIG_NVME_MULTIPATH
> > +static int nvme_generic_ns_open(struct inode *inode, struct file *file)
> > +{
> 
> The ifdef here means that the cases of either a subsystem that can't
> have multiple controllers or the multipath=N option can't be handled
> properly.  We really need two different code paths:  one with the
> cdev in the namespace, and one with the cdev in the ns_head.  Just
> like the gendisk.

Agreed.  Kanchan has reported that if multiple controller in CMIC is not
supported, the head disk instance is not allocated and this code will
just return -ENODEV.  As you pointed out here, will make it work in the
next version.

> > +	return nvme_ns_head_open(generic_ns->head->disk->part0, file->f_mode);
> 
> Calling this based on the bdev is a little silly when we can just
> use the trivial underlying functionality.

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.

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index a06634e7bad1..c92328eedc00 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2377,15 +2377,18 @@ static const struct block_device_operations nvme_bdev_ops = {
 };
 
 #ifdef CONFIG_NVME_MULTIPATH
-static int nvme_ns_head_open(struct block_device *bdev, fmode_t mode)
+static int nvme_ns_head_get(struct nvme_ns_head *head)
 {
-	struct nvme_ns_head *head = bdev->bd_disk->private_data;
-
 	if (!kref_get_unless_zero(&head->ref))
 		return -ENXIO;
 	return 0;
 }
 
+static int nvme_ns_head_open(struct block_device *bdev, fmode_t mode)
+{
+	return nvme_ns_head_get(bdev->bd_disk->private_data);
+}
+
 static void nvme_ns_head_release(struct gendisk *disk, fmode_t mode)
 {
 	nvme_put_ns_head(disk->private_data);
@@ -3877,7 +3880,7 @@ static int nvme_generic_ns_open(struct inode *inode, struct file *file)
 		return -ENODEV;
 
 	file->private_data = generic_ns;
-	return nvme_ns_head_open(generic_ns->head->disk->part0, file->f_mode);
+	return nvme_ns_head_get(generic_ns->head);
 }
 
 static int nvme_generic_ns_release(struct inode *inode, struct file *file)


> > +	device_initialize(&generic_ns->device);
> > +	generic_ns->device.devt = MKDEV(MAJOR(nvme_generic_ns_devt), minor);
> > +	generic_ns->device.class = nvme_generic_ns_class;
> > +	generic_ns->device.parent = ctrl->device;
> 
> We can't reference the controller for something hanging off the ns_head.

Got your point.  Let me fix this up.

> > +	/*
> > +	 * If the namespace update fails in a graceful manner, hide the block
> > +	 * device, but still allow for the generic namespae device to be
> > +	 * craeted.
> > +	 */
> > +	if (nvme_update_ns_info(ns, id)) {
> > +		if (generic_ns)
> > +			ns->disk->flags |= GENHD_FL_HIDDEN;
> > +		else
> > +			goto out_put_disk;
> > +	}
> 
> We must still fail when the error is one that does not just indicate
> a not supported feature.

Okay.  Let me first list up the real error cases.

> > +struct nvme_generic_ns {
> > +	struct device		device;
> > +	struct cdev		cdev;
> > +	struct nvme_ns_head	*head;
> > +
> > +	/* targted namespace instance.  only valid in non-multipath */
> > +	struct nvme_ns		*ns;
> > +};
> 
> I don't really see point of this extra structure.

As you pointed out above, we just can't rely on the nvme_ns_head where
disk might not be allocated.  Earlier, I was trying to put this to
nvme_ns_head, but now possibililties are two.  Where do you think for
this generic device fields to be located properly ?

> FYI, I've looked into addressing some of these points and will send
> out an updated patch that sits on top of an ioctl and multipath
> refatoring series I planned a while ago that I resurrected for this.

Sure.  Once series are applied, we can make it on top of that series.
Thanks for sharing!

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

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

[Christoph Hellwig]

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

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

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

[Minwoo Im]

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

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

[Christoph Hellwig]

On Thu, Apr 08, 2021 at 12:35:36AM +0900, Minwoo Im wrote:
> 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.....

Yeal issue, stupid rebase error when I stated to split and move a few
more pieces for better reuse from the chardev..

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

This looks pretty sensible.  If you give me a signoff I'll commit this
as a separate prep patch.

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

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

[Minwoo Im]

On 21-04-07 17:40:00, Christoph Hellwig wrote:
> On Thu, Apr 08, 2021 at 12:35:36AM +0900, Minwoo Im wrote:
> > 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.....
> 
> Yeal issue, stupid rebase error when I stated to split and move a few
> more pieces for better reuse from the chardev..

Got it!

> > 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.
> 
> This looks pretty sensible.  If you give me a signoff I'll commit this
> as a separate prep patch.

Please have a tag about this one :)

Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>

Thanks!

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

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

[Minwoo Im]

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

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

[Christoph Hellwig]

I've force pushed the branch with the updates, let me know if this
looks good to you.

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

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

[Minwoo Im]

On 21-04-07 18:48:18, Christoph Hellwig wrote:
> I've force pushed the branch with the updates, let me know if this
> looks good to you.

Checked the commit.  IS_ENABLED() makes code much more cleaner by
removing the #ifdef from the .c file.  Also checked that the
mutex_unlock in nvme_find_get_live_ctrl().

Otherwise, the generic ns patch commit description looks like needs to
be updated: it's still saying that nvme-generic-XnY, and also about the
module parameter that's been removed in this series.

Tested again based on the force pushed branch.

Thanks!

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

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

[Minwoo Im]

On 21-04-07 18:48:18, Christoph Hellwig wrote:
> I've force pushed the branch with the updates, let me know if this
> looks good to you.

One more thing I'd like to discuss is that the current branch does not
create generic cdev in case that nvme_update_ns_info fails.  And this is
the one of the use cases of the generic devices, so do you think it
should approach as an another patch ?  Or, should we make it supported
in the current patch ?

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

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

[Christoph Hellwig]

On Thu, Apr 08, 2021 at 02:09:05AM +0900, Minwoo Im wrote:
> On 21-04-07 18:48:18, Christoph Hellwig wrote:
> > I've force pushed the branch with the updates, let me know if this
> > looks good to you.
> 
> One more thing I'd like to discuss is that the current branch does not
> create generic cdev in case that nvme_update_ns_info fails.  And this is
> the one of the use cases of the generic devices, so do you think it
> should approach as an another patch ?  Or, should we make it supported
> in the current patch ?

It should create the ng devices when nvme_update_ns_info fais with
-ENODEV, which should handle all the ZNS cases is was created.  Or did
I mess that up?

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

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

[Javier González]

On 07.04.2021 19:14, Christoph Hellwig wrote:
>On Thu, Apr 08, 2021 at 02:09:05AM +0900, Minwoo Im wrote:
>> On 21-04-07 18:48:18, Christoph Hellwig wrote:
>> > I've force pushed the branch with the updates, let me know if this
>> > looks good to you.
>>
>> One more thing I'd like to discuss is that the current branch does not
>> create generic cdev in case that nvme_update_ns_info fails.  And this is
>> the one of the use cases of the generic devices, so do you think it
>> should approach as an another patch ?  Or, should we make it supported
>> in the current patch ?
>
>It should create the ng devices when nvme_update_ns_info fais with
>-ENODEV, which should handle all the ZNS cases is was created.  Or did
>I mess that up?

This would be useful for devices with metadata that is not supported by
the block layer, i.e., nvme_configure_metadata().

What do you think?

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

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

[Christoph Hellwig]

On Thu, Apr 08, 2021 at 09:11:28AM +0200, Javier González wrote:
> This would be useful for devices with metadata that is not supported by
> the block layer, i.e., nvme_configure_metadata().

You mean in nvme_update_disk_info?  It would be the right thing to do,
but back in the day the maintainers opted for the zero capacity
block device hack.  At least we now have the char device as well.

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

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

[Javier González]

On 08.04.2021 09:26, Christoph Hellwig wrote:
>On Thu, Apr 08, 2021 at 09:11:28AM +0200, Javier González wrote:
>> This would be useful for devices with metadata that is not supported by
>> the block layer, i.e., nvme_configure_metadata().
>
>You mean in nvme_update_disk_info?  It would be the right thing to do,
>but back in the day the maintainers opted for the zero capacity
>block device hack.  At least we now have the char device as well.

Yes. This path starts in nvme_update_ns_info() and part of it is
nvme_configure_metadata(), which might fail is the metadata is not
supported.

In this case, I believe we should still create the char device so that
any OME-specific NVMe device can still be used from user-space without
requiring SPDK and a PCI detach.

Does this sound good to you?

Regarding the current logic, I would not mind that the block device
still appears as a zero capacity too. We can think of removing this in
the future as this path becomes more stable. It requeries Kanchan's work
on io_uring to be usable for I/O.

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

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

[Christoph Hellwig]

On Thu, Apr 08, 2021 at 12:26:25PM +0200, Javier González wrote:
> In this case, I believe we should still create the char device so that
> any OME-specific NVMe device can still be used from user-space without
> requiring SPDK and a PCI detach.

OME?

> Does this sound good to you?
>
> Regarding the current logic, I would not mind that the block device
> still appears as a zero capacity too. We can think of removing this in
> the future as this path becomes more stable. It requeries Kanchan's work
> on io_uring to be usable for I/O.

I think with the current path this should just work.  If you have
a device that triggers this case please test it.

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

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

[Javier González]

On 08.04.2021 13:27, Christoph Hellwig wrote:
>On Thu, Apr 08, 2021 at 12:26:25PM +0200, Javier González wrote:
>> In this case, I believe we should still create the char device so that
>> any OME-specific NVMe device can still be used from user-space without
>> requiring SPDK and a PCI detach.
>
>OME?

OEM... Sorry.

>
>> Does this sound good to you?
>>
>> Regarding the current logic, I would not mind that the block device
>> still appears as a zero capacity too. We can think of removing this in
>> the future as this path becomes more stable. It requeries Kanchan's work
>> on io_uring to be usable for I/O.
>
>I think with the current path this should just work.  If you have
>a device that triggers this case please test it.

Cool. I'll test that.

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

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

[Minwoo Im]

On 21-04-07 19:14:00, Christoph Hellwig wrote:
> On Thu, Apr 08, 2021 at 02:09:05AM +0900, Minwoo Im wrote:
> > On 21-04-07 18:48:18, Christoph Hellwig wrote:
> > > I've force pushed the branch with the updates, let me know if this
> > > looks good to you.
> > 
> > One more thing I'd like to discuss is that the current branch does not
> > create generic cdev in case that nvme_update_ns_info fails.  And this is
> > the one of the use cases of the generic devices, so do you think it
> > should approach as an another patch ?  Or, should we make it supported
> > in the current patch ?
> 
> It should create the ng devices when nvme_update_ns_info fais with
> -ENODEV, which should handle all the ZNS cases is was created.  Or did
> I mess that up?

Oh, I missed the patch [1] about the unsupported features (-ENODEV) from
the branch.

Thanks :)

[1] https://lore.kernel.org/linux-nvme/20210408120842.1450092-13-hch@lst.de/T/#u

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