[PATCH 0/6] nvme-cli fixes and ANA updates

[PATCH 0/6] nvme-cli fixes and ANA updates

[Hannes Reinecke]

Hi Keith,

here's a small set of fixes and an update to have nvme list-subsys printing
out the ANA state for all paths.
This brings nvme-cli output on par with that from multipath-tools.

As usual, comments and reviews welcome.

Hannes Reinecke (6):
  nvme-ana-log: fixup compiler warning in show_ana_log()
  nvme-vendor: fixup c99 declaration in huawei plugin
  nvme-discover: sanitize options
  nvme-discover: Retry discovery log if the generation counter changes
  nvme-list-subsys: Print PCIe controller address
  nvme-list-subsys: Add device name argument and print out ANA state

 Documentation/nvme-discover.txt    |  13 ++-
 Documentation/nvme-list-subsys.txt |   4 +-
 fabrics.c                          |  15 ++--
 huawei-nvme.c                      |   3 +-
 nvme-print.c                       |  13 ++-
 nvme.c                             | 169 ++++++++++++++++++++++++++++++++++---
 nvme.h                             |   1 +
 7 files changed, 195 insertions(+), 23 deletions(-)

-- 
2.13.7

[PATCH 1/6] nvme-ana-log: fixup compiler warning in show_ana_log()

[Hannes Reinecke]

Add the correct type cast to keep the compiler happy.

Signed-off-by: Hannes Reinecke <hare at suse.com>
---
 nvme-print.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/nvme-print.c b/nvme-print.c
index e0da949..8a604c5 100644
--- a/nvme-print.c
+++ b/nvme-print.c
@@ -1460,7 +1460,8 @@ void show_ana_log(struct nvme_ana_rsp_hdr *ana_log, const char *devname)
 		offset += sizeof(*desc);
 		printf("grpid	:	%u\n", le32_to_cpu(desc->grpid));
 		printf("nnsids	:	%u\n", le32_to_cpu(desc->nnsids));
-		printf("chgcnt	:	%"PRIu64"\n", le64_to_cpu(desc->chgcnt));
+		printf("chgcnt	:	%"PRIu64"\n",
+		       (uint64_t)le64_to_cpu(desc->chgcnt));
 		printf("state	:	%s\n",
 				nvme_ana_state_to_string(desc->state));
 		for (j = 0; j < le32_to_cpu(desc->nnsids); j++)
-- 
2.13.7

[PATCH 2/6] nvme-vendor: fixup c99 declaration in huawei plugin

[Hannes Reinecke]

We can't use c99 declaration with older compilers.

Signed-off-by: Hannes Reinecke <hare at suse.com>
---
 huawei-nvme.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/huawei-nvme.c b/huawei-nvme.c
index f305405..f91d3e3 100644
--- a/huawei-nvme.c
+++ b/huawei-nvme.c
@@ -245,12 +245,13 @@ static void huawei_print_list_item(struct huawei_list_item list_item,
 	char usage[128];
 	char nguid_buf[2 * sizeof(list_item.ns.nguid) + 1];
 	char *nguid = nguid_buf;
+	int i;
 
 	sprintf(usage,"%6.2f %2sB / %6.2f %2sB", nuse, u_suffix,
 		nsze, s_suffix);
 
 	memset(nguid, 0, sizeof(nguid_buf));
-	for (int i = 0; i < sizeof(list_item.ns.nguid); i++)
+	for (i = 0; i < sizeof(list_item.ns.nguid); i++)
 		nguid += sprintf(nguid, "%02x", list_item.ns.nguid[i]);
 
 	printf("%-*.*s %-*.*s %-*.*s %-*d %-*.*s %-*.*s\n",
-- 
2.13.7

[PATCH 3/6] nvme-discover: sanitize options

[Hannes Reinecke]

The discovery controller doesn't accept any KATO or number of I/O
queue changes, but we do want to change the reconnect delay and
the controller loss timeout.

Signed-off-by: Hannes Reinecke <hare at suse.com>
---
 Documentation/nvme-discover.txt | 13 ++++++++++++-
 fabrics.c                       |  4 +---
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/Documentation/nvme-discover.txt b/Documentation/nvme-discover.txt
index 4fcf1ca..51d63e4 100644
--- a/Documentation/nvme-discover.txt
+++ b/Documentation/nvme-discover.txt
@@ -14,7 +14,9 @@ SYNOPSIS
 		[--trsvcid=<trsvcid>      | -s <trsvcid>]
 		[--host-traddr=<traddr>   | -w <traddr>]
 		[--hostnqn=<hostnqn>      | -q <hostnqn>]
-		[--raw=<filename>         | -r <filename>]
+		[--raw=<filename>	  | -r <filename>]
+		[--reconnect-delay=<#>	  | -c <#>]
+		[--ctrl-loss-tmo=<#>	  | -l <#>]
 
 DESCRIPTION
 -----------
@@ -99,6 +101,15 @@ OPTIONS
 	and dump it to a raw binary file. By default 'nvme discover' will
 	dump the output to stdout.
 
+-c <#>::
+--reconnect-delay=<#>::
+	Overrides the default delay (in seconds) before reconnect is attempted
+	after a connect loss.
+
+-l <#>::
+--ctrl-loss-tmo=<#>::
+	Overrides the default controller loss timeout period (in seconds).
+
 EXAMPLES
 --------
 * Query the Discover Controller with IP4 address 192.168.1.3 for all
diff --git a/fabrics.c b/fabrics.c
index 28e97c5..9bf2594 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -830,12 +830,10 @@ int discover(const char *desc, int argc, char **argv, bool connect)
 		{"host-traddr", 'w', "LIST", CFG_STRING, &cfg.host_traddr, required_argument, "host traddr (e.g. FC WWN's)" },
 		{"hostnqn",     'q', "LIST", CFG_STRING, &cfg.hostnqn,     required_argument, "user-defined hostnqn (if default not used)" },
 		{"hostid",      'I', "LIST", CFG_STRING, &cfg.hostid,      required_argument, "user-defined hostid (if default not used)"},
-		{"queue-size",  'Q', "LIST", CFG_STRING, &cfg.queue_size,  required_argument, "number of io queue elements to use (default 128)" },
-		{"nr-io-queues",'i', "LIST", CFG_STRING, &cfg.nr_io_queues,required_argument, "number of io queues to use (default is core count)" },
 		{"raw",         'r', "LIST", CFG_STRING, &cfg.raw,         required_argument, "raw output file" },
 		{"keep-alive-tmo",  'k', "LIST", CFG_STRING, &cfg.keep_alive_tmo,  required_argument, "keep alive timeout period in seconds" },
+		{"reconnect-delay", 'c', "LIST", CFG_STRING, &cfg.reconnect_delay, required_argument, "reconnect timeout period in seconds" },
 		{"ctrl-loss-tmo",   'l', "LIST", CFG_STRING, &cfg.ctrl_loss_tmo,   required_argument, "controller loss timeout period in seconds" },
-
 		{NULL},
 	};
 
-- 
2.13.7

[PATCH 4/6] nvme-discover: Retry discovery log if the generation counter changes

[Hannes Reinecke]

If the generation counter changes we need to validate if the number
of records has changed, too.
If so we need to retry retrieving the discovery log to the most recent
values. The retry will be terminated after MAX_DISC_RETRIES (currently
set to 30) to avoid infinite recursion.

Signed-off-by: Hannes Reinecke <hare at suse.com>
---
 fabrics.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/fabrics.c b/fabrics.c
index 9bf2594..b277d36 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -71,6 +71,7 @@ static struct config {
 #define PATH_NVMF_HOSTID	"/etc/nvme/hostid"
 #define SYS_NVME		"/sys/class/nvme"
 #define MAX_DISC_ARGS		10
+#define MAX_DISC_RETRIES	30
 
 enum {
 	OPT_INSTANCE,
@@ -281,7 +282,7 @@ static int nvmf_get_log_page_discovery(const char *dev_path,
 	struct nvmf_disc_rsp_page_hdr *log;
 	unsigned int log_size = 0;
 	unsigned long genctr;
-	int error, fd;
+	int error, fd, max_retries = MAX_DISC_RETRIES, retries = 0;
 
 	fd = open(dev_path, O_RDWR);
 	if (fd < 0) {
@@ -311,6 +312,7 @@ static int nvmf_get_log_page_discovery(const char *dev_path,
 		goto out_free_log;
 	}
 
+retry_log:
 	/* check numrec limits */
 	*numrec = le64_to_cpu(log->numrec);
 	genctr = le64_to_cpu(log->genctr);
@@ -346,7 +348,12 @@ static int nvmf_get_log_page_discovery(const char *dev_path,
 		goto out_free_log;
 	}
 
-	if (*numrec != le32_to_cpu(log->numrec) || genctr != le64_to_cpu(log->genctr)) {
+	if (genctr != le64_to_cpu(log->genctr) &&
+	    *numrec != le32_to_cpu(log->numrec) &&
+	    ++retries < max_retries)
+		goto retry_log;
+
+	if (*numrec != le32_to_cpu(log->numrec)) {
 		error = DISC_NOT_EQUAL;
 		goto out_free_log;
 	}
-- 
2.13.7

[PATCH 5/6] nvme-list-subsys: Print PCIe controller address

[Hannes Reinecke]

An NVMe PCIe controller doesn't have an address, so we should be
using the PCIe bus address.

Signed-off-by: Hannes Reinecke <hare at suse.com>
---
 nvme.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 50 insertions(+), 7 deletions(-)

diff --git a/nvme.c b/nvme.c
index 99640cb..0438187 100644
--- a/nvme.c
+++ b/nvme.c
@@ -1314,6 +1314,44 @@ err_free_trpath:
 	return NULL;
 }
 
+static char *get_nvme_pcie_address(char *path)
+{
+	char *devpath, *devlink, *str;
+	char *address = NULL;
+	ssize_t ret;
+
+	ret = asprintf(&devpath, "%s/device", path);
+	if (ret < 0)
+		return NULL;
+
+	devlink = calloc(1, 1024);
+	if (!devlink) {
+		free(devpath);
+		return NULL;
+	}
+
+	ret = readlink(devpath, devlink, 1024);
+	if (ret < 0) {
+		fprintf(stderr, "failed to read link %s, error %d\n",
+			devpath, (int)ret);
+		goto out;
+	}
+	str = strrchr(devlink, '/');
+	if (!str) {
+		fprintf(stderr, "invalid devlink %s\n", devlink);
+		goto out;
+	}
+	ret = asprintf(&address, "%s", str + 1);
+	if (ret < 0)
+		address = NULL;
+
+out:
+	free(devlink);
+	free(devpath);
+
+	return address;
+}
+
 static char *get_nvme_ctrl_address(char *path)
 {
 	char *addrpath;
@@ -1418,13 +1456,6 @@ int get_nvme_subsystem_info(char *name, char *path,
 		snprintf(ctrl_path, sizeof(ctrl_path), "%s/%s", path,
 			 item->ctrls[ccnt].name);
 
-		item->ctrls[ccnt].address = get_nvme_ctrl_address(ctrl_path);
-		if (!item->ctrls[ccnt].address) {
-			fprintf(stderr, "failed to get controller[%d] address.\n", i);
-			free_ctrl_list_item(&item->ctrls[ccnt]);
-			continue;
-		}
-
 		item->ctrls[ccnt].transport =
 				get_nvme_ctrl_transport(ctrl_path);
 		if (!item->ctrls[ccnt].transport) {
@@ -1433,6 +1464,18 @@ int get_nvme_subsystem_info(char *name, char *path,
 			continue;
 		}
 
+		if (!strncmp(item->ctrls[ccnt].transport, "pcie", 4))
+			item->ctrls[ccnt].address =
+				get_nvme_pcie_address(ctrl_path);
+		else
+			item->ctrls[ccnt].address =
+				get_nvme_ctrl_address(ctrl_path);
+		if (!item->ctrls[ccnt].address) {
+			fprintf(stderr, "failed to get controller[%d] address.\n", i);
+			free_ctrl_list_item(&item->ctrls[ccnt]);
+			continue;
+		}
+
 		ccnt++;
 	}
 
-- 
2.13.7

[PATCH 6/6] nvme-list-subsys: Add device name argument and print out ANA state

[Hannes Reinecke]

Update the 'nvme list-subsys' command to accept a device name and
print out the ANA state for all paths to that device.

Signed-off-by: Hannes Reinecke <hare at suse.com>
---
 Documentation/nvme-list-subsys.txt |   4 +-
 nvme-print.c                       |  10 +++-
 nvme.c                             | 114 +++++++++++++++++++++++++++++++++++--
 nvme.h                             |   1 +
 4 files changed, 120 insertions(+), 9 deletions(-)

diff --git a/Documentation/nvme-list-subsys.txt b/Documentation/nvme-list-subsys.txt
index c7de7ef..c40b708 100644
--- a/Documentation/nvme-list-subsys.txt
+++ b/Documentation/nvme-list-subsys.txt
@@ -8,12 +8,14 @@ nvme-list-subsys - List all NVMe subsystems
 SYNOPSIS
 --------
 [verse]
-'nvme list-subsys' [-o <fmt> | --output-format=<fmt>]
+'nvme list-subsys' [-o <fmt> | --output-format=<fmt>] <device>
 
 DESCRIPTION
 -----------
 Scan the sysfs tree for NVM Express subsystems and return the controllers
 for those subsystems as well as some pertinent information about them.
+If a device is given, print out only the values for the controllers
+and subsystems leading to the device.
 
 OPTIONS
 -------
diff --git a/nvme-print.c b/nvme-print.c
index 8a604c5..cc8c716 100644
--- a/nvme-print.c
+++ b/nvme-print.c
@@ -2762,9 +2762,11 @@ static void show_nvme_subsystem(struct subsys_list_item *item)
 	printf("\\\n");
 
 	for (i = 0; i < item->nctrls; i++) {
-		printf(" +- %s %s %s\n", item->ctrls[i].name,
+		printf(" +- %s %s %s %s\n", item->ctrls[i].name,
 				item->ctrls[i].transport,
-				item->ctrls[i].address);
+				item->ctrls[i].address,
+				item->ctrls[i].ana_state ?
+					item->ctrls[i].ana_state : "");
 	}
 
 }
@@ -2810,6 +2812,10 @@ void json_print_nvme_subsystem_list(struct subsys_list_item *slist, int n)
 					slist[i].ctrls[j].transport);
 			json_object_add_value_string(path_attrs, "Address",
 					slist[i].ctrls[j].address);
+			if (slist[i].ctrls[j].ana_state)
+				json_object_add_value_string(path_attrs,
+						"State",
+						slist[i].ctrls[j].ana_state);
 			json_array_add_value_object(paths, path_attrs);
 		}
 		if (j) {
diff --git a/nvme.c b/nvme.c
index 0438187..b5c9779 100644
--- a/nvme.c
+++ b/nvme.c
@@ -1398,6 +1398,72 @@ err_free_addrpath:
 
 	return NULL;
 }
+
+static int scan_ctrl_paths_filter(const struct dirent *d)
+{
+	int id, cntlid, nsid;
+
+	if (d->d_name[0] == '.')
+		return 0;
+
+	if (strstr(d->d_name, "nvme")) {
+		if (sscanf(d->d_name, "nvme%dc%dn%d", &id, &cntlid, &nsid) != 3)
+			return 0;
+		return 1;
+	}
+
+	return 0;
+}
+
+static char *get_nvme_ctrl_path_ana_state(char *path, int nsid)
+{
+	struct dirent **paths;
+	char *ana_state;
+	int i, n;
+
+	ana_state = calloc(1, 16);
+	if (!ana_state)
+		return NULL;
+
+	n = scandir(path, &paths, scan_ctrl_paths_filter, alphasort);
+	if (n <= 0) {
+		free(ana_state);
+		return NULL;
+	}
+	for (i = 0; i < n; i++) {
+		int id, cntlid, ns, fd;
+		ssize_t ret;
+		char ctrl_path[256];
+
+		if (sscanf(paths[i]->d_name, "nvme%dc%dn%d",
+			   &id, &cntlid, &ns) != 3)
+			continue;
+
+		if (ns != nsid)
+			continue;
+
+		sprintf(ctrl_path, "%s/%s/ana_state", path, paths[i]->d_name);
+		fd = open(ctrl_path, O_RDONLY);
+		if (fd < 0) {
+			fprintf(stderr, "Failed to open ANA state %s\n",
+				ctrl_path);
+			return NULL;
+		}
+		ret = read(fd, ana_state, 16);
+		if (ret < 0) {
+			fprintf(stderr, "Failed to read ANA state from %s\n",
+				ctrl_path);
+			free(ana_state);
+			ana_state = NULL;
+		} else if (ana_state[strlen(ana_state) - 1] == '\n')
+			ana_state[strlen(ana_state) - 1] = '\0';
+		close(fd);
+		break;
+	}
+
+	return ana_state;
+}
+
 static int scan_ctrls_filter(const struct dirent *d)
 {
 	int id, nsid;
@@ -1422,7 +1488,7 @@ static void free_ctrl_list_item(struct ctrl_list_item *ctrls)
 }
 
 int get_nvme_subsystem_info(char *name, char *path,
-				struct subsys_list_item *item)
+			    struct subsys_list_item *item, int nsid)
 {
 	char ctrl_path[512];
 	struct dirent **ctrls;
@@ -1463,7 +1529,6 @@ int get_nvme_subsystem_info(char *name, char *path,
 			free_ctrl_list_item(&item->ctrls[ccnt]);
 			continue;
 		}
-
 		if (!strncmp(item->ctrls[ccnt].transport, "pcie", 4))
 			item->ctrls[ccnt].address =
 				get_nvme_pcie_address(ctrl_path);
@@ -1476,6 +1541,9 @@ int get_nvme_subsystem_info(char *name, char *path,
 			continue;
 		}
 
+		if (nsid)
+			item->ctrls[ccnt].ana_state =
+				get_nvme_ctrl_path_ana_state(ctrl_path, nsid);
 		ccnt++;
 	}
 
@@ -1544,9 +1612,10 @@ static int list_subsys(int argc, char **argv, struct command *cmd,
 		       struct plugin *plugin)
 {
 	char path[310];
+	char *subsysnqn = NULL;
 	struct dirent **subsys;
 	struct subsys_list_item *slist;
-	int fmt, n, i, ret = 0, subcnt = 0;
+	int fmt, n = 1, i, ret = 0, subcnt = 0, nsid = 0, id;
 	const char *desc = "Retrieve information for subsystems";
 	struct config {
 		char *output_format;
@@ -1566,13 +1635,42 @@ static int list_subsys(int argc, char **argv, struct command *cmd,
 	if (ret < 0)
 		return ret;
 
+	devicename = NULL;
+	if (optind < argc) {
+		devicename = basename(argv[optind]);
+		if (sscanf(devicename, "nvme%dn%d", &id, &nsid) != 2) {
+			fprintf(stderr, "%s is not a NVMe namespace device\n",
+				argv[optind]);
+			return -EINVAL;
+		}
+		sprintf(path, "/sys/block/%s/device", devicename);
+		subsysnqn = get_nvme_subsnqn(path);
+		if (!subsysnqn) {
+			fprintf(stderr, "Cannot read subsys NQN from %s\n",
+				devicename);
+			return -EINVAL;
+		}
+		optind++;
+	}
+
+	if (ret < 0) {
+		argconfig_print_help(desc, opts);
+		if (subsysnqn)
+			free(subsysnqn);
+		return ret;
+	}
 	fmt = validate_output_format(cfg.output_format);
 
-	if (fmt != JSON && fmt != NORMAL)
+	if (fmt != JSON && fmt != NORMAL) {
+		if (subsysnqn)
+			free(subsysnqn);
 		return -EINVAL;
+	}
 	n = scandir(subsys_dir, &subsys, scan_subsys_filter, alphasort);
 	if (n < 0) {
 		fprintf(stderr, "no NVMe subsystem(s) detected.\n");
+		if (subsysnqn)
+			free(subsysnqn);
 		return n;
 	}
 
@@ -1586,12 +1684,15 @@ static int list_subsys(int argc, char **argv, struct command *cmd,
 		snprintf(path, sizeof(path), "%s%s", subsys_dir,
 			subsys[i]->d_name);
 		ret = get_nvme_subsystem_info(subsys[i]->d_name, path,
-				&slist[subcnt]);
+				&slist[subcnt], nsid);
 		if (ret) {
 			fprintf(stderr,
 				"%s: failed to get subsystem info: %s\n",
 				path, strerror(errno));
 			free_subsys_list_item(&slist[subcnt]);
+		} else if (subsysnqn &&
+			   strncmp(slist[subcnt].subsysnqn, subsysnqn, 255)) {
+			free_subsys_list_item(&slist[subcnt]);
 		} else
 			subcnt++;
 	}
@@ -1607,7 +1708,8 @@ free_subsys:
 	for (i = 0; i < n; i++)
 		free(subsys[i]);
 	free(subsys);
-
+	if (subsysnqn)
+		free(subsysnqn);
 	return ret;
 }
 
diff --git a/nvme.h b/nvme.h
index 5098b0e..26d5b85 100644
--- a/nvme.h
+++ b/nvme.h
@@ -129,6 +129,7 @@ struct ctrl_list_item {
 	char *name;
 	char *address;
 	char *transport;
+	char *ana_state;
 };
 
 struct subsys_list_item {
-- 
2.13.7

[PATCH 1/6] nvme-ana-log: fixup compiler warning in show_ana_log()

[Christoph Hellwig]

On Fri, Aug 17, 2018@09:29:03AM +0200, Hannes Reinecke wrote:
> Add the correct type cast to keep the compiler happy.
> 
> Signed-off-by: Hannes Reinecke <hare at suse.com>
> ---
>  nvme-print.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/nvme-print.c b/nvme-print.c
> index e0da949..8a604c5 100644
> --- a/nvme-print.c
> +++ b/nvme-print.c
> @@ -1460,7 +1460,8 @@ void show_ana_log(struct nvme_ana_rsp_hdr *ana_log, const char *devname)
>  		offset += sizeof(*desc);
>  		printf("grpid	:	%u\n", le32_to_cpu(desc->grpid));
>  		printf("nnsids	:	%u\n", le32_to_cpu(desc->nnsids));
> -		printf("chgcnt	:	%"PRIu64"\n", le64_to_cpu(desc->chgcnt));
> +		printf("chgcnt	:	%"PRIu64"\n",
> +		       (uint64_t)le64_to_cpu(desc->chgcnt));

Wouldn't it be easier to just use %llu as the printf specified, given
that u64/__u64 are always and unsigned long long?

[PATCH 2/6] nvme-vendor: fixup c99 declaration in huawei plugin

[Christoph Hellwig]

On Fri, Aug 17, 2018@09:29:04AM +0200, Hannes Reinecke wrote:
> We can't use c99 declaration with older compilers.
> 
> Signed-off-by: Hannes Reinecke <hare at suse.com>

Not that I particularly like this feature, but do we really support
that old compilers?

Reviewed-by: Christoph Hellwig <hch at lst.de>

[PATCH 3/6] nvme-discover: sanitize options

[Christoph Hellwig]

On Fri, Aug 17, 2018@09:29:05AM +0200, Hannes Reinecke wrote:
> The discovery controller doesn't accept any KATO or number of I/O
> queue changes, but we do want to change the reconnect delay and
> the controller loss timeout.
> 
> Signed-off-by: Hannes Reinecke <hare at suse.com>

Patch and description don't seem to match, this mostly seems to add
new documentation.

[PATCH 4/6] nvme-discover: Retry discovery log if the generation counter changes

[Christoph Hellwig]

On Fri, Aug 17, 2018@09:29:06AM +0200, Hannes Reinecke wrote:
> If the generation counter changes we need to validate if the number
> of records has changed, too.
> If so we need to retry retrieving the discovery log to the most recent
> values. The retry will be terminated after MAX_DISC_RETRIES (currently
> set to 30) to avoid infinite recursion.

30 retries seems pretty crazy.

> @@ -311,6 +312,7 @@ static int nvmf_get_log_page_discovery(const char *dev_path,
>  		goto out_free_log;
>  	}
>  
> +retry_log:
>  	/* check numrec limits */
>  	*numrec = le64_to_cpu(log->numrec);
>  	genctr = le64_to_cpu(log->genctr);
> @@ -346,7 +348,12 @@ static int nvmf_get_log_page_discovery(const char *dev_path,
>  		goto out_free_log;
>  	}
>  
> -	if (*numrec != le32_to_cpu(log->numrec) || genctr != le64_to_cpu(log->genctr)) {
> +	if (genctr != le64_to_cpu(log->genctr) &&
> +	    *numrec != le32_to_cpu(log->numrec) &&
> +	    ++retries < max_retries)
> +		goto retry_log;

Any way we could factor this into a do { } while loop?

[PATCH 5/6] nvme-list-subsys: Print PCIe controller address

[Christoph Hellwig]

On Fri, Aug 17, 2018@09:29:07AM +0200, Hannes Reinecke wrote:
> An NVMe PCIe controller doesn't have an address, so we should be
> using the PCIe bus address.

Can we do this in the kernel instead of having to fix up every user
of the sysfs interface?

[PATCH 6/6] nvme-list-subsys: Add device name argument and print out ANA state

[Christoph Hellwig]

On Fri, Aug 17, 2018@09:29:08AM +0200, Hannes Reinecke wrote:
> Update the 'nvme list-subsys' command to accept a device name and
> print out the ANA state for all paths to that device.

I'm going to try this myself once I find a little time, but can
you send some example output in the meantime?

[PATCH 4/6] nvme-discover: Retry discovery log if the generation counter changes

[Hannes Reinecke]

On 08/17/2018 10:11 AM, Christoph Hellwig wrote:
> On Fri, Aug 17, 2018@09:29:06AM +0200, Hannes Reinecke wrote:
>> If the generation counter changes we need to validate if the number
>> of records has changed, too.
>> If so we need to retry retrieving the discovery log to the most recent
>> values. The retry will be terminated after MAX_DISC_RETRIES (currently
>> set to 30) to avoid infinite recursion.
> 
> 30 retries seems pretty crazy.
> 
So what would be a good choice then?

>> @@ -311,6 +312,7 @@ static int nvmf_get_log_page_discovery(const char *dev_path,
>>  		goto out_free_log;
>>  	}
>>  
>> +retry_log:
>>  	/* check numrec limits */
>>  	*numrec = le64_to_cpu(log->numrec);
>>  	genctr = le64_to_cpu(log->genctr);
>> @@ -346,7 +348,12 @@ static int nvmf_get_log_page_discovery(const char *dev_path,
>>  		goto out_free_log;
>>  	}
>>  
>> -	if (*numrec != le32_to_cpu(log->numrec) || genctr != le64_to_cpu(log->genctr)) {
>> +	if (genctr != le64_to_cpu(log->genctr) &&
>> +	    *numrec != le32_to_cpu(log->numrec) &&
>> +	    ++retries < max_retries)
>> +		goto retry_log;
> 
> Any way we could factor this into a do { } while loop?
> 
I'll be checking.

Cheers,

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

[PATCH 5/6] nvme-list-subsys: Print PCIe controller address

[Hannes Reinecke]

On 08/17/2018 10:12 AM, Christoph Hellwig wrote:
> On Fri, Aug 17, 2018@09:29:07AM +0200, Hannes Reinecke wrote:
>> An NVMe PCIe controller doesn't have an address, so we should be
>> using the PCIe bus address.
> 
> Can we do this in the kernel instead of having to fix up every user
> of the sysfs interface?
> 
Sure we can.

I'll be drafting up a patch.

Cheers,

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

[PATCH 1/6] nvme-ana-log: fixup compiler warning in show_ana_log()

[Chaitanya Kulkarni]

Not sure if this is relevant but le64_to_cpu() we are fixing warnings by the cast,
maybe follow the same pattern everywhere?

http://lists.infradead.org/pipermail/linux-nvme/2018-June/018585.html



From: Linux-nvme <linux-nvme-bounces@lists.infradead.org> on behalf of Christoph Hellwig <hch@lst.de>
Sent: Friday, August 17, 2018 1:09 AM
To: Hannes Reinecke
Cc: Keith Busch; Hannes Reinecke; Sagi Grimberg; linux-nvme at lists.infradead.org; Christoph Hellwig
Subject: Re: [PATCH 1/6] nvme-ana-log: fixup compiler warning in show_ana_log()
? 
 
On Fri, Aug 17, 2018@09:29:03AM +0200, Hannes Reinecke wrote:
> Add the correct type cast to keep the compiler happy.
> 
> Signed-off-by: Hannes Reinecke <hare at suse.com>
> ---
>? nvme-print.c | 3 ++-
>? 1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/nvme-print.c b/nvme-print.c
> index e0da949..8a604c5 100644
> --- a/nvme-print.c
> +++ b/nvme-print.c
> @@ -1460,7 +1460,8 @@ void show_ana_log(struct nvme_ana_rsp_hdr *ana_log, const char *devname)
>??????????????? offset += sizeof(*desc);
>??????????????? printf("grpid?? :?????? %u\n", le32_to_cpu(desc->grpid));
>??????????????? printf("nnsids? :?????? %u\n", le32_to_cpu(desc->nnsids));
> -???????????? printf("chgcnt? :?????? %"PRIu64"\n", le64_to_cpu(desc->chgcnt));
> +???????????? printf("chgcnt? :?????? %"PRIu64"\n",
> +??????????????????? (uint64_t)le64_to_cpu(desc->chgcnt));

Wouldn't it be easier to just use %llu as the printf specified, given
that u64/__u64 are always and unsigned long long?

_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme


Linux-nvme Info Page - lists.infradead.org
lists.infradead.org
To see the collection of prior postings to the list, visit the Linux-nvme Archives.. Using Linux-nvme: To post a message to all the list members, send email to linux-nvme at lists.infradead.org.

    

[PATCH 1/6] nvme-ana-log: fixup compiler warning in show_ana_log()

[Keith Busch]

On Fri, Aug 17, 2018@09:44:34PM +0000, Chaitanya Kulkarni wrote:
> 
> Not sure if this is relevant but le64_to_cpu() we are fixing warnings by the cast,
> maybe follow the same pattern everywhere?
> 
> http://lists.infradead.org/pipermail/linux-nvme/2018-June/018585.html

Yeah, consistency is always good. I'm fine with the PRI macros, but either
way is fine as long as we compile without warnings on all architectures.

[PATCH 2/6] nvme-vendor: fixup c99 declaration in huawei plugin

[Keith Busch]

On Fri, Aug 17, 2018@10:09:56AM +0200, Christoph Hellwig wrote:
> On Fri, Aug 17, 2018@09:29:04AM +0200, Hannes Reinecke wrote:
> > We can't use c99 declaration with older compilers.
> > 
> > Signed-off-by: Hannes Reinecke <hare at suse.com>
> 
> Not that I particularly like this feature, but do we really support
> that old compilers?

I don't think those old ones are supported, but I dislike mixed
declarations as well.

[PATCH 0/6] nvme-cli fixes and ANA updates

[Keith Busch]

On Fri, Aug 17, 2018@09:29:02AM +0200, Hannes Reinecke wrote:
> Hi Keith,
> 
> here's a small set of fixes and an update to have nvme list-subsys printing
> out the ANA state for all paths.
> This brings nvme-cli output on par with that from multipath-tools.
> 
> As usual, comments and reviews welcome.

These all look fine to me. I'll apply 1-3 immediately as those look
pretty trivial. I'll give a test on 4-6 early next week when I should
have more time for nvme again.

[PATCH 6/6] nvme-list-subsys: Add device name argument and print out ANA state

[Chaitanya Kulkarni]

From: Linux-nvme <linux-nvme-bounces@lists.infradead.org> on behalf of Hannes Reinecke <hare@suse.de>
Sent: Friday, August 17, 2018 12:29 AM
To: Keith Busch
Cc: Hannes Reinecke; Hannes Reinecke; Sagi Grimberg; linux-nvme at lists.infradead.org; Christoph Hellwig
Subject: [PATCH 6/6] nvme-list-subsys: Add device name argument and print out ANA state
? 
 
Update the 'nvme list-subsys' command to accept a device name and
print out the ANA state for all paths to that device.

Signed-off-by: Hannes Reinecke <hare at suse.com>
---
?Documentation/nvme-list-subsys.txt |?? 4 +-
?nvme-print.c?????????????????????? |? 10 +++-
?nvme.c???????????????????????????? | 114 +++++++++++++++++++++++++++++++++++--
?nvme.h???????????????????????????? |?? 1 +
?4 files changed, 120 insertions(+), 9 deletions(-)

diff --git a/Documentation/nvme-list-subsys.txt b/Documentation/nvme-list-subsys.txt
index c7de7ef..c40b708 100644
--- a/Documentation/nvme-list-subsys.txt
+++ b/Documentation/nvme-list-subsys.txt
@@ -8,12 +8,14 @@ nvme-list-subsys - List all NVMe subsystems
?SYNOPSIS
?--------
?[verse]
-'nvme list-subsys' [-o <fmt> | --output-format=<fmt>]
+'nvme list-subsys' [-o <fmt> | --output-format=<fmt>] <device>
?
?DESCRIPTION
?-----------
?Scan the sysfs tree for NVM Express subsystems and return the controllers
?for those subsystems as well as some pertinent information about them.
+If a device is given, print out only the values for the controllers
+and subsystems leading to the device.
?
?OPTIONS
?-------
diff --git a/nvme-print.c b/nvme-print.c
index 8a604c5..cc8c716 100644
--- a/nvme-print.c
+++ b/nvme-print.c
@@ -2762,9 +2762,11 @@ static void show_nvme_subsystem(struct subsys_list_item *item)
???????? printf("\\\n");
?
???????? for (i = 0; i < item->nctrls; i++) {
-?????????????? printf(" +- %s %s %s\n", item->ctrls[i].name,
+?????????????? printf(" +- %s %s %s %s\n", item->ctrls[i].name,
???????????????????????????????? item->ctrls[i].transport,
-?????????????????????????????? item->ctrls[i].address);
+?????????????????????????????? item->ctrls[i].address,
+?????????????????????????????? item->ctrls[i].ana_state ?
+?????????????????????????????????????? item->ctrls[i].ana_state : "");
???????? }
?
?}
@@ -2810,6 +2812,10 @@ void json_print_nvme_subsystem_list(struct subsys_list_item *slist, int n)
???????????????????????????????????????? slist[i].ctrls[j].transport);
???????????????????????? json_object_add_value_string(path_attrs, "Address",
???????????????????????????????????????? slist[i].ctrls[j].address);
+?????????????????????? if (slist[i].ctrls[j].ana_state)
+?????????????????????????????? json_object_add_value_string(path_attrs,
+?????????????????????????????????????????????? "State",
+?????????????????????????????????????????????? slist[i].ctrls[j].ana_state);
???????????????????????? json_array_add_value_object(paths, path_attrs);
???????????????? }
???????????????? if (j) {
diff --git a/nvme.c b/nvme.c
index 0438187..b5c9779 100644
--- a/nvme.c
+++ b/nvme.c
@@ -1398,6 +1398,72 @@ err_free_addrpath:
?
???????? return NULL;
?}
+
+static int scan_ctrl_paths_filter(const struct dirent *d)
+{
+?????? int id, cntlid, nsid;
+
+?????? if (d->d_name[0] == '.')
+?????????????? return 0;
+
+?????? if (strstr(d->d_name, "nvme")) {
+?????????????? if (sscanf(d->d_name, "nvme%dc%dn%d", &id, &cntlid, &nsid) != 3)
+?????????????????????? return 0;
+?????????????? return 1;
+?????? }
+
+?????? return 0;
+}
+
+static char *get_nvme_ctrl_path_ana_state(char *path, int nsid)
+{
+?????? struct dirent **paths;
+?????? char *ana_state;
+?????? int i, n;
+
+?????? ana_state = calloc(1, 16);
+?????? if (!ana_state)
+?????????????? return NULL;
+
+?????? n = scandir(path, &paths, scan_ctrl_paths_filter, alphasort);
+?????? if (n <= 0) {
+?????????????? free(ana_state);
+?????????????? return NULL;
+?????? }
+?????? for (i = 0; i < n; i++) {
+?????????????? int id, cntlid, ns, fd;
+?????????????? ssize_t ret;
+?????????????? char ctrl_path[256];
+
+?????????????? if (sscanf(paths[i]->d_name, "nvme%dc%dn%d",
+????????????????????????? &id, &cntlid, &ns) != 3)
+?????????????????????? continue;
+
+?????????????? if (ns != nsid)
+?????????????????????? continue;
+
+?????????????? sprintf(ctrl_path, "%s/%s/ana_state", path, paths[i]->d_name);
+?????????????? fd = open(ctrl_path, O_RDONLY);
+?????????????? if (fd < 0) {
+?????????????????????? fprintf(stderr, "Failed to open ANA state %s\n",
+?????????????????????????????? ctrl_path);
[CK] free (ana_state) here ?
+?????????????????????? return NULL;
+?????????????? }
+?????????????? ret = read(fd, ana_state, 16);
+?????????????? if (ret < 0) {
+?????????????????????? fprintf(stderr, "Failed to read ANA state from %s\n",
+?????????????????????????????? ctrl_path);
+?????????????????????? free(ana_state);
+?????????????????????? ana_state = NULL;
+?????????????? } else if (ana_state[strlen(ana_state) - 1] == '\n')
+?????????????????????? ana_state[strlen(ana_state) - 1] = '\0';
+?????????????? close(fd);
+?????????????? break;
+?????? }
+
[CK] Like your other patch, can we use only one return and error goto pattern ?
+?????? return ana_state;
+}
+
?static int scan_ctrls_filter(const struct dirent *d)
?{
???????? int id, nsid;
@@ -1422,7 +1488,7 @@ static void free_ctrl_list_item(struct ctrl_list_item *ctrls)
?}
?
?int get_nvme_subsystem_info(char *name, char *path,
-?????????????????????????????? struct subsys_list_item *item)
+?????????????????????????? struct subsys_list_item *item, int nsid)
?{
???????? char ctrl_path[512];
???????? struct dirent **ctrls;
@@ -1463,7 +1529,6 @@ int get_nvme_subsystem_info(char *name, char *path,
???????????????????????? free_ctrl_list_item(&item->ctrls[ccnt]);
???????????????????????? continue;
???????????????? }
-
???????????????? if (!strncmp(item->ctrls[ccnt].transport, "pcie", 4))
???????????????????????? item->ctrls[ccnt].address =
???????????????????????????????? get_nvme_pcie_address(ctrl_path);
@@ -1476,6 +1541,9 @@ int get_nvme_subsystem_info(char *name, char *path,
???????????????????????? continue;
???????????????? }
?
+?????????????? if (nsid)
+?????????????????????? item->ctrls[ccnt].ana_state =
+?????????????????????????????? get_nvme_ctrl_path_ana_state(ctrl_path, nsid);
???????????????? ccnt++;
???????? }
?
@@ -1544,9 +1612,10 @@ static int list_subsys(int argc, char **argv, struct command *cmd,
??????????????????????? struct plugin *plugin)
?{
???????? char path[310];
+?????? char *subsysnqn = NULL;
???????? struct dirent **subsys;
???????? struct subsys_list_item *slist;
-?????? int fmt, n, i, ret = 0, subcnt = 0;
+?????? int fmt, n = 1, i, ret = 0, subcnt = 0, nsid = 0, id;
???????? const char *desc = "Retrieve information for subsystems";
???????? struct config {
???????????????? char *output_format;
@@ -1566,13 +1635,42 @@ static int list_subsys(int argc, char **argv, struct command *cmd,
???????? if (ret < 0)
???????????????? return ret;
?
+?????? devicename = NULL;
+?????? if (optind < argc) {
+?????????????? devicename = basename(argv[optind]);
+?????????????? if (sscanf(devicename, "nvme%dn%d", &id, &nsid) != 2) {
+?????????????????????? fprintf(stderr, "%s is not a NVMe namespace device\n",
+?????????????????????????????? argv[optind]);
+?????????????????????? return -EINVAL;
+?????????????? }
+?????????????? sprintf(path, "/sys/block/%s/device", devicename);
+?????????????? subsysnqn = get_nvme_subsnqn(path);
+?????????????? if (!subsysnqn) {
+?????????????????????? fprintf(stderr, "Cannot read subsys NQN from %s\n",
+?????????????????????????????? devicename);
+?????????????????????? return -EINVAL;
+?????????????? }
+?????????????? optind++;
+?????? }
+
+?????? if (ret < 0) {
+?????????????? argconfig_print_help(desc, opts);
[CK] Not sure about if check here.
+?????????????? if (subsysnqn)
+?????????????????????? free(subsysnqn);
+?????????????? return ret;
+?????? }
???????? fmt = validate_output_format(cfg.output_format);
?
-?????? if (fmt != JSON && fmt != NORMAL)
+?????? if (fmt != JSON && fmt != NORMAL) {
+?????????????? if (subsysnqn)
+?????????????????????? free(subsysnqn);
???????????????? return -EINVAL;
+?????? }
???????? n = scandir(subsys_dir, &subsys, scan_subsys_filter, alphasort);
???????? if (n < 0) {
???????????????? fprintf(stderr, "no NVMe subsystem(s) detected.\n");
[CK] Not sure about check here.
+?????????????? if (subsysnqn)
+?????????????????????? free(subsysnqn);
???????????????? return n;
???????? }
?
@@ -1586,12 +1684,15 @@ static int list_subsys(int argc, char **argv, struct command *cmd,
???????????????? snprintf(path, sizeof(path), "%s%s", subsys_dir,
???????????????????????? subsys[i]->d_name);
???????????????? ret = get_nvme_subsystem_info(subsys[i]->d_name, path,
-?????????????????????????????? &slist[subcnt]);
+?????????????????????????????? &slist[subcnt], nsid);
???????????????? if (ret) {
???????????????????????? fprintf(stderr,
???????????????????????????????? "%s: failed to get subsystem info: %s\n",
???????????????????????????????? path, strerror(errno));
???????????????????????? free_subsys_list_item(&slist[subcnt]);
+?????????????? } else if (subsysnqn &&
+????????????????????????? strncmp(slist[subcnt].subsysnqn, subsysnqn, 255)) {
+?????????????????????? free_subsys_list_item(&slist[subcnt]);
???????????????? } else
???????????????????????? subcnt++;
???????? }
@@ -1607,7 +1708,8 @@ free_subsys:
???????? for (i = 0; i < n; i++)
???????????????? free(subsys[i]);
???????? free(subsys);
-
[CK] Not sure about check here.
+?????? if (subsysnqn)
+?????????????? free(subsysnqn);
???????? return ret;
?}
?
diff --git a/nvme.h b/nvme.h
index 5098b0e..26d5b85 100644
--- a/nvme.h
+++ b/nvme.h
@@ -129,6 +129,7 @@ struct ctrl_list_item {
???????? char *name;
???????? char *address;
???????? char *transport;
+?????? char *ana_state;
?};
?
?struct subsys_list_item {
-- 
2.13.7


_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme


Linux-nvme Info Page - lists.infradead.org
lists.infradead.org
To see the collection of prior postings to the list, visit the Linux-nvme Archives.. Using Linux-nvme: To post a message to all the list members, send email to linux-nvme at lists.infradead.org.