[PATCHv6 0/4] nvmet: unique discovery subsystems

[PATCHv6 0/4] nvmet: unique discovery subsystems

[Hannes Reinecke]

Hi all,

here's my next attempt to support unique discovery subsystems.

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.
Additionally a module parameter 'expose_discovery_subsys' is implemented,
which will allow (or, rather, require) the creation of the default discovery subsystem via configfs, too. Once that option is enabled only ports into which
a discovery subsystem is linked will be presenting a discovery service; on
other ports a discovery attempt will fail.

As usual, comments and reviews are welcome.

Changes to v5:
- Implement 'expose_discovery_subsys' module option to allow the creation of
  the standard discovery subsystem via configfs.
Changes to v4:
- Unset disc_subsys pointer when unique discovery subsystem
  gets unlinked
- Improve documentation
- Use port count to determine if a subsystem is linked to ports

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 (4):
  nvmet: make the subsystem type configurable
  nvmet: per-port discovery subsystem
  nvmet: include all configured ports in the discovery log page
  nvmet: expose discovery subsystem

 drivers/nvme/target/configfs.c  |  80 ++++++++++++++++++++++-
 drivers/nvme/target/core.c      |  15 ++++-
 drivers/nvme/target/discovery.c | 108 +++++++++++++++++++++++++-------
 drivers/nvme/target/nvmet.h     |   2 +
 4 files changed, 176 insertions(+), 29 deletions(-)

-- 
2.29.2

[PATCH 1/4] 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  | 60 +++++++++++++++++++++++++++++++++
 drivers/nvme/target/discovery.c |  2 +-
 drivers/nvme/target/nvmet.h     |  1 +
 3 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index e44b2988759e..38b0ab9fb721 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -834,6 +834,7 @@ static int nvmet_port_subsys_allow_link(struct config_item *parent,
 	}
 
 	list_add_tail(&link->entry, &port->subsystems);
+	subsys->port_count++;
 	nvmet_port_disc_changed(port, subsys);
 
 	up_write(&nvmet_config_sem);
@@ -862,6 +863,7 @@ static void nvmet_port_subsys_drop_link(struct config_item *parent,
 
 found:
 	list_del(&p->entry);
+	subsys->port_count--;
 	nvmet_port_del_ctrls(port, subsys);
 	nvmet_port_disc_changed(port, subsys);
 
@@ -1234,6 +1236,63 @@ 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 (subsys->port_count) {
+		pr_err("cannot change type when linked to ports\n");
+		up_write(&nvmet_config_sem);
+		return -EACCES;
+	}
+	if (!xa_empty(&subsys->namespaces)) {
+		pr_err("cannot change type when namespaces are configured\n");
+		up_write(&nvmet_config_sem);
+		return -EACCES;
+	}
+	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 +1322,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++;
 	}
 
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 69818752a33a..ecba3890ce65 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -216,6 +216,7 @@ struct nvmet_subsys {
 
 	struct mutex		lock;
 	struct kref		ref;
+	unsigned int		port_count;
 
 	struct xarray		namespaces;
 	unsigned int		nr_namespaces;
-- 
2.29.2

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

[Sagi Grimberg]

On 4/20/22 12:27, Hannes Reinecke wrote:
> 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  | 60 +++++++++++++++++++++++++++++++++
>   drivers/nvme/target/discovery.c |  2 +-
>   drivers/nvme/target/nvmet.h     |  1 +
>   3 files changed, 62 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
> index e44b2988759e..38b0ab9fb721 100644
> --- a/drivers/nvme/target/configfs.c
> +++ b/drivers/nvme/target/configfs.c
> @@ -834,6 +834,7 @@ static int nvmet_port_subsys_allow_link(struct config_item *parent,
>   	}
>   
>   	list_add_tail(&link->entry, &port->subsystems);
> +	subsys->port_count++;
>   	nvmet_port_disc_changed(port, subsys);
>   
>   	up_write(&nvmet_config_sem);
> @@ -862,6 +863,7 @@ static void nvmet_port_subsys_drop_link(struct config_item *parent,
>   
>   found:
>   	list_del(&p->entry);
> +	subsys->port_count--;
>   	nvmet_port_del_ctrls(port, subsys);
>   	nvmet_port_disc_changed(port, subsys);
>   
> @@ -1234,6 +1236,63 @@ 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" },

Didn't we agree that the internal enumeration would be different
than the content of the log page? It is just confusing reading it this
way...

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

[Hannes Reinecke]

On 5/10/22 11:36, Sagi Grimberg wrote:
> 
> 
> On 4/20/22 12:27, Hannes Reinecke wrote:
>> 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  | 60 +++++++++++++++++++++++++++++++++
>>   drivers/nvme/target/discovery.c |  2 +-
>>   drivers/nvme/target/nvmet.h     |  1 +
>>   3 files changed, 62 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/nvme/target/configfs.c 
>> b/drivers/nvme/target/configfs.c
>> index e44b2988759e..38b0ab9fb721 100644
>> --- a/drivers/nvme/target/configfs.c
>> +++ b/drivers/nvme/target/configfs.c
>> @@ -834,6 +834,7 @@ static int nvmet_port_subsys_allow_link(struct 
>> config_item *parent,
>>       }
>>       list_add_tail(&link->entry, &port->subsystems);
>> +    subsys->port_count++;
>>       nvmet_port_disc_changed(port, subsys);
>>       up_write(&nvmet_config_sem);
>> @@ -862,6 +863,7 @@ static void nvmet_port_subsys_drop_link(struct 
>> config_item *parent,
>>   found:
>>       list_del(&p->entry);
>> +    subsys->port_count--;
>>       nvmet_port_del_ctrls(port, subsys);
>>       nvmet_port_disc_changed(port, subsys);
>> @@ -1234,6 +1236,63 @@ 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" },
> 
> Didn't we agree that the internal enumeration would be different
> than the content of the log page? It is just confusing reading it this
> way...

And so we did. Will be changing it.

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

[PATCH 2/4] 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 if that link is removed.

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

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 38b0ab9fb721..44573fe0dfe4 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) {
+		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)
@@ -835,6 +841,8 @@ static int nvmet_port_subsys_allow_link(struct config_item *parent,
 
 	list_add_tail(&link->entry, &port->subsystems);
 	subsys->port_count++;
+	if (subsys->type == NVME_NQN_CURR)
+		port->disc_subsys = subsys;
 	nvmet_port_disc_changed(port, subsys);
 
 	up_write(&nvmet_config_sem);
@@ -866,6 +874,8 @@ static void nvmet_port_subsys_drop_link(struct config_item *parent,
 	subsys->port_count--;
 	nvmet_port_del_ctrls(port, subsys);
 	nvmet_port_disc_changed(port, subsys);
+	if (port->disc_subsys == subsys)
+		port->disc_subsys = NULL;
 
 	if (list_empty(&port->subsystems))
 		nvmet_disable_port(port);
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 90e75324dae0..afd80999a335 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1495,10 +1495,19 @@ static struct nvmet_subsys *nvmet_find_get_subsys(struct nvmet_port *port,
 	if (!port)
 		return NULL;
 
+	/*
+	 * As per spec discovery subsystems with a unique NQN
+	 * have to respond to both NQNs, the unique NQN and the
+	 * standard discovery NQN.
+	 */
 	if (!strcmp(NVME_DISC_SUBSYS_NAME, subsysnqn)) {
-		if (!kref_get_unless_zero(&nvmet_disc_subsys->ref))
-			return NULL;
-		return nvmet_disc_subsys;
+		struct nvmet_subsys *disc_subsys;
+
+		disc_subsys = port->disc_subsys ?
+			port->disc_subsys : nvmet_disc_subsys;
+		if (!kref_get_unless_zero(&disc_subsys->ref))
+				return NULL;
+		return disc_subsys;
 	}
 
 	down_read(&nvmet_config_sem);
diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c
index b5012df790d5..6b8aa6c4e752 100644
--- a/drivers/nvme/target/discovery.c
+++ b/drivers/nvme/target/discovery.c
@@ -29,18 +29,22 @@ void nvmet_port_disc_changed(struct nvmet_port *port,
 			     struct nvmet_subsys *subsys)
 {
 	struct nvmet_ctrl *ctrl;
+	struct nvmet_subsys *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) {
+	disc_subsys = port->disc_subsys ?
+		port->disc_subsys : nvmet_disc_subsys;
+
+	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 +52,23 @@ 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;
+
+	disc_subsys = port->disc_subsys ?
+		port->disc_subsys : nvmet_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 +84,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 +154,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)
+		entries++;
 
 	list_for_each_entry(p, &req->port->subsystems, entry) {
 		if (!nvmet_host_allowed(p->subsys, ctrl->hostnqn))
@@ -208,10 +219,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_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 ecba3890ce65..8df297a190bf 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/4] 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/4] nvmet: include all configured ports in the discovery log page

[Sagi Grimberg]

On 4/20/22 12:27, Hannes Reinecke 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
> 
> 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;

What is this condition for? What is this filtering out?

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

[Hannes Reinecke]

On 5/10/22 11:43, Sagi Grimberg wrote:
> 
> 
> On 4/20/22 12:27, Hannes Reinecke 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
>>
>> 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;
> 
> What is this condition for? What is this filtering out?

This filters out all ports into which the discovery subsystem
is _not_ linked (ie the statement 'Otherwise ...' in the function
description).

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

[PATCH 4/4] nvmet: expose discovery subsystem

[Hannes Reinecke]

Add a module option 'expose_discovery_subsys' to expose the
default discovery subsystem via configfs.
When this option is enabled the internal discovery subsystem
is disabled, and the admin has to create a discovery
subsystem manually.
The discovery subsystem then has to be linked into the ports
which should present the discovery subsystem.

o- /
  o- ports
  | o- 2
  |   o- subsystems
  |     o- nqn.io-2
  | o- 3
  |   o- subsystems
  |     o- nqn.2014-08.org.nvmexpress.discovery
  |     o- nqn.io-1
  o- subsystems
    o- nqn.2014-08.org.nvmexpress.discovery
    | o- namespaces
    o- nqn.io-1
    | o- namespaces
    o- nqn.io-2
      o- namespaces

So in this example the standard discovery service would be available on
port 3, presenting information about subsystem nqn.io-1.
Port 2 would not present a discovery service, but a controller can connect
to subsystem nqn.io-2 on that port.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/target/configfs.c  | 10 +++++++---
 drivers/nvme/target/core.c      |  2 +-
 drivers/nvme/target/discovery.c | 20 +++++++++++++++++---
 3 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 44573fe0dfe4..365d374d809d 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -1364,13 +1364,17 @@ static struct config_group *nvmet_subsys_make(struct config_group *group,
 		const char *name)
 {
 	struct nvmet_subsys *subsys;
+	enum nvme_subsys_type subsys_type = NVME_NQN_NVME;
 
 	if (sysfs_streq(name, NVME_DISC_SUBSYS_NAME)) {
-		pr_err("can't create discovery subsystem through configfs\n");
-		return ERR_PTR(-EINVAL);
+		if (nvmet_disc_subsys) {
+			pr_err("can't create discovery subsystem through configfs\n");
+			return ERR_PTR(-EINVAL);
+		}
+		subsys_type = NVME_NQN_CURR;
 	}
 
-	subsys = nvmet_subsys_alloc(name, NVME_NQN_NVME);
+	subsys = nvmet_subsys_alloc(name, subsys_type);
 	if (IS_ERR(subsys))
 		return ERR_CAST(subsys);
 
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index afd80999a335..ab458ff516db 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1505,7 +1505,7 @@ static struct nvmet_subsys *nvmet_find_get_subsys(struct nvmet_port *port,
 
 		disc_subsys = port->disc_subsys ?
 			port->disc_subsys : nvmet_disc_subsys;
-		if (!kref_get_unless_zero(&disc_subsys->ref))
+		if (!disc_subsys || !kref_get_unless_zero(&disc_subsys->ref))
 				return NULL;
 		return disc_subsys;
 	}
diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c
index ea8fce538342..3b94b60056cf 100644
--- a/drivers/nvme/target/discovery.c
+++ b/drivers/nvme/target/discovery.c
@@ -12,6 +12,11 @@ struct nvmet_subsys *nvmet_disc_subsys;
 
 static u64 nvmet_genctr;
 
+static int expose_discovery_subsys;
+module_param(expose_discovery_subsys, int, 0644);
+MODULE_PARM_DESC(expose_discovery_subsys,
+		 "Expose discovery subsytem in configfs");
+
 static void __nvmet_disc_changed(struct nvmet_port *port,
 				 struct nvmet_ctrl *ctrl)
 {
@@ -36,6 +41,8 @@ void nvmet_port_disc_changed(struct nvmet_port *port,
 
 	disc_subsys = port->disc_subsys ?
 		port->disc_subsys : nvmet_disc_subsys;
+	if (!disc_subsys)
+		return;
 
 	mutex_lock(&disc_subsys->lock);
 	list_for_each_entry(ctrl, &disc_subsys->ctrls, subsys_entry) {
@@ -59,6 +66,8 @@ static void __nvmet_subsys_disc_changed(struct nvmet_port *port,
 
 	disc_subsys = port->disc_subsys ?
 		port->disc_subsys : nvmet_disc_subsys;
+	if (!disc_subsys)
+		return;
 
 	mutex_lock(&disc_subsys->lock);
 
@@ -180,7 +189,7 @@ static size_t discovery_log_entries(struct nvmet_req *req)
 	struct nvmet_port *r;
 	size_t entries = 0;
 
-	if (!req->port->disc_subsys)
+	if (!req->port->disc_subsys && nvmet_disc_subsys)
 		entries++;
 
 	list_for_each_entry(r, nvmet_ports, global_entry) {
@@ -247,7 +256,7 @@ static void nvmet_execute_disc_get_log_page(struct nvmet_req *req)
 
 	nvmet_set_disc_traddr(req, req->port, traddr);
 
-	if (!req->port->disc_subsys) {
+	if (!req->port->disc_subsys && nvmet_disc_subsys) {
 		nvmet_format_discovery_entry(hdr, req->port,
 				nvmet_disc_subsys->subsysnqn,
 				traddr, NVME_NQN_CURR, numrec);
@@ -441,6 +450,10 @@ u16 nvmet_parse_discovery_cmd(struct nvmet_req *req)
 
 int __init nvmet_init_discovery(void)
 {
+	if (expose_discovery_subsys) {
+		nvmet_disc_subsys = NULL;
+		return 0;
+	}
 	nvmet_disc_subsys =
 		nvmet_subsys_alloc(NVME_DISC_SUBSYS_NAME, NVME_NQN_CURR);
 	return PTR_ERR_OR_ZERO(nvmet_disc_subsys);
@@ -448,5 +461,6 @@ int __init nvmet_init_discovery(void)
 
 void nvmet_exit_discovery(void)
 {
-	nvmet_subsys_put(nvmet_disc_subsys);
+	if (nvmet_disc_subsys)
+		nvmet_subsys_put(nvmet_disc_subsys);
 }
-- 
2.29.2

Re: [PATCH 4/4] nvmet: expose discovery subsystem

[Sagi Grimberg]

> Add a module option 'expose_discovery_subsys' to expose the
> default discovery subsystem via configfs.
> When this option is enabled the internal discovery subsystem
> is disabled, and the admin has to create a discovery
> subsystem manually.
> The discovery subsystem then has to be linked into the ports
> which should present the discovery subsystem.
> 
> o- /
>    o- ports
>    | o- 2
>    |   o- subsystems
>    |     o- nqn.io-2
>    | o- 3
>    |   o- subsystems
>    |     o- nqn.2014-08.org.nvmexpress.discovery
>    |     o- nqn.io-1
>    o- subsystems
>      o- nqn.2014-08.org.nvmexpress.discovery
>      | o- namespaces
>      o- nqn.io-1
>      | o- namespaces
>      o- nqn.io-2
>        o- namespaces
> 
> So in this example the standard discovery service would be available on
> port 3, presenting information about subsystem nqn.io-1.
> Port 2 would not present a discovery service, but a controller can connect
> to subsystem nqn.io-2 on that port.

So what is the behavior of this param?

expose_discovery_subsys=1:
both default and unique discovery subsystems function and behave
in the same way, i.e. user created, linked to dedicated ports and
answers log page info with subsystems on all ports they are linked to.

expose_discovery_subsys=0:
default discovery subsystem behaves like today but unique discovery
subsystems behave differently (i.e. like first option?)

This will be confusing to users I think.

I would say that in case expose_discovery_subsys=0 unique discovery
subsystems should still behave like the default discovery subsys,
meaning return a log-page with only port local information.

This way default and unique discovery subsystems are consistent with
each other. Then the modparam could be something like:

static int multiport_discovery_log_page;
module_param(multiport_discovery_log_page, int, 0644);
MODULE_PARM_DESC(multiport_discovery_log_page,
		 "Include subsystems from all linked ports in the "
		"discovery log-page information. In this mode the "
		"default discovery subsystem must also be created in "
		"configfs like any other subsystem.");

Thoughts?

Re: [PATCH 4/4] nvmet: expose discovery subsystem

[Hannes Reinecke]

On 5/10/22 20:59, Sagi Grimberg wrote:
>> Add a module option 'expose_discovery_subsys' to expose the
>> default discovery subsystem via configfs.
>> When this option is enabled the internal discovery subsystem
>> is disabled, and the admin has to create a discovery
>> subsystem manually.
>> The discovery subsystem then has to be linked into the ports
>> which should present the discovery subsystem.
>>
>> o- /
>>    o- ports
>>    | o- 2
>>    |   o- subsystems
>>    |     o- nqn.io-2
>>    | o- 3
>>    |   o- subsystems
>>    |     o- nqn.2014-08.org.nvmexpress.discovery
>>    |     o- nqn.io-1
>>    o- subsystems
>>      o- nqn.2014-08.org.nvmexpress.discovery
>>      | o- namespaces
>>      o- nqn.io-1
>>      | o- namespaces
>>      o- nqn.io-2
>>        o- namespaces
>>
>> So in this example the standard discovery service would be available on
>> port 3, presenting information about subsystem nqn.io-1.
>> Port 2 would not present a discovery service, but a controller can 
>> connect
>> to subsystem nqn.io-2 on that port.
> 
> So what is the behavior of this param?
> 
> expose_discovery_subsys=1:
> both default and unique discovery subsystems function and behave
> in the same way, i.e. user created, linked to dedicated ports and
> answers log page info with subsystems on all ports they are linked to.
> 
> expose_discovery_subsys=0:
> default discovery subsystem behaves like today but unique discovery
> subsystems behave differently (i.e. like first option?)
> 
> This will be confusing to users I think.
> 
> I would say that in case expose_discovery_subsys=0 unique discovery
> subsystems should still behave like the default discovery subsys,
> meaning return a log-page with only port local information.
> 
> This way default and unique discovery subsystems are consistent with
> each other. Then the modparam could be something like:
> 
> static int multiport_discovery_log_page;
> module_param(multiport_discovery_log_page, int, 0644);
> MODULE_PARM_DESC(multiport_discovery_log_page,
>           "Include subsystems from all linked ports in the "
>          "discovery log-page information. In this mode the "
>          "default discovery subsystem must also be created in "
>          "configfs like any other subsystem.");
> 
> Thoughts?

My idea here was that each discovery subsystem will be presenting 
informations about all ports it is linked in.
Consequently, to mimic the original behaviour you would create separate 
discovery subsystems such that each port has its own discovery subsystems.

Plus your suggested module parameter would influence the default 
discovery subsystem, too, right?
I tied it into exposed discovery subsystems on purpose, to allow the 
user to control which ports are exposed to which discovery subsystem.

So, in the end, I'm not sure which way to go.
I'm fine with your suggestion, but I like the current patchset, too ...

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