* [PATCH v2 0/3] Support discovery log change events @ 2019-07-12 18:02 Sagi Grimberg 2019-07-12 18:02 ` [PATCH v2 1/3] nvme-fabrics: allow discovery subsystems accept a kato Sagi Grimberg ` (3 more replies) 0 siblings, 4 replies; 23+ messages in thread From: Sagi Grimberg @ 2019-07-12 18:02 UTC (permalink / raw) We want to be able to support discovery log change events automatically without user intervention. The definition of discovery log change events applies on "persistent" long lived controllers, so first we need to have discovery controllers to stay for a long time and accept kato value. Then when we do happen to get a discovery log change event on the persistent discovery controller, we simply fire a udev event to user-space to re-query the discovery log page and connect to new subsystems in the fabric. This works with James' latest nvme-cli patches. Changes from v1: - rebase to nvme-5.3 - pass none if trsvcid is uninitialized - pass NVME_CTRL_NAME instead of NVME_CTRL_INSTANCE Sagi Grimberg (3): nvme-fabrics: allow discovery subsystems accept a kato nvme: enable aen also for discovery controllers nvme: fire discovery log page change events to userspace drivers/nvme/host/core.c | 39 ++++++++++++++++++++++++++++++++++--- drivers/nvme/host/fabrics.c | 12 ++---------- 2 files changed, 38 insertions(+), 13 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 1/3] nvme-fabrics: allow discovery subsystems accept a kato 2019-07-12 18:02 [PATCH v2 0/3] Support discovery log change events Sagi Grimberg @ 2019-07-12 18:02 ` Sagi Grimberg 2019-07-14 8:08 ` Minwoo Im ` (2 more replies) 2019-07-12 18:02 ` [PATCH v2 2/3] nvme: enable aen also for discovery controllers Sagi Grimberg ` (2 subsequent siblings) 3 siblings, 3 replies; 23+ messages in thread From: Sagi Grimberg @ 2019-07-12 18:02 UTC (permalink / raw) This modifies the behavior of discovery subsystems to accept a kato as a preparation to support discovery log change events. This also means that now every discovery controller will have a default kato value, and for non-persistent connections the host needs to pass in a zero kato value (keep_alive_tmo=0). Signed-off-by: Sagi Grimberg <sagi at grimberg.me> --- drivers/nvme/host/fabrics.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c index 1994d5b42f94..d0d066307bb4 100644 --- a/drivers/nvme/host/fabrics.c +++ b/drivers/nvme/host/fabrics.c @@ -381,8 +381,8 @@ int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl) * Set keep-alive timeout in seconds granularity (ms * 1000) * and add a grace period for controller kato enforcement */ - cmd.connect.kato = ctrl->opts->discovery_nqn ? 0 : - cpu_to_le32((ctrl->kato + NVME_KATO_GRACE) * 1000); + cmd.connect.kato = ctrl->kato ? + cpu_to_le32((ctrl->kato + NVME_KATO_GRACE) * 1000) : 0; if (ctrl->opts->disable_sqflow) cmd.connect.cattr |= NVME_CONNECT_DISABLE_SQFLOW; @@ -738,13 +738,6 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts, pr_warn("keep_alive_tmo 0 won't execute keep alives!!!\n"); } opts->kato = token; - - if (opts->discovery_nqn && opts->kato) { - pr_err("Discovery controllers cannot accept KATO != 0\n"); - ret = -EINVAL; - goto out; - } - break; case NVMF_OPT_CTRL_LOSS_TMO: if (match_int(args, &token)) { @@ -865,7 +858,6 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts, } if (opts->discovery_nqn) { - opts->kato = 0; opts->nr_io_queues = 0; opts->nr_write_queues = 0; opts->nr_poll_queues = 0; -- 2.17.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v2 1/3] nvme-fabrics: allow discovery subsystems accept a kato 2019-07-12 18:02 ` [PATCH v2 1/3] nvme-fabrics: allow discovery subsystems accept a kato Sagi Grimberg @ 2019-07-14 8:08 ` Minwoo Im 2019-07-14 15:00 ` James Smart 2019-07-19 13:48 ` Hannes Reinecke 2 siblings, 0 replies; 23+ messages in thread From: Minwoo Im @ 2019-07-14 8:08 UTC (permalink / raw) On 19-07-12 11:02:09, Sagi Grimberg wrote: > This modifies the behavior of discovery subsystems to accept > a kato as a preparation to support discovery log change > events. This also means that now every discovery controller > will have a default kato value, and for non-persistent connections > the host needs to pass in a zero kato value (keep_alive_tmo=0). > > Signed-off-by: Sagi Grimberg <sagi at grimberg.me> This looks good to me. Reviewed-by: Minwoo Im <minwoo.im.dev at gmail.com> Thanks, ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 1/3] nvme-fabrics: allow discovery subsystems accept a kato 2019-07-12 18:02 ` [PATCH v2 1/3] nvme-fabrics: allow discovery subsystems accept a kato Sagi Grimberg 2019-07-14 8:08 ` Minwoo Im @ 2019-07-14 15:00 ` James Smart 2019-07-19 13:48 ` Hannes Reinecke 2 siblings, 0 replies; 23+ messages in thread From: James Smart @ 2019-07-14 15:00 UTC (permalink / raw) On 7/12/2019 11:02 AM, Sagi Grimberg wrote: > This modifies the behavior of discovery subsystems to accept > a kato as a preparation to support discovery log change > events. This also means that now every discovery controller > will have a default kato value, and for non-persistent connections > the host needs to pass in a zero kato value (keep_alive_tmo=0). > > Signed-off-by: Sagi Grimberg <sagi at grimberg.me> > --- > drivers/nvme/host/fabrics.c | 12 ++---------- > 1 file changed, 2 insertions(+), 10 deletions(-) > > Reviewed-by: James Smart <james.smart at broadcom.com> ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 1/3] nvme-fabrics: allow discovery subsystems accept a kato 2019-07-12 18:02 ` [PATCH v2 1/3] nvme-fabrics: allow discovery subsystems accept a kato Sagi Grimberg 2019-07-14 8:08 ` Minwoo Im 2019-07-14 15:00 ` James Smart @ 2019-07-19 13:48 ` Hannes Reinecke 2 siblings, 0 replies; 23+ messages in thread From: Hannes Reinecke @ 2019-07-19 13:48 UTC (permalink / raw) On 7/12/19 8:02 PM, Sagi Grimberg wrote: > This modifies the behavior of discovery subsystems to accept > a kato as a preparation to support discovery log change > events. This also means that now every discovery controller > will have a default kato value, and for non-persistent connections > the host needs to pass in a zero kato value (keep_alive_tmo=0). > > Signed-off-by: Sagi Grimberg <sagi at grimberg.me> > --- > drivers/nvme/host/fabrics.c | 12 ++---------- > 1 file changed, 2 insertions(+), 10 deletions(-) > Reviewed-by: Hannes Reinecke <hare at suse.com> Cheers, Hannes -- Dr. Hannes Reinecke Teamlead Storage & Networking hare at suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg GF: Felix Imend?rffer, Mary Higgins, Sri Rasiah HRB 21284 (AG N?rnberg) ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 2/3] nvme: enable aen also for discovery controllers 2019-07-12 18:02 [PATCH v2 0/3] Support discovery log change events Sagi Grimberg 2019-07-12 18:02 ` [PATCH v2 1/3] nvme-fabrics: allow discovery subsystems accept a kato Sagi Grimberg @ 2019-07-12 18:02 ` Sagi Grimberg 2019-07-14 8:09 ` Minwoo Im ` (2 more replies) 2019-07-12 18:02 ` [PATCH v2 3/3] nvme: fire discovery log page change events to userspace Sagi Grimberg 2019-08-01 1:32 ` [PATCH v2 0/3] Support discovery log change events Sagi Grimberg 3 siblings, 3 replies; 23+ messages in thread From: Sagi Grimberg @ 2019-07-12 18:02 UTC (permalink / raw) If the controller supports discovery log page change events, we want to enable it on the host side as well. Signed-off-by: Sagi Grimberg <sagi at grimberg.me> --- drivers/nvme/host/core.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 3077cd4d75bf..116c210826c2 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -1190,6 +1190,8 @@ static void nvme_enable_aen(struct nvme_ctrl *ctrl) if (!supported_aens) return; + queue_work(nvme_wq, &ctrl->async_event_work); + status = nvme_set_features(ctrl, NVME_FEAT_ASYNC_EVENT, supported_aens, NULL, 0, &result); if (status) @@ -3746,10 +3748,13 @@ void nvme_start_ctrl(struct nvme_ctrl *ctrl) if (ctrl->kato) nvme_start_keep_alive(ctrl); + if (ctrl->queue_count > 1 || + (ctrl->ops->flags & NVME_F_FABRICS && + ctrl->opts->discovery_nqn)) + nvme_enable_aen(ctrl); + if (ctrl->queue_count > 1) { nvme_queue_scan(ctrl); - nvme_enable_aen(ctrl); - queue_work(nvme_wq, &ctrl->async_event_work); nvme_start_queues(ctrl); } } -- 2.17.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v2 2/3] nvme: enable aen also for discovery controllers 2019-07-12 18:02 ` [PATCH v2 2/3] nvme: enable aen also for discovery controllers Sagi Grimberg @ 2019-07-14 8:09 ` Minwoo Im 2019-07-14 15:04 ` James Smart 2019-07-19 13:49 ` Hannes Reinecke 2 siblings, 0 replies; 23+ messages in thread From: Minwoo Im @ 2019-07-14 8:09 UTC (permalink / raw) On 19-07-12 11:02:10, Sagi Grimberg wrote: > If the controller supports discovery log page change events, > we want to enable it on the host side as well. > > Signed-off-by: Sagi Grimberg <sagi at grimberg.me> This looks good to me. Reviewed-by: Minwoo Im <minwoo.im.dev at gmail.com> Thanks, ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 2/3] nvme: enable aen also for discovery controllers 2019-07-12 18:02 ` [PATCH v2 2/3] nvme: enable aen also for discovery controllers Sagi Grimberg 2019-07-14 8:09 ` Minwoo Im @ 2019-07-14 15:04 ` James Smart 2019-07-19 13:49 ` Hannes Reinecke 2 siblings, 0 replies; 23+ messages in thread From: James Smart @ 2019-07-14 15:04 UTC (permalink / raw) On 7/12/2019 11:02 AM, Sagi Grimberg wrote: > If the controller supports discovery log page change events, > we want to enable it on the host side as well. > > Signed-off-by: Sagi Grimberg <sagi at grimberg.me> > --- > drivers/nvme/host/core.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c > index 3077cd4d75bf..116c210826c2 100644 > --- a/drivers/nvme/host/core.c > +++ b/drivers/nvme/host/core.c > @@ -1190,6 +1190,8 @@ static void nvme_enable_aen(struct nvme_ctrl *ctrl) > if (!supported_aens) > return; > > + queue_work(nvme_wq, &ctrl->async_event_work); > + > status = nvme_set_features(ctrl, NVME_FEAT_ASYNC_EVENT, supported_aens, > NULL, 0, &result); > if (status) > @@ -3746,10 +3748,13 @@ void nvme_start_ctrl(struct nvme_ctrl *ctrl) > if (ctrl->kato) > nvme_start_keep_alive(ctrl); > > + if (ctrl->queue_count > 1 || > + (ctrl->ops->flags & NVME_F_FABRICS && > + ctrl->opts->discovery_nqn)) > + nvme_enable_aen(ctrl); > + > if (ctrl->queue_count > 1) { > nvme_queue_scan(ctrl); > - nvme_enable_aen(ctrl); > - queue_work(nvme_wq, &ctrl->async_event_work); > nvme_start_queues(ctrl); > } > } Looks fine, but curious why you needed to qualify the discovery_nqn with the FABRICS flag.? I would have assumed discovery_nqn would have been sufficient. Reviewed-by: James Smart <james.smart at broadcom.com> -- james ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 2/3] nvme: enable aen also for discovery controllers 2019-07-12 18:02 ` [PATCH v2 2/3] nvme: enable aen also for discovery controllers Sagi Grimberg 2019-07-14 8:09 ` Minwoo Im 2019-07-14 15:04 ` James Smart @ 2019-07-19 13:49 ` Hannes Reinecke 2 siblings, 0 replies; 23+ messages in thread From: Hannes Reinecke @ 2019-07-19 13:49 UTC (permalink / raw) On 7/12/19 8:02 PM, Sagi Grimberg wrote: > If the controller supports discovery log page change events, > we want to enable it on the host side as well. > > Signed-off-by: Sagi Grimberg <sagi at grimberg.me> > --- > drivers/nvme/host/core.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > Reviewed-by: Hannes Reinecke <hare at suse.com> Cheers, Hannes -- Dr. Hannes Reinecke Teamlead Storage & Networking hare at suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg GF: Felix Imend?rffer, Mary Higgins, Sri Rasiah HRB 21284 (AG N?rnberg) ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 3/3] nvme: fire discovery log page change events to userspace 2019-07-12 18:02 [PATCH v2 0/3] Support discovery log change events Sagi Grimberg 2019-07-12 18:02 ` [PATCH v2 1/3] nvme-fabrics: allow discovery subsystems accept a kato Sagi Grimberg 2019-07-12 18:02 ` [PATCH v2 2/3] nvme: enable aen also for discovery controllers Sagi Grimberg @ 2019-07-12 18:02 ` Sagi Grimberg 2019-07-14 8:14 ` Minwoo Im ` (3 more replies) 2019-08-01 1:32 ` [PATCH v2 0/3] Support discovery log change events Sagi Grimberg 3 siblings, 4 replies; 23+ messages in thread From: Sagi Grimberg @ 2019-07-12 18:02 UTC (permalink / raw) Provide userspace with nvme discovery controller device instance, controller traddr and trsvcid. We'd expect userspace to handle this event by issuing a discovery + connect. Signed-off-by: Sagi Grimberg <sagi at grimberg.me> --- drivers/nvme/host/core.c | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 116c210826c2..76cd3dd8736a 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -1180,7 +1180,8 @@ int nvme_set_queue_count(struct nvme_ctrl *ctrl, int *count) EXPORT_SYMBOL_GPL(nvme_set_queue_count); #define NVME_AEN_SUPPORTED \ - (NVME_AEN_CFG_NS_ATTR | NVME_AEN_CFG_FW_ACT | NVME_AEN_CFG_ANA_CHANGE) + (NVME_AEN_CFG_NS_ATTR | NVME_AEN_CFG_FW_ACT | NVME_AEN_CFG_ANA_CHANGE | \ + NVME_AEN_CFG_DISC_CHANGE) static void nvme_enable_aen(struct nvme_ctrl *ctrl) { @@ -3612,6 +3613,30 @@ static void nvme_aen_uevent(struct nvme_ctrl *ctrl) kfree(envp[0]); } +static void nvme_disc_aen_uevent(struct nvme_ctrl *ctrl) +{ + struct nvmf_ctrl_options *opts = ctrl->opts; + char *envp[16]; + int i, envloc = 0; + + envp[envloc++] = kasprintf(GFP_KERNEL, "NVME_EVENT=discovery"); + envp[envloc++] = kasprintf(GFP_KERNEL, "NVME_CTRL_NAME=%s", + dev_name(ctrl->device)); + envp[envloc++] = kasprintf(GFP_KERNEL, "NVME_TRTYPE=%s", opts->transport); + envp[envloc++] = kasprintf(GFP_KERNEL, "NVME_TRADDR=%s", opts->traddr); + envp[envloc++] = kasprintf(GFP_KERNEL, "NVME_TRSVCID=%s", + opts->trsvcid ?: "none"); + envp[envloc++] = kasprintf(GFP_KERNEL, "NVME_HOST_TRADDR=%s", + opts->host_traddr ?: "none"); + envp[envloc] = NULL; + + for (i = 0; i < envloc; i++) + dev_dbg(ctrl->device, "%s\n", envp[i]); + kobject_uevent_env(&ctrl->device->kobj, KOBJ_CHANGE, envp); + for (i = 0; i < envloc; i++) + kfree(envp[i]); +} + static void nvme_async_event_work(struct work_struct *work) { struct nvme_ctrl *ctrl = @@ -3702,6 +3727,9 @@ static void nvme_handle_aen_notice(struct nvme_ctrl *ctrl, u32 result) queue_work(nvme_wq, &ctrl->ana_work); break; #endif + case NVME_AER_NOTICE_DISC_CHANGED: + nvme_disc_aen_uevent(ctrl); + break; default: dev_warn(ctrl->device, "async event result %08x\n", result); } -- 2.17.1 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v2 3/3] nvme: fire discovery log page change events to userspace 2019-07-12 18:02 ` [PATCH v2 3/3] nvme: fire discovery log page change events to userspace Sagi Grimberg @ 2019-07-14 8:14 ` Minwoo Im 2019-07-14 15:13 ` James Smart ` (2 subsequent siblings) 3 siblings, 0 replies; 23+ messages in thread From: Minwoo Im @ 2019-07-14 8:14 UTC (permalink / raw) On 19-07-12 11:02:11, Sagi Grimberg wrote: > Provide userspace with nvme discovery controller device instance, > controller traddr and trsvcid. We'd expect userspace to handle > this event by issuing a discovery + connect. > > Signed-off-by: Sagi Grimberg <sagi at grimberg.me> Sagi, Thanks for this patch! Reviewed-by: Minwoo Im <minwoo.im.dev at gmail.com> ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 3/3] nvme: fire discovery log page change events to userspace 2019-07-12 18:02 ` [PATCH v2 3/3] nvme: fire discovery log page change events to userspace Sagi Grimberg 2019-07-14 8:14 ` Minwoo Im @ 2019-07-14 15:13 ` James Smart 2019-07-19 13:49 ` Hannes Reinecke [not found] ` <20190822002328.GP9511@lst.de> 3 siblings, 0 replies; 23+ messages in thread From: James Smart @ 2019-07-14 15:13 UTC (permalink / raw) On 7/12/2019 11:02 AM, Sagi Grimberg wrote: > Provide userspace with nvme discovery controller device instance, > controller traddr and trsvcid. We'd expect userspace to handle > this event by issuing a discovery + connect. > > Signed-off-by: Sagi Grimberg <sagi at grimberg.me> > --- > drivers/nvme/host/core.c | 30 +++++++++++++++++++++++++++++- > 1 file changed, 29 insertions(+), 1 deletion(-) > > Reviewed-by: James Smart <james.smart at broadcom.com> ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 3/3] nvme: fire discovery log page change events to userspace 2019-07-12 18:02 ` [PATCH v2 3/3] nvme: fire discovery log page change events to userspace Sagi Grimberg 2019-07-14 8:14 ` Minwoo Im 2019-07-14 15:13 ` James Smart @ 2019-07-19 13:49 ` Hannes Reinecke [not found] ` <20190822002328.GP9511@lst.de> 3 siblings, 0 replies; 23+ messages in thread From: Hannes Reinecke @ 2019-07-19 13:49 UTC (permalink / raw) On 7/12/19 8:02 PM, Sagi Grimberg wrote: > Provide userspace with nvme discovery controller device instance, > controller traddr and trsvcid. We'd expect userspace to handle > this event by issuing a discovery + connect. > > Signed-off-by: Sagi Grimberg <sagi at grimberg.me> > --- > drivers/nvme/host/core.c | 30 +++++++++++++++++++++++++++++- > 1 file changed, 29 insertions(+), 1 deletion(-) > Reviewed-by: Hannes Reinecke <hare at suse.com> Cheers, Hannes -- Dr. Hannes Reinecke Teamlead Storage & Networking hare at suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg GF: Felix Imend?rffer, Mary Higgins, Sri Rasiah HRB 21284 (AG N?rnberg) ^ permalink raw reply [flat|nested] 23+ messages in thread
[parent not found: <20190822002328.GP9511@lst.de>]
[parent not found: <205d06ab-fedc-739d-323f-b358aff2cbfe@grimberg.me>]
[parent not found: <e4603511-6dae-e26d-12a9-e9fa727a8d03@grimberg.me>]
[parent not found: <20190826065639.GA11036@lst.de>]
[parent not found: <20190826075916.GA30396@kroah.com>]
[parent not found: <ac168168-fed2-2b57-493e-e88261ead73b@grimberg.me>]
[parent not found: <20190830055514.GC8492@lst.de>]
* Re: [PATCH v2 3/3] nvme: fire discovery log page change events to userspace [not found] ` <20190830055514.GC8492@lst.de> @ 2019-08-30 18:08 ` Sagi Grimberg 2019-08-30 18:36 ` James Smart 0 siblings, 1 reply; 23+ messages in thread From: Sagi Grimberg @ 2019-08-30 18:08 UTC (permalink / raw) To: Christoph Hellwig Cc: Greg Kroah-Hartman, James Smart, linux-nvme, linux-kernel, Keith Busch, Hannes Reinecke >>> Yes we do, userspace should use it to order events. Does udev not >>> handle that properly today? >> >> The problem is not ordering of events, its really about the fact that >> the chardev can be removed and reallocated for a different controller >> (could be a completely different discovery controller) by the time >> that userspace handles the event. > > The same is generally true for lot of kernel devices. We could reduce > the chance by using the idr cyclic allocator. Well, it was raised by Hannes and James, so I'll ask them respond here because I don't mind having it this way. I personally think that this is a better approach than having a cyclic idr allocator. In general, I don't necessarily think that this is a good idea to have cyclic controller enumerations if we don't absolutely have to... _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 3/3] nvme: fire discovery log page change events to userspace 2019-08-30 18:08 ` Sagi Grimberg @ 2019-08-30 18:36 ` James Smart 2019-08-30 21:07 ` Sagi Grimberg 0 siblings, 1 reply; 23+ messages in thread From: James Smart @ 2019-08-30 18:36 UTC (permalink / raw) To: Sagi Grimberg, Christoph Hellwig Cc: Keith Busch, Greg Kroah-Hartman, linux-kernel, linux-nvme, Hannes Reinecke On 8/30/2019 11:08 AM, Sagi Grimberg wrote: > >>>> Yes we do, userspace should use it to order events. Does udev not >>>> handle that properly today? >>> >>> The problem is not ordering of events, its really about the fact that >>> the chardev can be removed and reallocated for a different controller >>> (could be a completely different discovery controller) by the time >>> that userspace handles the event. >> >> The same is generally true for lot of kernel devices. We could reduce >> the chance by using the idr cyclic allocator. > > Well, it was raised by Hannes and James, so I'll ask them respond here > because I don't mind having it this way. I personally think that this > is a better approach than having a cyclic idr allocator. In general, I > don't necessarily think that this is a good idea to have cyclic > controller enumerations if we don't absolutely have to... We hit it right and left without the cyclic allocator, but that won't necessarily remove it. Perhaps we should have had a unique token assigned to the controller, and have the event pass the name and the token. The cli would then, if the token is present, validate it via an ioctl before proceeding with other ioctls. Where all the connection arguments were added we due to the reuse issue and then solving the question of how to verify and/or lookup the desired controller, by using the shotgun approach rather than being very pointed, which is what the name/token would do. -- james _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 3/3] nvme: fire discovery log page change events to userspace 2019-08-30 18:36 ` James Smart @ 2019-08-30 21:07 ` Sagi Grimberg 2019-08-30 22:24 ` James Smart 0 siblings, 1 reply; 23+ messages in thread From: Sagi Grimberg @ 2019-08-30 21:07 UTC (permalink / raw) To: James Smart, Christoph Hellwig Cc: Keith Busch, Greg Kroah-Hartman, linux-kernel, linux-nvme, Hannes Reinecke >>>>> Yes we do, userspace should use it to order events. Does udev not >>>>> handle that properly today? >>>> >>>> The problem is not ordering of events, its really about the fact that >>>> the chardev can be removed and reallocated for a different controller >>>> (could be a completely different discovery controller) by the time >>>> that userspace handles the event. >>> >>> The same is generally true for lot of kernel devices. We could reduce >>> the chance by using the idr cyclic allocator. >> >> Well, it was raised by Hannes and James, so I'll ask them respond here >> because I don't mind having it this way. I personally think that this >> is a better approach than having a cyclic idr allocator. In general, I >> don't necessarily think that this is a good idea to have cyclic >> controller enumerations if we don't absolutely have to... > > We hit it right and left without the cyclic allocator, but that won't > necessarily remove it. > > Perhaps we should have had a unique token assigned to the controller, > and have the event pass the name and the token. The cli would then, if > the token is present, validate it via an ioctl before proceeding with > other ioctls. > > Where all the connection arguments were added we due to the reuse issue > and then solving the question of how to verify and/or lookup the desired > controller, by using the shotgun approach rather than being very > pointed, which is what the name/token would do. This unique token is: trtype:traddr:trsvcid:host-traddr ... _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 3/3] nvme: fire discovery log page change events to userspace 2019-08-30 21:07 ` Sagi Grimberg @ 2019-08-30 22:24 ` James Smart 2019-09-09 15:10 ` Hannes Reinecke 0 siblings, 1 reply; 23+ messages in thread From: James Smart @ 2019-08-30 22:24 UTC (permalink / raw) To: Sagi Grimberg, Christoph Hellwig Cc: Keith Busch, Greg Kroah-Hartman, linux-kernel, linux-nvme, Hannes Reinecke On 8/30/2019 2:07 PM, Sagi Grimberg wrote: > >>>>>> Yes we do, userspace should use it to order events. Does udev not >>>>>> handle that properly today? >>>>> >>>>> The problem is not ordering of events, its really about the fact that >>>>> the chardev can be removed and reallocated for a different controller >>>>> (could be a completely different discovery controller) by the time >>>>> that userspace handles the event. >>>> >>>> The same is generally true for lot of kernel devices. We could reduce >>>> the chance by using the idr cyclic allocator. >>> >>> Well, it was raised by Hannes and James, so I'll ask them respond here >>> because I don't mind having it this way. I personally think that this >>> is a better approach than having a cyclic idr allocator. In general, I >>> don't necessarily think that this is a good idea to have cyclic >>> controller enumerations if we don't absolutely have to... >> >> We hit it right and left without the cyclic allocator, but that won't >> necessarily remove it. >> >> Perhaps we should have had a unique token assigned to the controller, >> and have the event pass the name and the token. The cli would then, >> if the token is present, validate it via an ioctl before proceeding >> with other ioctls. >> >> Where all the connection arguments were added we due to the reuse >> issue and then solving the question of how to verify and/or lookup >> the desired controller, by using the shotgun approach rather than >> being very pointed, which is what the name/token would do. > > This unique token is: trtype:traddr:trsvcid:host-traddr ... well yes :) though rather verbose. There is still a minute window as we're comparing values in sysfs, prior to opening the device, so technically something could change in that window between when we checked sysfs and when we open'd. We can certain check after we open the device to solve that issue. There is some elegance to a 32-bit token for the controller (can be an incrementing value) passed in the event and used when servicing the event that avoids a bunch of work. Doing either of these would eliminate what Hannes liked - looking for the discovery controller with those attributes. Although, I don't know that looking for it is all that meaningful. _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 3/3] nvme: fire discovery log page change events to userspace 2019-08-30 22:24 ` James Smart @ 2019-09-09 15:10 ` Hannes Reinecke 0 siblings, 0 replies; 23+ messages in thread From: Hannes Reinecke @ 2019-09-09 15:10 UTC (permalink / raw) To: James Smart, Sagi Grimberg, Christoph Hellwig Cc: Keith Busch, Greg Kroah-Hartman, linux-kernel, linux-nvme On 8/31/19 12:24 AM, James Smart wrote: > On 8/30/2019 2:07 PM, Sagi Grimberg wrote: >> >>>>>>> Yes we do, userspace should use it to order events. Does udev not >>>>>>> handle that properly today? >>>>>> >>>>>> The problem is not ordering of events, its really about the fact that >>>>>> the chardev can be removed and reallocated for a different controller >>>>>> (could be a completely different discovery controller) by the time >>>>>> that userspace handles the event. >>>>> >>>>> The same is generally true for lot of kernel devices. We could reduce >>>>> the chance by using the idr cyclic allocator. >>>> >>>> Well, it was raised by Hannes and James, so I'll ask them respond here >>>> because I don't mind having it this way. I personally think that this >>>> is a better approach than having a cyclic idr allocator. In general, I >>>> don't necessarily think that this is a good idea to have cyclic >>>> controller enumerations if we don't absolutely have to... >>> >>> We hit it right and left without the cyclic allocator, but that won't >>> necessarily remove it. >>> >>> Perhaps we should have had a unique token assigned to the controller, >>> and have the event pass the name and the token. The cli would then, >>> if the token is present, validate it via an ioctl before proceeding >>> with other ioctls. >>> >>> Where all the connection arguments were added we due to the reuse >>> issue and then solving the question of how to verify and/or lookup >>> the desired controller, by using the shotgun approach rather than >>> being very pointed, which is what the name/token would do. >> >> This unique token is: trtype:traddr:trsvcid:host-traddr ... > > well yes :) though rather verbose. There is still a minute window as > we're comparing values in sysfs, prior to opening the device, so > technically something could change in that window between when we > checked sysfs and when we open'd. We can certain check after we open > the device to solve that issue. > > There is some elegance to a 32-bit token for the controller (can be an > incrementing value) passed in the event and used when servicing the > event that avoids a bunch of work. > > Doing either of these would eliminate what Hannes liked - looking for > the discovery controller with those attributes. Although, I don't know > that looking for it is all that meaningful. > yeah, we do have this fundamental problem with uevents; they do refer to a '/dev/nvmeX' device with those parameters, but this device might be long gone (or have been reallocated) by the time the event is processed. From my POV we have two choices here: - rely on the transport options to find a matching controller (ignoring the device name) and use that for sending out discovery requests. In the end, it shouldn't really matter device we're using if the transport options are identical. Although I'm not sure for RDMA; here we don't necessarily have a host transport address, so we _might_ send the discovery via the wrong controller in a CMIC enviroment. - Match the options in nvme-cli, and just discard the event if it doesn't match. Which is some additional coding in nvme-cli and might ran afoul if we ever miss events. I'd go for the second option; after considering the first introduces far too many conditions rendering debugging impractical. 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] 23+ messages in thread
[parent not found: <20190830062036.GA15257@kroah.com>]
* Re: [PATCH v2 3/3] nvme: fire discovery log page change events to userspace [not found] ` <20190830062036.GA15257@kroah.com> @ 2019-08-30 18:14 ` Sagi Grimberg 2019-09-02 19:33 ` Greg Kroah-Hartman 0 siblings, 1 reply; 23+ messages in thread From: Sagi Grimberg @ 2019-08-30 18:14 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Keith Busch, linux-kernel, Christoph Hellwig, linux-nvme, James Smart >>>>>> You are correct that this information can be derived from sysfs, but the >>>>>> main reason why we add these here, is because in udev rule we can't >>>>>> just go ahead and start looking these up and parsing these.. >>>>>> >>>>>> We could send the discovery aen with NVME_CTRL_NAME and have >>>>>> then have systemd run something like: >>>>>> >>>>>> nvme connect-all -d nvme0 --sysfs >>>>>> >>>>>> and have nvme-cli retrieve all this stuff from sysfs? >>>>> >>>>> Actually that may be a problem. >>>>> >>>>> There could be a hypothetical case where after the event was fired >>>>> and before it was handled, the discovery controller went away and >>>>> came back again with a different controller instance, and the old >>>>> instance is now a different discovery controller. >>>>> >>>>> This is why we need this information in the event. And we verify this >>>>> information in sysfs in nvme-cli. >>>> >>>> Well, that must be a usual issue with uevents, right? Don't we usually >>>> have a increasing serial number for that or something? >>> >>> Yes we do, userspace should use it to order events. Does udev not >>> handle that properly today? >> >> The problem is not ordering of events, its really about the fact that >> the chardev can be removed and reallocated for a different controller >> (could be a completely different discovery controller) by the time >> that userspace handles the event. > > So? You will have gotten the remove and then new addition uevent in > order showing you this. So your userspace code knows that something > went away and then came back properly so you should be kept in sync. Still don't understand how this is ok... I have /dev/nvme0 represents a network endpoint that I would discover from, it is raising me an event to do a discovery operation (namely to issue an ioctl to it) so my udev code calls a systemd script. By the time I actually get to do that, /dev/nvme0 represents now a new network endpoint (where the event is no longer relevant to). I would rather the discovery to explicitly fail than to give me something different, so we pass some arguments that we verify in the operation. Its a stretch case, but it was raised by people as a potential issue. _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 3/3] nvme: fire discovery log page change events to userspace 2019-08-30 18:14 ` Sagi Grimberg @ 2019-09-02 19:33 ` Greg Kroah-Hartman 2019-09-04 1:35 ` Sagi Grimberg 0 siblings, 1 reply; 23+ messages in thread From: Greg Kroah-Hartman @ 2019-09-02 19:33 UTC (permalink / raw) To: Sagi Grimberg Cc: Keith Busch, linux-kernel, Christoph Hellwig, linux-nvme, James Smart On Fri, Aug 30, 2019 at 11:14:39AM -0700, Sagi Grimberg wrote: > > > > > > > > You are correct that this information can be derived from sysfs, but the > > > > > > > main reason why we add these here, is because in udev rule we can't > > > > > > > just go ahead and start looking these up and parsing these.. > > > > > > > > > > > > > > We could send the discovery aen with NVME_CTRL_NAME and have > > > > > > > then have systemd run something like: > > > > > > > > > > > > > > nvme connect-all -d nvme0 --sysfs > > > > > > > > > > > > > > and have nvme-cli retrieve all this stuff from sysfs? > > > > > > > > > > > > Actually that may be a problem. > > > > > > > > > > > > There could be a hypothetical case where after the event was fired > > > > > > and before it was handled, the discovery controller went away and > > > > > > came back again with a different controller instance, and the old > > > > > > instance is now a different discovery controller. > > > > > > > > > > > > This is why we need this information in the event. And we verify this > > > > > > information in sysfs in nvme-cli. > > > > > > > > > > Well, that must be a usual issue with uevents, right? Don't we usually > > > > > have a increasing serial number for that or something? > > > > > > > > Yes we do, userspace should use it to order events. Does udev not > > > > handle that properly today? > > > > > > The problem is not ordering of events, its really about the fact that > > > the chardev can be removed and reallocated for a different controller > > > (could be a completely different discovery controller) by the time > > > that userspace handles the event. > > > > So? You will have gotten the remove and then new addition uevent in > > order showing you this. So your userspace code knows that something > > went away and then came back properly so you should be kept in sync. > > Still don't understand how this is ok... > > I have /dev/nvme0 represents a network endpoint that I would discover > from, it is raising me an event to do a discovery operation (namely to > issue an ioctl to it) so my udev code calls a systemd script. > > By the time I actually get to do that, /dev/nvme0 represents now a new > network endpoint (where the event is no longer relevant to). I would > rather the discovery to explicitly fail than to give me something > different, so we pass some arguments that we verify in the operation. > > Its a stretch case, but it was raised by people as a potential issue. Ok, and how do you handle this same thing for something like /dev/sda ? (hint, it isn't new, and is something we solved over a decade ago) If you worry about stuff like this, use a persistant device naming scheme and have your device node be pointed to by a symlink. Create that symlink by using the information in the initial 'ADD' uevent. That way, when userspace opens the device node, it "knows" exactly which one it opens. It sounds like you have a bunch of metadata to describe these uniquely, so pass that in the ADD event, not in some other crazy non-standard manner. thanks, greg k-h _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 3/3] nvme: fire discovery log page change events to userspace 2019-09-02 19:33 ` Greg Kroah-Hartman @ 2019-09-04 1:35 ` Sagi Grimberg 2019-09-04 5:25 ` Greg Kroah-Hartman 0 siblings, 1 reply; 23+ messages in thread From: Sagi Grimberg @ 2019-09-04 1:35 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Keith Busch, linux-kernel, Christoph Hellwig, linux-nvme, James Smart >> Still don't understand how this is ok... >> >> I have /dev/nvme0 represents a network endpoint that I would discover >> from, it is raising me an event to do a discovery operation (namely to >> issue an ioctl to it) so my udev code calls a systemd script. >> >> By the time I actually get to do that, /dev/nvme0 represents now a new >> network endpoint (where the event is no longer relevant to). I would >> rather the discovery to explicitly fail than to give me something >> different, so we pass some arguments that we verify in the operation. >> >> Its a stretch case, but it was raised by people as a potential issue. > > Ok, and how do you handle this same thing for something like /dev/sda ? > (hint, it isn't new, and is something we solved over a decade ago) > > If you worry about stuff like this, use a persistant device naming > scheme and have your device node be pointed to by a symlink. Create > that symlink by using the information in the initial 'ADD' uevent. > > That way, when userspace opens the device node, it "knows" exactly which > one it opens. It sounds like you have a bunch of metadata to describe > these uniquely, so pass that in the ADD event, not in some other crazy > non-standard manner. We could send these variables when adding the device and then validating them using a rich-text-explanatory symlink. Seems slightly backwards to me, but that would work too. We create the char device using device_add (in nvme_init_subsystem), I didn't find any way to append env variables to that ADD uevent. Did you mean that we should add another flavor of device_add that accepts char *envp_ext[]? What exactly is the "standard manner" to pass these variables to the chardev KOBJ_ADD uevent? All other examples I could find use KOBJ_CHANGE to pass private stuff.. _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 3/3] nvme: fire discovery log page change events to userspace 2019-09-04 1:35 ` Sagi Grimberg @ 2019-09-04 5:25 ` Greg Kroah-Hartman 0 siblings, 0 replies; 23+ messages in thread From: Greg Kroah-Hartman @ 2019-09-04 5:25 UTC (permalink / raw) To: Sagi Grimberg Cc: Keith Busch, linux-kernel, Christoph Hellwig, linux-nvme, James Smart On Tue, Sep 03, 2019 at 06:35:30PM -0700, Sagi Grimberg wrote: > > > > Still don't understand how this is ok... > > > > > > I have /dev/nvme0 represents a network endpoint that I would discover > > > from, it is raising me an event to do a discovery operation (namely to > > > issue an ioctl to it) so my udev code calls a systemd script. > > > > > > By the time I actually get to do that, /dev/nvme0 represents now a new > > > network endpoint (where the event is no longer relevant to). I would > > > rather the discovery to explicitly fail than to give me something > > > different, so we pass some arguments that we verify in the operation. > > > > > > Its a stretch case, but it was raised by people as a potential issue. > > > > Ok, and how do you handle this same thing for something like /dev/sda ? > > (hint, it isn't new, and is something we solved over a decade ago) > > > > If you worry about stuff like this, use a persistant device naming > > scheme and have your device node be pointed to by a symlink. Create > > that symlink by using the information in the initial 'ADD' uevent. > > > > That way, when userspace opens the device node, it "knows" exactly which > > one it opens. It sounds like you have a bunch of metadata to describe > > these uniquely, so pass that in the ADD event, not in some other crazy > > non-standard manner. > > We could send these variables when adding the device and then validating > them using a rich-text-explanatory symlink. Seems slightly backwards to > me, but that would work too. That's the way the driver model is expected to work, instead of having to do crazy device-specific stuff. > We create the char device using device_add (in nvme_init_subsystem), > I didn't find any way to append env variables to that ADD uevent. You do that in your uevent or dev_uevent callback like all other subsystems. Nothing "new" to do here, again, it's been working fine for everyone else for well over a decade now :) thanks, greg k-h _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 0/3] Support discovery log change events 2019-07-12 18:02 [PATCH v2 0/3] Support discovery log change events Sagi Grimberg ` (2 preceding siblings ...) 2019-07-12 18:02 ` [PATCH v2 3/3] nvme: fire discovery log page change events to userspace Sagi Grimberg @ 2019-08-01 1:32 ` Sagi Grimberg 3 siblings, 0 replies; 23+ messages in thread From: Sagi Grimberg @ 2019-08-01 1:32 UTC (permalink / raw) > We want to be able to support discovery log change events automatically > without user intervention. > > The definition of discovery log change events applies on "persistent" long > lived controllers, so first we need to have discovery controllers to stay > for a long time and accept kato value. > > Then when we do happen to get a discovery log change event on the persistent > discovery controller, we simply fire a udev event to user-space to re-query > the discovery log page and connect to new subsystems in the fabric. > > This works with James' latest nvme-cli patches. James, I've applied this to nvme-5.4 Are you sending out the nvme-cli part? If you can make sure that it works with what is in nvme-5.4 it will be great.. Thanks ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2019-09-09 15:11 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-12 18:02 [PATCH v2 0/3] Support discovery log change events Sagi Grimberg 2019-07-12 18:02 ` [PATCH v2 1/3] nvme-fabrics: allow discovery subsystems accept a kato Sagi Grimberg 2019-07-14 8:08 ` Minwoo Im 2019-07-14 15:00 ` James Smart 2019-07-19 13:48 ` Hannes Reinecke 2019-07-12 18:02 ` [PATCH v2 2/3] nvme: enable aen also for discovery controllers Sagi Grimberg 2019-07-14 8:09 ` Minwoo Im 2019-07-14 15:04 ` James Smart 2019-07-19 13:49 ` Hannes Reinecke 2019-07-12 18:02 ` [PATCH v2 3/3] nvme: fire discovery log page change events to userspace Sagi Grimberg 2019-07-14 8:14 ` Minwoo Im 2019-07-14 15:13 ` James Smart 2019-07-19 13:49 ` Hannes Reinecke [not found] ` <20190822002328.GP9511@lst.de> [not found] ` <205d06ab-fedc-739d-323f-b358aff2cbfe@grimberg.me> [not found] ` <e4603511-6dae-e26d-12a9-e9fa727a8d03@grimberg.me> [not found] ` <20190826065639.GA11036@lst.de> [not found] ` <20190826075916.GA30396@kroah.com> [not found] ` <ac168168-fed2-2b57-493e-e88261ead73b@grimberg.me> [not found] ` <20190830055514.GC8492@lst.de> 2019-08-30 18:08 ` Sagi Grimberg 2019-08-30 18:36 ` James Smart 2019-08-30 21:07 ` Sagi Grimberg 2019-08-30 22:24 ` James Smart 2019-09-09 15:10 ` Hannes Reinecke [not found] ` <20190830062036.GA15257@kroah.com> 2019-08-30 18:14 ` Sagi Grimberg 2019-09-02 19:33 ` Greg Kroah-Hartman 2019-09-04 1:35 ` Sagi Grimberg 2019-09-04 5:25 ` Greg Kroah-Hartman 2019-08-01 1:32 ` [PATCH v2 0/3] Support discovery log change events Sagi Grimberg
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).