Re: [PATCHv2] nvme: Assign subsy instance from first ctrl

2019-09-09 Thread Hannes Reinecke 
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 
> Signed-off-by: Keith Busch 
> ---
> v1 -> v2:
> 
>   Changelog: reduce sensationalism, fix spelling 
> 
>  drivers/nvme/host/core.c | 21 ++---
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
-- 
Dr. Hannes Reinecke   Teamlead Storage & Networking
h...@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

Re: [PATCHv2] nvme: Assign subsy instance from first ctrl

2019-09-07 Thread Minwoo Im 
> 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 
> Signed-off-by: Keith Busch 

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

Re: [PATCHv2] nvme: Assign subsy instance from first ctrl

2019-09-05 Thread Sagi Grimberg 

Looks good,

Reviewed-by: Sagi Grimberg 

Applied to nvme-5.4

Re: [PATCHv2] nvme: Assign subsy instance from first ctrl

2019-09-05 Thread Christoph Hellwig 
Looks good:

Reviewed-by: Christoph Hellwig

[PATCHv2] nvme: Assign subsy instance from first ctrl

2019-09-05 Thread Keith Busch 
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 
Signed-off-by: Keith Busch 
---
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(_subsystems_ida, subsys->instance);
+   if (subsys->instance >= 0)
+   ida_simple_remove(_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(_subsystems_ida, 0, 0, GFP_KERNEL);
-   if (ret < 0) {
-   kfree(subsys);
-   return ret;
-   }
-   subsys->instance = ret;
+
+   subsys->instance = -1;
mutex_init(>lock);
kref_init(>ref);
INIT_LIST_HEAD(>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(>dev, "nvme-subsys%d", subsys->instance);
+   dev_set_name(>dev, "nvme-subsys%d", ctrl->instance);
device_initialize(>dev);
 
mutex_lock(_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(>subsys_entry, >ctrls);
mutex_unlock(_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(_instance_ida, ctrl->instance);
+   if (subsys && ctrl->instance != subsys->instance)
+   ida_simple_remove(_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(_subsystems_ida);
class_destroy(nvme_subsys_class);
class_destroy(nvme_class);
unregister_chrdev_region(nvme_chr_devt, NVME_MINORS);
-- 
2.14.5