[PATCH 0/3] nvmet: unique discovery subsystem

[PATCH 0/3] nvmet: unique discovery subsystem

[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

[PATCH 1/3] nvmet: check for subsystem type in nvmet_find_get_subsys()

[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

[PATCH 2/3] nvmet: make the subsystem type configurable

[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

[PATCH 3/3] nvmet: include all configured ports in discovery log page for unique discover controller

[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

Re: [PATCH 3/3] nvmet: include all configured ports in discovery log page for unique discover controller

[Hannes Reinecke]

On 4/5/22 17:09, John Meneghini wrote:
> On 4/5/22 03:31, Christoph Hellwig wrote:
>> 2) (optionally) make the well known discovery controller go away
>>
>> Just renaming the subsystem will make the well known one go away,
>> but while that can be useful for some use cases, I think making it
>> always go away has the potential for breakage.  So maybe we need
>> to not rename but be able to create an alias and have a way to
>> disable the well known discovery controller if not used.
> 
> I agree. I think making the well known discovery controller "go away" 
> will have huge problems in a multi-vendor fabric. We really can't touch 
> the well known discovery controller because there are too many legacy 
> and non-TP-8010 compliant NVMe hosts on the fabric.
> 
> We have to assume that both new and old implementations will coexist. 
> And legacy hosts will NEVER look at the new, unique discovery NQN 
> because they won't even recognize the new NVME_NQN_CURR log page entries.
> 
It doesn't 'go away'.
The original, static, configfs entry will be unused, true.

But the unique discovery subsystem will _still_ react to the default 
discovery NQN, and any controller will continue to get information when 
using the default discovery NQN.

And even older implementations will continue to work _if_ they just skip 
unknown entries.

Cheers,

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

Re: [PATCH 3/3] nvmet: include all configured ports in discovery log page for unique discover controller

[John Meneghini]

On 4/5/22 03:31, Christoph Hellwig wrote:
> 2) (optionally) make the well known discovery controller go away
> 
> Just renaming the subsystem will make the well known one go away,
> but while that can be useful for some use cases, I think making it
> always go away has the potential for breakage.  So maybe we need
> to not rename but be able to create an alias and have a way to
> disable the well known discovery controller if not used.

I agree. I think making the well known discovery controller "go away" will have huge problems in a multi-vendor fabric. We 
really can't touch the well known discovery controller because there are too many legacy and non-TP-8010 compliant NVMe hosts on 
the fabric.

We have to assume that both new and old implementations will coexist. And legacy hosts will NEVER look at the new, unique 
discovery NQN because they won't even recognize the new NVME_NQN_CURR log page entries.

/John

Re: [PATCH 3/3] nvmet: include all configured ports in discovery log page for unique discover controller

[Sagi Grimberg]

>>>>> To make it configurable.
>>>>> Unique discovery controllers show up in configfs just like any other
>>>>> subsystems.
>>>>> And with that we need to clarify the relationship between the 
>>>>> discovery
>>>>> subsystem and the other subsystems, ie which subsystems should be 
>>>>> presented
>>>>> by this discovery subsystem.
>>>>>
>>>>> Linking the discovery subsystem into a given port makes it obvious 
>>>>> that
>>>>> a) this port will be presenting a discovery subsystem
>>>>> and
>>>>> b) that the discovery subsystem will be presenting all subsystems
>>>>> configured on that port.
>>>>>
>>>>> The built-in mechanism for discovery subsystems was okay as long as 
>>>>> the
>>>>> discovery subsystem was built-in, too.
>>>>> But with this patchset we're moving to an explicit configuration.
>>>>
>>>> Shouldn't we just require anything to be manually listed for this
>>>> case similar to how we configure referrals for the well known
>>>> discovery controller?
>>>
>>> Which is what I've tried with this attempt.
>>> I did _not_ want to create a new configuration mechanism, but rather 
>>> use the existing ones.
>>> And the existing mechanism we have is linking subsystems to ports.
>>>
>>> If we want to treat discovery subsystems differently (as you 
>>> proposed) we sure can have a different mechanism on how to configure it.
>>> But I wasn't sure if that's the direction we want to go.
>>
>> What is the concern here? that it will break existing users with
>> introducing an additional configuration step?
> 
> Yes. It would break backward compability.

So maybe hide it behind a modparam that we can deprecate in the future?

Re: [PATCH 3/3] nvmet: include all configured ports in discovery log page for unique discover controller

[Hannes Reinecke]

On 4/5/22 12:41, Sagi Grimberg wrote:
> 
>>>> To make it configurable.
>>>> Unique discovery controllers show up in configfs just like any other
>>>> subsystems.
>>>> And with that we need to clarify the relationship between the discovery
>>>> subsystem and the other subsystems, ie which subsystems should be 
>>>> presented
>>>> by this discovery subsystem.
>>>>
>>>> Linking the discovery subsystem into a given port makes it obvious that
>>>> a) this port will be presenting a discovery subsystem
>>>> and
>>>> b) that the discovery subsystem will be presenting all subsystems
>>>> configured on that port.
>>>>
>>>> The built-in mechanism for discovery subsystems was okay as long as the
>>>> discovery subsystem was built-in, too.
>>>> But with this patchset we're moving to an explicit configuration.
>>>
>>> Shouldn't we just require anything to be manually listed for this
>>> case similar to how we configure referrals for the well known
>>> discovery controller?
>>
>> Which is what I've tried with this attempt.
>> I did _not_ want to create a new configuration mechanism, but rather 
>> use the existing ones.
>> And the existing mechanism we have is linking subsystems to ports.
>>
>> If we want to treat discovery subsystems differently (as you proposed) 
>> we sure can have a different mechanism on how to configure it.
>> But I wasn't sure if that's the direction we want to go.
> 
> What is the concern here? that it will break existing users with
> introducing an additional configuration step?

Yes. It would break backward compability.

Cheers,

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

Re: [PATCH 3/3] nvmet: include all configured ports in discovery log page for unique discover controller

[Sagi Grimberg]

>>> To make it configurable.
>>> Unique discovery controllers show up in configfs just like any other
>>> subsystems.
>>> And with that we need to clarify the relationship between the discovery
>>> subsystem and the other subsystems, ie which subsystems should be 
>>> presented
>>> by this discovery subsystem.
>>>
>>> Linking the discovery subsystem into a given port makes it obvious that
>>> a) this port will be presenting a discovery subsystem
>>> and
>>> b) that the discovery subsystem will be presenting all subsystems
>>> configured on that port.
>>>
>>> The built-in mechanism for discovery subsystems was okay as long as the
>>> discovery subsystem was built-in, too.
>>> But with this patchset we're moving to an explicit configuration.
>>
>> Shouldn't we just require anything to be manually listed for this
>> case similar to how we configure referrals for the well known
>> discovery controller?
> 
> Which is what I've tried with this attempt.
> I did _not_ want to create a new configuration mechanism, but rather use 
> the existing ones.
> And the existing mechanism we have is linking subsystems to ports.
> 
> If we want to treat discovery subsystems differently (as you proposed) 
> we sure can have a different mechanism on how to configure it.
> But I wasn't sure if that's the direction we want to go.

What is the concern here? that it will break existing users with
introducing an additional configuration step?

Re: [PATCH 3/3] nvmet: include all configured ports in discovery log page for unique discover controller

[Sagi Grimberg]

> So maybe we'll take a step and think about what we actually want
> to support.  Here is my understanding, which might as well be incorrect
> and/or incomplete.
> 
>   1) support a uniqueue discovery controller for bidirectional
>      authentication to work properly
> 
> The only thing we really need is to have the existing discovery log for
> the well known discovery subsystems to be able to also show up as a
> unique discovery subsystem.  Either by just renaming it like your earlier
> patch did, or by supporting an alternative name in addition to that.

I think that making the discovery subsystem show up in configfs makes
sense, it can be attached to port(s) just like any other subsystem.

> 
>   2) (optionally) make the well known discovery controller go away
> 
> Just renaming the subsystem will make the well known one go away,
> but while that can be useful for some use cases, I think making it
> always go away has the potential for breakage.  So maybe we need
> to not rename but be able to create an alias and have a way to
> disable the well known discovery controller if not used.

agreed.

> 
>   3) Actually have a different content.
> 
> Can you explain the use cases here a bit more?
> 
> And maybe add anything else we'd like to consider.  (Everyone else
> on the list please also chime in)

I don't think we should support multiple discovery subsystems on
the same target. Don't know why would anyone need it. I also think
that reporting subsystems that are attached to all ports is just a
a routing failure waiting to happen. But if anyone really really
wants it, we can keep a flag on the discovery subsystem to report
on all ports if explicitly set.

Re: [PATCH 3/3] nvmet: include all configured ports in discovery log page for unique discover controller

[Christoph Hellwig]

So maybe we'll take a step and think about what we actually want
to support.  Here is my understanding, which might as well be incorrect
and/or incomplete.

 1) support a uniqueue discovery controller for bidirectional
    authentication to work properly

The only thing we really need is to have the existing discovery log for
the well known discovery subsystems to be able to also show up as a
unique discovery subsystem.  Either by just renaming it like your earlier
patch did, or by supporting an alternative name in addition to that.

 2) (optionally) make the well known discovery controller go away

Just renaming the subsystem will make the well known one go away,
but while that can be useful for some use cases, I think making it
always go away has the potential for breakage.  So maybe we need
to not rename but be able to create an alias and have a way to
disable the well known discovery controller if not used.

 3) Actually have a different content.

Can you explain the use cases here a bit more?

And maybe add anything else we'd like to consider.  (Everyone else
on the list please also chime in)

Re: [PATCH 3/3] nvmet: include all configured ports in discovery log page for unique discover controller

[Hannes Reinecke]

On 4/5/22 08:19, Christoph Hellwig wrote:
> On Tue, Apr 05, 2022 at 08:15:51AM +0200, Hannes Reinecke wrote:
>> To make it configurable.
>> Unique discovery controllers show up in configfs just like any other
>> subsystems.
>> And with that we need to clarify the relationship between the discovery
>> subsystem and the other subsystems, ie which subsystems should be presented
>> by this discovery subsystem.
>>
>> Linking the discovery subsystem into a given port makes it obvious that
>> a) this port will be presenting a discovery subsystem
>> and
>> b) that the discovery subsystem will be presenting all subsystems
>> configured on that port.
>>
>> The built-in mechanism for discovery subsystems was okay as long as the
>> discovery subsystem was built-in, too.
>> But with this patchset we're moving to an explicit configuration.
> 
> Shouldn't we just require anything to be manually listed for this
> case similar to how we configure referrals for the well known
> discovery controller?

Which is what I've tried with this attempt.
I did _not_ want to create a new configuration mechanism, but rather use 
the existing ones.
And the existing mechanism we have is linking subsystems to ports.

If we want to treat discovery subsystems differently (as you proposed) 
we sure can have a different mechanism on how to configure it.
But I wasn't sure if that's the direction we want to go.

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

Re: [PATCH 3/3] nvmet: include all configured ports in discovery log page for unique discover controller

[Christoph Hellwig]

On Tue, Apr 05, 2022 at 08:15:51AM +0200, Hannes Reinecke wrote:
> To make it configurable.
> Unique discovery controllers show up in configfs just like any other 
> subsystems.
> And with that we need to clarify the relationship between the discovery 
> subsystem and the other subsystems, ie which subsystems should be presented 
> by this discovery subsystem.
>
> Linking the discovery subsystem into a given port makes it obvious that
> a) this port will be presenting a discovery subsystem
> and
> b) that the discovery subsystem will be presenting all subsystems 
> configured on that port.
>
> The built-in mechanism for discovery subsystems was okay as long as the 
> discovery subsystem was built-in, too.
> But with this patchset we're moving to an explicit configuration.

Shouldn't we just require anything to be manually listed for this
case similar to how we configure referrals for the well known
discovery controller?

Re: [PATCH 3/3] nvmet: include all configured ports in discovery log page for unique discover controller

[Hannes Reinecke]

On 4/5/22 07:46, Christoph Hellwig wrote:
> On Thu, Mar 17, 2022 at 03:26:34PM +0100, Hannes Reinecke wrote:
>> 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.
> 
> Why?

To make it configurable.
Unique discovery controllers show up in configfs just like any other 
subsystems.
And with that we need to clarify the relationship between the discovery 
subsystem and the other subsystems, ie which subsystems should be 
presented by this discovery subsystem.

Linking the discovery subsystem into a given port makes it obvious that
a) this port will be presenting a discovery subsystem
and
b) that the discovery subsystem will be presenting all subsystems 
configured on that port.

The built-in mechanism for discovery subsystems was okay as long as the 
discovery subsystem was built-in, too.
But with this patchset we're moving to an explicit configuration.

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

Re: [PATCH 3/3] nvmet: include all configured ports in discovery log page for unique discover controller

[Christoph Hellwig]

On Thu, Mar 17, 2022 at 03:26:34PM +0100, Hannes Reinecke wrote:
> 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.

Why?

[PATCH 3/3] nvmet: include all configured ports in discovery log page for unique discover controller

[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 | 63 +++++++++++++++++++++++----------
 1 file changed, 45 insertions(+), 18 deletions(-)

diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c
index bceeec00099a..d69e6ba3fb22 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,38 @@ 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(r, nvmet_ports, global_entry) {
+			nvmet_set_disc_traddr(req, r, traddr);
+
+			list_for_each_entry(p, &r->subsystems, entry) {
+				if (!nvmet_host_allowed(p->subsys,
+							ctrl->hostnqn))
+					continue;
+				nvmet_format_discovery_entry(hdr, r,
+						p->subsys->subsysnqn, traddr,
+						p->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