[PATCHv4 0/3] nvmet: unique discovery subsystems

[PATCHv4 0/3] nvmet: unique discovery subsystems

[Hannes Reinecke]

Hi all,

here's my next attempt to support unique discovery subsystems, again heavily
modified from the previous attempts.

As per suggestion from Christoph this patchset allows to have a per-port
discovery subsystem. For that a normal NVMe subsystem needs to be created
via configfs, the type needs to be changed to 'discovery', and then linked
into the port where this discovery subsystem should be visible.

Once that is done the discovery log page output will include all ports
into which the same discovery controller is linked.

If the discovery subsystem is unlinked the default behaviour is reinstated.

As usual, comments and reviews are welcome.

Changes to v3:
- Rework to use per-port discovery subsystems as suggested by hch

Changes to v2:
- Heavily rework after discussion on the mailing list

Changes to the original submission:
- Include all subsystems in the discovery log output

Hannes Reinecke (3):
  nvmet: make the subsystem type configurable
  nvmet: per-port discovery subsystem
  nvmet: include all configured ports in the discovery log page

 drivers/nvme/target/configfs.c  | 82 +++++++++++++++++++++++++++++++++
 drivers/nvme/target/core.c      |  6 +--
 drivers/nvme/target/discovery.c | 82 ++++++++++++++++++++++++---------
 drivers/nvme/target/nvmet.h     |  1 +
 4 files changed, 145 insertions(+), 26 deletions(-)

-- 
2.29.2

[PATCH 1/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  | 71 +++++++++++++++++++++++++++++++++
 drivers/nvme/target/discovery.c |  2 +-
 2 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index e44b2988759e..07c2d563d11b 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -1234,6 +1234,76 @@ 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);
+	struct nvmet_port *p;
+	struct nvmet_subsys_link *s;
+	int i;
+
+	if (subsys->subsys_discovered)
+		return -EACCES;
+
+	/*
+	 * Do not allow to change the subsystem type if it's
+	 * already linked to ports; the user should unlink it first.
+	 */
+	down_read(&nvmet_config_sem);
+	list_for_each_entry(p, &nvmet_ports_list, global_entry) {
+		list_for_each_entry(s, &p->subsystems, entry) {
+			if (s->subsys == subsys) {
+				up_read(&nvmet_config_sem);
+				return -EACCES;
+			}
+		}
+	}
+	up_read(&nvmet_config_sem);
+
+	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 (!xa_empty(&subsys->namespaces)) {
+			pr_err("discovery subsystem cannot have namespaces\n");
+			return -EINVAL;
+		}
+	}
+	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)
@@ -1263,6 +1333,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/discovery.c b/drivers/nvme/target/discovery.c
index c2162eef8ce1..b5012df790d5 100644
--- a/drivers/nvme/target/discovery.c
+++ b/drivers/nvme/target/discovery.c
@@ -219,7 +219,7 @@ static void nvmet_execute_disc_get_log_page(struct nvmet_req *req)
 
 		nvmet_format_discovery_entry(hdr, req->port,
 				p->subsys->subsysnqn, traddr,
-				NVME_NQN_NVME, numrec);
+				p->subsys->type, numrec);
 		numrec++;
 	}
 
-- 
2.29.2

[PATCH 2/3] nvmet: per-port discovery subsystem

[Hannes Reinecke]

Add a 'disc_subsys' pointer to each port to specify which discovery
subsystem to use.
The pointer is set when a discovery subsystem is linked into a port,
and reset to the original, built-in discovery subsystem if that link
is removed.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/target/configfs.c  | 11 +++++++++++
 drivers/nvme/target/core.c      |  6 +++---
 drivers/nvme/target/discovery.c | 32 +++++++++++++++++++-------------
 drivers/nvme/target/nvmet.h     |  1 +
 4 files changed, 34 insertions(+), 16 deletions(-)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 07c2d563d11b..a3af14e687f2 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -821,6 +821,12 @@ static int nvmet_port_subsys_allow_link(struct config_item *parent,
 	link->subsys = subsys;
 
 	down_write(&nvmet_config_sem);
+	if (subsys->type == NVME_NQN_CURR &&
+	    port->disc_subsys != nvmet_disc_subsys) {
+		pr_err("discovery subsystem already present\n");
+		ret = -EAGAIN;
+		goto out_free_link;
+	}
 	ret = -EEXIST;
 	list_for_each_entry(p, &port->subsystems, entry) {
 		if (p->subsys == subsys)
@@ -834,6 +840,8 @@ static int nvmet_port_subsys_allow_link(struct config_item *parent,
 	}
 
 	list_add_tail(&link->entry, &port->subsystems);
+	if (subsys->type == NVME_NQN_CURR)
+		port->disc_subsys = subsys;
 	nvmet_port_disc_changed(port, subsys);
 
 	up_write(&nvmet_config_sem);
@@ -864,6 +872,8 @@ static void nvmet_port_subsys_drop_link(struct config_item *parent,
 	list_del(&p->entry);
 	nvmet_port_del_ctrls(port, subsys);
 	nvmet_port_disc_changed(port, subsys);
+	if (port->disc_subsys == subsys)
+		port->disc_subsys = nvmet_disc_subsys;
 
 	if (list_empty(&port->subsystems))
 		nvmet_disable_port(port);
@@ -1690,6 +1700,7 @@ static struct config_group *nvmet_ports_make(struct config_group *group,
 	INIT_LIST_HEAD(&port->subsystems);
 	INIT_LIST_HEAD(&port->referrals);
 	port->inline_data_size = -1;	/* < 0 == let the transport choose */
+	port->disc_subsys = nvmet_disc_subsys;
 
 	port->disc_addr.portid = cpu_to_le16(portid);
 	port->disc_addr.adrfam = NVMF_ADDR_FAMILY_MAX;
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 90e75324dae0..cb69ca04c6c7 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1496,9 +1496,9 @@ static struct nvmet_subsys *nvmet_find_get_subsys(struct nvmet_port *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;
+		if (!kref_get_unless_zero(&port->disc_subsys->ref))
+				return NULL;
+		return port->disc_subsys;
 	}
 
 	down_read(&nvmet_config_sem);
diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c
index b5012df790d5..e3d8cc407a94 100644
--- a/drivers/nvme/target/discovery.c
+++ b/drivers/nvme/target/discovery.c
@@ -29,18 +29,19 @@ void nvmet_port_disc_changed(struct nvmet_port *port,
 			     struct nvmet_subsys *subsys)
 {
 	struct nvmet_ctrl *ctrl;
+	struct nvmet_subsys *disc_subsys = port->disc_subsys;
 
 	lockdep_assert_held(&nvmet_config_sem);
 	nvmet_genctr++;
 
-	mutex_lock(&nvmet_disc_subsys->lock);
-	list_for_each_entry(ctrl, &nvmet_disc_subsys->ctrls, subsys_entry) {
+	mutex_lock(&disc_subsys->lock);
+	list_for_each_entry(ctrl, &disc_subsys->ctrls, subsys_entry) {
 		if (subsys && !nvmet_host_allowed(subsys, ctrl->hostnqn))
 			continue;
 
 		__nvmet_disc_changed(port, ctrl);
 	}
-	mutex_unlock(&nvmet_disc_subsys->lock);
+	mutex_unlock(&disc_subsys->lock);
 
 	/* If transport can signal change, notify transport */
 	if (port->tr_ops && port->tr_ops->discovery_chg)
@@ -48,19 +49,19 @@ void nvmet_port_disc_changed(struct nvmet_port *port,
 }
 
 static void __nvmet_subsys_disc_changed(struct nvmet_port *port,
-					struct nvmet_subsys *subsys,
 					struct nvmet_host *host)
 {
 	struct nvmet_ctrl *ctrl;
+	struct nvmet_subsys *disc_subsys = port->disc_subsys;
 
-	mutex_lock(&nvmet_disc_subsys->lock);
-	list_for_each_entry(ctrl, &nvmet_disc_subsys->ctrls, subsys_entry) {
+	mutex_lock(&disc_subsys->lock);
+	list_for_each_entry(ctrl, &disc_subsys->ctrls, subsys_entry) {
 		if (host && strcmp(nvmet_host_name(host), ctrl->hostnqn))
 			continue;
 
 		__nvmet_disc_changed(port, ctrl);
 	}
-	mutex_unlock(&nvmet_disc_subsys->lock);
+	mutex_unlock(&disc_subsys->lock);
 }
 
 void nvmet_subsys_disc_changed(struct nvmet_subsys *subsys,
@@ -76,7 +77,7 @@ void nvmet_subsys_disc_changed(struct nvmet_subsys *subsys,
 		list_for_each_entry(s, &port->subsystems, entry) {
 			if (s->subsys != subsys)
 				continue;
-			__nvmet_subsys_disc_changed(port, subsys, host);
+			__nvmet_subsys_disc_changed(port, host);
 		}
 }
 
@@ -146,7 +147,10 @@ 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;
+	size_t entries = 0;
+
+	if (req->port->disc_subsys == nvmet_disc_subsys)
+		entries++;
 
 	list_for_each_entry(p, &req->port->subsystems, entry) {
 		if (!nvmet_host_allowed(p->subsys, ctrl->hostnqn))
@@ -208,10 +212,12 @@ static void nvmet_execute_disc_get_log_page(struct nvmet_req *req)
 
 	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++;
+	if (req->port->disc_subsys == nvmet_disc_subsys) {
+		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))
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 69818752a33a..e9a2f3257195 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -144,6 +144,7 @@ struct nvmet_port {
 	struct list_head		global_entry;
 	struct config_group		ana_groups_group;
 	struct nvmet_ana_group		ana_default_group;
+	struct nvmet_subsys		*disc_subsys;
 	enum nvme_ana_state		*ana_state;
 	void				*priv;
 	bool				enabled;
-- 
2.29.2

[PATCH 3/3] nvmet: include all configured ports in the discovery log page

[Hannes Reinecke]

When a per-port discovery subsystem is used we should include
all configured ports in the discovery log page, not just that one
through which the controller was connected.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/target/discovery.c | 52 ++++++++++++++++++++++++++-------
 1 file changed, 41 insertions(+), 11 deletions(-)

diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c
index e3d8cc407a94..6d414096d09a 100644
--- a/drivers/nvme/target/discovery.c
+++ b/drivers/nvme/target/discovery.c
@@ -152,10 +152,19 @@ static size_t discovery_log_entries(struct nvmet_req *req)
 	if (req->port->disc_subsys == nvmet_disc_subsys)
 		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, nvmet_ports, global_entry) {
+		if (req->port->disc_subsys == nvmet_disc_subsys) {
+			if (r != req->port)
+				continue;
+		} else {
+			if (ctrl->subsys != r->disc_subsys)
+				continue;
+		}
+		list_for_each_entry(p, &r->subsystems, entry) {
+			if (!nvmet_host_allowed(p->subsys, ctrl->hostnqn))
+				continue;
+			entries++;
+		}
 	}
 	list_for_each_entry(r, &req->port->referrals, entry)
 		entries++;
@@ -219,14 +228,35 @@ static void nvmet_execute_disc_get_log_page(struct nvmet_req *req)
 		numrec++;
 	}
 
-	list_for_each_entry(p, &req->port->subsystems, entry) {
-		if (!nvmet_host_allowed(p->subsys, ctrl->hostnqn))
-			continue;
+	list_for_each_entry(r, nvmet_ports, global_entry) {
+		nvmet_set_disc_traddr(req, r, traddr);
 
-		nvmet_format_discovery_entry(hdr, req->port,
-				p->subsys->subsysnqn, traddr,
-				p->subsys->type, numrec);
-		numrec++;
+		if (req->port->disc_subsys == nvmet_disc_subsys) {
+			/*
+			 * Present information about the port to which
+			 * the controller is connected.
+			 */
+			if (r != req->port)
+				continue;
+		} else {
+			/*
+			 * If a per-port discovery subsystem is used present
+			 * information about all ports into which the
+			 * same discovery subsystem is linked.
+			 */
+			if (ctrl->subsys != r->disc_subsys)
+				continue;
+		}
+
+		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) {
-- 
2.29.2

Re: [PATCH 3/3] nvmet: include all configured ports in the discovery log page

[Sagi Grimberg]

> When a per-port discovery subsystem is used we should include
> all configured ports in the discovery log page, not just that one
> through which the controller was connected.

Why?

Re: [PATCH 3/3] nvmet: include all configured ports in the discovery log page

[Hannes Reinecke]

On 4/7/22 15:29, Sagi Grimberg wrote:
> 
>> When a per-port discovery subsystem is used we should include
>> all configured ports in the discovery log page, not just that one
>> through which the controller was connected.
> 
> Why?
> 
Because that's the whole point if it being configurable.

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 the discovery log page

[Christoph Hellwig]

On Thu, Apr 07, 2022 at 03:31:51PM +0200, Hannes Reinecke wrote:
> On 4/7/22 15:29, Sagi Grimberg wrote:
>>
>>> When a per-port discovery subsystem is used we should include
>>> all configured ports in the discovery log page, not just that one
>>> through which the controller was connected.
>>
>> Why?
>>
> Because that's the whole point if it being configurable.

I'm really confused about this.  Please try to explain "the whole point"
a bit better.

Re: [PATCH 3/3] nvmet: include all configured ports in the discovery log page

[Sagi Grimberg]

>>> When a per-port discovery subsystem is used we should include
>>> all configured ports in the discovery log page, not just that one
>>> through which the controller was connected.
>>
>> Why?
>>
> Because that's the whole point if it being configurable.

Maybe I'm missing something. It is one thing to allow configurable
discovery NQNs, it is another thing to have discovery subsystems that
are producing a log page about all connected ports.

I suggest that if we want to have discovery subsystems that will
produce entries on different ports, the log page content needs to be
programmed explicitly in some form. Doing that is going to produce
routing issues for sure (subnets, vlans, etc etc etc will cause hosts
to get log entries for subsystems they can't connect to).

Re: [PATCH 3/3] nvmet: include all configured ports in the discovery log page

[Hannes Reinecke]

On 4/7/22 15:38, Sagi Grimberg wrote:
> 
>>>> When a per-port discovery subsystem is used we should include
>>>> all configured ports in the discovery log page, not just that one
>>>> through which the controller was connected.
>>>
>>> Why?
>>>
>> Because that's the whole point if it being configurable.
> 
> Maybe I'm missing something. It is one thing to allow configurable
> discovery NQNs, it is another thing to have discovery subsystems that
> are producing a log page about all connected ports.
>

Not all connected ports. All ports which have the _same_ discovery 
subsystem configured.

- /
   o- ports
   | o- 1 .. [trtype=tcp, traddr=127.0.0.1, trsvcid=8009]
   | | o- subsystems
   | |   o- nqn.discovery
   | o- 2 .. [trtype=tcp, traddr=127.0.0.1, trsvcid=4420]
   | | o- subsystems
   | |   o- nqn.discovery
   | |   o- nqn.io-1
   | o- 3 .. [trtype=tcp, traddr=127.0.0.1, trsvcid=4421]
   |   o- subsystems
   |     o- nqn.io-2
   o- subsystems
     o- nqn.discovery
     o- nqn.io-1
     | o- namespaces
     |   o- 1  [path=/tmp/ns1]
     |   o- 2  [path=/tmp/ns2]
     o- nqn.io-2
       o- namespaces
         o- 1  [path=/tmp/ns3]
         o- 2  [path=/tmp/ns4]

Here a discovery on port 8009 will print information about
- nqn.discovery at port 8009
- nqn.discovery at port 4420
- nqn.io-1 at port 4420
Discovery on port 4420 will get the same information, and discovery on 
port 4421 will return information about
- standard discovery subsyste on port 4421
- nqn.io-2 on port 4421

That's what I meant with 'configurable': you can define which 
information gets returned from which port.

> I suggest that if we want to have discovery subsystems that will
> produce entries on different ports, the log page content needs to be
> programmed explicitly in some form. Doing that is going to produce
> routing issues for sure (subnets, vlans, etc etc etc will cause hosts
> to get log entries for subsystems they can't connect to).

But this is nothing new, and in fact there are subsystem out there which 
_do_ provide log pages containing information about _all_ connected 
ports, even for different transport protocols.
And nvme-cli is already fixed up because of this.

But the whole point is that it _is_ configurable. If you don't want 
invalid entries, fine, configure it to not display potential invalid 
entries. The model allows you to create individual discovery subsystem 
for every port, simulating existing behaviour.
Or configure different discovery subsystem, and link them only to ports 
of the same type. Or same VLAN. Or whatever you care to.
Why should we restrict the choices here?

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 the discovery log page

[Christoph Hellwig]

On Thu, Apr 07, 2022 at 04:51:28PM +0200, Hannes Reinecke wrote:
> Not all connected ports. All ports which have the _same_ discovery 
> subsystem configured.

That makes a lot more sense.  Please add that information (maybe
including the ASCII art) to the commit log for the next version.

Re: [PATCH 3/3] nvmet: include all configured ports in the discovery log page

[Christoph Hellwig]

On Thu, Apr 07, 2022 at 12:48:08PM +0200, Hannes Reinecke wrote:
> +		if (req->port->disc_subsys == nvmet_disc_subsys) {
> +			/*
> +			 * Present information about the port to which
> +			 * the controller is connected.
> +			 */
> +			if (r != req->port)
> +				continue;
> +		} else {
> +			/*
> +			 * If a per-port discovery subsystem is used present
> +			 * information about all ports into which the
> +			 * same discovery subsystem is linked.
> +			 */
> +			if (ctrl->subsys != r->disc_subsys)
> +				continue;
> +		}

Please factor this logic in a helper given that it is also used
by discovery_log_entries (and to document the logic a bit better).

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

[Christoph Hellwig]

On Thu, Apr 07, 2022 at 12:48:06PM +0200, Hannes Reinecke wrote:
> +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);
> +	struct nvmet_port *p;
> +	struct nvmet_subsys_link *s;
> +	int i;
> +
> +	if (subsys->subsys_discovered)
> +		return -EACCES;
> +
> +	/*
> +	 * Do not allow to change the subsystem type if it's
> +	 * already linked to ports; the user should unlink it first.
> +	 */
> +	down_read(&nvmet_config_sem);
> +	list_for_each_entry(p, &nvmet_ports_list, global_entry) {
> +		list_for_each_entry(s, &p->subsystems, entry) {
> +			if (s->subsys == subsys) {
> +				up_read(&nvmet_config_sem);
> +				return -EACCES;
> +			}
> +		}
> +	}
> +	up_read(&nvmet_config_sem);

Does this scale?  Do we want a flag in the subsystem instead?

> +	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 (!xa_empty(&subsys->namespaces)) {
> +			pr_err("discovery subsystem cannot have namespaces\n");
> +			return -EINVAL;
> +		}
> +	}

We can probably just check this unconditionally, as something that was
a discovery subsystem before by definition can't have namespaces.

Re: [PATCH 2/3] nvmet: per-port discovery subsystem

[Christoph Hellwig]

On Thu, Apr 07, 2022 at 12:48:07PM +0200, Hannes Reinecke wrote:
> Add a 'disc_subsys' pointer to each port to specify which discovery
> subsystem to use.
> The pointer is set when a discovery subsystem is linked into a port,
> and reset to the original, built-in discovery subsystem if that link
> is removed.

This doesn't really make much sense stanadlone without the next
patch, so I'd be tempted to say they should be merged.

>  	down_write(&nvmet_config_sem);
> +	if (subsys->type == NVME_NQN_CURR &&
> +	    port->disc_subsys != nvmet_disc_subsys) {

Curious, would NULL not be a better encoding for the default discovery
subsystem?

> +++ b/drivers/nvme/target/core.c
> @@ -1496,9 +1496,9 @@ static struct nvmet_subsys *nvmet_find_get_subsys(struct nvmet_port *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;
> +		if (!kref_get_unless_zero(&port->disc_subsys->ref))
> +				return NULL;
> +		return port->disc_subsys;

This has an extra tab indent.  But: should we even redirect from the
well known discovery NQN for a configured discovery subsystem here?
If yes at least this needs a big fat comment explaining why we do it.

> +	if (req->port->disc_subsys == nvmet_disc_subsys)
> +		entries++;

> +	if (req->port->disc_subsys == nvmet_disc_subsys) {
> +		nvmet_format_discovery_entry(hdr, req->port,
> +				nvmet_disc_subsys->subsysnqn,
> +				traddr, NVME_NQN_CURR, numrec);
> +		numrec++;
> +	}

Why don't we report the current discovery subsystem for if it isn't
the well known one?

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

[Hannes Reinecke]

On 4/7/22 17:44, Christoph Hellwig wrote:
> On Thu, Apr 07, 2022 at 12:48:06PM +0200, Hannes Reinecke wrote:
>> +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);
>> +	struct nvmet_port *p;
>> +	struct nvmet_subsys_link *s;
>> +	int i;
>> +
>> +	if (subsys->subsys_discovered)
>> +		return -EACCES;
>> +
>> +	/*
>> +	 * Do not allow to change the subsystem type if it's
>> +	 * already linked to ports; the user should unlink it first.
>> +	 */
>> +	down_read(&nvmet_config_sem);
>> +	list_for_each_entry(p, &nvmet_ports_list, global_entry) {
>> +		list_for_each_entry(s, &p->subsystems, entry) {
>> +			if (s->subsys == subsys) {
>> +				up_read(&nvmet_config_sem);
>> +				return -EACCES;
>> +			}
>> +		}
>> +	}
>> +	up_read(&nvmet_config_sem);
> 
> Does this scale?  Do we want a flag in the subsystem instead?
> 
Hmm. Seeing that it's a synchronous userspace operation I don't really 
care; worst case the 'ln -s' call takes a bit longer.
But okay, I'll have a look.

>> +	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 (!xa_empty(&subsys->namespaces)) {
>> +			pr_err("discovery subsystem cannot have namespaces\n");
>> +			return -EINVAL;
>> +		}
>> +	}
> 
> We can probably just check this unconditionally, as something that was
> a discovery subsystem before by definition can't have namespaces.

Right.

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: per-port discovery subsystem

[Hannes Reinecke]

On 4/7/22 17:49, Christoph Hellwig wrote:
> On Thu, Apr 07, 2022 at 12:48:07PM +0200, Hannes Reinecke wrote:
>> Add a 'disc_subsys' pointer to each port to specify which discovery
>> subsystem to use.
>> The pointer is set when a discovery subsystem is linked into a port,
>> and reset to the original, built-in discovery subsystem if that link
>> is removed.
> 
> This doesn't really make much sense stanadlone without the next
> patch, so I'd be tempted to say they should be merged.
> 

Sure. I just didn't want to make the patches too complicated initially.

>>   	down_write(&nvmet_config_sem);
>> +	if (subsys->type == NVME_NQN_CURR &&
>> +	    port->disc_subsys != nvmet_disc_subsys) {
> 
> Curious, would NULL not be a better encoding for the default discovery
> subsystem?
> 
Hmm. Sure, could do.

>> +++ b/drivers/nvme/target/core.c
>> @@ -1496,9 +1496,9 @@ static struct nvmet_subsys *nvmet_find_get_subsys(struct nvmet_port *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;
>> +		if (!kref_get_unless_zero(&port->disc_subsys->ref))
>> +				return NULL;
>> +		return port->disc_subsys;
> 
> This has an extra tab indent.  But: should we even redirect from the
> well known discovery NQN for a configured discovery subsystem here?
> If yes at least this needs a big fat comment explaining why we do it.
> 
Yes. This is mandated by the spec
(section 3.1.2.3 Discovery Controller):
If the Discovery subsystem provides a unique NQN, then that Discovery 
subsystem shall support both the unique subsystem NQN and the well-known 
discovery service NQN.

Will be adding a comment.

>> +	if (req->port->disc_subsys == nvmet_disc_subsys)
>> +		entries++;
> 
>> +	if (req->port->disc_subsys == nvmet_disc_subsys) {
>> +		nvmet_format_discovery_entry(hdr, req->port,
>> +				nvmet_disc_subsys->subsysnqn,
>> +				traddr, NVME_NQN_CURR, numrec);
>> +		numrec++;
>> +	}
> 
> Why don't we report the current discovery subsystem for if it isn't
> the well known one?

Thing is, unique discovery subsytems are just 'normal' subsystems; the 
only thing which is different is the type.
The static discovery subsystem OTOH is special as it's not linked to the 
normal subsystem list and consequently doesn't show up in the 
port->subsystem list.
Hence the extra entry here.

Linking it into the port->subsystems list seemed overly complicated for 
no real gain.

At the same time using the ->disc_subsys link directly here would have 
meant that I have to filter out that subsystem in the 
list_for_each_loop(); also an awkward choice.

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 the discovery log page

[Hannes Reinecke]

On 4/7/22 17:41, Christoph Hellwig wrote:
> On Thu, Apr 07, 2022 at 12:48:08PM +0200, Hannes Reinecke wrote:
>> +		if (req->port->disc_subsys == nvmet_disc_subsys) {
>> +			/*
>> +			 * Present information about the port to which
>> +			 * the controller is connected.
>> +			 */
>> +			if (r != req->port)
>> +				continue;
>> +		} else {
>> +			/*
>> +			 * If a per-port discovery subsystem is used present
>> +			 * information about all ports into which the
>> +			 * same discovery subsystem is linked.
>> +			 */
>> +			if (ctrl->subsys != r->disc_subsys)
>> +				continue;
>> +		}
> 
> Please factor this logic in a helper given that it is also used
> by discovery_log_entries (and to document the logic a bit better).

Sure, will do.

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: make the subsystem type configurable

[Christoph Hellwig]

On Thu, Apr 07, 2022 at 06:55:09PM +0200, Hannes Reinecke wrote:
>>> +	down_read(&nvmet_config_sem);
>>> +	list_for_each_entry(p, &nvmet_ports_list, global_entry) {
>>> +		list_for_each_entry(s, &p->subsystems, entry) {
>>> +			if (s->subsys == subsys) {
>>> +				up_read(&nvmet_config_sem);
>>> +				return -EACCES;
>>> +			}
>>> +		}
>>> +	}
>>> +	up_read(&nvmet_config_sem);
>>
>> Does this scale?  Do we want a flag in the subsystem instead?
>>
> Hmm. Seeing that it's a synchronous userspace operation I don't really 
> care; worst case the 'ln -s' call takes a bit longer.
> But okay, I'll have a look.

Well, it is quadratic behavior, wich we generally avoid.  Something
like a counter of enabled ports in the subsystem should easily remove
the need for it.

Re: [PATCH 2/3] nvmet: per-port discovery subsystem

[Christoph Hellwig]

On Thu, Apr 07, 2022 at 07:21:17PM +0200, Hannes Reinecke wrote:
> Sure. I just didn't want to make the patches too complicated initially.
>
>>>   	down_write(&nvmet_config_sem);
>>> +	if (subsys->type == NVME_NQN_CURR &&
>>> +	    port->disc_subsys != nvmet_disc_subsys) {
>>
>> Curious, would NULL not be a better encoding for the default discovery
>> subsystem?
>>
> Hmm. Sure, could do.

Looking at the rest of the patches it might not be a very good idea
after all.  So I'll let you decide.

>> This has an extra tab indent.  But: should we even redirect from the
>> well known discovery NQN for a configured discovery subsystem here?
>> If yes at least this needs a big fat comment explaining why we do it.
>>
> Yes. This is mandated by the spec
> (section 3.1.2.3 Discovery Controller):
> If the Discovery subsystem provides a unique NQN, then that Discovery 
> subsystem shall support both the unique subsystem NQN and the well-known 
> discovery service NQN.
>
> Will be adding a comment.

Thanks!

>>> +	if (req->port->disc_subsys == nvmet_disc_subsys) {
>>> +		nvmet_format_discovery_entry(hdr, req->port,
>>> +				nvmet_disc_subsys->subsysnqn,
>>> +				traddr, NVME_NQN_CURR, numrec);
>>> +		numrec++;
>>> +	}
>>
>> Why don't we report the current discovery subsystem for if it isn't
>> the well known one?
>
> Thing is, unique discovery subsytems are just 'normal' subsystems; the only 
> thing which is different is the type.
> The static discovery subsystem OTOH is special as it's not linked to the 
> normal subsystem list and consequently doesn't show up in the 
> port->subsystem list.
> Hence the extra entry here.

Ok. Please add a comment that the unique discovery controllers will be
handled later in the loop.

[PATCH 3/3] nvmet: include all configured ports in the discovery log page

[Hannes Reinecke]

When a per-port discovery subsystem is used we should include
all configured ports in the discovery log page, not just that one
through which the controller was connected.

- /
  o- ports
  | o- 1 .. [trtype=tcp, traddr=127.0.0.1, trsvcid=8009]
  | | o- subsystems
  | |   o- nqn.discovery
  | o- 2 .. [trtype=tcp, traddr=127.0.0.1, trsvcid=4420]
  | | o- subsystems
  | |   o- nqn.discovery
  | |   o- nqn.io-1
  | o- 3 .. [trtype=tcp, traddr=127.0.0.1, trsvcid=4421]
  |   o- subsystems
  |     o- nqn.io-2
  o- subsystems
    o- nqn.discovery
    o- nqn.io-1
    | o- namespaces
    o- nqn.io-2
      o- namespaces

A discovery on port 8009 will return information about
- nqn.discovery at port 8009
- nqn.discovery at port 4420
- nqn.io-1 at port 4420
A discovery on port 4420 will return the same information.
A discovery on port 4421 will return information about
- standard discovery subsystem on port 4421
- nqn.io-2 on port 4421

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/target/discovery.c | 53 +++++++++++++++++++++++++++------
 1 file changed, 44 insertions(+), 9 deletions(-)

diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c
index 6b8aa6c4e752..ea8fce538342 100644
--- a/drivers/nvme/target/discovery.c
+++ b/drivers/nvme/target/discovery.c
@@ -149,6 +149,30 @@ static void nvmet_set_disc_traddr(struct nvmet_req *req, struct nvmet_port *port
 		memcpy(traddr, port->disc_addr.traddr, NVMF_TRADDR_SIZE);
 }
 
+/*
+ * discovery_port_match - filter eligible ports for discovery log page
+ *
+ * If the port through which the controller is connected has no discovery
+ * subsystem linked, use the original behaviour of just including information
+ * about that port in the discovery log page.
+ * Otherwise include information about all ports into which the specified
+ * discovery subsystem is linked.
+ */
+
+static bool discovery_port_match(struct nvmet_req *req, struct nvmet_port *r)
+{
+	struct nvmet_ctrl *ctrl = req->sq->ctrl;
+
+	if (!req->port->disc_subsys) {
+		if (r != req->port)
+			return false;
+	} else {
+		if (ctrl->subsys != r->disc_subsys)
+			return false;
+	}
+	return true;
+}
+
 static size_t discovery_log_entries(struct nvmet_req *req)
 {
 	struct nvmet_ctrl *ctrl = req->sq->ctrl;
@@ -159,10 +183,14 @@ static size_t discovery_log_entries(struct nvmet_req *req)
 	if (!req->port->disc_subsys)
 		entries++;
 
-	list_for_each_entry(p, &req->port->subsystems, entry) {
-		if (!nvmet_host_allowed(p->subsys, ctrl->hostnqn))
+	list_for_each_entry(r, nvmet_ports, global_entry) {
+		if (!discovery_port_match(req, r))
 			continue;
-		entries++;
+		list_for_each_entry(p, &r->subsystems, entry) {
+			if (!nvmet_host_allowed(p->subsys, ctrl->hostnqn))
+				continue;
+			entries++;
+		}
 	}
 	list_for_each_entry(r, &req->port->referrals, entry)
 		entries++;
@@ -226,14 +254,21 @@ static void nvmet_execute_disc_get_log_page(struct nvmet_req *req)
 		numrec++;
 	}
 
-	list_for_each_entry(p, &req->port->subsystems, entry) {
-		if (!nvmet_host_allowed(p->subsys, ctrl->hostnqn))
+	list_for_each_entry(r, nvmet_ports, global_entry) {
+		nvmet_set_disc_traddr(req, r, traddr);
+
+		if (!discovery_port_match(req, r))
 			continue;
 
-		nvmet_format_discovery_entry(hdr, req->port,
-				p->subsys->subsysnqn, traddr,
-				p->subsys->type, numrec);
-		numrec++;
+		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) {
-- 
2.29.2

Re: [PATCH 3/3] nvmet: include all configured ports in the discovery log page

[Sagi Grimberg]

> When a per-port discovery subsystem is used we should include
> all configured ports in the discovery log page, not just that one
> through which the controller was connected.
> 
> - /
>    o- ports
>    | o- 1 .. [trtype=tcp, traddr=127.0.0.1, trsvcid=8009]
>    | | o- subsystems
>    | |   o- nqn.discovery
>    | o- 2 .. [trtype=tcp, traddr=127.0.0.1, trsvcid=4420]
>    | | o- subsystems
>    | |   o- nqn.discovery
>    | |   o- nqn.io-1
>    | o- 3 .. [trtype=tcp, traddr=127.0.0.1, trsvcid=4421]
>    |   o- subsystems
>    |     o- nqn.io-2
>    o- subsystems
>      o- nqn.discovery
>      o- nqn.io-1
>      | o- namespaces
>      o- nqn.io-2
>        o- namespaces
> 
> A discovery on port 8009 will return information about
> - nqn.discovery at port 8009
> - nqn.discovery at port 4420
> - nqn.io-1 at port 4420
> A discovery on port 4420 will return the same information.
> A discovery on port 4421 will return information about
> - standard discovery subsystem on port 4421
> - nqn.io-2 on port 4421

This is a different functionality than supporting unique discovery
NQNs.

If I want in your example to use a unique discovery subsystem but to
report only on the port local I have to use two different unique
discovery subsystems. Which is different than the standard discovery
subsystem, why?

I'd submit to you that IMO unique discovery subsystems should not behave
differently than the standard discovery subsystem. Please explain why
do you think that unique discovery subsystems should behave differently
with respect to the content of the log page.

> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>   drivers/nvme/target/discovery.c | 53 +++++++++++++++++++++++++++------
>   1 file changed, 44 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c
> index 6b8aa6c4e752..ea8fce538342 100644
> --- a/drivers/nvme/target/discovery.c
> +++ b/drivers/nvme/target/discovery.c
> @@ -149,6 +149,30 @@ static void nvmet_set_disc_traddr(struct nvmet_req *req, struct nvmet_port *port
>   		memcpy(traddr, port->disc_addr.traddr, NVMF_TRADDR_SIZE);
>   }
>   
> +/*
> + * discovery_port_match - filter eligible ports for discovery log page
> + *
> + * If the port through which the controller is connected has no discovery
> + * subsystem linked, use the original behaviour of just including information
> + * about that port in the discovery log page.
> + * Otherwise include information about all ports into which the specified
> + * discovery subsystem is linked.
> + */
> +
> +static bool discovery_port_match(struct nvmet_req *req, struct nvmet_port *r)
> +{
> +	struct nvmet_ctrl *ctrl = req->sq->ctrl;
> +
> +	if (!req->port->disc_subsys) {
> +		if (r != req->port)
> +			return false;
> +	} else {
> +		if (ctrl->subsys != r->disc_subsys)
> +			return false;
> +	}
> +	return true;
> +}
> +
>   static size_t discovery_log_entries(struct nvmet_req *req)
>   {
>   	struct nvmet_ctrl *ctrl = req->sq->ctrl;
> @@ -159,10 +183,14 @@ static size_t discovery_log_entries(struct nvmet_req *req)
>   	if (!req->port->disc_subsys)
>   		entries++;
>   
> -	list_for_each_entry(p, &req->port->subsystems, entry) {
> -		if (!nvmet_host_allowed(p->subsys, ctrl->hostnqn))
> +	list_for_each_entry(r, nvmet_ports, global_entry) {
> +		if (!discovery_port_match(req, r))
>   			continue;
> -		entries++;
> +		list_for_each_entry(p, &r->subsystems, entry) {
> +			if (!nvmet_host_allowed(p->subsys, ctrl->hostnqn))
> +				continue;
> +			entries++;
> +		}
>   	}
>   	list_for_each_entry(r, &req->port->referrals, entry)
>   		entries++;
> @@ -226,14 +254,21 @@ static void nvmet_execute_disc_get_log_page(struct nvmet_req *req)
>   		numrec++;
>   	}
>   
> -	list_for_each_entry(p, &req->port->subsystems, entry) {
> -		if (!nvmet_host_allowed(p->subsys, ctrl->hostnqn))
> +	list_for_each_entry(r, nvmet_ports, global_entry) {
> +		nvmet_set_disc_traddr(req, r, traddr);
> +
> +		if (!discovery_port_match(req, r))
>   			continue;
>   
> -		nvmet_format_discovery_entry(hdr, req->port,
> -				p->subsys->subsysnqn, traddr,
> -				p->subsys->type, numrec);
> -		numrec++;
> +		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) {

Re: [PATCH 3/3] nvmet: include all configured ports in the discovery log page

[Hannes Reinecke]

On 4/11/22 12:54, Sagi Grimberg wrote:
> 
>> When a per-port discovery subsystem is used we should include
>> all configured ports in the discovery log page, not just that one
>> through which the controller was connected.
>>
>> - /
>>    o- ports
>>    | o- 1 .. [trtype=tcp, traddr=127.0.0.1, trsvcid=8009]
>>    | | o- subsystems
>>    | |   o- nqn.discovery
>>    | o- 2 .. [trtype=tcp, traddr=127.0.0.1, trsvcid=4420]
>>    | | o- subsystems
>>    | |   o- nqn.discovery
>>    | |   o- nqn.io-1
>>    | o- 3 .. [trtype=tcp, traddr=127.0.0.1, trsvcid=4421]
>>    |   o- subsystems
>>    |     o- nqn.io-2
>>    o- subsystems
>>      o- nqn.discovery
>>      o- nqn.io-1
>>      | o- namespaces
>>      o- nqn.io-2
>>        o- namespaces
>>
>> A discovery on port 8009 will return information about
>> - nqn.discovery at port 8009
>> - nqn.discovery at port 4420
>> - nqn.io-1 at port 4420
>> A discovery on port 4420 will return the same information.
>> A discovery on port 4421 will return information about
>> - standard discovery subsystem on port 4421
>> - nqn.io-2 on port 4421
> 
> This is a different functionality than supporting unique discovery
> NQNs.
> 

Yes.

> If I want in your example to use a unique discovery subsystem but to
> report only on the port local I have to use two different unique
> discovery subsystems. Which is different than the standard discovery
> subsystem, why?
> 
Because this was a logical step from the way I've implemented unique 
discovery subsystems.

As discovery subsystems (in my implementation) are treated like 'normal' 
subsystems, they should be able to be linked to ports. If they are not 
linked we would have to implement some magic on which ports this 
subsystem will show up, and also making it inconsistent with all other 
subsystems.

> I'd submit to you that IMO unique discovery subsystems should not behave
> differently than the standard discovery subsystem. Please explain why
> do you think that unique discovery subsystems should behave differently
> with respect to the content of the log page.
> 
Well, arguably.
But I haven't been able to find a good way of keeping the original 
behaviour _and_ support unique discovery subsystems.

Sure, one could implement some magic variable in configfs like 
discovery_nqn, but that would have to be in configfs root directory, and 
really doesn't match up with current configfs layout.
One could go with your suggestion of having a discovery_subsystem entry 
per port, but we're back to the issue which killed my original patch:
How do we ensure uniqueness?
Each NQN here _need_ to be validated with the existing (and future) I/O 
subsystem NQNs, as they need to be unique. That requires a lookup over 
all ports and all referenced subsystems.
And we have to figure out if we allow for duplicated discovery NQNs; one 
can find arguments for either side.

That's why I came up with this approach.

And that's also why this is a separate patch, as it does expand existing 
functionality :-)

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 the discovery log page

[Sagi Grimberg]

>>> When a per-port discovery subsystem is used we should include
>>> all configured ports in the discovery log page, not just that one
>>> through which the controller was connected.
>>>
>>> - /
>>>    o- ports
>>>    | o- 1 .. [trtype=tcp, traddr=127.0.0.1, trsvcid=8009]
>>>    | | o- subsystems
>>>    | |   o- nqn.discovery
>>>    | o- 2 .. [trtype=tcp, traddr=127.0.0.1, trsvcid=4420]
>>>    | | o- subsystems
>>>    | |   o- nqn.discovery
>>>    | |   o- nqn.io-1
>>>    | o- 3 .. [trtype=tcp, traddr=127.0.0.1, trsvcid=4421]
>>>    |   o- subsystems
>>>    |     o- nqn.io-2
>>>    o- subsystems
>>>      o- nqn.discovery
>>>      o- nqn.io-1
>>>      | o- namespaces
>>>      o- nqn.io-2
>>>        o- namespaces
>>>
>>> A discovery on port 8009 will return information about
>>> - nqn.discovery at port 8009
>>> - nqn.discovery at port 4420
>>> - nqn.io-1 at port 4420
>>> A discovery on port 4420 will return the same information.
>>> A discovery on port 4421 will return information about
>>> - standard discovery subsystem on port 4421
>>> - nqn.io-2 on port 4421
>>
>> This is a different functionality than supporting unique discovery
>> NQNs.
>>
> 
> Yes.
> 
>> If I want in your example to use a unique discovery subsystem but to
>> report only on the port local I have to use two different unique
>> discovery subsystems. Which is different than the standard discovery
>> subsystem, why?
>>
> Because this was a logical step from the way I've implemented unique 
> discovery subsystems.
> 
> As discovery subsystems (in my implementation) are treated like 'normal' 
> subsystems, they should be able to be linked to ports. If they are not 
> linked we would have to implement some magic on which ports this 
> subsystem will show up, and also making it inconsistent with all other 
> subsystems.

They can be linked to ports, but why do they need to return subsystems
that are attached to all ports differently from the normal discovery
subsystem?

>> I'd submit to you that IMO unique discovery subsystems should not behave
>> differently than the standard discovery subsystem. Please explain why
>> do you think that unique discovery subsystems should behave differently
>> with respect to the content of the log page.
>>
> Well, arguably.
> But I haven't been able to find a good way of keeping the original 
> behaviour _and_ support unique discovery subsystems.
> 
> Sure, one could implement some magic variable in configfs like 
> discovery_nqn, but that would have to be in configfs root directory, and 
> really doesn't match up with current configfs layout.
> One could go with your suggestion of having a discovery_subsystem entry 
> per port, but we're back to the issue which killed my original patch:
> How do we ensure uniqueness?
> Each NQN here _need_ to be validated with the existing (and future) I/O 
> subsystem NQNs, as they need to be unique. That requires a lookup over 
> all ports and all referenced subsystems.
> And we have to figure out if we allow for duplicated discovery NQNs; one 
> can find arguments for either side.
> 
> That's why I came up with this approach.

I'm just talking about the content of the log page. You are making the
functionality of unique discovery subsystems different from the standard
one. I'd either make the unique return subsystems attached to the
current port like the global one, or change the standard one to return
on all ports, or make it explicitly configurable that you can turn on
for each discovery subsys. But making the unique and the standard behave
inherently different is not coherent.

> 
> And that's also why this is a separate patch, as it does expand existing 
> functionality :-)

My issue is that its causing the unique disc-susbsys behavior diverge
from the standard one...

Re: [PATCH 3/3] nvmet: include all configured ports in the discovery log page

[Hannes Reinecke]

On 4/12/22 12:49, Sagi Grimberg wrote:
> 
>>>> When a per-port discovery subsystem is used we should include
>>>> all configured ports in the discovery log page, not just that one
>>>> through which the controller was connected.
>>>>
>>>> - /
>>>>    o- ports
>>>>    | o- 1 .. [trtype=tcp, traddr=127.0.0.1, trsvcid=8009]
>>>>    | | o- subsystems
>>>>    | |   o- nqn.discovery
>>>>    | o- 2 .. [trtype=tcp, traddr=127.0.0.1, trsvcid=4420]
>>>>    | | o- subsystems
>>>>    | |   o- nqn.discovery
>>>>    | |   o- nqn.io-1
>>>>    | o- 3 .. [trtype=tcp, traddr=127.0.0.1, trsvcid=4421]
>>>>    |   o- subsystems
>>>>    |     o- nqn.io-2
>>>>    o- subsystems
>>>>      o- nqn.discovery
>>>>      o- nqn.io-1
>>>>      | o- namespaces
>>>>      o- nqn.io-2
>>>>        o- namespaces
>>>>
>>>> A discovery on port 8009 will return information about
>>>> - nqn.discovery at port 8009
>>>> - nqn.discovery at port 4420
>>>> - nqn.io-1 at port 4420
>>>> A discovery on port 4420 will return the same information.
>>>> A discovery on port 4421 will return information about
>>>> - standard discovery subsystem on port 4421
>>>> - nqn.io-2 on port 4421
>>>
>>> This is a different functionality than supporting unique discovery
>>> NQNs.
>>>
>>
>> Yes.
>>
>>> If I want in your example to use a unique discovery subsystem but to
>>> report only on the port local I have to use two different unique
>>> discovery subsystems. Which is different than the standard discovery
>>> subsystem, why?
>>>
>> Because this was a logical step from the way I've implemented unique 
>> discovery subsystems.
>>
>> As discovery subsystems (in my implementation) are treated like 
>> 'normal' subsystems, they should be able to be linked to ports. If 
>> they are not linked we would have to implement some magic on which 
>> ports this subsystem will show up, and also making it inconsistent 
>> with all other subsystems.
> 
> They can be linked to ports, but why do they need to return subsystems
> that are attached to all ports differently from the normal discovery
> subsystem?
> 
>>> I'd submit to you that IMO unique discovery subsystems should not behave
>>> differently than the standard discovery subsystem. Please explain why
>>> do you think that unique discovery subsystems should behave differently
>>> with respect to the content of the log page.
>>>
>> Well, arguably.
>> But I haven't been able to find a good way of keeping the original 
>> behaviour _and_ support unique discovery subsystems.
>>
>> Sure, one could implement some magic variable in configfs like 
>> discovery_nqn, but that would have to be in configfs root directory, 
>> and really doesn't match up with current configfs layout.
>> One could go with your suggestion of having a discovery_subsystem 
>> entry per port, but we're back to the issue which killed my original 
>> patch:
>> How do we ensure uniqueness?
>> Each NQN here _need_ to be validated with the existing (and future) 
>> I/O subsystem NQNs, as they need to be unique. That requires a lookup 
>> over all ports and all referenced subsystems.
>> And we have to figure out if we allow for duplicated discovery NQNs; 
>> one can find arguments for either side.
>>
>> That's why I came up with this approach.
> 
> I'm just talking about the content of the log page. You are making the
> functionality of unique discovery subsystems different from the standard
> one. I'd either make the unique return subsystems attached to the
> current port like the global one, or change the standard one to return
> on all ports, or make it explicitly configurable that you can turn on
> for each discovery subsys. But making the unique and the standard behave
> inherently different is not coherent.
> 
>>
>> And that's also why this is a separate patch, as it does expand 
>> existing functionality :-)
> 
> My issue is that its causing the unique disc-susbsys behavior diverge
> from the standard one...

Ah. Okay.
That is true.

But having the default setup reflected in configfs is hard, as then you 
would need to create links from the default discovery subsystem to each 
port (on port creation!).
The current configfs design simply doesn't allow you to do that, so 
attempting something like that would break compability.

Which is the big plus for this patchset; it doesn't change the user 
interface, and nvmetcli can be used as-is.

So sure, I can have a look to expose the default discovery subsystem, 
too. But that would require a module or configfs parameter, _and_ will 
require changes to nvmetcli.

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 the discovery log page

[Sagi Grimberg]

>> My issue is that its causing the unique disc-susbsys behavior diverge
>> from the standard one...
> 
> Ah. Okay.
> That is true.
> 
> But having the default setup reflected in configfs is hard, as then you 
> would need to create links from the default discovery subsystem to each 
> port (on port creation!).
> The current configfs design simply doesn't allow you to do that, so 
> attempting something like that would break compability.
> 
> Which is the big plus for this patchset; it doesn't change the user 
> interface, and nvmetcli can be used as-is.
> 
> So sure, I can have a look to expose the default discovery subsystem, 
> too. But that would require a module or configfs parameter, _and_ will 
> require changes to nvmetcli.

Before this patch, you added support for unique disc-subsys NQNs, which
behaved similarly to the standard disc-subsys. In this patch, you
made unique disc-subsys return a different discovery log-page. That is
my issue.

There are 3 approaches we can take instead:
1. ignore this patch, and make the unique disc-subsys to be attached
to multiple ports, but return only subsystems in the same port where
the host is accessing it.

2. make the standard disc-subsys return subsystems on all ports like
what you proposed for unique disc-subsys in this patch.

3. introduce an explicit setting for unique disc-subsys to return cross
port log page. i.e. make the divergence explicit.