Skip to contentSkip to conversation

nvme-cli fixes and ANA updates

PATCH New Hannes Reinecke 5 participants 20 messages Aug 17, 2018

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover nvme-cli fixes and ANA updates

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(-)
Signature · 1 line
2.13.7
#1 lore raw

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

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(-)
nvme-print.c +2 -1
index e0da949..8a604c5 100644
@@ -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++)
Signature · 1 line
2.13.7
#2 lore raw

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

Quoted 12 lines · 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(-)
nvme-print.c +0 -0
index e0da949..8a604c5 100644

nvme-print.c void show_ana_log(struct nvme_ana_rsp_hdr *ana_log, const char *devname) · line 1464

Quoted 6 lines
nvme-print.c +2 -1
@@ -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?

#3 lore raw

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

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 <[email protected]> on behalf of Christoph Hellwig <[email protected]>
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()
?

Quoted 12 lines · 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(-)

nvme-print.c +0 -0
index e0da949..8a604c5 100644

nvme-print.c void show_ana_log(struct nvme_ana_rsp_hdr *ana_log, const char *devname) · line 1464

Quoted 6 lines
nvme-print.c +2 -1
@@ -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.

#4 lore raw

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

Quoted 5 lines · 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

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.

#5 lore raw

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

In reply to 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(-)
huawei-nvme.c +2 -1
index f305405..f91d3e3 100644
@@ -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",
Signature · 1 line
2.13.7
#6 lore raw

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

Quoted 3 lines · Hannes Reinecke

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>
#7 lore raw

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

Quoted 7 lines · Christoph Hellwig
Quoted 3 lines · Hannes Reinecke

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.

#8 lore raw

PATCH 3/6 nvme-discover: sanitize options

In reply to 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(-)
Documentation/nvme-discover.txt +12 -1
index 4fcf1ca..51d63e4 100644
@@ -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
fabrics.c +1 -3
index 28e97c5..9bf2594 100644
@@ -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},
 	};
 
Signature · 1 line
2.13.7
#9 lore raw

PATCH 3/6 nvme-discover: sanitize options

Quoted 5 lines · 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>

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

#10 lore raw

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

In reply to 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(-)
fabrics.c +9 -2
index 9bf2594..b277d36 100644
@@ -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;
 	}
Signature · 1 line
2.13.7
#11 lore raw

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

Quoted 5 lines · 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.

30 retries seems pretty crazy.

Quoted 9 lines
fabrics.c +1 -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);

fabrics.c static int nvmf_get_log_page_discovery(const char *dev_path, · line 354

Quoted 8 lines
fabrics.c +4 -1
@@ -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?

#12 lore raw

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

Quoted 9 lines · Christoph Hellwig
Quoted 5 lines · 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.

30 retries seems pretty crazy.

So what would be a good choice then?

Quoted 12 lines
Quoted 12 lines
fabrics.c +1 -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;
 	}
 
Quoted 8 lines

fabrics.c static int nvmf_get_log_page_discovery(const char *dev_path, · line 354

Quoted 5 lines
fabrics.c +4 -1
-	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

Signature · 5 lines
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)
#13 lore raw

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

In reply to 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(-)
nvme.c +50 -7
index 99640cb..0438187 100644
@@ -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++;
 	}
 
Signature · 1 line
2.13.7
#14 lore raw

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

Quoted 2 lines · Hannes Reinecke

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?

#15 lore raw

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

Quoted 7 lines · Christoph Hellwig
Quoted 2 lines · Hannes Reinecke

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

Signature · 5 lines
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)
#16 lore raw

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

In reply to 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(-)
Documentation/nvme-list-subsys.txt +3 -1
index c7de7ef..c40b708 100644
@@ -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
 -------
nvme-print.c +8 -2
index 8a604c5..cc8c716 100644
@@ -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) {
nvme.c +108 -6

175 lines not on this page · raw message

nvme.h +1 -0
index 5098b0e..26d5b85 100644
@@ -129,6 +129,7 @@ struct ctrl_list_item {
 	char *name;
 	char *address;
 	char *transport;
+	char *ana_state;
 };
 
 struct subsys_list_item {
Signature · 1 line
2.13.7
#17 lore raw

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

Quoted 2 lines · 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.

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

#18 lore raw

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

In reply to Hannes Reinecke

From: Linux-nvme <[email protected]> on behalf of Hannes Reinecke <[email protected]>
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(-)

Documentation/nvme-list-subsys.txt +0 -0
index c7de7ef..c40b708 100644
@@ -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
?-------

nvme-print.c +0 -0
index 8a604c5..cc8c716 100644
@@ -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) {

nvme.c +0 -0
index 0438187..b5c9779 100644
@@ -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;
?}
?

nvme.h +0 -0
index 5098b0e..26d5b85 100644
@@ -129,6 +129,7 @@ struct ctrl_list_item {

???????? char *name;
???????? char *address;
???????? char *transport;
+?????? char *ana_state;
?};
?
?struct subsys_list_item {

Signature · 12 lines
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.
#19 lore raw

cover nvme-cli fixes and ANA updates

In reply to Hannes Reinecke
Quoted 7 lines · 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.

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.