[PATCHv5 0/3] nvmet: unique discovery subsystems

[PATCHv5 0/3] 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.

As usual, comments and reviews are welcome.

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 (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  | 70 +++++++++++++++++++++++++
 drivers/nvme/target/core.c      | 15 ++++--
 drivers/nvme/target/discovery.c | 92 +++++++++++++++++++++++++--------
 drivers/nvme/target/nvmet.h     |  2 +
 4 files changed, 154 insertions(+), 25 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  | 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

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

[Sagi Grimberg]

On 4/8/22 09:59, 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--;

minor nit, the decrement should go before the list deletion (although
it's safe this way as well).

Otherwise looks good,
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

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

[Sagi Grimberg]

> 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" },
> +};

Wait, what does this mean? I don't understand how the type maps to
the string?

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

[Sagi Grimberg]

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

Why not make discovery_subsystems/ subdir in a port? We can restrict
it to have only one element if needed.

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

The CURR is really confusing to me...
Can you explain the NQN types mapping here?

> +		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;
>   	}

Why use the port discovery subsys? why not just use the standard
discovery 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);

Why remove the subsys argument from this function?

>   }
>   
>   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++;
> +	}

For the unique discovery you don't need this?

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

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

[Hannes Reinecke]

On 4/11/22 12:32, Sagi Grimberg wrote:
> 
> 
> On 4/8/22 09:59, 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--;
> 
> minor nit, the decrement should go before the list deletion (although
> it's safe this way as well).
> 
Well, it's changed while nvmet_config_sem is held.
But anyway, will be adjusting it for the next version.

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.

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 2/3] nvmet: per-port discovery subsystem

[Hannes Reinecke]

On 4/11/22 12:45, Sagi Grimberg 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 if that link is removed.
> 
> Why not make discovery_subsystems/ subdir in a port? We can restrict
> it to have only one element if needed.
> 

Because that would make the discovery subsystems 'special'.
My idea was to treat discovery subsystems just like 'normal' subsystems,
ie being part of the subsystem list per port, and able to be created 
like other subsystems.

And using a 'discovery_subsys' subdir does raise issues with handling 
the default discovery subsystem.
If it's not listed one would assume that this port doesn't have a 
discovery subsystem.
If it's listed we cannot make it a default entry, as then we can't 
remove it.
And I've yet to find a way how to create dynamic entries from kernel 
space. If you have a way, please tell.

>>
>> 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) {
> 
> The CURR is really confusing to me...
> Can you explain the NQN types mapping here?
> 

TPAR 8014 changed the meaning of the original 'discovery subsystem' type 
to 'discovery subsystem referral' (as this was the actual meaning of the 
original spec), and added a new entry 'current discovery subsystem' to 
return information about the current subsystem (ie that one where the 
controller is attached to).

>> +        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;
>>       }
> 
> Why use the port discovery subsys? why not just use the standard
> discovery subsys?
> 

Because (due to TPAR8014) we have to return information about the 
current discovery subsystem, including all port details.

>>       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);
> 
> Why remove the subsys argument from this function?
> 

Because it's not needed?
But I can make it a separate patch if you prefer.

>>   }
>>   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++;
>> +    }
> 
> For the unique discovery you don't need this?
> 
No.
As mentioned above, unique discovery subsystems are handled identically 
to 'normal' subsystems, which in this case means that they will be part 
of the list of subsystems linked to a port.
So we'll get information about the discovery subsystem with the loop a 
few lines below that code.

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

[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 2/3] nvmet: per-port discovery subsystem

[Sagi Grimberg]

On 4/11/22 15:07, Hannes Reinecke wrote:
> On 4/11/22 12:45, Sagi Grimberg 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 if that link is removed.
>>
>> Why not make discovery_subsystems/ subdir in a port? We can restrict
>> it to have only one element if needed.
>>
> 
> Because that would make the discovery subsystems 'special'.
> My idea was to treat discovery subsystems just like 'normal' subsystems,
> ie being part of the subsystem list per port, and able to be created 
> like other subsystems.

They are obviously special.

> And using a 'discovery_subsys' subdir does raise issues with handling 
> the default discovery subsystem.
> If it's not listed one would assume that this port doesn't have a 
> discovery subsystem.
> If it's listed we cannot make it a default entry, as then we can't 
> remove it.

I think that the current interface is confusing as well.

> And I've yet to find a way how to create dynamic entries from kernel 
> space. If you have a way, please tell.
> 
>>>
>>> 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) {
>>
>> The CURR is really confusing to me...
>> Can you explain the NQN types mapping here?
>>
> 
> TPAR 8014 changed the meaning of the original 'discovery subsystem' type 
> to 'discovery subsystem referral' (as this was the actual meaning of the 
> original spec), and added a new entry 'current discovery subsystem' to 
> return information about the current subsystem (ie that one where the 
> controller is attached to).

Eh, at least the usage of CURR in the code is weird. I understand what
it means in the log page, but the usage here is strange. I suggest to
use a different enumeration naming in the code and leave CURR to the
log page, where it makes sense.

> 
>>> +        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;
>>>       }
>>
>> Why use the port discovery subsys? why not just use the standard
>> discovery subsys?
>>
> 
> Because (due to TPAR8014) we have to return information about the 
> current discovery subsystem, including all port details.

So you got a discovery log page request on the default discovery
subsys and you are filling the log page with content of the unique
discovery subsys? The current discovery subsystem is the default
discovery subsystem in this case, no?

Maybe I'm missing something...

> 
>>>       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);
>>
>> Why remove the subsys argument from this function?
>>
> 
> Because it's not needed?
> But I can make it a separate patch if you prefer.

But different hosts can be connected to different discovery subsystems.

Is the implementation intent is that you always use the unique
discovery subsystem also when hosts refer to the default discovery
subsystem?

If so, that behavior was not clear in the cover-letter/change-log at
all.

>>>   }
>>>   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++;
>>> +    }
>>
>> For the unique discovery you don't need this?
>>
> No.
> As mentioned above, unique discovery subsystems are handled identically 
> to 'normal' subsystems, which in this case means that they will be part 
> of the list of subsystems linked to a port.
> So we'll get information about the discovery subsystem with the loop a 
> few lines below that code.

I see. makes sense.

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 2/3] nvmet: per-port discovery subsystem

[Hannes Reinecke]

On 4/12/22 12:40, Sagi Grimberg wrote:
> 
> 
> On 4/11/22 15:07, Hannes Reinecke wrote:
>> On 4/11/22 12:45, Sagi Grimberg 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 if that link is removed.
>>>
>>> Why not make discovery_subsystems/ subdir in a port? We can restrict
>>> it to have only one element if needed.
>>>
>>
>> Because that would make the discovery subsystems 'special'.
>> My idea was to treat discovery subsystems just like 'normal' subsystems,
>> ie being part of the subsystem list per port, and able to be created 
>> like other subsystems.
> 
> They are obviously special.
> 
Yes and no. Both are 'subsystems'. And I wanted to avoid having to 
special-case things (like we do for referrals), as this would require 
changes to nvmetcli.
So aim was to make the necessary user interface changes as small as 
possible.

>> And using a 'discovery_subsys' subdir does raise issues with handling 
>> the default discovery subsystem.
>> If it's not listed one would assume that this port doesn't have a 
>> discovery subsystem.
>> If it's listed we cannot make it a default entry, as then we can't 
>> remove it.
> 
> I think that the current interface is confusing as well.
> 
Yes. And that's why I wanted to have the discovery subsystem showing up 
in configfs.

>> And I've yet to find a way how to create dynamic entries from kernel 
>> space. If you have a way, please tell.
>>
>>>>
>>>> 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) {
>>>
>>> The CURR is really confusing to me...
>>> Can you explain the NQN types mapping here?
>>>
>>
>> TPAR 8014 changed the meaning of the original 'discovery subsystem' 
>> type to 'discovery subsystem referral' (as this was the actual meaning 
>> of the original spec), and added a new entry 'current discovery 
>> subsystem' to return information about the current subsystem (ie that 
>> one where the controller is attached to).
> 
> Eh, at least the usage of CURR in the code is weird. I understand what
> it means in the log page, but the usage here is strange. I suggest to
> use a different enumeration naming in the code and leave CURR to the
> log page, where it makes sense.
> 
Okay, will be doing so.

>>
>>>> +        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;
>>>>       }
>>>
>>> Why use the port discovery subsys? why not just use the standard
>>> discovery subsys?
>>>
>>
>> Because (due to TPAR8014) we have to return information about the 
>> current discovery subsystem, including all port details.
> 
> So you got a discovery log page request on the default discovery
> subsys and you are filling the log page with content of the unique
> discovery subsys? The current discovery subsystem is the default
> discovery subsystem in this case, no?
> 
> Maybe I'm missing something...
> 
That's what TP8013 is all about.
The unique discovery subsystem has to react to both subsystem NQNs, the 
unique NQN _and_ the default discovery subsystem NQN.
But there will be only _one_ discovery subsystem per port.

>>
>>>>       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);
>>>
>>> Why remove the subsys argument from this function?
>>>
>>
>> Because it's not needed?
>> But I can make it a separate patch if you prefer.
> 
> But different hosts can be connected to different discovery subsystems.
> 

Sure. But the 'subsys' argument is unused even in the current code.

> Is the implementation intent is that you always use the unique
> discovery subsystem also when hosts refer to the default discovery
> subsystem?
> 
Yes. That is what TPAR8013 mandates.

> If so, that behavior was not clear in the cover-letter/change-log at
> all.
> 
Okay, I'll change it.

Unless you object to this approach in general; then it hardly makes 
sense to continue.

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

[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 2/3] nvmet: per-port discovery subsystem

[Sagi Grimberg]

>>>>> 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.
>>>>
>>>> Why not make discovery_subsystems/ subdir in a port? We can restrict
>>>> it to have only one element if needed.
>>>>
>>>
>>> Because that would make the discovery subsystems 'special'.
>>> My idea was to treat discovery subsystems just like 'normal' subsystems,
>>> ie being part of the subsystem list per port, and able to be created 
>>> like other subsystems.
>>
>> They are obviously special.
>>
> Yes and no. Both are 'subsystems'. And I wanted to avoid having to 
> special-case things (like we do for referrals), as this would require 
> changes to nvmetcli.
> So aim was to make the necessary user interface changes as small as 
> possible.
> 
>>> And using a 'discovery_subsys' subdir does raise issues with handling 
>>> the default discovery subsystem.
>>> If it's not listed one would assume that this port doesn't have a 
>>> discovery subsystem.
>>> If it's listed we cannot make it a default entry, as then we can't 
>>> remove it.
>>
>> I think that the current interface is confusing as well.
>>
> Yes. And that's why I wanted to have the discovery subsystem showing up 
> in configfs.
> 
>>> And I've yet to find a way how to create dynamic entries from kernel 
>>> space. If you have a way, please tell.
>>>
>>>>>
>>>>> 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) {
>>>>
>>>> The CURR is really confusing to me...
>>>> Can you explain the NQN types mapping here?
>>>>
>>>
>>> TPAR 8014 changed the meaning of the original 'discovery subsystem' 
>>> type to 'discovery subsystem referral' (as this was the actual 
>>> meaning of the original spec), and added a new entry 'current 
>>> discovery subsystem' to return information about the current 
>>> subsystem (ie that one where the controller is attached to).
>>
>> Eh, at least the usage of CURR in the code is weird. I understand what
>> it means in the log page, but the usage here is strange. I suggest to
>> use a different enumeration naming in the code and leave CURR to the
>> log page, where it makes sense.
>>
> Okay, will be doing so.
> 
>>>
>>>>> +        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;
>>>>>       }
>>>>
>>>> Why use the port discovery subsys? why not just use the standard
>>>> discovery subsys?
>>>>
>>>
>>> Because (due to TPAR8014) we have to return information about the 
>>> current discovery subsystem, including all port details.
>>
>> So you got a discovery log page request on the default discovery
>> subsys and you are filling the log page with content of the unique
>> discovery subsys? The current discovery subsystem is the default
>> discovery subsystem in this case, no?
>>
>> Maybe I'm missing something...
>>
> That's what TP8013 is all about.
> The unique discovery subsystem has to react to both subsystem NQNs, the 
> unique NQN _and_ the default discovery subsystem NQN.
> But there will be only _one_ discovery subsystem per port.

Not sure why the spec mandates that, but OK.
In essence the unique discovery subsystem "takes over" the port
and fills up whatever it has, regardless of the disc-subsys NQN
the host is accessing nor accessed in the past...

>>>>>       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);
>>>>
>>>> Why remove the subsys argument from this function?
>>>>
>>>
>>> Because it's not needed?
>>> But I can make it a separate patch if you prefer.
>>
>> But different hosts can be connected to different discovery subsystems.
>>
> 
> Sure. But the 'subsys' argument is unused even in the current code.
> 
>> Is the implementation intent is that you always use the unique
>> discovery subsystem also when hosts refer to the default discovery
>> subsystem?
>>
> Yes. That is what TPAR8013 mandates.

I wasn't aware of that. This makes a stronger case to why the unique
disc subsys should behave similar to the standard one.

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.

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.

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

[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