[PATCHv2 0/3] nvmet: unique discovery subsystem

[PATCHv2 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, 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

[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

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

[Christoph Hellwig]

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.

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

[Hannes Reinecke]

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

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

[Christoph Hellwig]

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.

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

[John Meneghini]

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

[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

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

[Christoph Hellwig]

On Thu, Mar 17, 2022 at 03:26:33PM +0100, Hannes Reinecke wrote:
> Make the subsystem type configurable to allow for unique
> discovery subsystems by changing the subsystem type to
> 'discovery'.

Does it make sense to change the type?  Or should we have a new top-level
interface to create and configure discovery subsystems from the start?

If we had e.g.

/sys/kernel/config/nvmet/discovery-subsystems/

we could start that out with a pre-filled well known discovery subsystem
but also allow disabling that for example if we really want.

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

[Hannes Reinecke]

On 4/5/22 07:47, Christoph Hellwig wrote:
> On Thu, Mar 17, 2022 at 03:26:33PM +0100, Hannes Reinecke wrote:
>> Make the subsystem type configurable to allow for unique
>> discovery subsystems by changing the subsystem type to
>> 'discovery'.
> 
> Does it make sense to change the type?  Or should we have a new top-level
> interface to create and configure discovery subsystems from the start?
> 
> If we had e.g.
> 
> /sys/kernel/config/nvmet/discovery-subsystems/
> 
> we could start that out with a pre-filled well known discovery subsystem
> but also allow disabling that for example if we really want.

I actually looked into that with my previous attempt, and really found 
no good way of having an entry which is
a) pre-filled
and
b) removable by the admin later on.
 From what I've seen all pre-filled entries are static; I haven't found 
a way to do pre-filled dynamic entries.

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 2/3] nvmet: make the subsystem type configurable

[Christoph Hellwig]

On Tue, Apr 05, 2022 at 08:00:25AM +0200, Hannes Reinecke wrote:
> I actually looked into that with my previous attempt, and really found no 
> good way of having an entry which is
> a) pre-filled
> and
> b) removable by the admin later on.
> From what I've seen all pre-filled entries are static; I haven't found a 
> way to do pre-filled dynamic entries.

I don't think we need to remove the well known discovery subsystem. But
we can allow removing all entries from it and thus reporting an empty
log.

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

[Hannes Reinecke]

On 4/5/22 08:09, Christoph Hellwig wrote:
> On Tue, Apr 05, 2022 at 08:00:25AM +0200, Hannes Reinecke wrote:
>> I actually looked into that with my previous attempt, and really found no
>> good way of having an entry which is
>> a) pre-filled
>> and
>> b) removable by the admin later on.
>>  From what I've seen all pre-filled entries are static; I haven't found a
>> way to do pre-filled dynamic entries.
> 
> I don't think we need to remove the well known discovery subsystem. But
> we can allow removing all entries from it and thus reporting an empty
> log.

Hmm.
We still would need to pre-fill these entries when creating subsystems.
Bringing us back to square one, namely there is no way to create dynamic 
pre-filled entries.

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 2/3] nvmet: make the subsystem type configurable

[Sagi Grimberg]

>>> I actually looked into that with my previous attempt, and really 
>>> found no
>>> good way of having an entry which is
>>> a) pre-filled
>>> and
>>> b) removable by the admin later on.
>>>  From what I've seen all pre-filled entries are static; I haven't 
>>> found a
>>> way to do pre-filled dynamic entries.
>>
>> I don't think we need to remove the well known discovery subsystem. But
>> we can allow removing all entries from it and thus reporting an empty
>> log.
> 
> Hmm.
> We still would need to pre-fill these entries when creating subsystems.
> Bringing us back to square one, namely there is no way to create dynamic 
> pre-filled entries.

Why do we need to remove it altogether? Why not just make these
subsystems attach to port(s) like any other subsystem?

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

[Hannes Reinecke]

On 4/5/22 12:35, Sagi Grimberg wrote:
> 
>>>> I actually looked into that with my previous attempt, and really 
>>>> found no
>>>> good way of having an entry which is
>>>> a) pre-filled
>>>> and
>>>> b) removable by the admin later on.
>>>>  From what I've seen all pre-filled entries are static; I haven't 
>>>> found a
>>>> way to do pre-filled dynamic entries.
>>>
>>> I don't think we need to remove the well known discovery subsystem. But
>>> we can allow removing all entries from it and thus reporting an empty
>>> log.
>>
>> Hmm.
>> We still would need to pre-fill these entries when creating subsystems.
>> Bringing us back to square one, namely there is no way to create 
>> dynamic pre-filled entries.
> 
> Why do we need to remove it altogether? Why not just make these
> subsystems attach to port(s) like any other subsystem?

That's what I tried in my first attempt, which got rejected as it does 
break existing installations.
We could introduce a module option for this (and I think that's what I 
did in my first round).
And yes, ideally I would like to have the discovery subsystem as a 
'normal' subsystem just like the others; should be trivial to expose the 
current one in configfs.
But then changing the subsystem NQN becomes awkward:
- Either we create a new subsystem with the unique NQN, but then we have 
to change the type later on (as creation happens with a simple 'mkdir', 
and one cannot really pass arguments to that).
- Or we have a distinct discovery subsystem type (ie the idea Christoph 
mentioned), but then we have to cross-check the directory names against 
the existing one in the 'normal' subsystem directory.
And, of course, we still end up with a defunct discovery subsystem if we 
create a unique discovery subsystem.

Alternatively: if we're going to break existing configurations anyway 
(or, rather, have to modify nvmetcli to handle the new discovery 
mechanism), why do we need to start off with a default discovery subsystem?
Can't we require the user to create one?
We would be requiring a module option here, but that looks like the best 
approach here as we won't have to deal with stale subsystems ...

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 2/3] nvmet: make the subsystem type configurable

[John Meneghini]

On 4/5/22 07:12, Hannes Reinecke wrote:
> That's what I tried in my first attempt, which got rejected as it does break existing installations.
> We could introduce a module option for this (and I think that's what I did in my first round).
> And yes, ideally I would like to have the discovery subsystem as a 'normal' subsystem just like the others; should be trivial to 
> expose the current one in configfs.
> But then changing the subsystem NQN becomes awkward: > - Either we create a new subsystem with the unique NQN, but then we have to change the type later on (as creation happens with a
 > simple 'mkdir', and one cannot really pass arguments to that).
 > - Or we have a distinct discovery subsystem type (ie the idea Christoph mentioned), but then we have to cross-check the
 > directory names against the existing one in the 'normal' subsystem directory.

I don't think we need to change the discovery subsystem.  I think what we want, in the simple case, is 2 discovery subsystems: 
1) the existing well known discovery controller with the well-known discovery NQN, 2) a unique discovery controller with a 
unique discovery NQN.

The first well known discovery controller exists today and doesn't need to be configurable (unless you're going to do something 
stupid like support fabric authenticate with the well known discovery NQN).

The second unique discovery controller can be configured just like any other NVM subsystem, but its discovery log pages show up 
in the well known discovery controller log page with Subsystem Type (SUBTYPE): 03h - as defined by TP-8014.

So the well known discovery controller always returns log page entries for three types of log pages:

NVME_NQN_DISC,	"referral"
NVME_NQN_NVME,	"nvme"
NVME_NQN_CURR,	"discovery"

When there is no unique discovery controller configured the NVME_NQN_CURR log page has no entries and everything works as it 
does today.

When someone configures a unique discovery NQN, the controller is created and all of the NVME_NQN_NVME log page entries move
to the new unique discovery controller while the NVME_NQN_CURR log page entries on the well-known discovery controller report 
only the NVME_NQN_CURR and NVME_NQN_DISC log page entires.

This is one possible approach.

> And, of course, we still end up with a defunct discovery subsystem if we create a unique discovery subsystem.

Why do we have to defunct the discovery subsystem if we create a unique discover subsystem?

> Alternatively: if we're going to break existing configurations anyway (or, rather, have to modify nvmetcli to handle the new 
> discovery mechanism), why do we need to start off with a default discovery subsystem?

> Can't we require the user to create one?

I would say yes. The user has to create the unique discovery controller and assign it a unique NQN

> We would be requiring a module option here, but that looks like the best approach here as we won't have to deal with stale 
> subsystems ...

I don't understand why there needs to be any stale subsystems.  If we add a optional subsystem (the unique discovery controller) 
we don't need to take anything away.

/John

> Cheers,
> 
> Hannes

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

[John Meneghini]

On 4/5/22 02:09, Christoph Hellwig wrote:
> 
> I don't think we need to remove the well known discovery subsystem. But
> we can allow removing all entries from it and thus reporting an empty
> log.
> 

I agree. This was exactly my vision for how things should work with TP-8013 and TP-8014

/John

[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

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?

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 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 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]

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

[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

[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

[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

[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

[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?
> 
> Yes. It would break backward compability.

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

[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 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