[PATCH 0/4] nvme: enable per-namespace char device

[PATCH 0/4] nvme: enable per-namespace char device

[javier]

From: Javier González <javier.gonz@samsung.com>

This series enables a per-namespace char device that is always presented
to the host, independently of the block device associated with the
namespace. This allows user-space to use the char device's IOCTL
interface when the kernel rejects the block device due to unsupported
features. Examples of these features include unsupported PI
configurations, ZNS features such as zoc, or unsupported command sets
such as KV.

One of the things that I would appreciate more input on is the naming
convention. This series follows the hidden device naming nvmeXcYnZ, but
I am not sure how this will work out with multipath. The concern is if
we will have unfixable naming collisions that will jepardize user-space
tools depending on this naming.

Thanks to Christoph for proposing this in the first place and Christoph
and Keith for early reviews of this patchset!

Javier

Javier González (4):
  nvme: remove unnecessary return values
  nvme: rename controller base dev_t char device
  nvme: rename bdev operations
  nvme: enable char device per namespace

 drivers/nvme/host/core.c | 172 ++++++++++++++++++++++++++++++++-------
 drivers/nvme/host/nvme.h |   3 +
 2 files changed, 146 insertions(+), 29 deletions(-)

-- 
2.17.1


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

[PATCH 1/4] nvme: remove unnecessary return values

[javier]

From: Javier González <javier.gonz@samsung.com>

Cleanup unnecessary ret values that are not checked or used in
nvme_alloc_ns().

Signed-off-by: Javier González <javier.gonz@samsung.com>
---
 drivers/nvme/host/core.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 170745d4a34b..a965a8d28ba0 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3831,7 +3831,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
 	struct gendisk *disk;
 	struct nvme_id_ns *id;
 	char disk_name[DISK_NAME_LEN];
-	int node = ctrl->numa_node, flags = GENHD_FL_EXT_DEVT, ret;
+	int node = ctrl->numa_node, flags = GENHD_FL_EXT_DEVT;
 
 	if (nvme_identify_ns(ctrl, nsid, ids, &id))
 		return;
@@ -3855,8 +3855,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
 	ns->ctrl = ctrl;
 	kref_init(&ns->kref);
 
-	ret = nvme_init_ns_head(ns, nsid, ids, id->nmic & NVME_NS_NMIC_SHARED);
-	if (ret)
+	if (nvme_init_ns_head(ns, nsid, ids, id->nmic & NVME_NS_NMIC_SHARED))
 		goto out_free_queue;
 	nvme_set_disk_name(disk_name, ns, ctrl, &flags);
 
@@ -3875,8 +3874,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
 		goto out_put_disk;
 
 	if ((ctrl->quirks & NVME_QUIRK_LIGHTNVM) && id->vs[0] == 0x1) {
-		ret = nvme_nvm_register(ns, disk_name, node);
-		if (ret) {
+		if (nvme_nvm_register(ns, disk_name, node)) {
 			dev_warn(ctrl->device, "LightNVM init failure\n");
 			goto out_put_disk;
 		}
-- 
2.17.1


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

Re: [PATCH 1/4] nvme: remove unnecessary return values

[Minwoo Im]

Hello,

On 20-12-01 13:56:07, javier@javigon.com wrote:
> From: Javier González <javier.gonz@samsung.com>
> 
> Cleanup unnecessary ret values that are not checked or used in
> nvme_alloc_ns().
> 
> Signed-off-by: Javier González <javier.gonz@samsung.com>

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

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

[PATCH 2/4] nvme: rename controller base dev_t char device

[javier]

From: Javier González <javier.gonz@samsung.com>

Rename controller base dev_t char device in preparation for adding a
namespace char device.

Signed-off-by: Javier González <javier.gonz@samsung.com>
---
 drivers/nvme/host/core.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index a965a8d28ba0..c23c891a8fb8 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -85,7 +85,7 @@ static LIST_HEAD(nvme_subsystems);
 static DEFINE_MUTEX(nvme_subsystems_lock);
 
 static DEFINE_IDA(nvme_instance_ida);
-static dev_t nvme_chr_devt;
+static dev_t nvme_ctrl_base_chr_devt;
 static struct class *nvme_class;
 static struct class *nvme_subsys_class;
 
@@ -4470,7 +4470,8 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
 
 	device_initialize(&ctrl->ctrl_device);
 	ctrl->device = &ctrl->ctrl_device;
-	ctrl->device->devt = MKDEV(MAJOR(nvme_chr_devt), ctrl->instance);
+	ctrl->device->devt = MKDEV(MAJOR(nvme_ctrl_base_chr_devt),
+			ctrl->instance);
 	ctrl->device->class = nvme_class;
 	ctrl->device->parent = ctrl->dev;
 	ctrl->device->groups = nvme_dev_attr_groups;
@@ -4679,7 +4680,8 @@ static int __init nvme_core_init(void)
 	if (!nvme_delete_wq)
 		goto destroy_reset_wq;
 
-	result = alloc_chrdev_region(&nvme_chr_devt, 0, NVME_MINORS, "nvme");
+	result = alloc_chrdev_region(&nvme_ctrl_base_chr_devt, 0,
+			NVME_MINORS, "nvme");
 	if (result < 0)
 		goto destroy_delete_wq;
 
@@ -4700,7 +4702,7 @@ static int __init nvme_core_init(void)
 destroy_class:
 	class_destroy(nvme_class);
 unregister_chrdev:
-	unregister_chrdev_region(nvme_chr_devt, NVME_MINORS);
+	unregister_chrdev_region(nvme_ctrl_base_chr_devt, NVME_MINORS);
 destroy_delete_wq:
 	destroy_workqueue(nvme_delete_wq);
 destroy_reset_wq:
@@ -4715,7 +4717,7 @@ static void __exit nvme_core_exit(void)
 {
 	class_destroy(nvme_subsys_class);
 	class_destroy(nvme_class);
-	unregister_chrdev_region(nvme_chr_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);
-- 
2.17.1


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

Re: [PATCH 2/4] nvme: rename controller base dev_t char device

[Minwoo Im]

Hello,

On 20-12-01 13:56:08, javier@javigon.com wrote:
> From: Javier González <javier.gonz@samsung.com>
> 
> Rename controller base dev_t char device in preparation for adding a
> namespace char device.
> 
> Signed-off-by: Javier González <javier.gonz@samsung.com>

I think it would be better to describe things about following patches
which represent why this kind of renaming is needed in this patch.

Anyway, looks good to me.

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

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

[PATCH 3/4] nvme: rename bdev operations

[javier]

From: Javier González <javier.gonz@samsung.com>

Remane block device operations in preparation to add char device file
operations.

Signed-off-by: Javier González <javier.gonz@samsung.com>
---
 drivers/nvme/host/core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index c23c891a8fb8..2c23ea6dc296 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2292,7 +2292,7 @@ int nvme_sec_submit(void *data, u16 spsp, u8 secp, void *buffer, size_t len,
 EXPORT_SYMBOL_GPL(nvme_sec_submit);
 #endif /* CONFIG_BLK_SED_OPAL */
 
-static const struct block_device_operations nvme_fops = {
+static const struct block_device_operations nvme_bdev_ops = {
 	.owner		= THIS_MODULE,
 	.ioctl		= nvme_ioctl,
 	.compat_ioctl	= nvme_compat_ioctl,
@@ -3301,7 +3301,7 @@ static inline struct nvme_ns_head *dev_to_ns_head(struct device *dev)
 {
 	struct gendisk *disk = dev_to_disk(dev);
 
-	if (disk->fops == &nvme_fops)
+	if (disk->fops == &nvme_bdev_ops)
 		return nvme_get_ns_from_dev(dev)->head;
 	else
 		return disk->private_data;
@@ -3410,7 +3410,7 @@ static umode_t nvme_ns_id_attrs_are_visible(struct kobject *kobj,
 	}
 #ifdef CONFIG_NVME_MULTIPATH
 	if (a == &dev_attr_ana_grpid.attr || a == &dev_attr_ana_state.attr) {
-		if (dev_to_disk(dev)->fops != &nvme_fops) /* per-path attr */
+		if (dev_to_disk(dev)->fops != &nvme_bdev_ops) /* per-path attr */
 			return 0;
 		if (!nvme_ctrl_use_ana(nvme_get_ns_from_dev(dev)->ctrl))
 			return 0;
@@ -3863,7 +3863,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
 	if (!disk)
 		goto out_unlink_ns;
 
-	disk->fops = &nvme_fops;
+	disk->fops = &nvme_bdev_ops;
 	disk->private_data = ns;
 	disk->queue = ns->queue;
 	disk->flags = flags;
-- 
2.17.1


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

Re: [PATCH 3/4] nvme: rename bdev operations

[Minwoo Im]

Hello,

On 20-12-01 13:56:09, javier@javigon.com wrote:
> From: Javier González <javier.gonz@samsung.com>
> 
> Remane block device operations in preparation to add char device file
> operations.
> 
> Signed-off-by: Javier González <javier.gonz@samsung.com>

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

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

[PATCH 4/4] nvme: enable char device per namespace

[javier]

From: Javier González <javier.gonz@samsung.com>

Create a char device per NVMe namespace. This char device is always
initialized, independently of whether thedeatures implemented by the
device are supported by the kernel. User-space can therefore always
issue IOCTLs to the NVMe driver using this char device.

The char device is presented as /dev/nvmeXcYnZ to follow the hidden
block device. This naming also aligns with nvme-cli filters, so the char
device should be usable without tool changes.

Signed-off-by: Javier González <javier.gonz@samsung.com>
---
 drivers/nvme/host/core.c | 144 +++++++++++++++++++++++++++++++++++----
 drivers/nvme/host/nvme.h |   3 +
 2 files changed, 132 insertions(+), 15 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 2c23ea6dc296..9c4acf2725f3 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -86,7 +86,9 @@ static DEFINE_MUTEX(nvme_subsystems_lock);
 
 static DEFINE_IDA(nvme_instance_ida);
 static dev_t nvme_ctrl_base_chr_devt;
+static dev_t nvme_ns_base_chr_devt;
 static struct class *nvme_class;
+static struct class *nvme_ns_class;
 static struct class *nvme_subsys_class;
 
 static void nvme_put_subsystem(struct nvme_subsystem *subsys);
@@ -497,6 +499,7 @@ static void nvme_free_ns(struct kref *kref)
 	if (ns->ndev)
 		nvme_nvm_unregister(ns);
 
+	cdev_device_del(&ns->cdev, &ns->cdev_device);
 	put_disk(ns->disk);
 	nvme_put_ns_head(ns->head);
 	nvme_put_ctrl(ns->ctrl);
@@ -1696,15 +1699,15 @@ static int nvme_handle_ctrl_ioctl(struct nvme_ns *ns, unsigned int cmd,
 	return ret;
 }
 
-static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
-		unsigned int cmd, unsigned long arg)
+static int __nvme_ns_ioctl(struct gendisk *disk, unsigned int cmd,
+			   unsigned long arg)
 {
 	struct nvme_ns_head *head = NULL;
 	void __user *argp = (void __user *)arg;
 	struct nvme_ns *ns;
 	int srcu_idx, ret;
 
-	ns = nvme_get_ns_from_disk(bdev->bd_disk, &head, &srcu_idx);
+	ns = nvme_get_ns_from_disk(disk, &head, &srcu_idx);
 	if (unlikely(!ns))
 		return -EWOULDBLOCK;
 
@@ -1741,6 +1744,18 @@ static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
 	return ret;
 }
 
+static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
+		      unsigned int cmd, unsigned long arg)
+{
+	return __nvme_ns_ioctl(bdev->bd_disk, cmd, arg);
+}
+
+static long nvme_cdev_ioctl(struct file *file, unsigned int cmd,
+			    unsigned long arg)
+{
+	return __nvme_ns_ioctl((struct gendisk *)file->private_data, cmd, arg);
+}
+
 #ifdef CONFIG_COMPAT
 struct nvme_user_io32 {
 	__u8	opcode;
@@ -1782,10 +1797,8 @@ static int nvme_compat_ioctl(struct block_device *bdev, fmode_t mode,
 #define nvme_compat_ioctl	NULL
 #endif /* CONFIG_COMPAT */
 
-static int nvme_open(struct block_device *bdev, fmode_t mode)
+static int __nvme_open(struct nvme_ns *ns)
 {
-	struct nvme_ns *ns = bdev->bd_disk->private_data;
-
 #ifdef CONFIG_NVME_MULTIPATH
 	/* should never be called due to GENHD_FL_HIDDEN */
 	if (WARN_ON_ONCE(ns->head->disk))
@@ -1804,12 +1817,24 @@ static int nvme_open(struct block_device *bdev, fmode_t mode)
 	return -ENXIO;
 }
 
+static void __nvme_release(struct nvme_ns *ns)
+{
+	module_put(ns->ctrl->ops->module);
+	nvme_put_ns(ns);
+}
+
+static int nvme_open(struct block_device *bdev, fmode_t mode)
+{
+	struct nvme_ns *ns = bdev->bd_disk->private_data;
+
+	return __nvme_open(ns);
+}
+
 static void nvme_release(struct gendisk *disk, fmode_t mode)
 {
 	struct nvme_ns *ns = disk->private_data;
 
-	module_put(ns->ctrl->ops->module);
-	nvme_put_ns(ns);
+	__nvme_release(ns);
 }
 
 static int nvme_getgeo(struct block_device *bdev, struct hd_geometry *geo)
@@ -1821,6 +1846,26 @@ static int nvme_getgeo(struct block_device *bdev, struct hd_geometry *geo)
 	return 0;
 }
 
+static int nvme_cdev_open(struct inode *inode, struct file *file)
+{
+	struct nvme_ns *ns = container_of(inode->i_cdev, struct nvme_ns, cdev);
+	int ret;
+
+	ret = __nvme_open(ns);
+	if (!ret)
+		file->private_data = ns->disk;
+
+	return ret;
+}
+
+static int nvme_cdev_release(struct inode *inode, struct file *file)
+{
+	struct nvme_ns *ns = container_of(inode->i_cdev, struct nvme_ns, cdev);
+
+	__nvme_release(ns);
+	return 0;
+}
+
 #ifdef CONFIG_BLK_DEV_INTEGRITY
 static void nvme_init_integrity(struct gendisk *disk, u16 ms, u8 pi_type,
 				u32 max_integrity_segments)
@@ -2303,6 +2348,14 @@ static const struct block_device_operations nvme_bdev_ops = {
 	.pr_ops		= &nvme_pr_ops,
 };
 
+static const struct file_operations nvme_cdev_fops = {
+	.owner		= THIS_MODULE,
+	.open		= nvme_cdev_open,
+	.release	= nvme_cdev_release,
+	.unlocked_ioctl	= nvme_cdev_ioctl,
+	.compat_ioctl	= compat_ptr_ioctl,
+};
+
 #ifdef CONFIG_NVME_MULTIPATH
 static int nvme_ns_head_open(struct block_device *bdev, fmode_t mode)
 {
@@ -3301,6 +3354,9 @@ static inline struct nvme_ns_head *dev_to_ns_head(struct device *dev)
 {
 	struct gendisk *disk = dev_to_disk(dev);
 
+	if (dev->class == nvme_ns_class)
+		return ((struct nvme_ns *)dev_get_drvdata(dev))->head;
+
 	if (disk->fops == &nvme_bdev_ops)
 		return nvme_get_ns_from_dev(dev)->head;
 	else
@@ -3390,7 +3446,7 @@ static struct attribute *nvme_ns_id_attrs[] = {
 };
 
 static umode_t nvme_ns_id_attrs_are_visible(struct kobject *kobj,
-		struct attribute *a, int n)
+	       struct attribute *a, int n)
 {
 	struct device *dev = container_of(kobj, struct device, kobj);
 	struct nvme_ns_ids *ids = &dev_to_ns_head(dev)->ids;
@@ -3432,6 +3488,11 @@ const struct attribute_group *nvme_ns_id_attr_groups[] = {
 	NULL,
 };
 
+const struct attribute_group *nvme_ns_char_id_attr_groups[] = {
+	&nvme_ns_id_attr_group,
+	NULL,
+};
+
 #define nvme_show_str_function(field)						\
 static ssize_t  field##_show(struct device *dev,				\
 			    struct device_attribute *attr, char *buf)		\
@@ -3824,6 +3885,36 @@ struct nvme_ns *nvme_find_get_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 }
 EXPORT_SYMBOL_NS_GPL(nvme_find_get_ns, NVME_TARGET_PASSTHRU);
 
+static int nvme_alloc_chardev_ns(struct nvme_ctrl *ctrl, struct nvme_ns *ns)
+{
+	char cdisk_name[DISK_NAME_LEN];
+	int ret = 0;
+
+	device_initialize(&ns->cdev_device);
+	ns->cdev_device.devt = MKDEV(MAJOR(nvme_ns_base_chr_devt),
+				     ns->head->instance);
+	ns->cdev_device.class = nvme_ns_class;
+	ns->cdev_device.parent = ctrl->device;
+	ns->cdev_device.groups = nvme_ns_char_id_attr_groups;
+	dev_set_drvdata(&ns->cdev_device, ns);
+
+	sprintf(cdisk_name, "nvme%dc%dn%d", ctrl->subsys->instance,
+			ctrl->instance, ns->head->instance);
+
+	ret = dev_set_name(&ns->cdev_device, "%s", cdisk_name);
+	if (ret)
+		return ret;
+
+	cdev_init(&ns->cdev, &nvme_cdev_fops);
+	ns->cdev.owner = ctrl->ops->module;
+
+	ret = cdev_device_add(&ns->cdev, &ns->cdev_device);
+	if (ret)
+		kfree_const(ns->cdev_device.kobj.name);
+
+	return ret;
+}
+
 static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
 		struct nvme_ns_ids *ids)
 {
@@ -3870,8 +3961,12 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
 	memcpy(disk->disk_name, disk_name, DISK_NAME_LEN);
 	ns->disk = disk;
 
+	/* When the device does not support any of the features required by the
+	 * kernel (or viceversa), hide the block device. We can still rely on
+	 * the namespace char device for submitting IOCTLs
+	 */
 	if (nvme_update_ns_info(ns, id))
-		goto out_put_disk;
+		disk->flags |= GENHD_FL_HIDDEN;
 
 	if ((ctrl->quirks & NVME_QUIRK_LIGHTNVM) && id->vs[0] == 0x1) {
 		if (nvme_nvm_register(ns, disk_name, node)) {
@@ -3887,9 +3982,12 @@ 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);
-
 	nvme_mpath_add_disk(ns, id);
 	nvme_fault_inject_init(&ns->fault_inject, ns->disk->disk_name);
+
+	if (nvme_alloc_chardev_ns(ctrl, ns))
+		goto out_put_disk;
+
 	kfree(id);
 
 	return;
@@ -4685,23 +4783,38 @@ static int __init nvme_core_init(void)
 	if (result < 0)
 		goto destroy_delete_wq;
 
+	result = alloc_chrdev_region(&nvme_ns_base_chr_devt, 0,
+			NVME_MINORS, "nvmec");
+	if (result < 0)
+		goto unregister_dev_chrdev;
+
 	nvme_class = class_create(THIS_MODULE, "nvme");
 	if (IS_ERR(nvme_class)) {
 		result = PTR_ERR(nvme_class);
-		goto unregister_chrdev;
+		goto unregister_ns_chrdev;
 	}
 	nvme_class->dev_uevent = nvme_class_uevent;
 
+	nvme_ns_class = class_create(THIS_MODULE, "nvme-ns");
+	if (IS_ERR(nvme_ns_class)) {
+		result = PTR_ERR(nvme_ns_class);
+		goto destroy_dev_class;
+	}
+
 	nvme_subsys_class = class_create(THIS_MODULE, "nvme-subsystem");
 	if (IS_ERR(nvme_subsys_class)) {
 		result = PTR_ERR(nvme_subsys_class);
-		goto destroy_class;
+		goto destroy_ns_class;
 	}
 	return 0;
 
-destroy_class:
+destroy_ns_class:
+	class_destroy(nvme_ns_class);
+destroy_dev_class:
 	class_destroy(nvme_class);
-unregister_chrdev:
+unregister_ns_chrdev:
+	unregister_chrdev_region(nvme_ns_base_chr_devt, NVME_MINORS);
+unregister_dev_chrdev:
 	unregister_chrdev_region(nvme_ctrl_base_chr_devt, NVME_MINORS);
 destroy_delete_wq:
 	destroy_workqueue(nvme_delete_wq);
@@ -4717,6 +4830,7 @@ static void __exit nvme_core_exit(void)
 {
 	class_destroy(nvme_subsys_class);
 	class_destroy(nvme_class);
+	unregister_chrdev_region(nvme_ns_base_chr_devt, NVME_MINORS);
 	unregister_chrdev_region(nvme_ctrl_base_chr_devt, NVME_MINORS);
 	destroy_workqueue(nvme_delete_wq);
 	destroy_workqueue(nvme_reset_wq);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index cdfce7250da0..533bd54f9194 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -436,6 +436,9 @@ struct nvme_ns {
 	struct kref kref;
 	struct nvme_ns_head *head;
 
+	struct device cdev_device;	/* char device */
+	struct cdev cdev;
+
 	int lba_shift;
 	u16 ms;
 	u16 sgs;
-- 
2.17.1


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

Re: [PATCH 4/4] nvme: enable char device per namespace

[Minwoo Im]

Hello,

On 20-12-01 13:56:10, javier@javigon.com wrote:
> From: Javier González <javier.gonz@samsung.com>
> 
> Create a char device per NVMe namespace. This char device is always
> initialized, independently of whether thedeatures implemented by the
> device are supported by the kernel. User-space can therefore always
> issue IOCTLs to the NVMe driver using this char device.
> 
> The char device is presented as /dev/nvmeXcYnZ to follow the hidden
> block device. This naming also aligns with nvme-cli filters, so the char
> device should be usable without tool changes.
> 
> Signed-off-by: Javier González <javier.gonz@samsung.com>
> ---
>  drivers/nvme/host/core.c | 144 +++++++++++++++++++++++++++++++++++----
>  drivers/nvme/host/nvme.h |   3 +
>  2 files changed, 132 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 2c23ea6dc296..9c4acf2725f3 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -86,7 +86,9 @@ static DEFINE_MUTEX(nvme_subsystems_lock);
>  
>  static DEFINE_IDA(nvme_instance_ida);
>  static dev_t nvme_ctrl_base_chr_devt;
> +static dev_t nvme_ns_base_chr_devt;
>  static struct class *nvme_class;
> +static struct class *nvme_ns_class;
>  static struct class *nvme_subsys_class;
>  
>  static void nvme_put_subsystem(struct nvme_subsystem *subsys);
> @@ -497,6 +499,7 @@ static void nvme_free_ns(struct kref *kref)
>  	if (ns->ndev)
>  		nvme_nvm_unregister(ns);
>  
> +	cdev_device_del(&ns->cdev, &ns->cdev_device);
>  	put_disk(ns->disk);
>  	nvme_put_ns_head(ns->head);
>  	nvme_put_ctrl(ns->ctrl);
> @@ -1696,15 +1699,15 @@ static int nvme_handle_ctrl_ioctl(struct nvme_ns *ns, unsigned int cmd,
>  	return ret;
>  }
>  
> -static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
> -		unsigned int cmd, unsigned long arg)
> +static int __nvme_ns_ioctl(struct gendisk *disk, unsigned int cmd,
> +			   unsigned long arg)
>  {
>  	struct nvme_ns_head *head = NULL;
>  	void __user *argp = (void __user *)arg;
>  	struct nvme_ns *ns;
>  	int srcu_idx, ret;
>  
> -	ns = nvme_get_ns_from_disk(bdev->bd_disk, &head, &srcu_idx);
> +	ns = nvme_get_ns_from_disk(disk, &head, &srcu_idx);
>  	if (unlikely(!ns))
>  		return -EWOULDBLOCK;
>  
> @@ -1741,6 +1744,18 @@ static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
>  	return ret;
>  }
>  
> +static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
> +		      unsigned int cmd, unsigned long arg)
> +{
> +	return __nvme_ns_ioctl(bdev->bd_disk, cmd, arg);
> +}
> +
> +static long nvme_cdev_ioctl(struct file *file, unsigned int cmd,
> +			    unsigned long arg)
> +{
> +	return __nvme_ns_ioctl((struct gendisk *)file->private_data, cmd, arg);
> +}
> +
>  #ifdef CONFIG_COMPAT
>  struct nvme_user_io32 {
>  	__u8	opcode;
> @@ -1782,10 +1797,8 @@ static int nvme_compat_ioctl(struct block_device *bdev, fmode_t mode,
>  #define nvme_compat_ioctl	NULL
>  #endif /* CONFIG_COMPAT */
>  
> -static int nvme_open(struct block_device *bdev, fmode_t mode)
> +static int __nvme_open(struct nvme_ns *ns)
>  {
> -	struct nvme_ns *ns = bdev->bd_disk->private_data;
> -
>  #ifdef CONFIG_NVME_MULTIPATH
>  	/* should never be called due to GENHD_FL_HIDDEN */
>  	if (WARN_ON_ONCE(ns->head->disk))
> @@ -1804,12 +1817,24 @@ static int nvme_open(struct block_device *bdev, fmode_t mode)
>  	return -ENXIO;
>  }
>  
> +static void __nvme_release(struct nvme_ns *ns)
> +{
> +	module_put(ns->ctrl->ops->module);
> +	nvme_put_ns(ns);
> +}
> +
> +static int nvme_open(struct block_device *bdev, fmode_t mode)
> +{
> +	struct nvme_ns *ns = bdev->bd_disk->private_data;
> +
> +	return __nvme_open(ns);
> +}
> +
>  static void nvme_release(struct gendisk *disk, fmode_t mode)
>  {
>  	struct nvme_ns *ns = disk->private_data;
>  
> -	module_put(ns->ctrl->ops->module);
> -	nvme_put_ns(ns);
> +	__nvme_release(ns);
>  }
>  
>  static int nvme_getgeo(struct block_device *bdev, struct hd_geometry *geo)
> @@ -1821,6 +1846,26 @@ static int nvme_getgeo(struct block_device *bdev, struct hd_geometry *geo)
>  	return 0;
>  }
>  
> +static int nvme_cdev_open(struct inode *inode, struct file *file)
> +{
> +	struct nvme_ns *ns = container_of(inode->i_cdev, struct nvme_ns, cdev);
> +	int ret;
> +
> +	ret = __nvme_open(ns);
> +	if (!ret)
> +		file->private_data = ns->disk;
> +
> +	return ret;
> +}
> +
> +static int nvme_cdev_release(struct inode *inode, struct file *file)
> +{
> +	struct nvme_ns *ns = container_of(inode->i_cdev, struct nvme_ns, cdev);
> +
> +	__nvme_release(ns);
> +	return 0;
> +}
> +
>  #ifdef CONFIG_BLK_DEV_INTEGRITY
>  static void nvme_init_integrity(struct gendisk *disk, u16 ms, u8 pi_type,
>  				u32 max_integrity_segments)
> @@ -2303,6 +2348,14 @@ static const struct block_device_operations nvme_bdev_ops = {
>  	.pr_ops		= &nvme_pr_ops,
>  };
>  
> +static const struct file_operations nvme_cdev_fops = {
> +	.owner		= THIS_MODULE,
> +	.open		= nvme_cdev_open,
> +	.release	= nvme_cdev_release,
> +	.unlocked_ioctl	= nvme_cdev_ioctl,
> +	.compat_ioctl	= compat_ptr_ioctl,
> +};
> +
>  #ifdef CONFIG_NVME_MULTIPATH
>  static int nvme_ns_head_open(struct block_device *bdev, fmode_t mode)
>  {
> @@ -3301,6 +3354,9 @@ static inline struct nvme_ns_head *dev_to_ns_head(struct device *dev)
>  {
>  	struct gendisk *disk = dev_to_disk(dev);
>  
> +	if (dev->class == nvme_ns_class)
> +		return ((struct nvme_ns *)dev_get_drvdata(dev))->head;

I think it would be better if it can have inline function
nvme_get_ns_from_cdev() just like nvme_get_ns_from_dev().

> +
>  	if (disk->fops == &nvme_bdev_ops)
>  		return nvme_get_ns_from_dev(dev)->head;
>  	else
> @@ -3390,7 +3446,7 @@ static struct attribute *nvme_ns_id_attrs[] = {
>  };
>  
>  static umode_t nvme_ns_id_attrs_are_visible(struct kobject *kobj,
> -		struct attribute *a, int n)
> +	       struct attribute *a, int n)

Unrelated changes for this patch.

>  {
>  	struct device *dev = container_of(kobj, struct device, kobj);
>  	struct nvme_ns_ids *ids = &dev_to_ns_head(dev)->ids;
> @@ -3432,6 +3488,11 @@ const struct attribute_group *nvme_ns_id_attr_groups[] = {
>  	NULL,
>  };
>  
> +const struct attribute_group *nvme_ns_char_id_attr_groups[] = {
> +	&nvme_ns_id_attr_group,
> +	NULL,
> +};
> +
>  #define nvme_show_str_function(field)						\
>  static ssize_t  field##_show(struct device *dev,				\
>  			    struct device_attribute *attr, char *buf)		\
> @@ -3824,6 +3885,36 @@ struct nvme_ns *nvme_find_get_ns(struct nvme_ctrl *ctrl, unsigned nsid)
>  }
>  EXPORT_SYMBOL_NS_GPL(nvme_find_get_ns, NVME_TARGET_PASSTHRU);
>  
> +static int nvme_alloc_chardev_ns(struct nvme_ctrl *ctrl, struct nvme_ns *ns)
> +{
> +	char cdisk_name[DISK_NAME_LEN];
> +	int ret = 0;

Unnecessary initialization for local variable.

> +
> +	device_initialize(&ns->cdev_device);
> +	ns->cdev_device.devt = MKDEV(MAJOR(nvme_ns_base_chr_devt),
> +				     ns->head->instance);
> +	ns->cdev_device.class = nvme_ns_class;
> +	ns->cdev_device.parent = ctrl->device;
> +	ns->cdev_device.groups = nvme_ns_char_id_attr_groups;
> +	dev_set_drvdata(&ns->cdev_device, ns);
> +
> +	sprintf(cdisk_name, "nvme%dc%dn%d", ctrl->subsys->instance,
> +			ctrl->instance, ns->head->instance);

In multi-path, private namespaces for a head are not in /dev, so I don't
think this will hurt private namespaces (e.g., nvme0c0n1), But it looks
like it will make a little bit confusions between chardev and hidden blkdev.

I don't against to update nvme-cli things also even naming conventions are
going to become different than nvmeXcYnZ.

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

Re: [PATCH 4/4] nvme: enable char device per namespace

[Javier González]

[-- Attachment #1: Type: text/plain, Size: 8041 bytes --]

On 01.12.2020 23:03, Minwoo Im wrote:
>Hello,
>
>On 20-12-01 13:56:10, javier@javigon.com wrote:
>> From: Javier González <javier.gonz@samsung.com>
>>
>> Create a char device per NVMe namespace. This char device is always
>> initialized, independently of whether thedeatures implemented by the
>> device are supported by the kernel. User-space can therefore always
>> issue IOCTLs to the NVMe driver using this char device.
>>
>> The char device is presented as /dev/nvmeXcYnZ to follow the hidden
>> block device. This naming also aligns with nvme-cli filters, so the char
>> device should be usable without tool changes.
>>
>> Signed-off-by: Javier González <javier.gonz@samsung.com>
>> ---
>>  drivers/nvme/host/core.c | 144 +++++++++++++++++++++++++++++++++++----
>>  drivers/nvme/host/nvme.h |   3 +
>>  2 files changed, 132 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index 2c23ea6dc296..9c4acf2725f3 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -86,7 +86,9 @@ static DEFINE_MUTEX(nvme_subsystems_lock);
>>
>>  static DEFINE_IDA(nvme_instance_ida);
>>  static dev_t nvme_ctrl_base_chr_devt;
>> +static dev_t nvme_ns_base_chr_devt;
>>  static struct class *nvme_class;
>> +static struct class *nvme_ns_class;
>>  static struct class *nvme_subsys_class;
>>
>>  static void nvme_put_subsystem(struct nvme_subsystem *subsys);
>> @@ -497,6 +499,7 @@ static void nvme_free_ns(struct kref *kref)
>>  	if (ns->ndev)
>>  		nvme_nvm_unregister(ns);
>>
>> +	cdev_device_del(&ns->cdev, &ns->cdev_device);
>>  	put_disk(ns->disk);
>>  	nvme_put_ns_head(ns->head);
>>  	nvme_put_ctrl(ns->ctrl);
>> @@ -1696,15 +1699,15 @@ static int nvme_handle_ctrl_ioctl(struct nvme_ns *ns, unsigned int cmd,
>>  	return ret;
>>  }
>>
>> -static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
>> -		unsigned int cmd, unsigned long arg)
>> +static int __nvme_ns_ioctl(struct gendisk *disk, unsigned int cmd,
>> +			   unsigned long arg)
>>  {
>>  	struct nvme_ns_head *head = NULL;
>>  	void __user *argp = (void __user *)arg;
>>  	struct nvme_ns *ns;
>>  	int srcu_idx, ret;
>>
>> -	ns = nvme_get_ns_from_disk(bdev->bd_disk, &head, &srcu_idx);
>> +	ns = nvme_get_ns_from_disk(disk, &head, &srcu_idx);
>>  	if (unlikely(!ns))
>>  		return -EWOULDBLOCK;
>>
>> @@ -1741,6 +1744,18 @@ static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
>>  	return ret;
>>  }
>>
>> +static int nvme_ioctl(struct block_device *bdev, fmode_t mode,
>> +		      unsigned int cmd, unsigned long arg)
>> +{
>> +	return __nvme_ns_ioctl(bdev->bd_disk, cmd, arg);
>> +}
>> +
>> +static long nvme_cdev_ioctl(struct file *file, unsigned int cmd,
>> +			    unsigned long arg)
>> +{
>> +	return __nvme_ns_ioctl((struct gendisk *)file->private_data, cmd, arg);
>> +}
>> +
>>  #ifdef CONFIG_COMPAT
>>  struct nvme_user_io32 {
>>  	__u8	opcode;
>> @@ -1782,10 +1797,8 @@ static int nvme_compat_ioctl(struct block_device *bdev, fmode_t mode,
>>  #define nvme_compat_ioctl	NULL
>>  #endif /* CONFIG_COMPAT */
>>
>> -static int nvme_open(struct block_device *bdev, fmode_t mode)
>> +static int __nvme_open(struct nvme_ns *ns)
>>  {
>> -	struct nvme_ns *ns = bdev->bd_disk->private_data;
>> -
>>  #ifdef CONFIG_NVME_MULTIPATH
>>  	/* should never be called due to GENHD_FL_HIDDEN */
>>  	if (WARN_ON_ONCE(ns->head->disk))
>> @@ -1804,12 +1817,24 @@ static int nvme_open(struct block_device *bdev, fmode_t mode)
>>  	return -ENXIO;
>>  }
>>
>> +static void __nvme_release(struct nvme_ns *ns)
>> +{
>> +	module_put(ns->ctrl->ops->module);
>> +	nvme_put_ns(ns);
>> +}
>> +
>> +static int nvme_open(struct block_device *bdev, fmode_t mode)
>> +{
>> +	struct nvme_ns *ns = bdev->bd_disk->private_data;
>> +
>> +	return __nvme_open(ns);
>> +}
>> +
>>  static void nvme_release(struct gendisk *disk, fmode_t mode)
>>  {
>>  	struct nvme_ns *ns = disk->private_data;
>>
>> -	module_put(ns->ctrl->ops->module);
>> -	nvme_put_ns(ns);
>> +	__nvme_release(ns);
>>  }
>>
>>  static int nvme_getgeo(struct block_device *bdev, struct hd_geometry *geo)
>> @@ -1821,6 +1846,26 @@ static int nvme_getgeo(struct block_device *bdev, struct hd_geometry *geo)
>>  	return 0;
>>  }
>>
>> +static int nvme_cdev_open(struct inode *inode, struct file *file)
>> +{
>> +	struct nvme_ns *ns = container_of(inode->i_cdev, struct nvme_ns, cdev);
>> +	int ret;
>> +
>> +	ret = __nvme_open(ns);
>> +	if (!ret)
>> +		file->private_data = ns->disk;
>> +
>> +	return ret;
>> +}
>> +
>> +static int nvme_cdev_release(struct inode *inode, struct file *file)
>> +{
>> +	struct nvme_ns *ns = container_of(inode->i_cdev, struct nvme_ns, cdev);
>> +
>> +	__nvme_release(ns);
>> +	return 0;
>> +}
>> +
>>  #ifdef CONFIG_BLK_DEV_INTEGRITY
>>  static void nvme_init_integrity(struct gendisk *disk, u16 ms, u8 pi_type,
>>  				u32 max_integrity_segments)
>> @@ -2303,6 +2348,14 @@ static const struct block_device_operations nvme_bdev_ops = {
>>  	.pr_ops		= &nvme_pr_ops,
>>  };
>>
>> +static const struct file_operations nvme_cdev_fops = {
>> +	.owner		= THIS_MODULE,
>> +	.open		= nvme_cdev_open,
>> +	.release	= nvme_cdev_release,
>> +	.unlocked_ioctl	= nvme_cdev_ioctl,
>> +	.compat_ioctl	= compat_ptr_ioctl,
>> +};
>> +
>>  #ifdef CONFIG_NVME_MULTIPATH
>>  static int nvme_ns_head_open(struct block_device *bdev, fmode_t mode)
>>  {
>> @@ -3301,6 +3354,9 @@ static inline struct nvme_ns_head *dev_to_ns_head(struct device *dev)
>>  {
>>  	struct gendisk *disk = dev_to_disk(dev);
>>
>> +	if (dev->class == nvme_ns_class)
>> +		return ((struct nvme_ns *)dev_get_drvdata(dev))->head;
>
>I think it would be better if it can have inline function
>nvme_get_ns_from_cdev() just like nvme_get_ns_from_dev().

Good point. Will add that.

>
>> +
>>  	if (disk->fops == &nvme_bdev_ops)
>>  		return nvme_get_ns_from_dev(dev)->head;
>>  	else
>> @@ -3390,7 +3446,7 @@ static struct attribute *nvme_ns_id_attrs[] = {
>>  };
>>
>>  static umode_t nvme_ns_id_attrs_are_visible(struct kobject *kobj,
>> -		struct attribute *a, int n)
>> +	       struct attribute *a, int n)
>
>Unrelated changes for this patch.
>
>>  {
>>  	struct device *dev = container_of(kobj, struct device, kobj);
>>  	struct nvme_ns_ids *ids = &dev_to_ns_head(dev)->ids;
>> @@ -3432,6 +3488,11 @@ const struct attribute_group *nvme_ns_id_attr_groups[] = {
>>  	NULL,
>>  };
>>
>> +const struct attribute_group *nvme_ns_char_id_attr_groups[] = {
>> +	&nvme_ns_id_attr_group,
>> +	NULL,
>> +};
>> +
>>  #define nvme_show_str_function(field)						\
>>  static ssize_t  field##_show(struct device *dev,				\
>>  			    struct device_attribute *attr, char *buf)		\
>> @@ -3824,6 +3885,36 @@ struct nvme_ns *nvme_find_get_ns(struct nvme_ctrl *ctrl, unsigned nsid)
>>  }
>>  EXPORT_SYMBOL_NS_GPL(nvme_find_get_ns, NVME_TARGET_PASSTHRU);
>>
>> +static int nvme_alloc_chardev_ns(struct nvme_ctrl *ctrl, struct nvme_ns *ns)
>> +{
>> +	char cdisk_name[DISK_NAME_LEN];
>> +	int ret = 0;
>
>Unnecessary initialization for local variable.
>
>> +
>> +	device_initialize(&ns->cdev_device);
>> +	ns->cdev_device.devt = MKDEV(MAJOR(nvme_ns_base_chr_devt),
>> +				     ns->head->instance);
>> +	ns->cdev_device.class = nvme_ns_class;
>> +	ns->cdev_device.parent = ctrl->device;
>> +	ns->cdev_device.groups = nvme_ns_char_id_attr_groups;
>> +	dev_set_drvdata(&ns->cdev_device, ns);
>> +
>> +	sprintf(cdisk_name, "nvme%dc%dn%d", ctrl->subsys->instance,
>> +			ctrl->instance, ns->head->instance);
>
>In multi-path, private namespaces for a head are not in /dev, so I don't
>think this will hurt private namespaces (e.g., nvme0c0n1), But it looks
>like it will make a little bit confusions between chardev and hidden blkdev.
>
>I don't against to update nvme-cli things also even naming conventions are
>going to become different than nvmeXcYnZ.

Agree. But as I understand it, Keith had a good argument to keep names
aligned with the hidden bdev. It is also true that in that comment he
suggested nesting the char device in /dev/nvme

Keith, would you mind looking over this? Thanks!


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



[-- Attachment #3: Type: text/plain, Size: 158 bytes --]

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

Re: [PATCH 4/4] nvme: enable char device per namespace

[Keith Busch]

On Tue, Dec 01, 2020 at 07:57:32PM +0100, Javier González wrote:
> On 01.12.2020 23:03, Minwoo Im wrote:
> > > +
> > > +	device_initialize(&ns->cdev_device);
> > > +	ns->cdev_device.devt = MKDEV(MAJOR(nvme_ns_base_chr_devt),
> > > +				     ns->head->instance);
> > > +	ns->cdev_device.class = nvme_ns_class;
> > > +	ns->cdev_device.parent = ctrl->device;
> > > +	ns->cdev_device.groups = nvme_ns_char_id_attr_groups;
> > > +	dev_set_drvdata(&ns->cdev_device, ns);
> > > +
> > > +	sprintf(cdisk_name, "nvme%dc%dn%d", ctrl->subsys->instance,
> > > +			ctrl->instance, ns->head->instance);
> > 
> > In multi-path, private namespaces for a head are not in /dev, so I don't
> > think this will hurt private namespaces (e.g., nvme0c0n1), But it looks
> > like it will make a little bit confusions between chardev and hidden blkdev.
> > 
> > I don't against to update nvme-cli things also even naming conventions are
> > going to become different than nvmeXcYnZ.
> 
> Agree. But as I understand it, Keith had a good argument to keep names
> aligned with the hidden bdev. 

My suggested naming makes it as obvious as possible that the character
device in /dev/ and the hidden block device in /sys/ are referring to
the same thing. What is confusing about that?

> It is also true that in that comment he suggested nesting the char
> device in /dev/nvme

Yeah, I'm okay with sub-directories for these special handles, but there
are arguments against it too. I don't feel that strongly about it either
way.

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

Re: [PATCH 4/4] nvme: enable char device per namespace

[Christoph Hellwig]

On Wed, Dec 02, 2020 at 04:30:02AM +0900, Keith Busch wrote:
> > > In multi-path, private namespaces for a head are not in /dev, so I don't
> > > think this will hurt private namespaces (e.g., nvme0c0n1), But it looks
> > > like it will make a little bit confusions between chardev and hidden blkdev.
> > > 
> > > I don't against to update nvme-cli things also even naming conventions are
> > > going to become different than nvmeXcYnZ.
> > 
> > Agree. But as I understand it, Keith had a good argument to keep names
> > aligned with the hidden bdev. 
> 
> My suggested naming makes it as obvious as possible that the character
> device in /dev/ and the hidden block device in /sys/ are referring to
> the same thing. What is confusing about that?
> 
> > It is also true that in that comment he suggested nesting the char
> > device in /dev/nvme
> 
> Yeah, I'm okay with sub-directories for these special handles, but there
> are arguments against it too. I don't feel that strongly about it either
> way.

I'd prefer different naming for the char vs the block devices.  Yes,
this will require a little work in the userspace tools to support the
character device, but I think it is much cleaner.

Devices in subdirectories of /dev/ are very rare and keep causing problem
with userspace tooling for the few drivers that use them, so I don't
think they are a good idea.

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

Re: [PATCH 4/4] nvme: enable char device per namespace

[Javier González]

On 01.12.2020 20:38, Christoph Hellwig wrote:
>On Wed, Dec 02, 2020 at 04:30:02AM +0900, Keith Busch wrote:
>> > > In multi-path, private namespaces for a head are not in /dev, so I don't
>> > > think this will hurt private namespaces (e.g., nvme0c0n1), But it looks
>> > > like it will make a little bit confusions between chardev and hidden blkdev.
>> > >
>> > > I don't against to update nvme-cli things also even naming conventions are
>> > > going to become different than nvmeXcYnZ.
>> >
>> > Agree. But as I understand it, Keith had a good argument to keep names
>> > aligned with the hidden bdev.
>>
>> My suggested naming makes it as obvious as possible that the character
>> device in /dev/ and the hidden block device in /sys/ are referring to
>> the same thing. What is confusing about that?

Ok. I see your point. I was thinking of the case where the multipath
bdev is also enabled so we would have the same name in different places
referring to a different device.

>>
>> > It is also true that in that comment he suggested nesting the char
>> > device in /dev/nvme
>>
>> Yeah, I'm okay with sub-directories for these special handles, but there
>> are arguments against it too. I don't feel that strongly about it either
>> way.
>
>I'd prefer different naming for the char vs the block devices.  Yes,
>this will require a little work in the userspace tools to support the
>character device, but I think it is much cleaner.
>
>Devices in subdirectories of /dev/ are very rare and keep causing problem
>with userspace tooling for the few drivers that use them, so I don't
>think they are a good idea.

Would something like nvmeXnYc work here or your prefer something
different where we need to implement new, dedicated filters for
user-space? I thing you suggested nvmegXnY at some point?

Here, the naming would be for both the char device and the sysfs
entry.

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

Re: [PATCH 4/4] nvme: enable char device per namespace

[Christoph Hellwig]

On Tue, Dec 01, 2020 at 09:44:56PM +0100, Javier González wrote:
>> Devices in subdirectories of /dev/ are very rare and keep causing problem
>> with userspace tooling for the few drivers that use them, so I don't
>> think they are a good idea.
>
> Would something like nvmeXnYc work here or your prefer something
> different where we need to implement new, dedicated filters for
> user-space? I thing you suggested nvmegXnY at some point?

I think I suggested /dev/ng*, but maybe that is to short for the modern
times, so the above would work for me.

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

Re: [PATCH 4/4] nvme: enable char device per namespace

[Minwoo Im]

On 20-12-02 04:30:02, Keith Busch wrote:
> On Tue, Dec 01, 2020 at 07:57:32PM +0100, Javier González wrote:
> > On 01.12.2020 23:03, Minwoo Im wrote:
> > > > +
> > > > +	device_initialize(&ns->cdev_device);
> > > > +	ns->cdev_device.devt = MKDEV(MAJOR(nvme_ns_base_chr_devt),
> > > > +				     ns->head->instance);
> > > > +	ns->cdev_device.class = nvme_ns_class;
> > > > +	ns->cdev_device.parent = ctrl->device;
> > > > +	ns->cdev_device.groups = nvme_ns_char_id_attr_groups;
> > > > +	dev_set_drvdata(&ns->cdev_device, ns);
> > > > +
> > > > +	sprintf(cdisk_name, "nvme%dc%dn%d", ctrl->subsys->instance,
> > > > +			ctrl->instance, ns->head->instance);
> > > 
> > > In multi-path, private namespaces for a head are not in /dev, so I don't
> > > think this will hurt private namespaces (e.g., nvme0c0n1), But it looks
> > > like it will make a little bit confusions between chardev and hidden blkdev.
> > > 
> > > I don't against to update nvme-cli things also even naming conventions are
> > > going to become different than nvmeXcYnZ.
> > 
> > Agree. But as I understand it, Keith had a good argument to keep names
> > aligned with the hidden bdev. 
> 
> My suggested naming makes it as obvious as possible that the character
> device in /dev/ and the hidden block device in /sys/ are referring to
> the same thing. What is confusing about that?

I meant that someone might misunderstand tht /dev/nvmeXcYnZ is also a
blkdev just like /dev/nvmeXnY.  I'm just saying it might be, but I'm
fine with suggested naming as those two are indicating a single
concept (namespace).

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

Re: [PATCH 0/4] nvme: enable per-namespace char device

[Christoph Hellwig]

I've applied patches 1-3 to nvme-5.11.

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