[PATCH 0/2] nvme-discover: Use 'nvme_info' sysfs attribute for fc transport

[PATCH 0/2] nvme-discover: Use 'nvme_info' sysfs attribute for fc transport

[Hannes Reinecke]

Hi all,

here's a small patchset to use the 'nvme_info' sysfs attribute from the SCSI
host to provide the parameters for 'nvme discover'.
So with this patch it's sufficient to state

nvme discover -t fc

and all discovery log entries are returned.

I know that sysfs attribute is non-standard, but I'll be working on a patch
to implement it for qla2xxx, too.

As usual, comments and reviews are welcome.

Hannes Reinecke (2):
  nvme-discover: use ASCII characters in documentation
  nvme-discover: parse 'nvme_info' sysfs attribute for fc transport

 Documentation/nvme-discover.txt |   7 +-
 fabrics.c                       | 193 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 198 insertions(+), 2 deletions(-)

-- 
2.16.4

[PATCH 1/2] nvme-discover: use ASCII characters in documentation

[Hannes Reinecke]

Somehow a non-ASCII 's' slipped into the documentation for
'nvme discover'.

Signed-off-by: Hannes Reinecke <hare at suse.com>
---
 Documentation/nvme-discover.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/nvme-discover.txt b/Documentation/nvme-discover.txt
index 51d63e4..b4ae2fc 100644
--- a/Documentation/nvme-discover.txt
+++ b/Documentation/nvme-discover.txt
@@ -29,7 +29,7 @@ Discovery commands to run.  If no /etc/nvme/discovery.conf file
 exists, the command will quit with an error.
 
 Otherwise, a specific Discovery Controller should be specified using the
---transport, --traddr, and if necessary the --trsvcid flags. A Di?covery
+--transport, --traddr, and if necessary the --trsvcid flags. A Discovery
 request will then be sent to the specified Discovery Controller.
 
 BACKGROUND
-- 
2.16.4

[PATCH 2/2] nvme-connect: parse 'nvme_info' sysfs attribute for fc transport

[Hannes Reinecke]

The 'nvme_info' sysfs attribute for the SCSI host already contains
all relevant information to issue a NVMe discovery. So for those
cases we could use that information to format a discovery command
and don't have to specify a discovery.conf file.

This patch adds the infrastructure for parsing the information in
the 'nvme_info' sysfs attribute and uses that information to attempt
to connect to the discovery controller on that transport address.

The user can select this feature by using 'nvme discover -t fc'.

Signed-off-by: Hannes Reinecke <hare at suse.com>
---
 Documentation/nvme-discover.txt |   5 ++
 fabrics.c                       | 193 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 197 insertions(+), 1 deletion(-)

diff --git a/Documentation/nvme-discover.txt b/Documentation/nvme-discover.txt
index b4ae2fc..7a9fad2 100644
--- a/Documentation/nvme-discover.txt
+++ b/Documentation/nvme-discover.txt
@@ -28,6 +28,11 @@ find a /etc/nvme/discovery.conf file to use to supply a list of
 Discovery commands to run.  If no /etc/nvme/discovery.conf file
 exists, the command will quit with an error.
 
+If just --transport=fc is specified, then 'nvme discover' will attempt
+to read the sysfs attribute 'nvme_info' from the SCSI Host sysfs
+directory, and send the Discovery request to the Discovery Controllers
+specified by the transport addresses found in that file.
+
 Otherwise, a specific Discovery Controller should be specified using the
 --transport, --traddr, and if necessary the --trsvcid flags. A Discovery
 request will then be sent to the specified Discovery Controller.
diff --git a/fabrics.c b/fabrics.c
index 9bf2594..190991a 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -34,6 +34,7 @@
 #include <libgen.h>
 #include <sys/stat.h>
 #include <stddef.h>
+#include <limits.h>
 
 #include "parser.h"
 #include "nvme-ioctl.h"
@@ -756,6 +757,193 @@ static int do_discover(char *argstr, bool connect)
 	return ret;
 }
 
+static int parse_nvme_traddr(char *buf, char *portstr,
+			     uint64_t *wwnn, uint64_t *wwpn)
+{
+	char *str = buf, *eptr;
+	char *port;
+
+	port = strstr(str, portstr);
+	if (!port)
+		return 0;
+
+	str = strstr(port, "WWPN");
+	if (!str) {
+		fprintf(stderr, "failed to find WWPN\n");
+		return port - buf;
+	}
+	str += 5;
+	if (*str != 'x') {
+		fprintf(stderr, "parse error looking for WWPN %s\n",
+			str);
+		return str - buf;
+	}
+	str++;
+	*wwpn = strtoul(str, &eptr, 16);
+	if (str == eptr || *wwpn == ULONG_MAX) {
+		fprintf(stderr, "WWPN parse error for %s\n", str);
+		return str - buf;
+	}
+	str = strstr(eptr, "WWNN");
+	if (!str) {
+		fprintf(stderr, "failed to find lport WWNN\n");
+		return eptr - buf;
+	}
+	str += 5;
+	if (*str != 'x') {
+		fprintf(stderr, "parse error looking for WWNN %s\n",
+			str);
+		return str - buf;
+	}
+	str++;
+	*wwnn = strtoul(str, &eptr, 16);
+	if (str == eptr || *wwnn == ULONG_MAX) {
+		fprintf(stderr, "WWNN parse error for %s\n", str);
+		return str - buf;
+	}
+	return eptr - buf;
+}
+
+static int parse_nvme_info(char *nvme_info_path, char *argstr, int max_len,
+			   bool connect)
+{
+	int fd, ret, len;
+	struct stat stat;
+	char *buf, *str, *traddr, *host_traddr;
+	off_t buf_len;
+	uint64_t lpwwpn, lpwwnn, wwpn, wwnn;
+
+	fd = open(nvme_info_path, O_RDONLY);
+	if (fd < 0)
+		return fd;
+
+	if (fstat(fd, &stat) < 0) {
+		perror("stat");
+		return -1;
+	}
+	buf = malloc(stat.st_size);
+	if (!buf) {
+		perror("malloc");
+		return -ENOMEM;
+	}
+	buf_len = read(fd, buf, stat.st_size);
+	if (buf_len < 0) {
+		perror("read");
+		return -EIO;
+	}
+	close(fd);
+
+	ret = parse_nvme_traddr(buf, "LPORT", &lpwwnn, &lpwwpn);
+	if (ret <= 0)
+		return -EINVAL;
+
+	ret = asprintf(&host_traddr, "nn-0x%"PRIx64":pn-0x%"PRIx64"",
+		       lpwwnn, lpwwpn);
+	if (ret < 0)
+		return ret;
+
+	cfg.host_traddr = host_traddr;
+
+	str = buf + ret;
+
+	while (str < buf + buf_len) {
+		int err;
+
+		len = parse_nvme_traddr(str, "RPORT", &wwnn, &wwpn);
+		if (len <= 0)
+			break;
+
+		if (asprintf(&traddr, "nn-0x%"PRIx64":pn-0x%"PRIx64"",
+			     wwnn, wwpn) < 0)
+			break;
+
+		cfg.traddr = traddr;
+
+		err = build_options(argstr, BUF_SIZE);
+		if (!err)
+			err = do_discover(argstr, connect);
+
+		if (err)
+			ret = err;
+		free(traddr);
+		cfg.traddr = NULL;
+
+		str += len;
+	}
+	free(buf);
+
+	free(host_traddr);
+	cfg.host_traddr = NULL;
+
+	return ret;
+}
+
+static int scan_sys_nvme_info_filter(const struct dirent *d)
+{
+	if (!strcmp(d->d_name, "nvme_info"))
+		return 1;
+	return 0;
+}
+
+static int discover_scsi_hosts(const char *shost, char *argstr, bool connect)
+{
+	char *path, nvme_path[257];
+	struct dirent **nvme_info;
+	int i, n, ret, max_len = BUF_SIZE;
+
+	ret = asprintf(&path, "/sys/class/scsi_host/%s", shost);
+	if (ret < 0)
+		return -ENOMEM;
+
+	n = scandir(path, &nvme_info, scan_sys_nvme_info_filter, alphasort);
+	if (n < 0)
+		return n;
+
+	for (i = 0; i < n; i++) {
+		strcpy(nvme_path, path);
+		strcat(nvme_path, "/");
+		strncat(nvme_path, nvme_info[i]->d_name, 256);
+		ret += parse_nvme_info(nvme_path, argstr, max_len, connect);
+	}
+
+	for (i = 0; i < n; i++)
+		free(nvme_info[i]);
+
+	free(nvme_info);
+	return ret;
+}
+
+static int scan_sys_scsi_filter(const struct dirent *d)
+{
+	if (!strcmp(d->d_name, "."))
+		return 0;
+	if (!strcmp(d->d_name, ".."))
+		return 0;
+	return 1;
+}
+
+static int discover_from_nvme_info(const char *desc, char *argstr,
+		const struct argconfig_commandline_options *opts, bool connect)
+{
+	struct dirent **scsi_hosts = NULL;
+	int i, n, ret = 0;
+
+	n = scandir("/sys/class/scsi_host", &scsi_hosts, scan_sys_scsi_filter,
+		    alphasort);
+	if (n < 0)
+		return n;
+
+	for (i = 0; i < n; i++)
+		ret += discover_scsi_hosts(scsi_hosts[i]->d_name,
+					   argstr, connect);
+
+	for (i = 0; i < n; i++)
+		free(scsi_hosts[i]);
+
+	free(scsi_hosts);
+	return ret;
+}
+
 static int discover_from_conf_file(const char *desc, char *argstr,
 		const struct argconfig_commandline_options *opts, bool connect)
 {
@@ -844,7 +1032,10 @@ int discover(const char *desc, int argc, char **argv, bool connect)
 
 	cfg.nqn = NVME_DISC_SUBSYS_NAME;
 
-	if (!cfg.transport && !cfg.traddr) {
+	if (cfg.transport && !cfg.traddr && !strncmp(cfg.transport, "fc", 2)) {
+		return discover_from_nvme_info(desc, argstr,
+				command_line_options, connect);
+	} else if (!cfg.transport && !cfg.traddr) {
 		return discover_from_conf_file(desc, argstr,
 				command_line_options, connect);
 	} else {
-- 
2.16.4

[PATCH 2/2] nvme-connect: parse 'nvme_info' sysfs attribute for fc transport

[James Smart]

On 8/20/2018 3:34 AM, Hannes Reinecke wrote:
> The 'nvme_info' sysfs attribute for the SCSI host already contains
> all relevant information to issue a NVMe discovery. So for those
> cases we could use that information to format a discovery command
> and don't have to specify a discovery.conf file.
>
> This patch adds the infrastructure for parsing the information in
> the 'nvme_info' sysfs attribute and uses that information to attempt
> to connect to the discovery controller on that transport address.
>
> The user can select this feature by using 'nvme discover -t fc'.
>
> Signed-off-by: Hannes Reinecke <hare at suse.com>
> ---
>   Documentation/nvme-discover.txt |   5 ++
>   fabrics.c                       | 193 +++++++++++++++++++++++++++++++++++++++-
>   2 files changed, 197 insertions(+), 1 deletion(-)

Well....?? I know it probably works well, but it was not in the 
direction I was going.

Here's my issues:
1. the nvme_info sysfs parameter is not part of any transport and is a 
lpfc-specific attribute. So adding this into nvmecli concerns me. I 
don't know that lpfc has even considered the text format sufficient for 
being an "api" that can be called by "nvmecli" - meaning I don't know 
that it will pass the backward-compatibility tests.
2. What you are looking for is unnecessary: The patch to repost 
connectivity events has been post. Christoph just needs to review it 
again. I'll repost.
3. I also don't like parsing scsi stuff in the nvme cli. If we really 
need to pull fc-isms from it, then we should start the work for moving 
the fc transport out of scsi and into it's own layer that binds with 
scsi and nvme.? But I'd rather finish the fixes in nvme cli to address 
async connections, retries while controller being reconnected to, etc - 
that all enable the simple udev script.

-- james

[PATCH 0/2] nvme-discover: Use 'nvme_info' sysfs attribute for fc transport

[Sagi Grimberg]

> Hi all,
> 
> here's a small patchset to use the 'nvme_info' sysfs attribute from the SCSI
> host to provide the parameters for 'nvme discover'.
> So with this patch it's sufficient to state
> 
> nvme discover -t fc
> 
> and all discovery log entries are returned.

Using arbitrary nvme info from scsi lpfc implementation is not going to
work... We need something that can be more generic and bound within
nvme.

Besides, fc already has udev infra to auto connect IIRC.

[PATCH 2/2] nvme-connect: parse 'nvme_info' sysfs attribute for fc transport

[Hannes Reinecke]

On 08/20/2018 06:23 PM, James Smart wrote:
> On 8/20/2018 3:34 AM, Hannes Reinecke wrote:
>> The 'nvme_info' sysfs attribute for the SCSI host already contains
>> all relevant information to issue a NVMe discovery. So for those
>> cases we could use that information to format a discovery command
>> and don't have to specify a discovery.conf file.
>>
>> This patch adds the infrastructure for parsing the information in
>> the 'nvme_info' sysfs attribute and uses that information to attempt
>> to connect to the discovery controller on that transport address.
>>
>> The user can select this feature by using 'nvme discover -t fc'.
>>
>> Signed-off-by: Hannes Reinecke <hare at suse.com>
>> ---
>> ? Documentation/nvme-discover.txt |?? 5 ++
>> ? fabrics.c?????????????????????? | 193
>> +++++++++++++++++++++++++++++++++++++++-
>> ? 2 files changed, 197 insertions(+), 1 deletion(-)
> 
> Well....?? I know it probably works well, but it was not in the
> direction I was going.
> 
> Here's my issues:
> 1. the nvme_info sysfs parameter is not part of any transport and is a
> lpfc-specific attribute. So adding this into nvmecli concerns me. I
> don't know that lpfc has even considered the text format sufficient for
> being an "api" that can be called by "nvmecli" - meaning I don't know
> that it will pass the backward-compatibility tests.
> 2. What you are looking for is unnecessary: The patch to repost
> connectivity events has been post. Christoph just needs to review it
> again. I'll repost.
> 3. I also don't like parsing scsi stuff in the nvme cli. If we really
> need to pull fc-isms from it, then we should start the work for moving
> the fc transport out of scsi and into it's own layer that binds with
> scsi and nvme.? But I'd rather finish the fixes in nvme cli to address
> async connections, retries while controller being reconnected to, etc -
> that all enable the simple udev script.
> 
Well, _actually_ I sort-of-expected this response :-)
It was primarily a reminder to folks that there are open issues where we
need to look into.
(And actually I'm waiting for TP8002 to be ratified, as this implements
quite some key logic I'd need...)

My plan here is as follows:

- Implement an 'async-connect' option to nvme-cli, which would instruct
the kernel code to _not_ wait for the connect/reconnect workqueue
function but rather return immediately.
- Implement a workqueue function for the discovery controller to
retrieve the discovery log page, store it in a sysfs attribute, and send
an uevent for each found subsystem NQN.
- Invoke this workqueue whenever 'nvme discover' is called with the
async-connect option enabled.

With this we have the following workflow:

1. FC-NVMe posts an uevent whenever an FC-NVMe target port has been found
2. A udev rule parses this event and calls 'nvme discover
--async-connect' to the target address of these ports
3. The 'discovery' workqueue function retrieves the discovery log page
and issues uevents for each found subsystem NQN
4. A udev rule parses this event and calls 'nvme connect
--async-connect' for each of these events

The first step is actually optional; we can invoke 'nvme discover
--async-connect' even manually from a systemd service during boot up.
Which has the nice advantage of working for RDMA, too.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare at suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: F. Imend?rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N?rnberg)

[PATCH 2/2] nvme-connect: parse 'nvme_info' sysfs attribute for fc transport

[Johannes Thumshirn]

On Mon, Aug 20, 2018@09:23:20AM -0700, James Smart wrote:
> 3. I also don't like parsing scsi stuff in the nvme cli. If we really need
> to pull fc-isms from it, then we should start the work for moving the fc
> transport out of scsi and into it's own layer that binds with scsi and
> nvme.

This is something I'd love to see actually. Something like net/fc or
drivers/fc. IIRC we had drivers/fc back in the 2.4 days, didn't we?

Byte,
	Johannes
-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850