[PATCH nvme-cli 0/2] add "State" field in command "nvme list"

[PATCH nvme-cli 0/2] add "State" field in command "nvme list"

[chengjike]

When NVMe Subsystem is connected with one or more controllers via fabric, such as NVMe-oF over RDMA,  
if some links are down, some nvme disks on the host are faulty and they won't be deleted immediately. 
But the "nvme list" command does not show these faulty disks. 
So add "State" field  in command "nvme list", and display the status of each disk for users.
For example:
[root@localhost nvme-cli]# nvme list 
Node        SN                   Model         Namespace Usage                    Format           FW Rev   Status       
----------- -------------------- ------------- --------- ------------------------ ---------------- -------- --------     
nvme0n2     25658552656655456265 Huawei-XSG1   2         2.83  GB /  10.74  GB    512   B +  0 B   1000001  live            
nvme0n1     25658552656655456265 Huawei-XSG1   1         2.78  GB /  10.74  GB    512   B +  0 B   1000001  faulty            
[root@localhost nvme-cli]# 

chengjike (1):
  add "State" entry

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

chengjike (1):
  set disk state

 src/nvme/private.h |  1 +
 src/nvme/tree.c    | 59 +++++++++++++++++++++++++++++++++++++++++-----
 src/nvme/tree.h    |  8 +++++++
 3 files changed, 62 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 nvme-cli 1/2] add "State" entry

[chengjike]

From: chengjike <jiangtao62@huawei.com>

Add "State" entry in nvme-print.c

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 d321ec0..f2b12c0 100644
--- a/nvme-print.c
+++ b/nvme-print.c
@@ -5647,10 +5647,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 %-8s\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_state(n));
 }
 
 static void nvme_show_simple_list(nvme_root_t r)
@@ -5660,10 +5660,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 %-8s\n",
+	    "Node", "SN", "Model", "Namespace", "Usage", "Format", "FW Rev", "State");
+	printf("%-.21s %-.20s %-.40s %-.9s %-.26s %-.16s %-.8s %-.8s\n", dash, dash,
+		dash, dash, dash, dash, dash, dash);
 
 	nvme_for_each_host(r, h) {
 		nvme_for_each_subsystem(h, s) {
@@ -5839,6 +5839,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, "state", nvme_ns_get_state(n));
 
 					json_array_add_value_object(jnss, jns);
 				}
@@ -5871,6 +5872,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, "state", nvme_ns_get_state(n));
 
 				json_array_add_value_object(jnss, jns);
 			}
@@ -5905,6 +5907,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, "state", nvme_ns_get_state(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 nvme-cli 1/2] add "State" entry

[Chaitanya Kulkarni]

On 9/15/21 6:29 AM, chengjike wrote:
> From: chengjike <jiangtao62@huawei.com>
> 
> Add "State" entry in nvme-print.c
> 
> 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 d321ec0..f2b12c0 100644
> --- a/nvme-print.c
> +++ b/nvme-print.c
> @@ -5647,10 +5647,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 %-8s\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_state(n));
>   }
>   


Where is the definition of the nvme_ns_get_state() ? I didn't find in
this patch, and your commit log is not helpful at all for the reviewers.


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

Re: [PATCH nvme-cli 1/2] add "State" entry

[Keith Busch]

On Thu, Sep 16, 2021 at 04:57:55PM +0000, Chaitanya Kulkarni wrote:
> On 9/15/21 6:29 AM, chengjike wrote:
> > From: chengjike <jiangtao62@huawei.com>
> > 
> > Add "State" entry in nvme-print.c
> > 
> > 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 d321ec0..f2b12c0 100644
> > --- a/nvme-print.c
> > +++ b/nvme-print.c
> > @@ -5647,10 +5647,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 %-8s\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_state(n));
> >   }
> >   
> 
> 
> Where is the definition of the nvme_ns_get_state() ? I didn't find in
> this patch, and your commit log is not helpful at all for the reviewers.

It looks like patch 1 and 2 are reversed.

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

Re: [PATCH nvme-cli 1/2] add "State" entry

[Chaitanya Kulkarni]

>>>
>>
>>
>> Where is the definition of the nvme_ns_get_state() ? I didn't find in
>> this patch, and your commit log is not helpful at all for the reviewers.
> 
> It looks like patch 1 and 2 are reversed.
> 

Maybe resend with the right order with proper commit description ?


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

Re: [PATCH nvme-cli 1/2] add "State" entry

[Chengjike (ISSP)]

在 2021/9/17 7:01, Chaitanya Kulkarni 写道:
>>>
>>> Where is the definition of the nvme_ns_get_state() ? I didn't find in
>>> this patch, and your commit log is not helpful at all for the reviewers.
>> It looks like patch 1 and 2 are reversed.
>>
> Maybe resend with the right order with proper commit description ?
>
Yes

It is my fault. Patch 1 and 2 are reversed.


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

[PATCH nvme-cli 2/2] set disk state

[chengjike]

From: chengjike <jiangtao62@huawei.com>

When it is failed to get data from NVMe Subsystem, the "State" of this disk is set to "faulty".
Some displaying content of fault device can be obtained from the disk attribute files.

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

diff --git a/src/nvme/private.h b/src/nvme/private.h
index 2a151bf..2c62084 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 *state;
 
 	int lba_shift;
 	int lba_size;
diff --git a/src/nvme/tree.c b/src/nvme/tree.c
index 293b0c0..9564bb4 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->state);
 	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_state(nvme_ns_t n)
+{
+	return n->state;
+}
+
 const char *nvme_ns_get_model(nvme_ns_t n)
 {
 	return n->c ? n->c->model : n->s->model;
@@ -1515,6 +1519,37 @@ 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;
+	}
+	memset(n, 0, sizeof(struct nvme_ns));
+
+	n->name = strdup(name);
+	n->fd = -1;
+	ns_id = nvme_get_attr(path, "nsid");
+	if (!ns_id) 
+		goto free_ns;
+	n->nsid = atoi(ns_id);
+	free(ns_id);
+
+	list_head_init(&n->paths);
+	list_node_init(&n->entry);
+
+	return n;
+
+free_ns:
+	free(n->name);
+	free(n);
+	return NULL;
+}
+
 static struct nvme_ns *__nvme_scan_namespace(const char *sysfs_dir, const char *name)
 {
 	struct nvme_ns *n;
@@ -1528,9 +1563,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->state, "%s", "faulty");
+		if (ret < 0) 
+			goto free_path;
+ 		goto get_attr;
+	}
+
+	ret = asprintf(&n->state, "%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..5254b19 100644
--- a/src/nvme/tree.h
+++ b/src/nvme/tree.h
@@ -452,6 +452,14 @@ const char *nvme_ns_get_sysfs_dir(nvme_ns_t n);
  */
 const char *nvme_ns_get_name(nvme_ns_t n);
 
+/**
+ * nvme_ns_get_status() -
+ * @n:
+ *
+ * Return: 
+ */
+const char *nvme_ns_get_state(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

Re: [PATCH nvme-cli 0/2] add "State" field in command "nvme list"

[Chaitanya Kulkarni]

On 9/15/21 6:29 AM, chengjike wrote:
> When NVMe Subsystem is connected with one or more controllers via fabric, such as NVMe-oF over RDMA,
> if some links are down, some nvme disks on the host are faulty and they won't be deleted immediately.
> But the "nvme list" command does not show these faulty disks.
> So add "State" field  in command "nvme list", and display the status of each disk for users.
> For example:
> [root@localhost nvme-cli]# nvme list
> Node        SN                   Model         Namespace Usage                    Format           FW Rev   Status
> ----------- -------------------- ------------- --------- ------------------------ ---------------- -------- --------
> nvme0n2     25658552656655456265 Huawei-XSG1   2         2.83  GB /  10.74  GB    512   B +  0 B   1000001  live
> nvme0n1     25658552656655456265 Huawei-XSG1   1         2.78  GB /  10.74  GB    512   B +  0 B   1000001  faulty
> 

what if namespace is non NVMeOF ? also you need to make the printing
status optional.


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

Re: [PATCH nvme-cli 0/2] add "State" field in command "nvme list"

[Chengjike (ISSP)]

在 2021/9/17 0:58, Chaitanya Kulkarni 写道:
> On 9/15/21 6:29 AM, chengjike wrote:
>> When NVMe Subsystem is connected with one or more controllers via fabric, such as NVMe-oF over RDMA,
>> if some links are down, some nvme disks on the host are faulty and they won't be deleted immediately.
>> But the "nvme list" command does not show these faulty disks.
>> So add "State" field  in command "nvme list", and display the status of each disk for users.
>> For example:
>> [root@localhost nvme-cli]# nvme list
>> Node        SN                   Model         Namespace Usage                    Format           FW Rev   Status
>> ----------- -------------------- ------------- --------- ------------------------ ---------------- -------- --------
>> nvme0n2     25658552656655456265 Huawei-XSG1   2         2.83  GB /  10.74  GB    512   B +  0 B   1000001  live
>> nvme0n1     25658552656655456265 Huawei-XSG1   1         2.78  GB /  10.74  GB    512   B +  0 B   1000001  faulty
>>
> what if namespace is non NVMeOF ? also you need to make the printing
> status optional.


Sorry for the late reply(we are on vacation in our country).
Yes. If namespace is non NVMeOF, the command of "nvme list"
also displays the status of each disk.


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

Re: [PATCH nvme-cli 0/2] add "State" field in command "nvme list"

[Sagi Grimberg]

> When NVMe Subsystem is connected with one or more controllers via fabric, such as NVMe-oF over RDMA,
> if some links are down, some nvme disks on the host are faulty and they won't be deleted immediately.
> But the "nvme list" command does not show these faulty disks.
> So add "State" field  in command "nvme list", and display the status of each disk for users.
> For example:
> [root@localhost nvme-cli]# nvme list
> Node        SN                   Model         Namespace Usage                    Format           FW Rev   Status
> ----------- -------------------- ------------- --------- ------------------------ ---------------- -------- --------
> nvme0n2     25658552656655456265 Huawei-XSG1   2         2.83  GB /  10.74  GB    512   B +  0 B   1000001  live
> nvme0n1     25658552656655456265 Huawei-XSG1   1         2.78  GB /  10.74  GB    512   B +  0 B   1000001  faulty
> [root@localhost nvme-cli]#

State is too generic, Maybe "Path Access" or something?
The point here is to provide general feedback for the user to go and
look in nvme list-subsys?

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

Re: [PATCH nvme-cli 0/2] add "State" field in command "nvme list"

[Chengjike (ISSP)]

在 2021/9/19 22:05, Sagi Grimberg 写道:
>
>> When NVMe Subsystem is connected with one or more controllers via 
>> fabric, such as NVMe-oF over RDMA,
>> if some links are down, some nvme disks on the host are faulty and 
>> they won't be deleted immediately.
>> But the "nvme list" command does not show these faulty disks.
>> So add "State" field  in command "nvme list", and display the status 
>> of each disk for users.
>> For example:
>> [root@localhost nvme-cli]# nvme list
>> Node        SN                   Model         Namespace 
>> Usage                    Format           FW Rev   Status
>> ----------- -------------------- ------------- --------- 
>> ------------------------ ---------------- -------- --------
>> nvme0n2     25658552656655456265 Huawei-XSG1   2         2.83 GB /  
>> 10.74  GB    512   B +  0 B   1000001  live
>> nvme0n1     25658552656655456265 Huawei-XSG1   1         2.78 GB /  
>> 10.74  GB    512   B +  0 B   1000001  faulty
>> [root@localhost nvme-cli]#
>
> State is too generic, Maybe "Path Access" or something?
> The point here is to provide general feedback for the user to go and
> look in nvme list-subsys?
> .
Yes, I think "Path Access" is better appropriate than "State".

The purpose of this patch is to improve usability for users.
Indeed, when some links are down, We run "nvme list" and "nvme list-subsys"
to determine whether the disk is faulty or
has been deleted(the "nvme list" command does not show faulty disks).
After add "Path Access" field in command "nvme list",
the status of disk will be more intuitive for users
and just only run "nvme list".

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