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

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

[chengjike]

This patch V2 is to add "Path Access" field in command "nvme list".
I have adjusted the patches order, added proper commit description,
modified the "State" to "Path Access".

When NVMe Subsystem is connected with one or more controllers via fabric, such as NVMe-oF over RDMA,  
if the link or NVMe Subsystem array is faulty, some nvme disks on the host are unavailable and 
they won't be deleted immediately. But the "nvme list" command does not show these faulty disks. 
So add "Path Access" field  in command "nvme list", and display the access status of each disk for users.
for examples:

[root@localhost nvme-cli]# nvme list 
Node      SN                   Model        Namespace Usage                      Format           FW Rev   Path Access 
--------- -------------------- ------------ --------- -------------------------- ---------------- -------- ------------
nvme1n1   2102352TSB10KC000015 Huawei-XSG1  1           0.00   B /   0.00   B      0   B +  0 B   1000001  faulty      
nvme0n1   2102352TSB10KC000015 Huawei-XSG1  1           0.00   B /   1.07  GB    512   B +  0 B   1000001  live   
[root@localhost nvme-cli]#


chengjike (1):
  initialize disk "access" variable

 src/nvme/private.h |  1 +
 src/nvme/tree.c    | 57 ++++++++++++++++++++++++++++++++++++++++++----
 src/nvme/tree.h    |  8 +++++++
 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 V2 nvme-cli 1/2] initialize disk "access" variable

[chengjike]

When it is failed to get nvme_id_ns data from NVMe Subsystem, the failure may be caused
by a link disconnection or an error code returned by the subsystem(such as NVME_SC_*).
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.

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..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..dea0f45 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,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->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..7d3d049 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_access() -
+ * @n:
+ *
+ * Return: 
+ */
+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 V2 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 V2 nvme-cli 1/2] initialize disk "access" variable

[Chaitanya Kulkarni]

On 9/26/21 6:41 PM, chengjike wrote:
> External email: Use caution opening links or attachments
> 
> 
> When it is failed to get nvme_id_ns data from NVMe Subsystem, the failure may be caused
> by a link disconnection or an error code returned by the subsystem(such as NVME_SC_*).
> 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.
> 

commit log is overly long and somewhat needs an improvement about what 
code changes we are doing... see below comments...

> 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..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..dea0f45 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;
> +}
> +

why introduce a function that is not used in this patch ? Is this a prep 
patch if so please update the commit log accordingly.

>   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));
> +

setting the memory to zero when allocating from calloc()  ?

> +       n->name = strdup(name);
> +       n->fd = -1;
> +       ns_id = nvme_get_attr(path, "nsid");
> +       if (!ns_id)
> +               goto free_ns;

this label is used only once just return here and remove the goto label.

> +       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;

above error handling code should be moved to if (!ns_id) ...

> +}
> +
>   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->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..7d3d049 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_access() -
> + * @n:
> + *
> + * Return:
> + */

Wrong function documentation comment, you need to be more descriptive
wht the function is doing...

> +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

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

[Chengjike (ISSP)]

在 2021/9/28 3:34, Chaitanya Kulkarni 写道:
> On 9/26/21 6:41 PM, chengjike wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> When it is failed to get nvme_id_ns data from NVMe Subsystem, the failure may be caused
>> by a link disconnection or an error code returned by the subsystem(such as NVME_SC_*).
>> 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.
>>
> commit log is overly long and somewhat needs an improvement about what
> code changes we are doing... see below comments...
>
>> 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..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..dea0f45 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;
>> +}
>> +
> why introduce a function that is not used in this patch ? Is this a prep
> patch if so please update the commit log accordingly.
>
>>    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));
>> +
> setting the memory to zero when allocating from calloc()  ?
>
>> +       n->name = strdup(name);
>> +       n->fd = -1;
>> +       ns_id = nvme_get_attr(path, "nsid");
>> +       if (!ns_id)
>> +               goto free_ns;
> this label is used only once just return here and remove the goto label.
>
>> +       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;
> above error handling code should be moved to if (!ns_id) ...
>
>> +}
>> +
>>    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->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..7d3d049 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_access() -
>> + * @n:
>> + *
>> + * Return:
>> + */
> Wrong function documentation comment, you need to be more descriptive
> wht the function is doing...
>
>> +const char *nvme_ns_get_access(nvme_ns_t n);
>> +
>>    /**
>>     * nvme_ns_get_firmware() -
>>     * @n:
>> --
>> 2.21.0.windows.1
>>
Hi, Chaitanya

Thank you for your comments very much.
This patch is a prep patch, and the "nvme_ns_get_access" function is 
used in the next patch.
In addition, I will resend patch V3 based on your comments.




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