Nvmet discovery implementation. Questions and a patch / proposal.

* Nvmet discovery implementation. Questions and a patch / proposal.
@ 2022-02-28 16:39 Mark Ruijter
  2022-03-01  7:05 ` Hannes Reinecke
  0 siblings, 1 reply; 6+ messages in thread
From: Mark Ruijter @ 2022-02-28 16:39 UTC (permalink / raw)
  To: linux-nvme; +Cc: Sagi Grimberg, Hannes Reinecke

[-- Attachment #1: Type: text/plain, Size: 5772 bytes --]


The current discovery implementation only works after a subsystem has been created and has been linked to a port.
As a result, 'nvme discover' from an initiator will only show a discovery device after the first subsystem has been exported.

The patch that I propose adds these features:
A. Make discovery work immediately after the target port has been configured, without needing to configure subsystems first.
B. Move the discovery_nqn configuration option to the port instead of having the same configuration option duplicated in every subsystem.
C. Allow to enable and disable a target port without needing to unlink target subsystems.

For example, when running discovery on a system with a configured target port, without exported subsystems, this is the result:

root@saturn:/sys/kernel/config/nvmet/subsystems# ls -l
total 0
root@saturn:/sys/kernel/config/nvmet/subsystems# nvme discover -t tcp -a 10.100.10.1 -s 4420
Failed to write to /dev/nvme-fabrics: Connection refused

To make discovery work currently requires something like this:

root@saturn:/sys/kernel/config/nvmet/subsystems# mkdir vol1
root@saturn:/sys/kernel/config/nvmet/subsystems# cd vol1/
root@saturn:/sys/kernel/config/nvmet/subsystems/vol1# echo 1 > attr_allow_any_host 
root@saturn:/sys/kernel/config/nvmet/subsystems/vol1# cd namespaces/
root@saturn:/sys/kernel/config/nvmet/subsystems/vol1/namespaces# mkdir 1
root@saturn:/sys/kernel/config/nvmet/subsystems/vol1/namespaces# cd 1
root@saturn:/sys/kernel/config/nvmet/subsystems/vol1/namespaces/1# echo /dev/nullb0 > device_path 
root@saturn:/sys/kernel/config/nvmet/subsystems/vol1/namespaces/1# echo 1 > enable 
root@saturn:/sys/kernel/config/nvmet/subsystems/vol1/namespaces/1# cd /sys/kernel/config/nvmet/ports/10120/
root@saturn:/sys/kernel/config/nvmet/ports/10120# cd subsystems/
root@saturn:/sys/kernel/config/nvmet/ports/10120/subsystems# ln -s /sys/kernel/config/nvmet/subsystems/vol1 vol1
root@saturn:/sys/kernel/config/nvmet/ports/10120/subsystems# nvme discover -t tcp -a 10.100.10.1 -s 4420

Discovery Log Number of Records 2, Generation counter 2
=====Discovery Log Entry 0======
trtype:  tcp
adrfam:  ipv4
subtype: unrecognized
treq:    not specified, sq flow control disable supported
portid:  10120
trsvcid: 4420
subnqn:  nqn.2014-08.org.nvmexpress.discovery
traddr:  10.100.10.1
sectype: none
=====Discovery Log Entry 1======
trtype:  tcp
adrfam:  ipv4
subtype: nvme subsystem
treq:    not specified, sq flow control disable supported
portid:  10120
trsvcid: 4420
subnqn:  vol1
traddr:  10.100.10.1
sectype: none

What is also misleading is that changing the discovery_nqn from a subsystem also changes the discovery_nqn for every other subsystem.
Since the attribute exists in every subsystem, a user could assume that it's only related to the subsystem.

root@saturn:/sys/kernel/config/nvmet/subsystems# cat vol1/attr_discovery_nqn 
nqn.2014-08.org.nvmexpress.discovery
root@saturn:/sys/kernel/config/nvmet/subsystems# echo nqn.2014-08.org.testme.discovery > vol2/attr_discovery_nqn 
root@saturn:/sys/kernel/config/nvmet/subsystems# cat vol1/attr_discovery_nqn 
nqn.2014-08.org.testme.discovery

In my opinion the preferred behavior would be that an initiator can discover a target port *immediately* after it has been configured.
Even when it doesn't have exported subsystems it is already extremely useful to show the discovery device.
This can be used to verify that clients and the targets can establish communication.

Since the discovery_nqn is not uniquely related to a subsystem, it should not be configured as part of a subsystem.
The patch attached to this email illustrates a slightly different discovery implementation.

First configure a target port.
root@saturn:/sys/kernel/config/nvmet/ports/10120# cat addr_traddr 
10.100.10.1
root@saturn:/sys/kernel/config/nvmet/ports/10120# cat addr_trtype 
tcp
root@saturn:/sys/kernel/config/nvmet/ports/10120# cat param_discovery_nqn 
nqn.2014-08.org.nvmexpress.discovery
cat param_enable_target 
0

At this point, even though the target has been configured, discovery still fails because the target port is still disabled.

root@saturn:/sys/kernel/config/nvmet/ports/10120# nvme discover -t tcp -a 10.100.10.1 -s 4420
Failed to write to /dev/nvme-fabrics: Connection refused
root@saturn:/sys/kernel/config/nvmet/ports/10120#

*After enabling the target discovery immediately works.*

root@saturn:/sys/kernel/config/nvmet/ports/10120# echo 1 > param_enable_target 
root@saturn:/sys/kernel/config/nvmet/ports/10120# nvme discover -t tcp -a 10.100.10.1 -s 4420

Discovery Log Number of Records 1, Generation counter 0
=====Discovery Log Entry 0======
trtype:  tcp
adrfam:  ipv4
subtype: unrecognized
treq:    not specified, sq flow control disable supported
portid:  10120
trsvcid: 4420
subnqn:  nqn.2014-08.org.nvmexpress.discovery
traddr:  10.100.10.1
sectype: none

I have removed the discovery_nqn configuration attribute from the subsystems:
root@saturn:/sys/kernel/config/nvmet/subsystems/vol1# ls
allowed_hosts  attr_allow_any_host  attr_cntlid_max  attr_cntlid_min  attr_model  attr_pi_enable  attr_serial  attr_version  namespaces

Maybe the discovery_nqn should be configurable per target port. 
This is not the case for that patch that I have provided. 

For example:
root@saturn:/sys/kernel/config/nvmet/ports# echo "nqn.2014-08.org.test.discovery" > 10120/param_discovery_nqn
root@saturn:/sys/kernel/config/nvmet/ports# cat 10030/param_discovery_nqn 
nqn.2014-08.org.test.discovery

Not sure if there is a real use case for having different discovery nqns for different target ports.

Is this making sense or am I missing something?



[-- Attachment #2: discover_example.patch --]
[-- Type: application/octet-stream, Size: 3773 bytes --]

diff --git drivers/nvme/target/configfs.c drivers/nvme/target/configfs.c
index 091a0ca..ce05023 100644
--- drivers/nvme/target/configfs.c
+++ drivers/nvme/target/configfs.c
@@ -276,6 +276,69 @@ static ssize_t nvmet_param_pi_enable_store(struct config_item *item,
 CONFIGFS_ATTR(nvmet_, param_pi_enable);
 #endif
 
+static ssize_t nvmet_param_enable_target_show(struct config_item *item, char *page)
+{
+	struct nvmet_port *port = to_nvmet_port(item);
+        return snprintf(page, PAGE_SIZE, "%d\n",
+                        port->enabled);
+}
+
+static ssize_t nvmet_param_enable_target_store(struct config_item *item,
+                const char *page, size_t count)
+{
+	struct nvmet_port *port = to_nvmet_port(item);
+	bool val;
+	int ret = 0;
+
+	if (strtobool(page, &val))
+		return -EINVAL;
+	down_write(&nvmet_config_sem);
+	if (val) {
+		if (!port->enabled)
+			ret = nvmet_enable_port(port);
+	} else {
+		if (port->enabled)
+			nvmet_disable_port(port);
+	}
+	up_write(&nvmet_config_sem);
+	if (ret)
+		return -EINVAL;
+	return count;
+}
+
+CONFIGFS_ATTR(nvmet_, param_enable_target);
+
+static ssize_t nvmet_param_discovery_nqn_show(struct config_item *item,
+                        char *page)
+{
+        return snprintf(page, PAGE_SIZE, "%s\n",
+                        nvmet_disc_subsys->subsysnqn);
+}
+
+static ssize_t nvmet_param_discovery_nqn_store(struct config_item *item,
+                        const char *page, size_t count)
+{
+        char *subsysnqn;
+        int len;
+
+        len = strcspn(page, "\n");
+        if (!len)
+                return -EINVAL;
+
+        subsysnqn = kmemdup_nul(page, len, GFP_KERNEL);
+        if (!subsysnqn)
+                return -ENOMEM;
+
+        down_write(&nvmet_config_sem);
+        kfree(nvmet_disc_subsys->subsysnqn);
+        nvmet_disc_subsys->subsysnqn = subsysnqn;
+        up_write(&nvmet_config_sem);
+
+        return count;
+}
+
+CONFIGFS_ATTR(nvmet_, param_discovery_nqn);
+
 static ssize_t nvmet_addr_trtype_show(struct config_item *item,
 		char *page)
 {
@@ -1233,44 +1296,6 @@ static ssize_t nvmet_subsys_attr_model_store(struct config_item *item,
 }
 CONFIGFS_ATTR(nvmet_subsys_, attr_model);
 
-static ssize_t nvmet_subsys_attr_discovery_nqn_show(struct config_item *item,
-			char *page)
-{
-	return snprintf(page, PAGE_SIZE, "%s\n",
-			nvmet_disc_subsys->subsysnqn);
-}
-
-static ssize_t nvmet_subsys_attr_discovery_nqn_store(struct config_item *item,
-			const char *page, size_t count)
-{
-	struct nvmet_subsys *subsys = to_subsys(item);
-	char *subsysnqn;
-	int len;
-
-	len = strcspn(page, "\n");
-	if (!len)
-		return -EINVAL;
-
-	subsysnqn = kmemdup_nul(page, len, GFP_KERNEL);
-	if (!subsysnqn)
-		return -ENOMEM;
-
-	/*
-	 * The discovery NQN must be different from subsystem NQN.
-	 */
-	if (!strcmp(subsysnqn, subsys->subsysnqn)) {
-		kfree(subsysnqn);
-		return -EBUSY;
-	}
-	down_write(&nvmet_config_sem);
-	kfree(nvmet_disc_subsys->subsysnqn);
-	nvmet_disc_subsys->subsysnqn = subsysnqn;
-	up_write(&nvmet_config_sem);
-
-	return count;
-}
-CONFIGFS_ATTR(nvmet_subsys_, attr_discovery_nqn);
-
 #ifdef CONFIG_BLK_DEV_INTEGRITY
 static ssize_t nvmet_subsys_attr_pi_enable_show(struct config_item *item,
 						char *page)
@@ -1300,7 +1325,6 @@ 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_discovery_nqn,
 #ifdef CONFIG_BLK_DEV_INTEGRITY
 	&nvmet_subsys_attr_attr_pi_enable,
 #endif
@@ -1610,6 +1634,8 @@ static struct configfs_attribute *nvmet_port_attrs[] = {
 #ifdef CONFIG_BLK_DEV_INTEGRITY
 	&nvmet_attr_param_pi_enable,
 #endif
+	&nvmet_attr_param_discovery_nqn,
+	&nvmet_attr_param_enable_target,
 	NULL,
 };
 

^ permalink raw reply related	[flat|nested] 6+ messages in thread