* [PATCH 0/3] nvmet: unique discovery subsystem @ 2022-03-17 13:18 Hannes Reinecke 2022-03-17 13:18 ` [PATCH 1/3] nvmet: check for subsystem type in nvmet_find_get_subsys() Hannes Reinecke ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Hannes Reinecke @ 2022-03-17 13:18 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Sagi Grimberg, Keith Busch, linux-nvme, Hannes Reinecke Hi all, here's my next attempt to support unique discovery subsystems. The main idea is to make the subsystem type configurable; if it's being set to 'discovery' it'll replace the static discovery subsystem. The admin then need to configure the subsystem as normal by linking it into the ports where the discovery subsystem should be visible. And the discovery log then includes all configured ports to all configured subsystems on the port providing the discovery controller. As usual, comments and reviews are welcome. Hannes Reinecke (3): nvmet: check for subsystem type in nvmet_find_get_subsys() nvmet: make the subsystem type configurable nvmet: include all configured ports in discovery log page for unique discover controller drivers/nvme/target/configfs.c | 65 ++++++++++++++++++++++++ drivers/nvme/target/core.c | 23 ++++++--- drivers/nvme/target/discovery.c | 89 +++++++++++++++++++++++++-------- drivers/nvme/target/nvmet.h | 3 ++ 4 files changed, 154 insertions(+), 26 deletions(-) -- 2.29.2 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/3] nvmet: check for subsystem type in nvmet_find_get_subsys() 2022-03-17 13:18 [PATCH 0/3] nvmet: unique discovery subsystem Hannes Reinecke @ 2022-03-17 13:18 ` Hannes Reinecke 2022-03-17 13:18 ` [PATCH 2/3] nvmet: make the subsystem type configurable Hannes Reinecke 2022-03-17 13:18 ` [PATCH 3/3] nvmet: include all configured ports in discovery log page for unique discover controller Hannes Reinecke 2 siblings, 0 replies; 9+ messages in thread From: Hannes Reinecke @ 2022-03-17 13:18 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Sagi Grimberg, Keith Busch, linux-nvme, Hannes Reinecke When looking for a subsystem we have two ways of finding the subsystem: either looking for the subsystem NQN itself, or check the type of the subsystem when looking for a discovery controller. This patch implements this check, and also moves the check for the static discovery controller to the end such that we can return unique discovery controllers. Signed-off-by: Hannes Reinecke <hare@suse.de> --- drivers/nvme/target/core.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c index b0dc6230d1b9..83eba511d098 100644 --- a/drivers/nvme/target/core.c +++ b/drivers/nvme/target/core.c @@ -1508,12 +1508,6 @@ static struct nvmet_subsys *nvmet_find_get_subsys(struct nvmet_port *port, if (!port) return NULL; - if (!strcmp(NVME_DISC_SUBSYS_NAME, subsysnqn)) { - if (!kref_get_unless_zero(&nvmet_disc_subsys->ref)) - return NULL; - return nvmet_disc_subsys; - } - down_read(&nvmet_config_sem); list_for_each_entry(p, &port->subsystems, entry) { if (!strncmp(p->subsys->subsysnqn, subsysnqn, @@ -1523,8 +1517,22 @@ static struct nvmet_subsys *nvmet_find_get_subsys(struct nvmet_port *port, up_read(&nvmet_config_sem); return p->subsys; } + if (!strcmp(NVME_DISC_SUBSYS_NAME, subsysnqn) && + nvmet_is_disc_subsys(p->subsys)) { + if (!kref_get_unless_zero(&p->subsys->ref)) + break; + up_read(&nvmet_config_sem); + return p->subsys; + } } up_read(&nvmet_config_sem); + + if (!strcmp(NVME_DISC_SUBSYS_NAME, subsysnqn)) { + if (!kref_get_unless_zero(&nvmet_disc_subsys->ref)) + return NULL; + return nvmet_disc_subsys; + } + return NULL; } -- 2.29.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/3] nvmet: make the subsystem type configurable 2022-03-17 13:18 [PATCH 0/3] nvmet: unique discovery subsystem Hannes Reinecke 2022-03-17 13:18 ` [PATCH 1/3] nvmet: check for subsystem type in nvmet_find_get_subsys() Hannes Reinecke @ 2022-03-17 13:18 ` Hannes Reinecke 2022-03-17 13:18 ` [PATCH 3/3] nvmet: include all configured ports in discovery log page for unique discover controller Hannes Reinecke 2 siblings, 0 replies; 9+ messages in thread From: Hannes Reinecke @ 2022-03-17 13:18 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Sagi Grimberg, Keith Busch, linux-nvme, Hannes Reinecke Make the subsystem type configurable to allow for unique discovery subsystems by changing the subsystem type to 'discovery'. Signed-off-by: Hannes Reinecke <hare@suse.de> --- drivers/nvme/target/configfs.c | 65 +++++++++++++++++++++++++++++++++ drivers/nvme/target/core.c | 3 ++ drivers/nvme/target/discovery.c | 20 +++++++++- drivers/nvme/target/nvmet.h | 3 ++ 4 files changed, 89 insertions(+), 2 deletions(-) diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c index 57a6795a2d8d..ae6815b72d7d 100644 --- a/drivers/nvme/target/configfs.c +++ b/drivers/nvme/target/configfs.c @@ -930,6 +930,9 @@ static void nvmet_port_subsys_drop_link(struct config_item *parent, found: list_del(&p->entry); + /* Reset to static discovery controller */ + if (nvmet_disc_subsys == subsys) + nvmet_set_disc_subsys(NULL); nvmet_port_del_ctrls(port, subsys); nvmet_port_disc_changed(port, subsys); @@ -1302,6 +1305,67 @@ static ssize_t nvmet_subsys_attr_model_store(struct config_item *item, } CONFIGFS_ATTR(nvmet_subsys_, attr_model); +static const struct nvmet_type_name_map nvmet_subsys_type_map[] = { + { NVME_NQN_DISC, "referral" }, + { NVME_NQN_NVME, "nvme" }, + { NVME_NQN_CURR, "discovery" }, +}; + +static ssize_t nvmet_subsys_attr_type_show(struct config_item *item, + char *page) +{ + u8 type = to_subsys(item)->type; + int i; + + for (i = 1; i < ARRAY_SIZE(nvmet_subsys_type_map); i++) { + if (nvmet_subsys_type_map[i].type == type) + return snprintf(page, PAGE_SIZE, "%s\n", + nvmet_subsys_type_map[i].name); + } + + return snprintf(page, PAGE_SIZE, "\n"); +} + +static ssize_t nvmet_subsys_attr_type_store(struct config_item *item, + const char *page, size_t count) +{ + struct nvmet_subsys *subsys = to_subsys(item); + int i; + + if (subsys->subsys_discovered) + return -EACCES; + + for (i = 0; i < ARRAY_SIZE(nvmet_subsys_type_map); i++) { + if (sysfs_streq(page, nvmet_subsys_type_map[i].name)) + goto found; + } + + pr_err("Invalid value '%s' for subsystem type\n", page); + return -EINVAL; + +found: + down_write(&nvmet_config_sem); + if (nvmet_subsys_type_map[i].type == NVME_NQN_CURR) { + if (nvmet_has_unique_disc_subsys()) { + pr_err("unique discovery subsystem already present\n"); + return -EINVAL; + } + if (!xa_empty(&subsys->namespaces)) { + pr_err("discovery subsystem cannot have namespaces\n"); + return -EINVAL; + } + nvmet_subsys_del_ctrls(nvmet_disc_subsys); + nvmet_set_disc_subsys(subsys); + } else if (subsys->type == NVME_NQN_CURR) + nvmet_set_disc_subsys(NULL); + + subsys->type = nvmet_subsys_type_map[i].type; + up_write(&nvmet_config_sem); + + return count; +} +CONFIGFS_ATTR(nvmet_subsys_, attr_type); + #ifdef CONFIG_BLK_DEV_INTEGRITY static ssize_t nvmet_subsys_attr_pi_enable_show(struct config_item *item, char *page) @@ -1331,6 +1395,7 @@ static struct configfs_attribute *nvmet_subsys_attrs[] = { &nvmet_subsys_attr_attr_cntlid_min, &nvmet_subsys_attr_attr_cntlid_max, &nvmet_subsys_attr_attr_model, + &nvmet_subsys_attr_attr_type, #ifdef CONFIG_BLK_DEV_INTEGRITY &nvmet_subsys_attr_attr_pi_enable, #endif diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c index 83eba511d098..a9d03dfe547e 100644 --- a/drivers/nvme/target/core.c +++ b/drivers/nvme/target/core.c @@ -1527,6 +1527,9 @@ static struct nvmet_subsys *nvmet_find_get_subsys(struct nvmet_port *port, } up_read(&nvmet_config_sem); + if (nvmet_has_unique_disc_subsys()) + return NULL; + if (!strcmp(NVME_DISC_SUBSYS_NAME, subsysnqn)) { if (!kref_get_unless_zero(&nvmet_disc_subsys->ref)) return NULL; diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c index c2162eef8ce1..bceeec00099a 100644 --- a/drivers/nvme/target/discovery.c +++ b/drivers/nvme/target/discovery.c @@ -9,9 +9,23 @@ #include "nvmet.h" struct nvmet_subsys *nvmet_disc_subsys; +static struct nvmet_subsys *_nvmet_disc_subsys; static u64 nvmet_genctr; +bool nvmet_has_unique_disc_subsys(void) +{ + return (_nvmet_disc_subsys != nvmet_disc_subsys); +} + +void nvmet_set_disc_subsys(struct nvmet_subsys *subsys) +{ + if (subsys) + nvmet_disc_subsys = subsys; + else + nvmet_disc_subsys = _nvmet_disc_subsys; +} + static void __nvmet_disc_changed(struct nvmet_port *port, struct nvmet_ctrl *ctrl) { @@ -393,12 +407,14 @@ u16 nvmet_parse_discovery_cmd(struct nvmet_req *req) int __init nvmet_init_discovery(void) { - nvmet_disc_subsys = + _nvmet_disc_subsys = nvmet_subsys_alloc(NVME_DISC_SUBSYS_NAME, NVME_NQN_CURR); + nvmet_disc_subsys = _nvmet_disc_subsys; return PTR_ERR_OR_ZERO(nvmet_disc_subsys); } void nvmet_exit_discovery(void) { - nvmet_subsys_put(nvmet_disc_subsys); + nvmet_disc_subsys = NULL; + nvmet_subsys_put(_nvmet_disc_subsys); } diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h index 81061aa8c6d3..658e3fd81f06 100644 --- a/drivers/nvme/target/nvmet.h +++ b/drivers/nvme/target/nvmet.h @@ -556,6 +556,9 @@ void nvmet_exit_discovery(void); extern struct nvmet_subsys *nvmet_disc_subsys; extern struct rw_semaphore nvmet_config_sem; +bool nvmet_has_unique_disc_subsys(void); +void nvmet_set_disc_subsys(struct nvmet_subsys *subsys); + extern u32 nvmet_ana_group_enabled[NVMET_MAX_ANAGRPS + 1]; extern u64 nvmet_ana_chgcnt; extern struct rw_semaphore nvmet_ana_sem; -- 2.29.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/3] nvmet: include all configured ports in discovery log page for unique discover controller 2022-03-17 13:18 [PATCH 0/3] nvmet: unique discovery subsystem Hannes Reinecke 2022-03-17 13:18 ` [PATCH 1/3] nvmet: check for subsystem type in nvmet_find_get_subsys() Hannes Reinecke 2022-03-17 13:18 ` [PATCH 2/3] nvmet: make the subsystem type configurable Hannes Reinecke @ 2022-03-17 13:18 ` Hannes Reinecke 2 siblings, 0 replies; 9+ messages in thread From: Hannes Reinecke @ 2022-03-17 13:18 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Sagi Grimberg, Keith Busch, linux-nvme, Hannes Reinecke When an unique discovery controller is configured we should be reporting all configured ports, and not just those which are reachable from the current port. Signed-off-by: Hannes Reinecke <hare@suse.de> --- drivers/nvme/target/discovery.c | 69 ++++++++++++++++++++++++--------- 1 file changed, 51 insertions(+), 18 deletions(-) diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c index bceeec00099a..6cefd4f60f9f 100644 --- a/drivers/nvme/target/discovery.c +++ b/drivers/nvme/target/discovery.c @@ -160,12 +160,24 @@ static size_t discovery_log_entries(struct nvmet_req *req) struct nvmet_ctrl *ctrl = req->sq->ctrl; struct nvmet_subsys_link *p; struct nvmet_port *r; - size_t entries = 1; - - list_for_each_entry(p, &req->port->subsystems, entry) { - if (!nvmet_host_allowed(p->subsys, ctrl->hostnqn)) - continue; + size_t entries = 0; + + if (nvmet_has_unique_disc_subsys()) { + list_for_each_entry(r, nvmet_ports, global_entry) { + list_for_each_entry(p, &r->subsystems, entry) { + if (!nvmet_host_allowed(p->subsys, + ctrl->hostnqn)) + continue; + entries++; + } + } + } else { entries++; + list_for_each_entry(p, &req->port->subsystems, entry) { + if (!nvmet_host_allowed(p->subsys, ctrl->hostnqn)) + continue; + entries++; + } } list_for_each_entry(r, &req->port->referrals, entry) entries++; @@ -220,23 +232,44 @@ static void nvmet_execute_disc_get_log_page(struct nvmet_req *req) } hdr = buffer; - nvmet_set_disc_traddr(req, req->port, traddr); - - nvmet_format_discovery_entry(hdr, req->port, - nvmet_disc_subsys->subsysnqn, - traddr, NVME_NQN_CURR, numrec); - numrec++; - - list_for_each_entry(p, &req->port->subsystems, entry) { - if (!nvmet_host_allowed(p->subsys, ctrl->hostnqn)) - continue; + if (!nvmet_has_unique_disc_subsys()) { + nvmet_set_disc_traddr(req, req->port, traddr); nvmet_format_discovery_entry(hdr, req->port, - p->subsys->subsysnqn, traddr, - NVME_NQN_NVME, numrec); + nvmet_disc_subsys->subsysnqn, + traddr, NVME_NQN_CURR, numrec); numrec++; - } + list_for_each_entry(p, &req->port->subsystems, entry) { + if (!nvmet_host_allowed(p->subsys, ctrl->hostnqn)) + continue; + + nvmet_format_discovery_entry(hdr, req->port, + p->subsys->subsysnqn, traddr, + NVME_NQN_NVME, numrec); + numrec++; + } + } else { + list_for_each_entry(p, &req->port->subsystems, entry) { + list_for_each_entry(r, nvmet_ports, global_entry) { + struct nvmet_subsys_link *l; + + nvmet_set_disc_traddr(req, r, traddr); + + list_for_each_entry(l, &r->subsystems, entry) { + if (p->subsys != l->subsys) + continue; + if (!nvmet_host_allowed(l->subsys, + ctrl->hostnqn)) + continue; + nvmet_format_discovery_entry(hdr, r, + l->subsys->subsysnqn, traddr, + l->subsys->type, numrec); + numrec++; + } + } + } + } list_for_each_entry(r, &req->port->referrals, entry) { nvmet_format_discovery_entry(hdr, r, NVME_DISC_SUBSYS_NAME, -- 2.29.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCHv2 0/3] nvmet: unique discovery subsystem @ 2022-03-17 14:26 Hannes Reinecke 2022-03-17 14:26 ` [PATCH 1/3] nvmet: check for subsystem type in nvmet_find_get_subsys() Hannes Reinecke 0 siblings, 1 reply; 9+ messages in thread From: Hannes Reinecke @ 2022-03-17 14:26 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Sagi Grimberg, Keith Busch, linux-nvme, Hannes Reinecke Hi all, here's my next attempt to support unique discovery subsystems. The main idea is to make the subsystem type configurable; if it's being set to 'discovery' it'll replace the static discovery subsystem. The admin then need to configure the subsystem as normal by linking it into the ports where the discovery subsystem should be visible. And the discovery log then includes all configured ports to all configured subsystems, as usual modified by whether the host may access this subsystem or not. As usual, comments and reviews are welcome. Changes to the original submission: - Include all subsystems in the discovery log output Hannes Reinecke (3): nvmet: check for subsystem type in nvmet_find_get_subsys() nvmet: make the subsystem type configurable nvmet: include all configured ports in discovery log page for unique discover controller drivers/nvme/target/configfs.c | 65 ++++++++++++++++++++++++++ drivers/nvme/target/core.c | 23 ++++++--- drivers/nvme/target/discovery.c | 83 +++++++++++++++++++++++++-------- drivers/nvme/target/nvmet.h | 3 ++ 4 files changed, 148 insertions(+), 26 deletions(-) -- 2.29.2 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/3] nvmet: check for subsystem type in nvmet_find_get_subsys() 2022-03-17 14:26 [PATCHv2 0/3] nvmet: unique discovery subsystem Hannes Reinecke @ 2022-03-17 14:26 ` Hannes Reinecke 2022-04-05 5:45 ` Christoph Hellwig 0 siblings, 1 reply; 9+ messages in thread From: Hannes Reinecke @ 2022-03-17 14:26 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Sagi Grimberg, Keith Busch, linux-nvme, Hannes Reinecke When looking for a subsystem we have two ways of finding the subsystem: either looking for the subsystem NQN itself, or check the type of the subsystem when looking for a discovery controller. This patch implements this check, and also moves the check for the static discovery controller to the end such that we can return unique discovery controllers. Signed-off-by: Hannes Reinecke <hare@suse.de> --- drivers/nvme/target/core.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c index b0dc6230d1b9..83eba511d098 100644 --- a/drivers/nvme/target/core.c +++ b/drivers/nvme/target/core.c @@ -1508,12 +1508,6 @@ static struct nvmet_subsys *nvmet_find_get_subsys(struct nvmet_port *port, if (!port) return NULL; - if (!strcmp(NVME_DISC_SUBSYS_NAME, subsysnqn)) { - if (!kref_get_unless_zero(&nvmet_disc_subsys->ref)) - return NULL; - return nvmet_disc_subsys; - } - down_read(&nvmet_config_sem); list_for_each_entry(p, &port->subsystems, entry) { if (!strncmp(p->subsys->subsysnqn, subsysnqn, @@ -1523,8 +1517,22 @@ static struct nvmet_subsys *nvmet_find_get_subsys(struct nvmet_port *port, up_read(&nvmet_config_sem); return p->subsys; } + if (!strcmp(NVME_DISC_SUBSYS_NAME, subsysnqn) && + nvmet_is_disc_subsys(p->subsys)) { + if (!kref_get_unless_zero(&p->subsys->ref)) + break; + up_read(&nvmet_config_sem); + return p->subsys; + } } up_read(&nvmet_config_sem); + + if (!strcmp(NVME_DISC_SUBSYS_NAME, subsysnqn)) { + if (!kref_get_unless_zero(&nvmet_disc_subsys->ref)) + return NULL; + return nvmet_disc_subsys; + } + return NULL; } -- 2.29.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] nvmet: check for subsystem type in nvmet_find_get_subsys() 2022-03-17 14:26 ` [PATCH 1/3] nvmet: check for subsystem type in nvmet_find_get_subsys() Hannes Reinecke @ 2022-04-05 5:45 ` Christoph Hellwig 2022-04-05 5:53 ` Hannes Reinecke 0 siblings, 1 reply; 9+ messages in thread From: Christoph Hellwig @ 2022-04-05 5:45 UTC (permalink / raw) To: Hannes Reinecke; +Cc: Christoph Hellwig, Sagi Grimberg, Keith Busch, linux-nvme On Thu, Mar 17, 2022 at 03:26:32PM +0100, Hannes Reinecke wrote: > When looking for a subsystem we have two ways of finding the > subsystem: either looking for the subsystem NQN itself, or check > the type of the subsystem when looking for a discovery controller. > This patch implements this check, and also moves the check for > the static discovery controller to the end such that we can > return unique discovery controllers. > > Signed-off-by: Hannes Reinecke <hare@suse.de> > --- > drivers/nvme/target/core.c | 20 ++++++++++++++------ > 1 file changed, 14 insertions(+), 6 deletions(-) > > diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c > index b0dc6230d1b9..83eba511d098 100644 > --- a/drivers/nvme/target/core.c > +++ b/drivers/nvme/target/core.c > @@ -1508,12 +1508,6 @@ static struct nvmet_subsys *nvmet_find_get_subsys(struct nvmet_port *port, > if (!port) > return NULL; > > - if (!strcmp(NVME_DISC_SUBSYS_NAME, subsysnqn)) { > - if (!kref_get_unless_zero(&nvmet_disc_subsys->ref)) > - return NULL; > - return nvmet_disc_subsys; > - } > - > down_read(&nvmet_config_sem); > list_for_each_entry(p, &port->subsystems, entry) { > if (!strncmp(p->subsys->subsysnqn, subsysnqn, > @@ -1523,8 +1517,22 @@ static struct nvmet_subsys *nvmet_find_get_subsys(struct nvmet_port *port, > up_read(&nvmet_config_sem); > return p->subsys; > } > + if (!strcmp(NVME_DISC_SUBSYS_NAME, subsysnqn) && > + nvmet_is_disc_subsys(p->subsys)) { I don't get this. The strcmp is on the passed in subqnen that we are looking for. So with this patch we now do the fairly expensive string compare for every subsystem in the loop, and the just retuns the first discovery subsystems when we look for the well known discovery subsystem. As-is there is only one, so this is pointless. If we add discovery subsystems with different names (as I assume the later patches do), this changes beahvior in a rather unexpected way. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] nvmet: check for subsystem type in nvmet_find_get_subsys() 2022-04-05 5:45 ` Christoph Hellwig @ 2022-04-05 5:53 ` Hannes Reinecke 2022-04-05 6:07 ` Christoph Hellwig 2022-04-05 13:06 ` John Meneghini 0 siblings, 2 replies; 9+ messages in thread From: Hannes Reinecke @ 2022-04-05 5:53 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Sagi Grimberg, Keith Busch, linux-nvme On 4/5/22 07:45, Christoph Hellwig wrote: > > > On Thu, Mar 17, 2022 at 03:26:32PM +0100, Hannes Reinecke wrote: >> When looking for a subsystem we have two ways of finding the >> subsystem: either looking for the subsystem NQN itself, or check >> the type of the subsystem when looking for a discovery controller. >> This patch implements this check, and also moves the check for >> the static discovery controller to the end such that we can >> return unique discovery controllers. >> >> Signed-off-by: Hannes Reinecke <hare@suse.de> >> --- >> drivers/nvme/target/core.c | 20 ++++++++++++++------ >> 1 file changed, 14 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c >> index b0dc6230d1b9..83eba511d098 100644 >> --- a/drivers/nvme/target/core.c >> +++ b/drivers/nvme/target/core.c >> @@ -1508,12 +1508,6 @@ static struct nvmet_subsys *nvmet_find_get_subsys(struct nvmet_port *port, >> if (!port) >> return NULL; >> >> - if (!strcmp(NVME_DISC_SUBSYS_NAME, subsysnqn)) { >> - if (!kref_get_unless_zero(&nvmet_disc_subsys->ref)) >> - return NULL; >> - return nvmet_disc_subsys; >> - } >> - >> down_read(&nvmet_config_sem); >> list_for_each_entry(p, &port->subsystems, entry) { >> if (!strncmp(p->subsys->subsysnqn, subsysnqn, >> @@ -1523,8 +1517,22 @@ static struct nvmet_subsys *nvmet_find_get_subsys(struct nvmet_port *port, >> up_read(&nvmet_config_sem); >> return p->subsys; >> } >> + if (!strcmp(NVME_DISC_SUBSYS_NAME, subsysnqn) && >> + nvmet_is_disc_subsys(p->subsys)) { > > I don't get this. The strcmp is on the passed in subqnen that we are > looking for. So with this patch we now do the fairly expensive string > compare for every subsystem in the loop, and the just retuns the first > discovery subsystems when we look for the well known discovery > subsystem. As-is there is only one, so this is pointless. If we add > discovery subsystems with different names (as I assume the later patches > do), this changes beahvior in a rather unexpected way. Will have to check; might be that this is a left-over from the previous attempt of handling discovery subsystems just like any other subsystem. However, this is just part of a larger patchset to support unique discovery controller NQNs. And the overarching question here is: Do we want to support unique discovery controller NQNs in nvmet? Previously there was a rather strict policy of implementing only the bare necessities in nvmet, and unique discovery controller NQNs is arguably not a necessary thing. So the whole discussion gets pretty pointless if we decide _not_ to support it after all. Cheers, Hannes -- Dr. Hannes Reinecke Kernel Storage Architect hare@suse.de +49 911 74053 688 SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] nvmet: check for subsystem type in nvmet_find_get_subsys() 2022-04-05 5:53 ` Hannes Reinecke @ 2022-04-05 6:07 ` Christoph Hellwig 2022-04-05 13:06 ` John Meneghini 1 sibling, 0 replies; 9+ messages in thread From: Christoph Hellwig @ 2022-04-05 6:07 UTC (permalink / raw) To: Hannes Reinecke; +Cc: Christoph Hellwig, Sagi Grimberg, Keith Busch, linux-nvme On Tue, Apr 05, 2022 at 07:53:43AM +0200, Hannes Reinecke wrote: > Do we want to support unique discovery controller NQNs in nvmet? > > Previously there was a rather strict policy of implementing only the bare > necessities in nvmet, and unique discovery controller NQNs is arguably not > a necessary thing. I don't think there has ever been a policy of only bare necessities. But more one of checking if features are useful for nvmet. Given that we basically need unique discovery controllers to usefull support authentication it seems like we have to support them, even if the feature on its down does not look all that useful. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] nvmet: check for subsystem type in nvmet_find_get_subsys() 2022-04-05 5:53 ` Hannes Reinecke 2022-04-05 6:07 ` Christoph Hellwig @ 2022-04-05 13:06 ` John Meneghini 1 sibling, 0 replies; 9+ messages in thread From: John Meneghini @ 2022-04-05 13:06 UTC (permalink / raw) To: Hannes Reinecke, Christoph Hellwig; +Cc: Sagi Grimberg, Keith Busch, linux-nvme On 4/5/22 01:53, Hannes Reinecke wrote: > > Will have to check; might be that this is a left-over from the previous attempt of handling discovery subsystems just like any > other subsystem. > > However, this is just part of a larger patchset to support unique discovery controller NQNs. And the overarching question here is: > > Do we want to support unique discovery controller NQNs in nvmet? > > Previously there was a rather strict policy of implementing only the bare necessities in nvmet, and unique discovery controller > NQNs is arguably not a necessary thing. I think the question is: do we want to support unique discovery controller NQNs at all? > So the whole discussion gets pretty pointless if we decide _not_ to support it after all. I'm really concerned about the idea of having NO support for unique discovery controller NQNs in nvmet. If Linux is going to support unique discovery NQNs in nvme/host then I think we need to have support for unique discovery NQNs in nvme/target. Without this, there is just no way to test anything. I want the ability to test NVMe protocol features like this w/out having to rely on external hardware, if possible. /John > Cheers, > > Hannes ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-04-05 13:07 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-03-17 13:18 [PATCH 0/3] nvmet: unique discovery subsystem Hannes Reinecke 2022-03-17 13:18 ` [PATCH 1/3] nvmet: check for subsystem type in nvmet_find_get_subsys() Hannes Reinecke 2022-03-17 13:18 ` [PATCH 2/3] nvmet: make the subsystem type configurable Hannes Reinecke 2022-03-17 13:18 ` [PATCH 3/3] nvmet: include all configured ports in discovery log page for unique discover controller Hannes Reinecke 2022-03-17 14:26 [PATCHv2 0/3] nvmet: unique discovery subsystem Hannes Reinecke 2022-03-17 14:26 ` [PATCH 1/3] nvmet: check for subsystem type in nvmet_find_get_subsys() Hannes Reinecke 2022-04-05 5:45 ` Christoph Hellwig 2022-04-05 5:53 ` Hannes Reinecke 2022-04-05 6:07 ` Christoph Hellwig 2022-04-05 13:06 ` John Meneghini
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.