Linux-NVME Archive on lore.kernel.org
 help / color / Atom feed
* [PATCHv2] nvme: Assign subsy instance from first ctrl
@ 2019-09-05 16:33 Keith Busch
  2019-09-05 16:39 ` Christoph Hellwig
                   ` (3 more replies)
  0 siblings, 4 replies; 5+ messages in thread
From: Keith Busch @ 2019-09-05 16:33 UTC (permalink / raw)
  To: Christoph Hellwig, linux-nvme, Sagi Grimberg
  Cc: Hannes Reinecke, Martin K . Petersen, linux-kernel, Jens Axboe,
	Keith Busch, Logan Gunthorpe

The namespace disk names must be unique for the lifetime of the
subsystem. This was accomplished by using their parent subsystems'
instances which were allocated independently from the controllers
connected to that subsystem. This allowed name prefixes assigned to
namespaces to match a controller from an unrelated subsystem, and has
created confusion among users examining device nodes.

Ensure a namespace's subsystem instance never clashes with a controller
instance of another subsystem by transferring the instance ownership
to the parent subsystem from the first controller discovered in that
subsystem.

Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
v1 -> v2:

  Changelog: reduce sensationalism, fix spelling 

 drivers/nvme/host/core.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 14c0bfb55615..8a8279ece5ee 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -81,7 +81,6 @@ EXPORT_SYMBOL_GPL(nvme_reset_wq);
 struct workqueue_struct *nvme_delete_wq;
 EXPORT_SYMBOL_GPL(nvme_delete_wq);
 
-static DEFINE_IDA(nvme_subsystems_ida);
 static LIST_HEAD(nvme_subsystems);
 static DEFINE_MUTEX(nvme_subsystems_lock);
 
@@ -2344,7 +2343,8 @@ static void nvme_release_subsystem(struct device *dev)
 	struct nvme_subsystem *subsys =
 		container_of(dev, struct nvme_subsystem, dev);
 
-	ida_simple_remove(&nvme_subsystems_ida, subsys->instance);
+	if (subsys->instance >= 0)
+		ida_simple_remove(&nvme_instance_ida, subsys->instance);
 	kfree(subsys);
 }
 
@@ -2473,12 +2473,8 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
 	subsys = kzalloc(sizeof(*subsys), GFP_KERNEL);
 	if (!subsys)
 		return -ENOMEM;
-	ret = ida_simple_get(&nvme_subsystems_ida, 0, 0, GFP_KERNEL);
-	if (ret < 0) {
-		kfree(subsys);
-		return ret;
-	}
-	subsys->instance = ret;
+
+	subsys->instance = -1;
 	mutex_init(&subsys->lock);
 	kref_init(&subsys->ref);
 	INIT_LIST_HEAD(&subsys->ctrls);
@@ -2497,7 +2493,7 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
 	subsys->dev.class = nvme_subsys_class;
 	subsys->dev.release = nvme_release_subsystem;
 	subsys->dev.groups = nvme_subsys_attrs_groups;
-	dev_set_name(&subsys->dev, "nvme-subsys%d", subsys->instance);
+	dev_set_name(&subsys->dev, "nvme-subsys%d", ctrl->instance);
 	device_initialize(&subsys->dev);
 
 	mutex_lock(&nvme_subsystems_lock);
@@ -2528,6 +2524,8 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
 		goto out_put_subsystem;
 	}
 
+	if (!found)
+		subsys->instance = ctrl->instance;
 	ctrl->subsys = subsys;
 	list_add_tail(&ctrl->subsys_entry, &subsys->ctrls);
 	mutex_unlock(&nvme_subsystems_lock);
@@ -3803,7 +3801,9 @@ static void nvme_free_ctrl(struct device *dev)
 		container_of(dev, struct nvme_ctrl, ctrl_device);
 	struct nvme_subsystem *subsys = ctrl->subsys;
 
-	ida_simple_remove(&nvme_instance_ida, ctrl->instance);
+	if (subsys && ctrl->instance != subsys->instance)
+		ida_simple_remove(&nvme_instance_ida, ctrl->instance);
+
 	kfree(ctrl->effects);
 	nvme_mpath_uninit(ctrl);
 	__free_page(ctrl->discard_page);
@@ -4085,7 +4085,6 @@ static int __init nvme_core_init(void)
 
 static void __exit nvme_core_exit(void)
 {
-	ida_destroy(&nvme_subsystems_ida);
 	class_destroy(nvme_subsys_class);
 	class_destroy(nvme_class);
 	unregister_chrdev_region(nvme_chr_devt, NVME_MINORS);
-- 
2.14.5


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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCHv2] nvme: Assign subsy instance from first ctrl
  2019-09-05 16:33 [PATCHv2] nvme: Assign subsy instance from first ctrl Keith Busch
@ 2019-09-05 16:39 ` Christoph Hellwig
  2019-09-05 20:05 ` Sagi Grimberg
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 5+ messages in thread
From: Christoph Hellwig @ 2019-09-05 16:39 UTC (permalink / raw)
  To: Keith Busch
  Cc: Hannes Reinecke, Sagi Grimberg, Martin K . Petersen,
	linux-kernel, linux-nvme, Jens Axboe, Logan Gunthorpe,
	Christoph Hellwig

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCHv2] nvme: Assign subsy instance from first ctrl
  2019-09-05 16:33 [PATCHv2] nvme: Assign subsy instance from first ctrl Keith Busch
  2019-09-05 16:39 ` Christoph Hellwig
@ 2019-09-05 20:05 ` Sagi Grimberg
       [not found] ` <CGME20190905163547epcas5p185b3455e7f33cff005c48a0f25745bab@epcms2p7>
  2019-09-09 10:32 ` Hannes Reinecke
  3 siblings, 0 replies; 5+ messages in thread
From: Sagi Grimberg @ 2019-09-05 20:05 UTC (permalink / raw)
  To: Keith Busch, Christoph Hellwig, linux-nvme
  Cc: Jens Axboe, Hannes Reinecke, Logan Gunthorpe, linux-kernel,
	Martin K . Petersen

Looks good,

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Applied to nvme-5.4

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCHv2] nvme: Assign subsy instance from first ctrl
       [not found] ` <CGME20190905163547epcas5p185b3455e7f33cff005c48a0f25745bab@epcms2p7>
@ 2019-09-07  6:13   ` Minwoo Im
  0 siblings, 0 replies; 5+ messages in thread
From: Minwoo Im @ 2019-09-07  6:13 UTC (permalink / raw)
  To: Keith Busch, Minwoo Im
  Cc: Hannes Reinecke, Sagi Grimberg, Martin K . Petersen,
	linux-kernel, linux-nvme, Jens Axboe, Logan Gunthorpe,
	Christoph Hellwig

> Subject: [PATCHv2] nvme: Assign subsy instance from first ctrl

I'm not sure but, I have not seen 'subsy' thing before.  Maybe
s/sybsy/subsys/ ?

> 
> The namespace disk names must be unique for the lifetime of the
> subsystem. This was accomplished by using their parent subsystems'
> instances which were allocated independently from the controllers
> connected to that subsystem. This allowed name prefixes assigned to
> namespaces to match a controller from an unrelated subsystem, and has
> created confusion among users examining device nodes.
> 
> Ensure a namespace's subsystem instance never clashes with a controller
> instance of another subsystem by transferring the instance ownership
> to the parent subsystem from the first controller discovered in that
> subsystem.
> 
> Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
> Signed-off-by: Keith Busch <kbusch@kernel.org>

Keith, Thanks for this patch.  I really like this concept which can
avoid from instance mistakes.

Otherwise looks good to me.

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

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCHv2] nvme: Assign subsy instance from first ctrl
  2019-09-05 16:33 [PATCHv2] nvme: Assign subsy instance from first ctrl Keith Busch
                   ` (2 preceding siblings ...)
       [not found] ` <CGME20190905163547epcas5p185b3455e7f33cff005c48a0f25745bab@epcms2p7>
@ 2019-09-09 10:32 ` Hannes Reinecke
  3 siblings, 0 replies; 5+ messages in thread
From: Hannes Reinecke @ 2019-09-09 10:32 UTC (permalink / raw)
  To: Keith Busch, Sagi Grimberg, linux-nvme, Christoph Hellwig
  Cc: Jens Axboe, Hannes Reinecke, Logan Gunthorpe, linux-kernel,
	Martin K . Petersen

On 9/5/19 6:33 PM, Keith Busch wrote:
> The namespace disk names must be unique for the lifetime of the
> subsystem. This was accomplished by using their parent subsystems'
> instances which were allocated independently from the controllers
> connected to that subsystem. This allowed name prefixes assigned to
> namespaces to match a controller from an unrelated subsystem, and has
> created confusion among users examining device nodes.
> 
> Ensure a namespace's subsystem instance never clashes with a controller
> instance of another subsystem by transferring the instance ownership
> to the parent subsystem from the first controller discovered in that
> subsystem.
> 
> Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
> v1 -> v2:
> 
>   Changelog: reduce sensationalism, fix spelling 
> 
>  drivers/nvme/host/core.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      Teamlead Storage & Networking
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-05 16:33 [PATCHv2] nvme: Assign subsy instance from first ctrl Keith Busch
2019-09-05 16:39 ` Christoph Hellwig
2019-09-05 20:05 ` Sagi Grimberg
     [not found] ` <CGME20190905163547epcas5p185b3455e7f33cff005c48a0f25745bab@epcms2p7>
2019-09-07  6:13   ` Minwoo Im
2019-09-09 10:32 ` Hannes Reinecke

Linux-NVME Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-nvme/0 linux-nvme/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-nvme linux-nvme/ https://lore.kernel.org/linux-nvme \
		linux-nvme@lists.infradead.org linux-nvme@archiver.kernel.org
	public-inbox-index linux-nvme


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-nvme


AGPL code for this site: git clone https://public-inbox.org/ public-inbox