[PATCH 0/3] nvmet: unique discovery subsystem

[PATCH 0/3] nvmet: unique discovery subsystem

[Hannes Reinecke]

Hi all,

here's my next attempt to support unique discovery subsystems.
The main idea is to make the subsystem type configurable; if it's being
set to 'discovery' it'll replace the static discovery subsystem.
The admin then need to configure the subsystem as normal by linking
it into the ports where the discovery subsystem should be visible.
And the discovery log then includes all configured ports to all
configured subsystems on the port providing the discovery controller.

As usual, comments and reviews are welcome.

Hannes Reinecke (3):
  nvmet: check for subsystem type in nvmet_find_get_subsys()
  nvmet: make the subsystem type configurable
  nvmet: include all configured ports in discovery log page for unique
    discover controller

 drivers/nvme/target/configfs.c  | 65 ++++++++++++++++++++++++
 drivers/nvme/target/core.c      | 23 ++++++---
 drivers/nvme/target/discovery.c | 89 +++++++++++++++++++++++++--------
 drivers/nvme/target/nvmet.h     |  3 ++
 4 files changed, 154 insertions(+), 26 deletions(-)

-- 
2.29.2

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

[Hannes Reinecke]

When looking for a subsystem we have two ways of finding the
subsystem: either looking for the subsystem NQN itself, or check
the type of the subsystem when looking for a discovery controller.
This patch implements this check, and also moves the check for
the static discovery controller to the end such that we can
return unique discovery controllers.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/target/core.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index b0dc6230d1b9..83eba511d098 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1508,12 +1508,6 @@ static struct nvmet_subsys *nvmet_find_get_subsys(struct nvmet_port *port,
 	if (!port)
 		return NULL;
 
-	if (!strcmp(NVME_DISC_SUBSYS_NAME, subsysnqn)) {
-		if (!kref_get_unless_zero(&nvmet_disc_subsys->ref))
-			return NULL;
-		return nvmet_disc_subsys;
-	}
-
 	down_read(&nvmet_config_sem);
 	list_for_each_entry(p, &port->subsystems, entry) {
 		if (!strncmp(p->subsys->subsysnqn, subsysnqn,
@@ -1523,8 +1517,22 @@ static struct nvmet_subsys *nvmet_find_get_subsys(struct nvmet_port *port,
 			up_read(&nvmet_config_sem);
 			return p->subsys;
 		}
+		if (!strcmp(NVME_DISC_SUBSYS_NAME, subsysnqn) &&
+		    nvmet_is_disc_subsys(p->subsys)) {
+			if (!kref_get_unless_zero(&p->subsys->ref))
+				break;
+			up_read(&nvmet_config_sem);
+			return p->subsys;
+		}
 	}
 	up_read(&nvmet_config_sem);
+
+	if (!strcmp(NVME_DISC_SUBSYS_NAME, subsysnqn)) {
+		if (!kref_get_unless_zero(&nvmet_disc_subsys->ref))
+			return NULL;
+		return nvmet_disc_subsys;
+	}
+
 	return NULL;
 }
 
-- 
2.29.2

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

[Hannes Reinecke]

Make the subsystem type configurable to allow for unique
discovery subsystems by changing the subsystem type to
'discovery'.

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

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 57a6795a2d8d..ae6815b72d7d 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -930,6 +930,9 @@ static void nvmet_port_subsys_drop_link(struct config_item *parent,
 
 found:
 	list_del(&p->entry);
+	/* Reset to static discovery controller */
+	if (nvmet_disc_subsys == subsys)
+		nvmet_set_disc_subsys(NULL);
 	nvmet_port_del_ctrls(port, subsys);
 	nvmet_port_disc_changed(port, subsys);
 
@@ -1302,6 +1305,67 @@ static ssize_t nvmet_subsys_attr_model_store(struct config_item *item,
 }
 CONFIGFS_ATTR(nvmet_subsys_, attr_model);
 
+static const struct nvmet_type_name_map nvmet_subsys_type_map[] = {
+	{ NVME_NQN_DISC,	"referral" },
+	{ NVME_NQN_NVME,	"nvme" },
+	{ NVME_NQN_CURR,	"discovery" },
+};
+
+static ssize_t nvmet_subsys_attr_type_show(struct config_item *item,
+						 char *page)
+{
+	u8 type = to_subsys(item)->type;
+	int i;
+
+	for (i = 1; i < ARRAY_SIZE(nvmet_subsys_type_map); i++) {
+		if (nvmet_subsys_type_map[i].type == type)
+			return snprintf(page, PAGE_SIZE, "%s\n",
+					nvmet_subsys_type_map[i].name);
+	}
+
+	return snprintf(page, PAGE_SIZE, "\n");
+}
+
+static ssize_t nvmet_subsys_attr_type_store(struct config_item *item,
+					    const char *page, size_t count)
+{
+	struct nvmet_subsys *subsys = to_subsys(item);
+	int i;
+
+	if (subsys->subsys_discovered)
+		return -EACCES;
+
+	for (i = 0; i < ARRAY_SIZE(nvmet_subsys_type_map); i++) {
+		if (sysfs_streq(page, nvmet_subsys_type_map[i].name))
+			goto found;
+	}
+
+	pr_err("Invalid value '%s' for subsystem type\n", page);
+	return -EINVAL;
+
+found:
+	down_write(&nvmet_config_sem);
+	if (nvmet_subsys_type_map[i].type == NVME_NQN_CURR) {
+		if (nvmet_has_unique_disc_subsys()) {
+			pr_err("unique discovery subsystem already present\n");
+			return -EINVAL;
+		}
+		if (!xa_empty(&subsys->namespaces)) {
+			pr_err("discovery subsystem cannot have namespaces\n");
+			return -EINVAL;
+		}
+		nvmet_subsys_del_ctrls(nvmet_disc_subsys);
+		nvmet_set_disc_subsys(subsys);
+	} else if (subsys->type == NVME_NQN_CURR)
+		nvmet_set_disc_subsys(NULL);
+
+	subsys->type = nvmet_subsys_type_map[i].type;
+	up_write(&nvmet_config_sem);
+
+	return count;
+}
+CONFIGFS_ATTR(nvmet_subsys_, attr_type);
+
 #ifdef CONFIG_BLK_DEV_INTEGRITY
 static ssize_t nvmet_subsys_attr_pi_enable_show(struct config_item *item,
 						char *page)
@@ -1331,6 +1395,7 @@ static struct configfs_attribute *nvmet_subsys_attrs[] = {
 	&nvmet_subsys_attr_attr_cntlid_min,
 	&nvmet_subsys_attr_attr_cntlid_max,
 	&nvmet_subsys_attr_attr_model,
+	&nvmet_subsys_attr_attr_type,
 #ifdef CONFIG_BLK_DEV_INTEGRITY
 	&nvmet_subsys_attr_attr_pi_enable,
 #endif
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 83eba511d098..a9d03dfe547e 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1527,6 +1527,9 @@ static struct nvmet_subsys *nvmet_find_get_subsys(struct nvmet_port *port,
 	}
 	up_read(&nvmet_config_sem);
 
+	if (nvmet_has_unique_disc_subsys())
+		return NULL;
+
 	if (!strcmp(NVME_DISC_SUBSYS_NAME, subsysnqn)) {
 		if (!kref_get_unless_zero(&nvmet_disc_subsys->ref))
 			return NULL;
diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c
index c2162eef8ce1..bceeec00099a 100644
--- a/drivers/nvme/target/discovery.c
+++ b/drivers/nvme/target/discovery.c
@@ -9,9 +9,23 @@
 #include "nvmet.h"
 
 struct nvmet_subsys *nvmet_disc_subsys;
+static struct nvmet_subsys *_nvmet_disc_subsys;
 
 static u64 nvmet_genctr;
 
+bool nvmet_has_unique_disc_subsys(void)
+{
+	return (_nvmet_disc_subsys != nvmet_disc_subsys);
+}
+
+void nvmet_set_disc_subsys(struct nvmet_subsys *subsys)
+{
+	if (subsys)
+		nvmet_disc_subsys = subsys;
+	else
+		nvmet_disc_subsys = _nvmet_disc_subsys;
+}
+
 static void __nvmet_disc_changed(struct nvmet_port *port,
 				 struct nvmet_ctrl *ctrl)
 {
@@ -393,12 +407,14 @@ u16 nvmet_parse_discovery_cmd(struct nvmet_req *req)
 
 int __init nvmet_init_discovery(void)
 {
-	nvmet_disc_subsys =
+	_nvmet_disc_subsys =
 		nvmet_subsys_alloc(NVME_DISC_SUBSYS_NAME, NVME_NQN_CURR);
+	nvmet_disc_subsys = _nvmet_disc_subsys;
 	return PTR_ERR_OR_ZERO(nvmet_disc_subsys);
 }
 
 void nvmet_exit_discovery(void)
 {
-	nvmet_subsys_put(nvmet_disc_subsys);
+	nvmet_disc_subsys = NULL;
+	nvmet_subsys_put(_nvmet_disc_subsys);
 }
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 81061aa8c6d3..658e3fd81f06 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -556,6 +556,9 @@ void nvmet_exit_discovery(void);
 extern struct nvmet_subsys *nvmet_disc_subsys;
 extern struct rw_semaphore nvmet_config_sem;
 
+bool nvmet_has_unique_disc_subsys(void);
+void nvmet_set_disc_subsys(struct nvmet_subsys *subsys);
+
 extern u32 nvmet_ana_group_enabled[NVMET_MAX_ANAGRPS + 1];
 extern u64 nvmet_ana_chgcnt;
 extern struct rw_semaphore nvmet_ana_sem;
-- 
2.29.2

[PATCH 3/3] nvmet: include all configured ports in discovery log page for unique discover controller

[Hannes Reinecke]

When an unique discovery controller is configured we should be reporting
all configured ports, and not just those which are reachable from the
current port.

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

diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c
index bceeec00099a..6cefd4f60f9f 100644
--- a/drivers/nvme/target/discovery.c
+++ b/drivers/nvme/target/discovery.c
@@ -160,12 +160,24 @@ static size_t discovery_log_entries(struct nvmet_req *req)
 	struct nvmet_ctrl *ctrl = req->sq->ctrl;
 	struct nvmet_subsys_link *p;
 	struct nvmet_port *r;
-	size_t entries = 1;
-
-	list_for_each_entry(p, &req->port->subsystems, entry) {
-		if (!nvmet_host_allowed(p->subsys, ctrl->hostnqn))
-			continue;
+	size_t entries = 0;
+
+	if (nvmet_has_unique_disc_subsys()) {
+		list_for_each_entry(r, nvmet_ports, global_entry) {
+			list_for_each_entry(p, &r->subsystems, entry) {
+				if (!nvmet_host_allowed(p->subsys,
+							ctrl->hostnqn))
+					continue;
+				entries++;
+			}
+		}
+	} else {
 		entries++;
+		list_for_each_entry(p, &req->port->subsystems, entry) {
+			if (!nvmet_host_allowed(p->subsys, ctrl->hostnqn))
+				continue;
+			entries++;
+		}
 	}
 	list_for_each_entry(r, &req->port->referrals, entry)
 		entries++;
@@ -220,23 +232,44 @@ static void nvmet_execute_disc_get_log_page(struct nvmet_req *req)
 	}
 	hdr = buffer;
 
-	nvmet_set_disc_traddr(req, req->port, traddr);
-
-	nvmet_format_discovery_entry(hdr, req->port,
-				     nvmet_disc_subsys->subsysnqn,
-				     traddr, NVME_NQN_CURR, numrec);
-	numrec++;
-
-	list_for_each_entry(p, &req->port->subsystems, entry) {
-		if (!nvmet_host_allowed(p->subsys, ctrl->hostnqn))
-			continue;
+	if (!nvmet_has_unique_disc_subsys()) {
+		nvmet_set_disc_traddr(req, req->port, traddr);
 
 		nvmet_format_discovery_entry(hdr, req->port,
-				p->subsys->subsysnqn, traddr,
-				NVME_NQN_NVME, numrec);
+					     nvmet_disc_subsys->subsysnqn,
+					     traddr, NVME_NQN_CURR, numrec);
 		numrec++;
-	}
 
+		list_for_each_entry(p, &req->port->subsystems, entry) {
+			if (!nvmet_host_allowed(p->subsys, ctrl->hostnqn))
+				continue;
+
+			nvmet_format_discovery_entry(hdr, req->port,
+					p->subsys->subsysnqn, traddr,
+					NVME_NQN_NVME, numrec);
+			numrec++;
+		}
+	} else {
+		list_for_each_entry(p, &req->port->subsystems, entry) {
+			list_for_each_entry(r, nvmet_ports, global_entry) {
+				struct nvmet_subsys_link *l;
+
+				nvmet_set_disc_traddr(req, r, traddr);
+
+				list_for_each_entry(l, &r->subsystems, entry) {
+					if (p->subsys != l->subsys)
+						continue;
+					if (!nvmet_host_allowed(l->subsys,
+							ctrl->hostnqn))
+						continue;
+					nvmet_format_discovery_entry(hdr, r,
+						l->subsys->subsysnqn, traddr,
+						l->subsys->type, numrec);
+					numrec++;
+				}
+			}
+		}
+	}
 	list_for_each_entry(r, &req->port->referrals, entry) {
 		nvmet_format_discovery_entry(hdr, r,
 				NVME_DISC_SUBSYS_NAME,
-- 
2.29.2

[PATCHv2 0/3] nvmet: unique discovery subsystem

[Hannes Reinecke]

Hi all,

here's my next attempt to support unique discovery subsystems.
The main idea is to make the subsystem type configurable; if it's being
set to 'discovery' it'll replace the static discovery subsystem.
The admin then need to configure the subsystem as normal by linking
it into the ports where the discovery subsystem should be visible.
And the discovery log then includes all configured ports to all
configured subsystems, as usual modified by whether the host may
access this subsystem or not.

As usual, comments and reviews are welcome.

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

Hannes Reinecke (3):
  nvmet: check for subsystem type in nvmet_find_get_subsys()
  nvmet: make the subsystem type configurable
  nvmet: include all configured ports in discovery log page for unique
    discover controller

 drivers/nvme/target/configfs.c  | 65 ++++++++++++++++++++++++++
 drivers/nvme/target/core.c      | 23 ++++++---
 drivers/nvme/target/discovery.c | 83 +++++++++++++++++++++++++--------
 drivers/nvme/target/nvmet.h     |  3 ++
 4 files changed, 148 insertions(+), 26 deletions(-)

-- 
2.29.2

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

[Hannes Reinecke]

Make the subsystem type configurable to allow for unique
discovery subsystems by changing the subsystem type to
'discovery'.

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

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 57a6795a2d8d..ae6815b72d7d 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -930,6 +930,9 @@ static void nvmet_port_subsys_drop_link(struct config_item *parent,
 
 found:
 	list_del(&p->entry);
+	/* Reset to static discovery controller */
+	if (nvmet_disc_subsys == subsys)
+		nvmet_set_disc_subsys(NULL);
 	nvmet_port_del_ctrls(port, subsys);
 	nvmet_port_disc_changed(port, subsys);
 
@@ -1302,6 +1305,67 @@ static ssize_t nvmet_subsys_attr_model_store(struct config_item *item,
 }
 CONFIGFS_ATTR(nvmet_subsys_, attr_model);
 
+static const struct nvmet_type_name_map nvmet_subsys_type_map[] = {
+	{ NVME_NQN_DISC,	"referral" },
+	{ NVME_NQN_NVME,	"nvme" },
+	{ NVME_NQN_CURR,	"discovery" },
+};
+
+static ssize_t nvmet_subsys_attr_type_show(struct config_item *item,
+						 char *page)
+{
+	u8 type = to_subsys(item)->type;
+	int i;
+
+	for (i = 1; i < ARRAY_SIZE(nvmet_subsys_type_map); i++) {
+		if (nvmet_subsys_type_map[i].type == type)
+			return snprintf(page, PAGE_SIZE, "%s\n",
+					nvmet_subsys_type_map[i].name);
+	}
+
+	return snprintf(page, PAGE_SIZE, "\n");
+}
+
+static ssize_t nvmet_subsys_attr_type_store(struct config_item *item,
+					    const char *page, size_t count)
+{
+	struct nvmet_subsys *subsys = to_subsys(item);
+	int i;
+
+	if (subsys->subsys_discovered)
+		return -EACCES;
+
+	for (i = 0; i < ARRAY_SIZE(nvmet_subsys_type_map); i++) {
+		if (sysfs_streq(page, nvmet_subsys_type_map[i].name))
+			goto found;
+	}
+
+	pr_err("Invalid value '%s' for subsystem type\n", page);
+	return -EINVAL;
+
+found:
+	down_write(&nvmet_config_sem);
+	if (nvmet_subsys_type_map[i].type == NVME_NQN_CURR) {
+		if (nvmet_has_unique_disc_subsys()) {
+			pr_err("unique discovery subsystem already present\n");
+			return -EINVAL;
+		}
+		if (!xa_empty(&subsys->namespaces)) {
+			pr_err("discovery subsystem cannot have namespaces\n");
+			return -EINVAL;
+		}
+		nvmet_subsys_del_ctrls(nvmet_disc_subsys);
+		nvmet_set_disc_subsys(subsys);
+	} else if (subsys->type == NVME_NQN_CURR)
+		nvmet_set_disc_subsys(NULL);
+
+	subsys->type = nvmet_subsys_type_map[i].type;
+	up_write(&nvmet_config_sem);
+
+	return count;
+}
+CONFIGFS_ATTR(nvmet_subsys_, attr_type);
+
 #ifdef CONFIG_BLK_DEV_INTEGRITY
 static ssize_t nvmet_subsys_attr_pi_enable_show(struct config_item *item,
 						char *page)
@@ -1331,6 +1395,7 @@ static struct configfs_attribute *nvmet_subsys_attrs[] = {
 	&nvmet_subsys_attr_attr_cntlid_min,
 	&nvmet_subsys_attr_attr_cntlid_max,
 	&nvmet_subsys_attr_attr_model,
+	&nvmet_subsys_attr_attr_type,
 #ifdef CONFIG_BLK_DEV_INTEGRITY
 	&nvmet_subsys_attr_attr_pi_enable,
 #endif
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 83eba511d098..a9d03dfe547e 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1527,6 +1527,9 @@ static struct nvmet_subsys *nvmet_find_get_subsys(struct nvmet_port *port,
 	}
 	up_read(&nvmet_config_sem);
 
+	if (nvmet_has_unique_disc_subsys())
+		return NULL;
+
 	if (!strcmp(NVME_DISC_SUBSYS_NAME, subsysnqn)) {
 		if (!kref_get_unless_zero(&nvmet_disc_subsys->ref))
 			return NULL;
diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c
index c2162eef8ce1..bceeec00099a 100644
--- a/drivers/nvme/target/discovery.c
+++ b/drivers/nvme/target/discovery.c
@@ -9,9 +9,23 @@
 #include "nvmet.h"
 
 struct nvmet_subsys *nvmet_disc_subsys;
+static struct nvmet_subsys *_nvmet_disc_subsys;
 
 static u64 nvmet_genctr;
 
+bool nvmet_has_unique_disc_subsys(void)
+{
+	return (_nvmet_disc_subsys != nvmet_disc_subsys);
+}
+
+void nvmet_set_disc_subsys(struct nvmet_subsys *subsys)
+{
+	if (subsys)
+		nvmet_disc_subsys = subsys;
+	else
+		nvmet_disc_subsys = _nvmet_disc_subsys;
+}
+
 static void __nvmet_disc_changed(struct nvmet_port *port,
 				 struct nvmet_ctrl *ctrl)
 {
@@ -393,12 +407,14 @@ u16 nvmet_parse_discovery_cmd(struct nvmet_req *req)
 
 int __init nvmet_init_discovery(void)
 {
-	nvmet_disc_subsys =
+	_nvmet_disc_subsys =
 		nvmet_subsys_alloc(NVME_DISC_SUBSYS_NAME, NVME_NQN_CURR);
+	nvmet_disc_subsys = _nvmet_disc_subsys;
 	return PTR_ERR_OR_ZERO(nvmet_disc_subsys);
 }
 
 void nvmet_exit_discovery(void)
 {
-	nvmet_subsys_put(nvmet_disc_subsys);
+	nvmet_disc_subsys = NULL;
+	nvmet_subsys_put(_nvmet_disc_subsys);
 }
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 81061aa8c6d3..658e3fd81f06 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -556,6 +556,9 @@ void nvmet_exit_discovery(void);
 extern struct nvmet_subsys *nvmet_disc_subsys;
 extern struct rw_semaphore nvmet_config_sem;
 
+bool nvmet_has_unique_disc_subsys(void);
+void nvmet_set_disc_subsys(struct nvmet_subsys *subsys);
+
 extern u32 nvmet_ana_group_enabled[NVMET_MAX_ANAGRPS + 1];
 extern u64 nvmet_ana_chgcnt;
 extern struct rw_semaphore nvmet_ana_sem;
-- 
2.29.2

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

[Christoph Hellwig]

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

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

If we had e.g.

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

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

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

[Hannes Reinecke]

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

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

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

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

[Christoph Hellwig]

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

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

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

[Hannes Reinecke]

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

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

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

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

[Sagi Grimberg]

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

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

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

[Hannes Reinecke]

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

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

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

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

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

[John Meneghini]

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

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

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

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

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

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

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

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

This is one possible approach.

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

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

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

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

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

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

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

/John

> Cheers,
> 
> Hannes

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

[John Meneghini]

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

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

/John