[PATCH V3 nvme-cli 0/2] add "Path Access" field in command "nvme list"

[PATCH V3 nvme-cli 0/2] add "Path Access" field in command "nvme list"

[chengjike]

This patch V3 is to add "Path Access" field in command "nvme list".
Changes on this V3 since the last version of this
 - updated the commit log by Chaitanya Kulkarni
 - added a function comment by Chaitanya Kulkarni
 - modified the inappropriate code by Chaitanya Kulkarni

patch V2
https://lore.kernel.org/linux-nvme/20210927014137.3756-1-chengjike.cheng@huawei.com/T/#t

chengjike (1):
  initialize disk "access" variable

 src/nvme/private.h |  1 +
 src/nvme/tree.c    | 54 +++++++++++++++++++++++++++++++++++++++++-----
 src/nvme/tree.h    | 11 ++++++++++
 3 files changed, 61 insertions(+), 5 deletions(-)

chengjike (1):
  add "Path Access" entry in command output

 nvme-print.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)
 
-- 
2.21.0.windows.1


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

[PATCH V3 nvme-cli 1/2] initialize disk "access" variable

[chengjike]

(This is a prep patch, which provides functions for the next "patch 2/2")
When it is failed to get nvme_id_ns data from NVMe Subsystem.
Then the "access" of disk is set to "faulty"(otherwise, set the value to "live").
Some displaying content of fault device can be obtained from the disk attribute files.
In addition, external function "nvme_ns_get_access" is added,
which will be used in the next patch.

Signed-off-by: chengjike <chengjike.cheng@huawei.com>
---
 src/nvme/private.h |  1 +
 src/nvme/tree.c    | 54 +++++++++++++++++++++++++++++++++++++++++-----
 src/nvme/tree.h    | 11 ++++++++++
 3 files changed, 61 insertions(+), 5 deletions(-)

diff --git a/src/nvme/private.h b/src/nvme/private.h
index 2a151bf..255c1b1 100644
--- a/src/nvme/private.h
+++ b/src/nvme/private.h
@@ -37,6 +37,7 @@ struct nvme_ns {
 	__u32 nsid;
 	char *name;
 	char *sysfs_dir;
+	char *access;
 
 	int lba_shift;
 	int lba_size;
diff --git a/src/nvme/tree.c b/src/nvme/tree.c
index 293b0c0..04d92c9 100644
--- a/src/nvme/tree.c
+++ b/src/nvme/tree.c
@@ -229,9 +229,11 @@ nvme_ns_t nvme_subsystem_next_ns(nvme_subsystem_t s, nvme_ns_t n)
 static void __nvme_free_ns(struct nvme_ns *n)
 {
 	list_del_init(&n->entry);
-	close(n->fd);
+	if (n->fd > 0)
+		close(n->fd);
 	free(n->name);
 	free(n->sysfs_dir);
+	free(n->access);
 	free(n);
 }
 
@@ -1023,9 +1025,6 @@ static int nvme_configure_ctrl(nvme_ctrl_t c, const char *path,
 	closedir(d);
 
 	c->fd = nvme_open(name);
-	if (c->fd < 0)
-		return c->fd;
-
 	c->name = strdup(name);
 	c->sysfs_dir = (char *)path;
 	c->firmware = nvme_get_ctrl_attr(c, "firmware_rev");
@@ -1277,6 +1276,11 @@ const char *nvme_ns_get_name(nvme_ns_t n)
 	return n->name;
 }
 
+const char *nvme_ns_get_access(nvme_ns_t n)
+{
+	return n->access;
+}
+
 const char *nvme_ns_get_model(nvme_ns_t n)
 {
 	return n->c ? n->c->model : n->s->model;
@@ -1515,6 +1519,34 @@ free_ns:
 	return NULL;
 }
 
+static nvme_ns_t nvme_ns_get_local_data(const char *name, const char *path)
+{
+	struct nvme_ns *n;
+	char *ns_id;
+
+	n = calloc(1, sizeof(*n));
+	if (!n) {
+		errno = ENOMEM;
+		return NULL;
+	}
+
+	n->name = strdup(name);
+	n->fd = -1;
+	ns_id = nvme_get_attr(path, "nsid");
+	if (!ns_id) {
+		free(n->name);
+		free(n);
+		return NULL;
+	}
+	n->nsid = atoi(ns_id);
+	free(ns_id);
+
+	list_head_init(&n->paths);
+	list_node_init(&n->entry);
+
+	return n;
+}
+
 static struct nvme_ns *__nvme_scan_namespace(const char *sysfs_dir, const char *name)
 {
 	struct nvme_ns *n;
@@ -1528,9 +1560,21 @@ static struct nvme_ns *__nvme_scan_namespace(const char *sysfs_dir, const char *
 	}
 
 	n = nvme_ns_open(name);
-	if (!n)
+	if (!n) {
+		n = nvme_ns_get_local_data(name, path);
+		if (!n)
+			goto free_path;
+		ret = asprintf(&n->access, "%s", "faulty");
+		if (ret < 0) 
+			goto free_path;
+ 		goto get_attr;
+	}
+
+	ret = asprintf(&n->access, "%s", "live");
+	if (ret < 0) 
 		goto free_path;
 
+get_attr:
 	n->sysfs_dir = path;
 	return n;
 
diff --git a/src/nvme/tree.h b/src/nvme/tree.h
index 68f5cbf..82895bd 100644
--- a/src/nvme/tree.h
+++ b/src/nvme/tree.h
@@ -452,6 +452,17 @@ const char *nvme_ns_get_sysfs_dir(nvme_ns_t n);
  */
 const char *nvme_ns_get_name(nvme_ns_t n);
 
+/**
+ * nvme_ns_get_access() - 
+ * @n: Address of the nvme namespace structure
+ *
+ * Get the access status of the nvme namespace
+ *
+ * Return: The 'access' field from 'nvme_ns'(for normal namespace, 
+ * 'live' is returned; for inaccessible namespace, 'faulty' is returned). 
+ */
+const char *nvme_ns_get_access(nvme_ns_t n);
+
 /**
  * nvme_ns_get_firmware() -
  * @n:
-- 
2.21.0.windows.1


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

[PATCH V3 nvme-cli 2/2] add "Path Access" entry in command output

[chengjike]

When users run commands such as nvme list, nvme list -o json, nvme list -v -o json,
the "Path Access" entry of each disk is displayed.

Signed-off-by: chengjike <chengjike.cheng@huawei.com>
---
 nvme-print.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/nvme-print.c b/nvme-print.c
index 7bb34cd..47c2070 100644
--- a/nvme-print.c
+++ b/nvme-print.c
@@ -5649,10 +5649,10 @@ static void nvme_show_list_item(nvme_ns_t n)
 	snprintf(format, sizeof(format), "%3.0f %2sB + %2d B", (double)lba,
 		l_suffix, nvme_ns_get_meta_size(n));
 
-	printf("%-21s %-20s %-40s %-9d %-26s %-16s %-8s\n",
+	printf("%-21s %-20s %-40s %-9d %-26s %-16s %-8s %-12s\n",
 		nvme_ns_get_name(n), nvme_ns_get_serial(n),
 		nvme_ns_get_model(n), nvme_ns_get_nsid(n), usage, format,
-		nvme_ns_get_firmware(n));
+		nvme_ns_get_firmware(n), nvme_ns_get_access(n));
 }
 
 static void nvme_show_simple_list(nvme_root_t r)
@@ -5662,10 +5662,10 @@ static void nvme_show_simple_list(nvme_root_t r)
 	nvme_ctrl_t c;
 	nvme_ns_t n;
 
-	printf("%-21s %-20s %-40s %-9s %-26s %-16s %-8s\n",
-	    "Node", "SN", "Model", "Namespace", "Usage", "Format", "FW Rev");
-	printf("%-.21s %-.20s %-.40s %-.9s %-.26s %-.16s %-.8s\n", dash, dash,
-		dash, dash, dash, dash, dash);
+	printf("%-21s %-20s %-40s %-9s %-26s %-16s %-8s %-12s\n",
+	    "Node", "SN", "Model", "Namespace", "Usage", "Format", "FW Rev", "Path Access");
+	printf("%-.21s %-.20s %-.40s %-.9s %-.26s %-.16s %-.8s %-.12s\n", dash, dash,
+		dash, dash, dash, dash, dash, dash);
 
 	nvme_for_each_host(r, h) {
 		nvme_for_each_subsystem(h, s) {
@@ -5841,6 +5841,7 @@ static void json_detail_list(nvme_root_t r)
 					json_object_add_value_int(jns, "maxlba", nvme_ns_get_lba_count(n));
 					json_object_add_value_int(jns, "capacity", nsze);
 					json_object_add_value_int(jns, "sector", lba);
+					json_object_add_value_string(jns, "path access", nvme_ns_get_access(n));
 
 					json_array_add_value_object(jnss, jns);
 				}
@@ -5873,6 +5874,7 @@ static void json_detail_list(nvme_root_t r)
 				json_object_add_value_int(jns, "maxlba", nvme_ns_get_lba_count(n));
 				json_object_add_value_int(jns, "capacity", nsze);
 				json_object_add_value_int(jns, "sector", lba);
+				json_object_add_value_string(jns, "path access", nvme_ns_get_access(n));
 
 				json_array_add_value_object(jnss, jns);
 			}
@@ -5907,6 +5909,7 @@ static struct json_object *json_list_item(nvme_ns_t n)
 	json_object_add_value_int(jdevice, "maxlba", nvme_ns_get_lba_count(n));
 	json_object_add_value_int(jdevice, "capacity", nsze);
 	json_object_add_value_int(jdevice, "sector", lba);
+	json_object_add_value_string(jdevice, "path access", nvme_ns_get_access(n));
 
 	return jdevice;
 }
-- 
2.21.0.windows.1


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

Re: [PATCH V3 nvme-cli 1/2] initialize disk "access" variable

[Sagi Grimberg]

> (This is a prep patch, which provides functions for the next "patch 2/2")
> When it is failed to get nvme_id_ns data from NVMe Subsystem.

If there was a failure the nsid should be -1, and all the size info 
should be zero, that is enough of an indication that the ns is "faulty".

Also live/faulty is not a good terminology for path access.
Maybe full/partial or complete/incomplete.

I thought that this was about showing the completeness of the
path access for a given ns, regardless of failure of identify-ns...

Meaning that if I have a ns with 2 paths, where one is live and the
other is connecting/resetting then "Path Access" would be "incomplete"
or "partial".. Also in the case where all paths are live, but some
have ANA state "inaccessible" or "persistent loss" then "Path Access"
would be "partial" and only if all paths are "live" and ANA accessible
i.e. "optimized/non-optimized" it will show "full/complete" Path
Access...

Otherwise, I don't see the point in adding this information to
nvme list output.

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

Re: [PATCH V3 nvme-cli 1/2] initialize disk "access" variable

[Chaitanya Kulkarni]

> Also live/faulty is not a good terminology for path access.
> Maybe full/partial or complete/incomplete.
> 
> I thought that this was about showing the completeness of the
> path access for a given ns, regardless of failure of identify-ns...
> 

since printing the path access and id-ns are two different things,
id-ns can fail and we still can read the path access that is why
we should make this an optional print with command line arg feature,
if it has been considered yet ...


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

Re: [PATCH V3 nvme-cli 1/2] initialize disk "access" variable

[Chengjike (ISSP)]

在 2021/9/29 4:45, Sagi Grimberg 写道:
>
>> (This is a prep patch, which provides functions for the next "patch 
>> 2/2")
>> When it is failed to get nvme_id_ns data from NVMe Subsystem.
>
> If there was a failure the nsid should be -1, and all the size info 
> should be zero, that is enough of an indication that the ns is "faulty".
>
> Also live/faulty is not a good terminology for path access.
> Maybe full/partial or complete/incomplete.
>
> I thought that this was about showing the completeness of the
> path access for a given ns, regardless of failure of identify-ns...
>
> Meaning that if I have a ns with 2 paths, where one is live and the
> other is connecting/resetting then "Path Access" would be "incomplete"
> or "partial".. Also in the case where all paths are live, but some
> have ANA state "inaccessible" or "persistent loss" then "Path Access"
> would be "partial" and only if all paths are "live" and ANA accessible
> i.e. "optimized/non-optimized" it will show "full/complete" Path
> Access...
>
> Otherwise, I don't see the point in adding this information to
> nvme list output.
> .

Hi,Sagi and Chaitanya

Thank you for your comments.
It is a good suggestion that setting nsid to "-1" indicates a faulty disk,
and I resend patch V4 based on your comments later.



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