All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 1/2] ndctl: add foreach helper for region badblocks in libndctl
@ 2017-05-10 18:01 Dave Jiang
  2017-05-10 18:03 ` [PATCH v3 2/2] ndctl: add list --media-errors support Dave Jiang
  2017-05-10 22:32 ` [PATCH v3 1/2] ndctl: add foreach helper for region badblocks in libndctl Vishal Verma
  0 siblings, 2 replies; 13+ messages in thread
From: Dave Jiang @ 2017-05-10 18:01 UTC (permalink / raw)
  To: dan.j.williams; +Cc: linux-nvdimm

The helper function allow iteration through the badblocks file that's part
of the region sysfs attributes. This will support the region list badblocks
code that's coming.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---

v2: added cleanup of opened badblocks file from Dan's comments.
v3: fixed inconsistent return code from Andy Rudoff's comments.

 ndctl/lib/libndctl.c   |   74 ++++++++++++++++++++++++++++++++++++++++++++++++
 ndctl/lib/libndctl.sym |    2 +
 ndctl/libndctl.h.in    |   10 ++++++
 3 files changed, 86 insertions(+)

diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index ac1fc63..d85f9ee 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -229,6 +229,8 @@ struct ndctl_region {
 		int state;
 		unsigned long long cookie;
 	} iset;
+	FILE *badblocks;
+	struct badblock bb;
 };
 
 /**
@@ -594,6 +596,8 @@ static void free_region(struct ndctl_region *region)
 	kmod_module_unref(region->module);
 	free(region->region_buf);
 	free(region->region_path);
+	if (region->badblocks)
+		fclose(region->badblocks);
 	free(region);
 }
 
@@ -1867,6 +1871,76 @@ NDCTL_EXPORT struct ndctl_dimm *ndctl_region_get_next_dimm(struct ndctl_region *
 	return NULL;
 }
 
+static int regions_badblocks_init(struct ndctl_region *region)
+{
+	struct ndctl_ctx *ctx = ndctl_region_get_ctx(region);
+	char *bb_path;
+	int rc = 0;
+
+	/* if the file is already open */
+	if (region->badblocks) {
+		fclose(region->badblocks);
+		region->badblocks = NULL;
+	}
+
+	if (asprintf(&bb_path, "%s/badblocks",
+				region->region_path) < 0) {
+		rc = -errno;
+		err(ctx, "region badblocks path allocation failure\n");
+		return rc;
+	}
+
+	region->badblocks = fopen(bb_path, "re");
+	if (!region->badblocks) {
+		rc = -errno;
+		free(bb_path);
+		return rc;
+	}
+
+	free(bb_path);
+	return rc;
+}
+
+NDCTL_EXPORT struct badblock *ndctl_region_get_next_badblock(struct ndctl_region *region)
+{
+	int rc;
+	char *buf = NULL;
+	size_t rlen = 0;
+
+	if (!region->badblocks)
+		return NULL;
+
+	rc = getline(&buf, &rlen, region->badblocks);
+	if (rc == -1) {
+		free(buf);
+		return NULL;
+	}
+
+	rc = sscanf(buf, "%llu %u", &region->bb.offset, &region->bb.len);
+	free(buf);
+	if (rc != 2) {
+		/* end of the road, clean up */
+		fclose(region->badblocks);
+		region->badblocks = NULL;
+		region->bb.offset = 0;
+		region->bb.len = 0;
+		return NULL;
+	}
+
+	return &region->bb;
+}
+
+NDCTL_EXPORT struct badblock *ndctl_region_get_first_badblock(struct ndctl_region *region)
+{
+	int rc;
+
+	rc = regions_badblocks_init(region);
+	if (rc < 0)
+		return NULL;
+
+	return ndctl_region_get_next_badblock(region);
+}
+
 static struct nd_cmd_vendor_tail *to_vendor_tail(struct ndctl_cmd *cmd)
 {
 	struct nd_cmd_vendor_tail *tail = (struct nd_cmd_vendor_tail *)
diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
index b5a085c..9bc36a3 100644
--- a/ndctl/lib/libndctl.sym
+++ b/ndctl/lib/libndctl.sym
@@ -140,6 +140,8 @@ global:
 	ndctl_region_get_ro;
 	ndctl_region_set_ro;
 	ndctl_region_get_resource;
+	ndctl_region_get_first_badblock;
+	ndctl_region_get_next_badblock;
 	ndctl_interleave_set_get_first;
 	ndctl_interleave_set_get_next;
 	ndctl_interleave_set_is_active;
diff --git a/ndctl/libndctl.h.in b/ndctl/libndctl.h.in
index 6ee8a35..2c45d2d 100644
--- a/ndctl/libndctl.h.in
+++ b/ndctl/libndctl.h.in
@@ -372,6 +372,10 @@ int ndctl_cmd_get_status(struct ndctl_cmd *cmd);
 unsigned int ndctl_cmd_get_firmware_status(struct ndctl_cmd *cmd);
 int ndctl_cmd_submit(struct ndctl_cmd *cmd);
 
+struct badblock {
+	unsigned long long offset;
+	unsigned int len;
+};
 struct ndctl_region;
 struct ndctl_region *ndctl_region_get_first(struct ndctl_bus *bus);
 struct ndctl_region *ndctl_region_get_next(struct ndctl_region *region);
@@ -379,6 +383,12 @@ struct ndctl_region *ndctl_region_get_next(struct ndctl_region *region);
         for (region = ndctl_region_get_first(bus); \
              region != NULL; \
              region = ndctl_region_get_next(region))
+struct badblock *ndctl_region_get_first_badblock(struct ndctl_region *region);
+struct badblock *ndctl_region_get_next_badblock(struct ndctl_region *region);
+#define ndctl_region_badblock_foreach(region, badblock) \
+        for (badblock = ndctl_region_get_first_badblock(region); \
+             badblock != NULL; \
+             badblock = ndctl_region_get_next_badblock(region))
 unsigned int ndctl_region_get_id(struct ndctl_region *region);
 const char *ndctl_region_get_devname(struct ndctl_region *region);
 unsigned int ndctl_region_get_interleave_ways(struct ndctl_region *region);

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v3 2/2] ndctl: add list --media-errors support
  2017-05-10 18:01 [PATCH v3 1/2] ndctl: add foreach helper for region badblocks in libndctl Dave Jiang
@ 2017-05-10 18:03 ` Dave Jiang
  2017-05-10 18:43   ` Kani, Toshimitsu
  2017-05-10 22:32 ` [PATCH v3 1/2] ndctl: add foreach helper for region badblocks in libndctl Vishal Verma
  1 sibling, 1 reply; 13+ messages in thread
From: Dave Jiang @ 2017-05-10 18:03 UTC (permalink / raw)
  To: dan.j.williams; +Cc: linux-nvdimm

ACPI NFIT enabled platforms provide media errors as absolute phyiscal
address offsets. Add an option to ndctl to display those media errors
tranlsated to region and namespace device level offsets in an "ndctl
list" listing.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---

v2: added fix to badblocks display from Toshi's testing result.
v3: fixed naming issues from Dan's comments.
    fixed badblocks boundary offset calculations from Toshi's testing.

 ndctl/list.c      |   17 +++++
 ndctl/namespace.c |    2 -
 util/json.c       |  164 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 util/json.h       |    5 +-
 4 files changed, 182 insertions(+), 6 deletions(-)

diff --git a/ndctl/list.c b/ndctl/list.c
index 536d333..5fb0c59 100644
--- a/ndctl/list.c
+++ b/ndctl/list.c
@@ -24,6 +24,7 @@ static struct {
 	bool idle;
 	bool health;
 	bool dax;
+	bool media_errors;
 } list;
 
 static struct {
@@ -99,7 +100,8 @@ static struct json_object *list_namespaces(struct ndctl_region *region,
 						jnamespaces);
 		}
 
-		jndns = util_namespace_to_json(ndns, list.idle, list.dax);
+		jndns = util_namespace_to_json(ndns, list.idle, list.dax,
+				list.media_errors);
 		if (!jndns) {
 			fail("\n");
 			continue;
@@ -117,7 +119,8 @@ static struct json_object *list_namespaces(struct ndctl_region *region,
 	return NULL;
 }
 
-static struct json_object *region_to_json(struct ndctl_region *region)
+static struct json_object *region_to_json(struct ndctl_region *region,
+		bool include_media_err)
 {
 	struct json_object *jregion = json_object_new_object();
 	struct json_object *jobj, *jmappings = NULL;
@@ -203,6 +206,12 @@ static struct json_object *region_to_json(struct ndctl_region *region)
 		json_object_object_add(jregion, "state", jobj);
 	}
 
+	if (include_media_err) {
+		jobj = util_region_badblocks_to_json(region);
+		if (jobj)
+			json_object_object_add(jregion, "badblocks", jobj);
+	}
+
 	list_namespaces(region, jregion, NULL, false);
 	return jregion;
  err:
@@ -240,6 +249,8 @@ int cmd_list(int argc, const char **argv, void *ctx)
 		OPT_BOOLEAN('X', "device-dax", &list.dax,
 				"include device-dax info"),
 		OPT_BOOLEAN('i', "idle", &list.idle, "include idle devices"),
+		OPT_BOOLEAN('M', "media-errors", &list.media_errors,
+				"include media errors"),
 		OPT_END(),
 	};
 	const char * const u[] = {
@@ -404,7 +415,7 @@ int cmd_list(int argc, const char **argv, void *ctx)
 							jregions);
 			}
 
-			jregion = region_to_json(region);
+			jregion = region_to_json(region, list.media_errors);
 			if (!jregion) {
 				fail("\n");
 				continue;
diff --git a/ndctl/namespace.c b/ndctl/namespace.c
index 89b9b6a..6e150b1 100644
--- a/ndctl/namespace.c
+++ b/ndctl/namespace.c
@@ -392,7 +392,7 @@ static int setup_namespace(struct ndctl_region *region,
 		error("%s: failed to enable\n",
 				ndctl_namespace_get_devname(ndns));
 	} else {
-		struct json_object *jndns = util_namespace_to_json(ndns, 0, 1);
+		struct json_object *jndns = util_namespace_to_json(ndns, 0, 1, 0);
 
 		if (jndns)
 			printf("%s\n", json_object_to_json_string_ext(jndns,
diff --git a/util/json.c b/util/json.c
index 07fd113..6b65466 100644
--- a/util/json.c
+++ b/util/json.c
@@ -233,8 +233,149 @@ struct json_object *util_daxctl_region_to_json(struct daxctl_region *region,
 	return NULL;
 }
 
+struct json_object *util_region_badblocks_to_json(struct ndctl_region *region)
+{
+	struct json_object *jbb = NULL, *jbbs, *jobj;
+	struct badblock *bb;
+	int bbs = 0;
+
+	jbbs = json_object_new_array();
+	if (!jbbs)
+		return NULL;
+
+	ndctl_region_badblock_foreach(region, bb) {
+		jbb = json_object_new_object();
+		if (!jbb)
+			goto err_array;
+
+		jobj = json_object_new_int64(bb->offset);
+		if (!jobj)
+			goto err;
+		json_object_object_add(jbb, "offset", jobj);
+
+		jobj = json_object_new_int(bb->len);
+		if (!jobj)
+			goto err;
+		json_object_object_add(jbb, "length", jobj);
+
+		json_object_array_add(jbbs, jbb);
+		bbs++;
+	}
+
+	if (bbs)
+		return jbbs;
+
+ err:
+	json_object_put(jbb);
+ err_array:
+	json_object_put(jbbs);
+	return NULL;
+}
+
+static struct json_object *dev_badblocks_to_json(struct ndctl_region *region,
+		unsigned long long dev_begin, unsigned long long dev_size)
+{
+	struct json_object *jbb = NULL, *jbbs, *jobj;
+	unsigned long long region_begin, dev_end, offset;
+	unsigned int len, bbs = 0;
+	struct badblock *bb;
+
+	region_begin = ndctl_region_get_resource(region);
+	if (region_begin == ULLONG_MAX)
+		return NULL;
+
+	dev_end = dev_begin + dev_size - 1;
+
+	jbbs = json_object_new_array();
+	if (!jbbs)
+		return NULL;
+
+	ndctl_region_badblock_foreach(region, bb) {
+		unsigned long long bb_begin, bb_end, begin, end;
+
+		bb_begin = region_begin + (bb->offset << 9);
+		bb_end = bb_begin + (bb->len << 9) - 1;
+
+		if (bb_end <= dev_begin || bb_begin >= dev_end)
+			continue;
+
+		if (bb_begin < dev_begin)
+			begin = dev_begin;
+		else
+			begin = bb_begin;
+
+		if (bb_end > dev_end)
+			end = dev_end;
+		else
+			end = bb_end;
+
+		offset = (begin - dev_begin) >> 9;
+		len = (end - begin + 1) >> 9;
+
+		/* add to json */
+		jbb = json_object_new_object();
+		if (!jbb)
+			goto err_array;
+
+		jobj = json_object_new_int64(offset);
+		if (!jobj)
+			goto err;
+		json_object_object_add(jbb, "offset", jobj);
+
+		jobj = json_object_new_int(len);
+		if (!jobj)
+			goto err;
+		json_object_object_add(jbb, "length", jobj);
+
+		json_object_array_add(jbbs, jbb);
+		bbs++;
+	}
+
+	if (bbs)
+		return jbbs;
+
+ err:
+	json_object_put(jbb);
+ err_array:
+	json_object_put(jbbs);
+	return NULL;
+}
+
+struct json_object *util_pfn_badblocks_to_json(struct ndctl_pfn *pfn)
+{
+	struct ndctl_region *region = ndctl_pfn_get_region(pfn);
+	unsigned long long pfn_begin, pfn_size;
+
+	pfn_begin = ndctl_pfn_get_resource(pfn);
+	if (pfn_begin == ULLONG_MAX)
+		return NULL;
+
+	pfn_size = ndctl_pfn_get_size(pfn);
+	if (pfn_size == ULLONG_MAX)
+		return NULL;
+
+	return dev_badblocks_to_json(region, pfn_begin, pfn_size);
+}
+
+struct json_object *util_dax_badblocks_to_json(struct ndctl_dax *dax)
+{
+	struct ndctl_region *region = ndctl_dax_get_region(dax);
+	unsigned long long dax_begin, dax_size;
+
+	dax_begin = ndctl_dax_get_resource(dax);
+	if (dax_begin == ULLONG_MAX)
+		return NULL;
+
+	dax_size = ndctl_dax_get_size(dax);
+	if (dax_size == ULLONG_MAX)
+		return NULL;
+
+	return dev_badblocks_to_json(region, dax_begin, dax_size);
+}
+
 struct json_object *util_namespace_to_json(struct ndctl_namespace *ndns,
-		bool include_idle, bool include_dax)
+		bool include_idle, bool include_dax,
+		bool include_media_errors)
 {
 	struct json_object *jndns = json_object_new_object();
 	unsigned long long size = ULLONG_MAX;
@@ -317,6 +458,12 @@ struct json_object *util_namespace_to_json(struct ndctl_namespace *ndns,
 			goto err;
 		json_object_object_add(jndns, "uuid", jobj);
 		bdev = ndctl_pfn_get_block_device(pfn);
+		if (include_media_errors) {
+			jobj = util_pfn_badblocks_to_json(pfn);
+			if (jobj)
+				json_object_object_add(jndns,
+						"badblocks", jobj);
+		}
 	} else if (dax) {
 		struct daxctl_region *dax_region;
 
@@ -333,6 +480,12 @@ struct json_object *util_namespace_to_json(struct ndctl_namespace *ndns,
 			if (jobj)
 				json_object_object_add(jndns, "daxregion", jobj);
 		}
+		if (include_media_errors) {
+			jobj = util_dax_badblocks_to_json(dax);
+			if (jobj)
+				json_object_object_add(jndns,
+						"badblocks", jobj);
+		}
 	} else if (ndctl_namespace_get_type(ndns) != ND_DEVICE_NAMESPACE_IO) {
 		const char *name;
 
@@ -351,6 +504,15 @@ struct json_object *util_namespace_to_json(struct ndctl_namespace *ndns,
 			json_object_object_add(jndns, "name", jobj);
 		}
 		bdev = ndctl_namespace_get_block_device(ndns);
+		if (include_media_errors) {
+			struct ndctl_region *region =
+				ndctl_namespace_get_region(ndns);
+
+			jobj = util_region_badblocks_to_json(region);
+			if (jobj)
+				json_object_object_add(jndns,
+						"badblocks", jobj);
+		}
 	} else
 		bdev = ndctl_namespace_get_block_device(ndns);
 
diff --git a/util/json.h b/util/json.h
index 2449c2d..ef1d606 100644
--- a/util/json.h
+++ b/util/json.h
@@ -10,9 +10,12 @@ struct json_object *util_bus_to_json(struct ndctl_bus *bus);
 struct json_object *util_dimm_to_json(struct ndctl_dimm *dimm);
 struct json_object *util_mapping_to_json(struct ndctl_mapping *mapping);
 struct json_object *util_namespace_to_json(struct ndctl_namespace *ndns,
-		bool include_idle, bool include_dax);
+		bool include_idle, bool include_dax, bool include_media_errs);
 struct daxctl_region;
 struct daxctl_dev;
+struct json_object *util_dax_badblocks_to_json(struct ndctl_dax *dax);
+struct json_object *util_pfn_badblocks_to_json(struct ndctl_pfn *pfn);
+struct json_object *util_region_badblocks_to_json(struct ndctl_region *region);
 struct json_object *util_daxctl_region_to_json(struct daxctl_region *region,
 		bool include_devs, const char *ident, bool include_idle);
 struct json_object *util_daxctl_dev_to_json(struct daxctl_dev *dev);

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 2/2] ndctl: add list --media-errors support
  2017-05-10 18:03 ` [PATCH v3 2/2] ndctl: add list --media-errors support Dave Jiang
@ 2017-05-10 18:43   ` Kani, Toshimitsu
  2017-05-10 19:56     ` Dave Jiang
  0 siblings, 1 reply; 13+ messages in thread
From: Kani, Toshimitsu @ 2017-05-10 18:43 UTC (permalink / raw)
  To: dan.j.williams, dave.jiang; +Cc: linux-nvdimm

On Wed, 2017-05-10 at 11:03 -0700, Dave Jiang wrote:
> ACPI NFIT enabled platforms provide media errors as absolute phyiscal
> address offsets. Add an option to ndctl to display those media errors
> tranlsated to region and namespace device level offsets in an "ndctl
> list" listing.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> 
> v2: added fix to badblocks display from Toshi's testing result.
> v3: fixed naming issues from Dan's comments.
>     fixed badblocks boundary offset calculations from Toshi's
> testing.

Thanks for the quick update.  This version works nicely for dax and
memory modes.  Is there particular reason why it does not list errors
for raw and sector modes?

-Toshi
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 2/2] ndctl: add list --media-errors support
  2017-05-10 18:43   ` Kani, Toshimitsu
@ 2017-05-10 19:56     ` Dave Jiang
  2017-05-10 20:57       ` Kani, Toshimitsu
  0 siblings, 1 reply; 13+ messages in thread
From: Dave Jiang @ 2017-05-10 19:56 UTC (permalink / raw)
  To: Kani, Toshimitsu, Williams, Dan J; +Cc: linux-nvdimm



On 05/10/2017 11:43 AM, Kani, Toshimitsu wrote:
> On Wed, 2017-05-10 at 11:03 -0700, Dave Jiang wrote:
>> ACPI NFIT enabled platforms provide media errors as absolute phyiscal
>> address offsets. Add an option to ndctl to display those media errors
>> tranlsated to region and namespace device level offsets in an "ndctl
>> list" listing.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>
>> v2: added fix to badblocks display from Toshi's testing result.
>> v3: fixed naming issues from Dan's comments.
>>     fixed badblocks boundary offset calculations from Toshi's
>> testing.
> 
> Thanks for the quick update.  This version works nicely for dax and
> memory modes.  Is there particular reason why it does not list errors
> for raw and sector modes?

So it should work for raw AFAICT. Dan said the BTT ones don't make sense
because the namespace relative offset is dynamic.


# ./ndctl/ndctl list -NM
[
  {
    "dev":"namespace5.0",
    "mode":"raw",
    "size":33554432,
    "uuid":"bb3539dd-065c-41bd-8bcc-dc3bbc3317a5",
    "badblocks":[
      {
        "offset":4,
        "length":1
      },
      {
        "offset":32768,
        "length":8
      },
      {
        "offset":65528,
        "length":8
      }
    ],
    "blockdev":"pmem5"
  },
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 2/2] ndctl: add list --media-errors support
  2017-05-10 19:56     ` Dave Jiang
@ 2017-05-10 20:57       ` Kani, Toshimitsu
  2017-05-10 21:13         ` Dave Jiang
  0 siblings, 1 reply; 13+ messages in thread
From: Kani, Toshimitsu @ 2017-05-10 20:57 UTC (permalink / raw)
  To: dan.j.williams, dave.jiang; +Cc: linux-nvdimm

On Wed, 2017-05-10 at 12:56 -0700, Dave Jiang wrote:
> 
> On 05/10/2017 11:43 AM, Kani, Toshimitsu wrote:
> > On Wed, 2017-05-10 at 11:03 -0700, Dave Jiang wrote:
> > > ACPI NFIT enabled platforms provide media errors as absolute
> > > phyiscal
> > > address offsets. Add an option to ndctl to display those media
> > > errors
> > > tranlsated to region and namespace device level offsets in an
> > > "ndctl
> > > list" listing.
> > > 
> > > Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> > > ---
> > > 
> > > v2: added fix to badblocks display from Toshi's testing result.
> > > v3: fixed naming issues from Dan's comments.
> > >     fixed badblocks boundary offset calculations from Toshi's
> > > testing.
> > 
> > Thanks for the quick update.  This version works nicely for dax and
> > memory modes.  Is there particular reason why it does not list
> > errors for raw and sector modes?
> 
> So it should work for raw AFAICT. Dan said the BTT ones don't make
> sense because the namespace relative offset is dynamic.
> 
> 
> # ./ndctl/ndctl list -NM
> [
>   {
>     "dev":"namespace5.0",
>     "mode":"raw",
>     "size":33554432,
>     "uuid":"bb3539dd-065c-41bd-8bcc-dc3bbc3317a5",
>     "badblocks":[
>       {
>         "offset":4,
>         "length":1
>       },
>       {
>         "offset":32768,
>         "length":8
>       },
>       {
>         "offset":65528,
>         "length":8
>       }
>     ],
>     "blockdev":"pmem5"
>   },

Attached "strace.log".  Note, there is no reference to "badblocks"
files in the log.

I agree that offsets do not make sense for BTT, but it leads to a false
impression that there is no error.  Perhaps, we can remove "offset" or
put "n/a" for BTT.  

Thanks,
-Toshi


_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 2/2] ndctl: add list --media-errors support
  2017-05-10 20:57       ` Kani, Toshimitsu
@ 2017-05-10 21:13         ` Dave Jiang
  2017-05-10 21:28           ` Kani, Toshimitsu
  2017-05-10 22:24           ` Dan Williams
  0 siblings, 2 replies; 13+ messages in thread
From: Dave Jiang @ 2017-05-10 21:13 UTC (permalink / raw)
  To: Kani, Toshimitsu, Williams, Dan J; +Cc: linux-nvdimm



On 05/10/2017 01:57 PM, Kani, Toshimitsu wrote:
> On Wed, 2017-05-10 at 12:56 -0700, Dave Jiang wrote:
>>
>> On 05/10/2017 11:43 AM, Kani, Toshimitsu wrote:
>>> On Wed, 2017-05-10 at 11:03 -0700, Dave Jiang wrote:
>>>> ACPI NFIT enabled platforms provide media errors as absolute
>>>> phyiscal
>>>> address offsets. Add an option to ndctl to display those media
>>>> errors
>>>> tranlsated to region and namespace device level offsets in an
>>>> "ndctl
>>>> list" listing.
>>>>
>>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>>> ---
>>>>
>>>> v2: added fix to badblocks display from Toshi's testing result.
>>>> v3: fixed naming issues from Dan's comments.
>>>>     fixed badblocks boundary offset calculations from Toshi's
>>>> testing.
>>>
>>> Thanks for the quick update.  This version works nicely for dax and
>>> memory modes.  Is there particular reason why it does not list
>>> errors for raw and sector modes?
>>
>> So it should work for raw AFAICT. Dan said the BTT ones don't make
>> sense because the namespace relative offset is dynamic.
>>
>>
>> # ./ndctl/ndctl list -NM
>> [
>>   {
>>     "dev":"namespace5.0",
>>     "mode":"raw",
>>     "size":33554432,
>>     "uuid":"bb3539dd-065c-41bd-8bcc-dc3bbc3317a5",
>>     "badblocks":[
>>       {
>>         "offset":4,
>>         "length":1
>>       },
>>       {
>>         "offset":32768,
>>         "length":8
>>       },
>>       {
>>         "offset":65528,
>>         "length":8
>>       }
>>     ],
>>     "blockdev":"pmem5"
>>   },
> 
> Attached "strace.log".  Note, there is no reference to "badblocks"
> files in the log.

Hmmm....is there any reason my raw namespace is different type than
yours? Do you know if yours hit the else branch on util/json.c:516 in
util_namespace_to_json()? That is the only thing I can think of that
would show no badblocks in the strace.

> 
> I agree that offsets do not make sense for BTT, but it leads to a false
> impression that there is no error.  Perhaps, we can remove "offset" or
> put "n/a" for BTT.

I'm ok with that if Dan has no problem with it.

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 2/2] ndctl: add list --media-errors support
  2017-05-10 21:13         ` Dave Jiang
@ 2017-05-10 21:28           ` Kani, Toshimitsu
  2017-05-10 21:34             ` Dave Jiang
  2017-05-10 22:24           ` Dan Williams
  1 sibling, 1 reply; 13+ messages in thread
From: Kani, Toshimitsu @ 2017-05-10 21:28 UTC (permalink / raw)
  To: dan.j.williams, dave.jiang; +Cc: linux-nvdimm

On Wed, 2017-05-10 at 14:13 -0700, Dave Jiang wrote:
> 
> On 05/10/2017 01:57 PM, Kani, Toshimitsu wrote:
> > On Wed, 2017-05-10 at 12:56 -0700, Dave Jiang wrote:
> > > 
> > > On 05/10/2017 11:43 AM, Kani, Toshimitsu wrote:
 :
> > 
> > Attached "strace.log".  Note, there is no reference to "badblocks"
> > files in the log.
> 
> Hmmm....is there any reason my raw namespace is different type than
> yours? Do you know if yours hit the else branch on util/json.c:516 in
> util_namespace_to_json()? That is the only thing I can think of that
> would show no badblocks in the strace.

Yes, it hits the else case.  Not sure if it's related, but our raw
device does not have any metadata so it does not have uuid.

Thanks,
-Toshi
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 2/2] ndctl: add list --media-errors support
  2017-05-10 21:28           ` Kani, Toshimitsu
@ 2017-05-10 21:34             ` Dave Jiang
  2017-05-10 21:47               ` Kani, Toshimitsu
  0 siblings, 1 reply; 13+ messages in thread
From: Dave Jiang @ 2017-05-10 21:34 UTC (permalink / raw)
  To: Kani, Toshimitsu, Williams, Dan J; +Cc: linux-nvdimm



On 05/10/2017 02:28 PM, Kani, Toshimitsu wrote:
> On Wed, 2017-05-10 at 14:13 -0700, Dave Jiang wrote:
>>
>> On 05/10/2017 01:57 PM, Kani, Toshimitsu wrote:
>>> On Wed, 2017-05-10 at 12:56 -0700, Dave Jiang wrote:
>>>>
>>>> On 05/10/2017 11:43 AM, Kani, Toshimitsu wrote:
>  :
>>>
>>> Attached "strace.log".  Note, there is no reference to "badblocks"
>>> files in the log.
>>
>> Hmmm....is there any reason my raw namespace is different type than
>> yours? Do you know if yours hit the else branch on util/json.c:516 in
>> util_namespace_to_json()? That is the only thing I can think of that
>> would show no badblocks in the strace.
> 
> Yes, it hits the else case.  Not sure if it's related, but our raw
> device does not have any metadata so it does not have uuid.

The raw namespace is not ND_DEVICE_NAMESPACE_IO for you. Maybe that's
correlated to not having uuid? I can add dumping of badblocks to the
else branch. Essentially it's a pass through of the region badblocks for
raw ns.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 2/2] ndctl: add list --media-errors support
  2017-05-10 21:34             ` Dave Jiang
@ 2017-05-10 21:47               ` Kani, Toshimitsu
  2017-05-10 22:07                 ` Dan Williams
  0 siblings, 1 reply; 13+ messages in thread
From: Kani, Toshimitsu @ 2017-05-10 21:47 UTC (permalink / raw)
  To: dan.j.williams, dave.jiang; +Cc: linux-nvdimm

On Wed, 2017-05-10 at 14:34 -0700, Dave Jiang wrote:
> 
> On 05/10/2017 02:28 PM, Kani, Toshimitsu wrote:
> > On Wed, 2017-05-10 at 14:13 -0700, Dave Jiang wrote:
> > > 
> > > On 05/10/2017 01:57 PM, Kani, Toshimitsu wrote:
> > > > On Wed, 2017-05-10 at 12:56 -0700, Dave Jiang wrote:
> > > > > 
> > > > > On 05/10/2017 11:43 AM, Kani, Toshimitsu wrote:
> > 
> >  :
> > > > 
> > > > Attached "strace.log".  Note, there is no reference to
> > > > "badblocks"
> > > > files in the log.
> > > 
> > > Hmmm....is there any reason my raw namespace is different type
> > > than yours? Do you know if yours hit the else branch on
> > > util/json.c:516 in util_namespace_to_json()? That is the only
> > > thing I can think of that would show no badblocks in the strace.
> > 
> > Yes, it hits the else case.  Not sure if it's related, but our raw
> > device does not have any metadata so it does not have uuid.
> 
> The raw namespace is not ND_DEVICE_NAMESPACE_IO for you. Maybe that's
> correlated to not having uuid? I can add dumping of badblocks to the
> else branch. Essentially it's a pass through of the region badblocks
> for raw ns.

Our raw namespace _is_ ND_DEVICE_NAMESPACE_IO.  Yes, it looks like this
is causing the issue.

Thanks,
-Toshi
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 2/2] ndctl: add list --media-errors support
  2017-05-10 21:47               ` Kani, Toshimitsu
@ 2017-05-10 22:07                 ` Dan Williams
  0 siblings, 0 replies; 13+ messages in thread
From: Dan Williams @ 2017-05-10 22:07 UTC (permalink / raw)
  To: Kani, Toshimitsu; +Cc: linux-nvdimm

On Wed, May 10, 2017 at 2:47 PM, Kani, Toshimitsu <toshi.kani@hpe.com> wrote:
> On Wed, 2017-05-10 at 14:34 -0700, Dave Jiang wrote:
>>
>> On 05/10/2017 02:28 PM, Kani, Toshimitsu wrote:
>> > On Wed, 2017-05-10 at 14:13 -0700, Dave Jiang wrote:
>> > >
>> > > On 05/10/2017 01:57 PM, Kani, Toshimitsu wrote:
>> > > > On Wed, 2017-05-10 at 12:56 -0700, Dave Jiang wrote:
>> > > > >
>> > > > > On 05/10/2017 11:43 AM, Kani, Toshimitsu wrote:
>> >
>> >  :
>> > > >
>> > > > Attached "strace.log".  Note, there is no reference to
>> > > > "badblocks"
>> > > > files in the log.
>> > >
>> > > Hmmm....is there any reason my raw namespace is different type
>> > > than yours? Do you know if yours hit the else branch on
>> > > util/json.c:516 in util_namespace_to_json()? That is the only
>> > > thing I can think of that would show no badblocks in the strace.
>> >
>> > Yes, it hits the else case.  Not sure if it's related, but our raw
>> > device does not have any metadata so it does not have uuid.
>>
>> The raw namespace is not ND_DEVICE_NAMESPACE_IO for you. Maybe that's
>> correlated to not having uuid? I can add dumping of badblocks to the
>> else branch. Essentially it's a pass through of the region badblocks
>> for raw ns.
>
> Our raw namespace _is_ ND_DEVICE_NAMESPACE_IO.  Yes, it looks like this
> is causing the issue.

Yeah, that's my fault. I told Dave that we did not need to worry about
memmap= namespaces, but as far as ndctl is concerned memmap= and other
NVDIMMs without labels are identical.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 2/2] ndctl: add list --media-errors support
  2017-05-10 21:13         ` Dave Jiang
  2017-05-10 21:28           ` Kani, Toshimitsu
@ 2017-05-10 22:24           ` Dan Williams
  2017-05-10 22:33             ` Kani, Toshimitsu
  1 sibling, 1 reply; 13+ messages in thread
From: Dan Williams @ 2017-05-10 22:24 UTC (permalink / raw)
  To: Dave Jiang; +Cc: linux-nvdimm

On Wed, May 10, 2017 at 2:13 PM, Dave Jiang <dave.jiang@intel.com> wrote:
>
>
> On 05/10/2017 01:57 PM, Kani, Toshimitsu wrote:
>> On Wed, 2017-05-10 at 12:56 -0700, Dave Jiang wrote:
>>>
>>> On 05/10/2017 11:43 AM, Kani, Toshimitsu wrote:
>>>> On Wed, 2017-05-10 at 11:03 -0700, Dave Jiang wrote:
>>>>> ACPI NFIT enabled platforms provide media errors as absolute
>>>>> phyiscal
>>>>> address offsets. Add an option to ndctl to display those media
>>>>> errors
>>>>> tranlsated to region and namespace device level offsets in an
>>>>> "ndctl
>>>>> list" listing.
>>>>>
>>>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>>>> ---
>>>>>
>>>>> v2: added fix to badblocks display from Toshi's testing result.
>>>>> v3: fixed naming issues from Dan's comments.
>>>>>     fixed badblocks boundary offset calculations from Toshi's
>>>>> testing.
>>>>
>>>> Thanks for the quick update.  This version works nicely for dax and
>>>> memory modes.  Is there particular reason why it does not list
>>>> errors for raw and sector modes?
>>>
>>> So it should work for raw AFAICT. Dan said the BTT ones don't make
>>> sense because the namespace relative offset is dynamic.
>>>
>>>
>>> # ./ndctl/ndctl list -NM
>>> [
>>>   {
>>>     "dev":"namespace5.0",
>>>     "mode":"raw",
>>>     "size":33554432,
>>>     "uuid":"bb3539dd-065c-41bd-8bcc-dc3bbc3317a5",
>>>     "badblocks":[
>>>       {
>>>         "offset":4,
>>>         "length":1
>>>       },
>>>       {
>>>         "offset":32768,
>>>         "length":8
>>>       },
>>>       {
>>>         "offset":65528,
>>>         "length":8
>>>       }
>>>     ],
>>>     "blockdev":"pmem5"
>>>   },
>>
>> Attached "strace.log".  Note, there is no reference to "badblocks"
>> files in the log.
>
> Hmmm....is there any reason my raw namespace is different type than
> yours? Do you know if yours hit the else branch on util/json.c:516 in
> util_namespace_to_json()? That is the only thing I can think of that
> would show no badblocks in the strace.
>
>>
>> I agree that offsets do not make sense for BTT, but it leads to a false
>> impression that there is no error.  Perhaps, we can remove "offset" or
>> put "n/a" for BTT.
>
> I'm ok with that if Dan has no problem with it.
>

It doesn't strike me as the best path forward. Ideally what we need is
a customized btt badblocks interface that knows how to reverse lookup
media errors into active / btt relative sector offsets. Until we have
that, to list the errors that are associated with the btt namespace
you need to reload it in raw mode or just relay on the parent region
media error list. Now, that said I wouldn't mind if we had a flag that
indicates whether badblocks are present or not by default, and then -M
tries to list them. Something like this where we have 2 namespaces in
the same region that has badblocks:

# ndctl list
[
  {
    "dev":"namespace0.0",
    "mode":"sector",
    "size":4294967296,
    "uuid":"3f45c754-5e8e-4d3e-89db-53ae5cc5a9bd",
    "blockdev":"pmem0s",
    "has_badblocks":true,
  },
  {
    "dev":"namespace0.1",
    "mode":"memory",
    "size":15852371968,
    "uuid":"6446b3bd-b64a-4163-b542-d82ed557fb78",
    "blockdev":"pmem0.1"
    "has_badblocks":true,
 },
]

# ndctl list -M
[
  {
    "dev":"namespace0.0",
    "mode":"sector",
    "size":4294967296,
    "uuid":"3f45c754-5e8e-4d3e-89db-53ae5cc5a9bd",
    "blockdev":"pmem0s",
    "has_badblocks":true,
  },
  {
    "dev":"namespace0.1",
    "mode":"memory",
    "size":15852371968,
    "uuid":"6446b3bd-b64a-4163-b542-d82ed557fb78",
    "blockdev":"pmem0.1"
    "has_badblocks":true,
    "badblocks":[
      {
        "offset":4,
        "length":1
      },
 },
]

In the common case where there are no badblocks we can just had the
"has_badblocks" boolean.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 1/2] ndctl: add foreach helper for region badblocks in libndctl
  2017-05-10 18:01 [PATCH v3 1/2] ndctl: add foreach helper for region badblocks in libndctl Dave Jiang
  2017-05-10 18:03 ` [PATCH v3 2/2] ndctl: add list --media-errors support Dave Jiang
@ 2017-05-10 22:32 ` Vishal Verma
  1 sibling, 0 replies; 13+ messages in thread
From: Vishal Verma @ 2017-05-10 22:32 UTC (permalink / raw)
  To: Dave Jiang; +Cc: linux-nvdimm

On 05/10, Dave Jiang wrote:
> The helper function allow iteration through the badblocks file that's part
> of the region sysfs attributes. This will support the region list badblocks
> code that's coming.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> 
> v2: added cleanup of opened badblocks file from Dan's comments.
> v3: fixed inconsistent return code from Andy Rudoff's comments.
> 
>  ndctl/lib/libndctl.c   |   74 ++++++++++++++++++++++++++++++++++++++++++++++++
>  ndctl/lib/libndctl.sym |    2 +
>  ndctl/libndctl.h.in    |   10 ++++++
>  3 files changed, 86 insertions(+)
> 
[...]
> +
> +NDCTL_EXPORT struct badblock *ndctl_region_get_next_badblock(struct ndctl_region *region)
> +{
> +	int rc;
> +	char *buf = NULL;
> +	size_t rlen = 0;
> +
> +	if (!region->badblocks)
> +		return NULL;
> +
> +	rc = getline(&buf, &rlen, region->badblocks);
> +	if (rc == -1) {
> +		free(buf);
> +		return NULL;
> +	}
> +
> +	rc = sscanf(buf, "%llu %u", &region->bb.offset, &region->bb.len);
> +	free(buf);
> +	if (rc != 2) {
> +		/* end of the road, clean up */

Quick nit - "end of road" doesn't make sense any more as this isn't
reached for EOF

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 2/2] ndctl: add list --media-errors support
  2017-05-10 22:24           ` Dan Williams
@ 2017-05-10 22:33             ` Kani, Toshimitsu
  0 siblings, 0 replies; 13+ messages in thread
From: Kani, Toshimitsu @ 2017-05-10 22:33 UTC (permalink / raw)
  To: dan.j.williams, dave.jiang; +Cc: linux-nvdimm

On Wed, 2017-05-10 at 15:24 -0700, Dan Williams wrote:
> On Wed, May 10, 2017 at 2:13 PM, Dave Jiang <dave.jiang@intel.com>
> wrote:
> > On 05/10/2017 01:57 PM, Kani, Toshimitsu wrote:
> > > On Wed, 2017-05-10 at 12:56 -0700, Dave Jiang wrote:
> > > > 
 :
> > > 
> > > I agree that offsets do not make sense for BTT, but it leads to a
> > > false impression that there is no error.  Perhaps, we can remove
> > > "offset" or put "n/a" for BTT.
> > 
> > I'm ok with that if Dan has no problem with it.
> > 
> 
> It doesn't strike me as the best path forward. Ideally what we need
> is a customized btt badblocks interface that knows how to reverse
> lookup media errors into active / btt relative sector offsets. Until
> we have that, to list the errors that are associated with the btt
> namespace you need to reload it in raw mode or just relay on the
> parent region media error list. Now, that said I wouldn't mind if we
> had a flag that indicates whether badblocks are present or not by
> default, and then -M tries to list them. Something like this where we
> have 2 namespaces in the same region that has badblocks:
> 
> # ndctl list
> [
>   {
>     "dev":"namespace0.0",
>     "mode":"sector",
>     "size":4294967296,
>     "uuid":"3f45c754-5e8e-4d3e-89db-53ae5cc5a9bd",
>     "blockdev":"pmem0s",
>     "has_badblocks":true,
>   },
>   {
>     "dev":"namespace0.1",
>     "mode":"memory",
>     "size":15852371968,
>     "uuid":"6446b3bd-b64a-4163-b542-d82ed557fb78",
>     "blockdev":"pmem0.1"
>     "has_badblocks":true,
>  },
> ]
> 
> # ndctl list -M
> [
>   {
>     "dev":"namespace0.0",
>     "mode":"sector",
>     "size":4294967296,
>     "uuid":"3f45c754-5e8e-4d3e-89db-53ae5cc5a9bd",
>     "blockdev":"pmem0s",
>     "has_badblocks":true,
>   },
>   {
>     "dev":"namespace0.1",
>     "mode":"memory",
>     "size":15852371968,
>     "uuid":"6446b3bd-b64a-4163-b542-d82ed557fb78",
>     "blockdev":"pmem0.1"
>     "has_badblocks":true,
>     "badblocks":[
>       {
>         "offset":4,
>         "length":1
>       },
>  },
> ]
> 
> In the common case where there are no badblocks we can just had the
> "has_badblocks" boolean.

Good idea!  With "has_badblocks" info, yes, we can avoid the false
impression I concerned about, and can wait for a customized btt
badblocks interface.

Thanks,
-Toshi


_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2017-05-10 22:34 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-10 18:01 [PATCH v3 1/2] ndctl: add foreach helper for region badblocks in libndctl Dave Jiang
2017-05-10 18:03 ` [PATCH v3 2/2] ndctl: add list --media-errors support Dave Jiang
2017-05-10 18:43   ` Kani, Toshimitsu
2017-05-10 19:56     ` Dave Jiang
2017-05-10 20:57       ` Kani, Toshimitsu
2017-05-10 21:13         ` Dave Jiang
2017-05-10 21:28           ` Kani, Toshimitsu
2017-05-10 21:34             ` Dave Jiang
2017-05-10 21:47               ` Kani, Toshimitsu
2017-05-10 22:07                 ` Dan Williams
2017-05-10 22:24           ` Dan Williams
2017-05-10 22:33             ` Kani, Toshimitsu
2017-05-10 22:32 ` [PATCH v3 1/2] ndctl: add foreach helper for region badblocks in libndctl Vishal Verma

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.