[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

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

[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

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

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