From: Logan Gunthorpe <logang@deltatee.com>
To: Keith Busch <kbusch@kernel.org>
Cc: Keith Busch <keith.busch@intel.com>,
Hannes Reinecke <hare@suse.com>, Sagi Grimberg <sagi@grimberg.me>,
"Martin K . Petersen" <martin.petersen@oracle.com>,
linux-kernel@vger.kernel.org, linux-nvme@lists.infradead.org,
Jens Axboe <axboe@fb.com>, Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCH] nvme-core: Fix subsystem instance mismatches
Date: Wed, 4 Sep 2019 11:01:22 -0600 [thread overview]
Message-ID: <f07e03f1-48f0-591e-fdf6-9499fa4dd9ab@deltatee.com> (raw)
In-Reply-To: <20190904163557.GF21302@localhost.localdomain>
On 2019-09-04 10:35 a.m., Keith Busch wrote:
> On Wed, Sep 04, 2019 at 10:07:12AM -0600, Logan Gunthorpe wrote:
>> Yes, I agree, we can't solve the mismatch problem in the general case:
>> with sequences of hot plug events there will always be a case that
>> mismatches. I just think we can do better in the simple common default case.
>
> This may be something where udev can help us. I might be able to find
> some time to look at that, but not today.
>
>>> Can we just ensure there is never a matching controller then? This
>>> patch will accomplish that and simpler than wrapping the instance in a
>>> refcount'ed object:
>>>
>>> http://lists.infradead.org/pipermail/linux-nvme/2019-May/024142.html
>>
>> I don't really like that idea. It reduces the confusion caused by
>> mismatching numbers, but causes the controller to never match the
>> namespace, which is also confusing but in a different way.
>>
>> I like the nvme_instance idea. It's not going to be perfect but it has
>> some nice properties: the subsystem will try to match the controller's
>> instance whenever possible, but in cases where it doesn't, the instance
>> number of the subsystem will never be the same as an existing controller.
>>
>> I'll see if I can work up a quick patch set and see what people think.
>
> How about this: we have the subsys copy the controller's instance,
> and the nvme_free_ctrl() doesn't release it if its subsys matches?
Oh, yes that's simpler than the struct/kref method and looks like it
will accomplish the same thing. I did some brief testing with it and it
seems to work for me (though I don't have any subsystems with multiple
controllers). If you want to make a patch out of it you can add my
Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
Thanks!
Logan
> ---
> 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);
> --
>
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
next prev parent reply other threads:[~2019-09-04 17:01 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-31 0:01 [PATCH] nvme-core: Fix subsystem instance mismatches Logan Gunthorpe
2019-08-31 15:29 ` Keith Busch
2019-09-03 16:08 ` Logan Gunthorpe
2019-09-03 16:46 ` Keith Busch
2019-09-03 18:13 ` Logan Gunthorpe
2019-09-04 6:05 ` Christoph Hellwig
2019-09-04 14:44 ` Keith Busch
2019-09-04 15:42 ` Christoph Hellwig
2019-09-04 15:54 ` Keith Busch
2019-09-04 16:02 ` Christoph Hellwig
2019-09-04 16:07 ` Logan Gunthorpe
2019-09-04 16:35 ` Keith Busch
2019-09-04 17:01 ` Logan Gunthorpe [this message]
2019-09-04 17:14 ` Keith Busch
2019-09-04 17:29 ` Logan Gunthorpe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f07e03f1-48f0-591e-fdf6-9499fa4dd9ab@deltatee.com \
--to=logang@deltatee.com \
--cc=axboe@fb.com \
--cc=hare@suse.com \
--cc=hch@lst.de \
--cc=kbusch@kernel.org \
--cc=keith.busch@intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nvme@lists.infradead.org \
--cc=martin.petersen@oracle.com \
--cc=sagi@grimberg.me \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).