nvdimm.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
* [ndctl PATCH 1/5] ndctl, lib: refactor badblocks retrieval routines
@ 2018-10-02  3:31 Vishal Verma
  2018-10-02  3:31 ` [ndctl PATCH 2/5] ndctl, lib: add APIs for retrieving namespace badblocks Vishal Verma
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Vishal Verma @ 2018-10-02  3:31 UTC (permalink / raw)
  To: linux-nvdimm

From: Dan Williams <dan.j.williams@intel.com>

From: Dan Williams <dan.j.williams@intel.com>

The kernel provides two locations in sysfs that contain badblocks
information. The only one ndctl currently uses is region based
badblocks. The problem with this is that this requires root privileges
to calculate namespace-offset badblocks. Since the kernel already
provides a world readable location for namespace badblocks, we can fall
back to that.

Refactor the badblocks retrieval routines to use a bb_iter structure
that contains information for what badblocks file in sysfs to use, to
prepare for a direct namespace-badblocks fallback.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
[vishal] remove an error print triggered by opening an invalid file
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 ndctl/lib/libndctl.c | 141 +++++++++++++++++++++++--------------------
 ndctl/lib/private.h  |   6 ++
 2 files changed, 82 insertions(+), 65 deletions(-)

diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index 481b110..d58be47 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -178,8 +178,7 @@ struct ndctl_region {
 		int state;
 		unsigned long long cookie;
 	} iset;
-	FILE *badblocks;
-	struct badblock bb;
+	struct badblocks_iter bb_iter;
 	enum ndctl_persistence_domain persistence_domain;
 	/* file descriptor for deep flush sysfs entry */
 	int flush_fd;
@@ -376,6 +375,77 @@ NDCTL_EXPORT struct ndctl_ctx *ndctl_ref(struct ndctl_ctx *ctx)
 	return ctx;
 }
 
+static void badblocks_iter_free(struct badblocks_iter *bb_iter)
+{
+	if (bb_iter->file)
+		fclose(bb_iter->file);
+}
+
+static int badblocks_iter_init(struct badblocks_iter *bb_iter, const char *path)
+{
+	char *bb_path;
+	int rc = 0;
+
+	/* if the file is already open */
+	if (bb_iter->file) {
+		fclose(bb_iter->file);
+		bb_iter->file = NULL;
+	}
+
+	if (asprintf(&bb_path, "%s/badblocks", path) < 0)
+		return -errno;
+
+	bb_iter->file = fopen(bb_path, "re");
+	if (!bb_iter->file) {
+		rc = -errno;
+		free(bb_path);
+		return rc;
+	}
+
+	free(bb_path);
+	return rc;
+}
+
+static struct badblock *badblocks_iter_next(struct badblocks_iter *bb_iter)
+{
+	int rc;
+	char *buf = NULL;
+	size_t rlen = 0;
+
+	if (!bb_iter->file)
+		return NULL;
+
+	rc = getline(&buf, &rlen, bb_iter->file);
+	if (rc == -1) {
+		free(buf);
+		return NULL;
+	}
+
+	rc = sscanf(buf, "%llu %u", &bb_iter->bb.offset, &bb_iter->bb.len);
+	free(buf);
+	if (rc != 2) {
+		fclose(bb_iter->file);
+		bb_iter->file = NULL;
+		bb_iter->bb.offset = 0;
+		bb_iter->bb.len = 0;
+		return NULL;
+	}
+
+	return &bb_iter->bb;
+}
+
+static struct badblock *badblocks_iter_first(struct badblocks_iter *bb_iter,
+		struct ndctl_ctx *ctx, const char *path)
+{
+	int rc;
+
+	rc = badblocks_iter_init(bb_iter, path);
+	if (rc < 0)
+		return NULL;
+
+	return badblocks_iter_next(bb_iter);
+}
+
 static void free_namespace(struct ndctl_namespace *ndns, struct list_head *head)
 {
 	struct ndctl_bb *bb, *next;
@@ -511,8 +581,7 @@ 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);
+	badblocks_iter_free(&region->bb_iter);
 	if (region->flush_fd > 0)
 		close(region->flush_fd);
 	free(region);
@@ -2253,73 +2322,15 @@ NDCTL_EXPORT int ndctl_region_get_numa_node(struct ndctl_region *region)
 	return region->numa_node;
 }
 
-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) {
-		fclose(region->badblocks);
-		region->badblocks = NULL;
-		region->bb.offset = 0;
-		region->bb.len = 0;
-		return NULL;
-	}
-
-	return &region->bb;
+	return badblocks_iter_next(&region->bb_iter);
 }
 
 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);
+	return badblocks_iter_first(&region->bb_iter,
+			ndctl_region_get_ctx(region), region->region_path);
 }
 
 NDCTL_EXPORT enum ndctl_persistence_domain
diff --git a/ndctl/lib/private.h b/ndctl/lib/private.h
index b6e438a..d8ed614 100644
--- a/ndctl/lib/private.h
+++ b/ndctl/lib/private.h
@@ -185,6 +185,11 @@ struct ndctl_lbasize {
 	int num;
 };
 
+struct badblocks_iter {
+	struct badblock bb;
+	FILE *file;
+};
+
 /**
  * struct ndctl_namespace - device claimed by the nd_blk or nd_pmem driver
  * @module: kernel module
@@ -209,6 +214,7 @@ struct ndctl_namespace {
 	int generation;
 	unsigned long long resource, size;
 	enum ndctl_namespace_mode enforce_mode;
+	struct badblocks_iter bb_iter;
 	char *alt_name;
 	uuid_t uuid;
 	struct ndctl_lbasize lbasize;
-- 
2.17.1

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

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

* [ndctl PATCH 2/5] ndctl, lib: add APIs for retrieving namespace badblocks
  2018-10-02  3:31 [ndctl PATCH 1/5] ndctl, lib: refactor badblocks retrieval routines Vishal Verma
@ 2018-10-02  3:31 ` Vishal Verma
  2018-10-02  3:31 ` [ndctl PATCH 3/5] util/json: fix an error check for region resource Vishal Verma
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 8+ messages in thread
From: Vishal Verma @ 2018-10-02  3:31 UTC (permalink / raw)
  To: linux-nvdimm

From: Dan Williams <dan.j.williams@intel.com>

From: Dan Williams <dan.j.williams@intel.com>

Use the new bb_iter mechanism to provide APIs that retrieve the
namespace badblocks from the kernel's block device representation in
sysfs. This paves the way for a namespace based fallback if
region-badblocks are unavailable.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
[vishal] add the new global symbols to libndctl.sym
[vishal] fix the badblocks path to remove the trailing 'badblocks'
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 ndctl/lib/libndctl.c   | 42 ++++++++++++++++++++++++++++++++++++++++++
 ndctl/lib/libndctl.sym |  6 ++++++
 ndctl/libndctl.h       |  6 ++++++
 3 files changed, 54 insertions(+)

diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index d58be47..d341100 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -3610,6 +3610,48 @@ NDCTL_EXPORT int ndctl_namespace_is_enabled(struct ndctl_namespace *ndns)
 	return is_enabled(ndctl_namespace_get_bus(ndns), path);
 }
 
+NDCTL_EXPORT struct badblock *ndctl_namespace_get_next_badblock(
+		struct ndctl_namespace *ndns)
+{
+	return badblocks_iter_next(&ndns->bb_iter);
+}
+
+NDCTL_EXPORT struct badblock *ndctl_namespace_get_first_badblock(
+		struct ndctl_namespace *ndns)
+{
+	struct ndctl_btt *btt = ndctl_namespace_get_btt(ndns);
+	struct ndctl_pfn *pfn = ndctl_namespace_get_pfn(ndns);
+	struct ndctl_dax *dax = ndctl_namespace_get_dax(ndns);
+	struct ndctl_ctx *ctx = ndctl_namespace_get_ctx(ndns);
+	const char *dev = ndctl_namespace_get_devname(ndns);
+	char path[SYSFS_ATTR_SIZE];
+	ssize_t len = sizeof(path);
+	const char *bdev;
+
+	if (btt || dax) {
+		dbg(ctx, "%s: badblocks not supported for %s\n", dev,
+				btt ? "btt" : "device-dax");
+		return NULL;
+	}
+
+	if (pfn)
+		bdev = ndctl_pfn_get_block_device(pfn);
+	else
+		bdev = ndctl_namespace_get_block_device(ndns);
+
+	if (!bdev) {
+		dbg(ctx, "%s: failed to determine block device\n", dev);
+		return NULL;
+	}
+
+	if (snprintf(path, len, "/sys/block/%s", bdev) >= len) {
+		err(ctx, "%s: buffer too small!\n", dev);
+		return NULL;
+	}
+
+	return badblocks_iter_first(&ndns->bb_iter, ctx, path);
+}
+
 static int ndctl_bind(struct ndctl_ctx *ctx, struct kmod_module *module,
 		const char *devname)
 {
diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
index a6849ee..185ee4a 100644
--- a/ndctl/lib/libndctl.sym
+++ b/ndctl/lib/libndctl.sym
@@ -378,3 +378,9 @@ global:
 	ndctl_region_get_max_available_extent;
 	ndctl_cmd_smart_inject_ctrl_temperature;
 } LIBNDCTL_16;
+
+LIBNDCTL_18 {
+global:
+	ndctl_namespace_get_first_badblock;
+	ndctl_namespace_get_next_badblock;
+} LIBNDCTL_17;
diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h
index 83d6c6c..500c7de 100644
--- a/ndctl/libndctl.h
+++ b/ndctl/libndctl.h
@@ -439,6 +439,12 @@ struct ndctl_namespace *ndctl_namespace_get_next(struct ndctl_namespace *ndns);
 	     ndns != NULL; \
 	     ndns = _ndns, \
 	     _ndns = _ndns ? ndctl_namespace_get_next(_ndns) : NULL)
+struct badblock *ndctl_namespace_get_first_badblock(struct ndctl_namespace *ndns);
+struct badblock *ndctl_namespace_get_next_badblock(struct ndctl_namespace *ndns);
+#define ndctl_namespace_badblock_foreach(ndns, badblock) \
+        for (badblock = ndctl_namespace_get_first_badblock(ndns); \
+             badblock != NULL; \
+             badblock = ndctl_namespace_get_next_badblock(ndns))
 struct ndctl_ctx *ndctl_namespace_get_ctx(struct ndctl_namespace *ndns);
 struct ndctl_bus *ndctl_namespace_get_bus(struct ndctl_namespace *ndns);
 struct ndctl_region *ndctl_namespace_get_region(struct ndctl_namespace *ndns);
-- 
2.17.1

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

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

* [ndctl PATCH 3/5] util/json: fix an error check for region resource
  2018-10-02  3:31 [ndctl PATCH 1/5] ndctl, lib: refactor badblocks retrieval routines Vishal Verma
  2018-10-02  3:31 ` [ndctl PATCH 2/5] ndctl, lib: add APIs for retrieving namespace badblocks Vishal Verma
@ 2018-10-02  3:31 ` Vishal Verma
  2018-10-03  1:47   ` Dan Williams
  2018-10-02  3:31 ` [ndctl PATCH 4/5] util/json: consolidate check for the UTIL_JSON_MEDIA_ERRORS flag Vishal Verma
  2018-10-02  3:31 ` [ndctl PATCH 5/5] util/json: add a util_namespace_badblocks_to_json() helper Vishal Verma
  3 siblings, 1 reply; 8+ messages in thread
From: Vishal Verma @ 2018-10-02  3:31 UTC (permalink / raw)
  To: linux-nvdimm

The return type of ndctl_region_get_resource() is 'unsigned long long',
and therefore the error checking for it should be done against
ULLONG_MAX. Fix an instance where we were checking against ULONG_MAX.

Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 util/json.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/json.c b/util/json.c
index 2d15859..30b56c3 100644
--- a/util/json.c
+++ b/util/json.c
@@ -470,7 +470,7 @@ struct json_object *util_region_badblocks_to_json(struct ndctl_region *region,
 
 		/* get start address of region */
 		addr = ndctl_region_get_resource(region);
-		if (addr == ULONG_MAX)
+		if (addr == ULLONG_MAX)
 			goto err_array;
 
 		/* get address of bad block */
-- 
2.17.1

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

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

* [ndctl PATCH 4/5] util/json: consolidate check for the UTIL_JSON_MEDIA_ERRORS flag
  2018-10-02  3:31 [ndctl PATCH 1/5] ndctl, lib: refactor badblocks retrieval routines Vishal Verma
  2018-10-02  3:31 ` [ndctl PATCH 2/5] ndctl, lib: add APIs for retrieving namespace badblocks Vishal Verma
  2018-10-02  3:31 ` [ndctl PATCH 3/5] util/json: fix an error check for region resource Vishal Verma
@ 2018-10-02  3:31 ` Vishal Verma
  2018-10-03  1:52   ` Dan Williams
  2018-10-02  3:31 ` [ndctl PATCH 5/5] util/json: add a util_namespace_badblocks_to_json() helper Vishal Verma
  3 siblings, 1 reply; 8+ messages in thread
From: Vishal Verma @ 2018-10-02  3:31 UTC (permalink / raw)
  To: linux-nvdimm

We don't need to check for the above flag for each badblock we're
iterating over. Remove the check in the respective loops, and return
early if it is not set.

Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 util/json.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/util/json.c b/util/json.c
index 30b56c3..4dd5fc3 100644
--- a/util/json.c
+++ b/util/json.c
@@ -457,7 +457,8 @@ struct json_object *util_region_badblocks_to_json(struct ndctl_region *region,
 		jbbs = json_object_new_array();
 		if (!jbbs)
 			return NULL;
-	}
+	} else
+		return NULL;
 
 	ndctl_region_badblock_foreach(region, bb) {
 		struct json_object *jdimms;
@@ -465,9 +466,6 @@ struct json_object *util_region_badblocks_to_json(struct ndctl_region *region,
 
 		bbs += bb->len;
 
-		if (!(flags & UTIL_JSON_MEDIA_ERRORS))
-			continue;
-
 		/* get start address of region */
 		addr = ndctl_region_get_resource(region);
 		if (addr == ULLONG_MAX)
@@ -527,7 +525,8 @@ static struct json_object *dev_badblocks_to_json(struct ndctl_region *region,
 		jbbs = json_object_new_array();
 		if (!jbbs)
 			return NULL;
-	}
+	} else
+		return NULL;
 
 	ndctl_region_badblock_foreach(region, bb) {
 		unsigned long long bb_begin, bb_end, begin, end;
@@ -554,9 +553,6 @@ static struct json_object *dev_badblocks_to_json(struct ndctl_region *region,
 
 		bbs += len;
 
-		if (!(flags & UTIL_JSON_MEDIA_ERRORS))
-			continue;
-
 		jbb = json_object_new_object();
 		if (!jbb)
 			goto err_array;
-- 
2.17.1

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

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

* [ndctl PATCH 5/5] util/json: add a util_namespace_badblocks_to_json() helper
  2018-10-02  3:31 [ndctl PATCH 1/5] ndctl, lib: refactor badblocks retrieval routines Vishal Verma
                   ` (2 preceding siblings ...)
  2018-10-02  3:31 ` [ndctl PATCH 4/5] util/json: consolidate check for the UTIL_JSON_MEDIA_ERRORS flag Vishal Verma
@ 2018-10-02  3:31 ` Vishal Verma
  2018-10-03  1:58   ` Dan Williams
  3 siblings, 1 reply; 8+ messages in thread
From: Vishal Verma @ 2018-10-02  3:31 UTC (permalink / raw)
  To: linux-nvdimm

When ndctl list --media-errors is invoked, we rely upon the 'region'
badblocks provided by the kernel, and reference them against the
namespace start to report namespace badblocks. This can fail for
non-root users, as the region and namespace resource information is
root-only. However the kernel also provides block-device badblocks
information in sysfs, and that is world readable. Provide a new helper
to retrieve this information, and use it as a fallback when the region
based badblocks are unavailable.

Note that when falling back to the namespace based badblocks, we are
unable to retrieve the DIMM on which the badblocks are present, since we
still need root privileges for both getting the resource information,
and for the address translation command.

Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 util/json.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 56 insertions(+), 3 deletions(-)

diff --git a/util/json.c b/util/json.c
index 4dd5fc3..fbc4b83 100644
--- a/util/json.c
+++ b/util/json.c
@@ -506,6 +506,52 @@ struct json_object *util_region_badblocks_to_json(struct ndctl_region *region,
 	return NULL;
 }
 
+static struct json_object *util_namespace_badblocks_to_json(
+			struct ndctl_namespace *ndns,
+			unsigned int *bb_count, unsigned long flags)
+{
+	struct json_object *jbb = NULL, *jbbs = NULL, *jobj;
+	struct badblock *bb;
+	int bbs = 0;
+
+	if (flags & UTIL_JSON_MEDIA_ERRORS) {
+		jbbs = json_object_new_array();
+		if (!jbbs)
+			return NULL;
+	} else
+		return NULL;
+
+	ndctl_namespace_badblock_foreach(ndns, bb) {
+		bbs += bb->len;
+
+		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);
+	}
+
+	*bb_count = 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,
 		unsigned int *bb_count, unsigned long flags)
@@ -593,8 +639,11 @@ static struct json_object *util_pfn_badblocks_to_json(struct ndctl_pfn *pfn,
 	unsigned long long pfn_begin, pfn_size;
 
 	pfn_begin = ndctl_pfn_get_resource(pfn);
-	if (pfn_begin == ULLONG_MAX)
-		return NULL;
+	if (pfn_begin == ULLONG_MAX) {
+		struct ndctl_namespace *ndns = ndctl_pfn_get_namespace(pfn);
+
+		return util_namespace_badblocks_to_json(ndns, bb_count, flags);
+	}
 
 	pfn_size = ndctl_pfn_get_size(pfn);
 	if (pfn_size == ULLONG_MAX)
@@ -873,10 +922,14 @@ struct json_object *util_namespace_to_json(struct ndctl_namespace *ndns,
 		jbbs = util_dax_badblocks_to_json(dax, &bb_count, flags);
 	else if (btt)
 		util_btt_badblocks_to_json(btt, &bb_count);
-	else
+	else {
 		jbbs = util_region_badblocks_to_json(
 				ndctl_namespace_get_region(ndns), &bb_count,
 				flags);
+		if (!jbbs)
+			jbbs = util_namespace_badblocks_to_json(ndns, &bb_count,
+					flags);
+	}
 
 	if (bb_count) {
 		jobj = json_object_new_int(bb_count);
-- 
2.17.1

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

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

* Re: [ndctl PATCH 3/5] util/json: fix an error check for region resource
  2018-10-02  3:31 ` [ndctl PATCH 3/5] util/json: fix an error check for region resource Vishal Verma
@ 2018-10-03  1:47   ` Dan Williams
  0 siblings, 0 replies; 8+ messages in thread
From: Dan Williams @ 2018-10-03  1:47 UTC (permalink / raw)
  To: Vishal L Verma; +Cc: linux-nvdimm

On Mon, Oct 1, 2018 at 8:38 PM Vishal Verma <vishal.l.verma@intel.com> wrote:
>
> The return type of ndctl_region_get_resource() is 'unsigned long long',
> and therefore the error checking for it should be done against
> ULLONG_MAX. Fix an instance where we were checking against ULONG_MAX.
>

Reviewed-by: Dan Williams <dan.j.williams@intel.com>
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

* Re: [ndctl PATCH 4/5] util/json: consolidate check for the UTIL_JSON_MEDIA_ERRORS flag
  2018-10-02  3:31 ` [ndctl PATCH 4/5] util/json: consolidate check for the UTIL_JSON_MEDIA_ERRORS flag Vishal Verma
@ 2018-10-03  1:52   ` Dan Williams
  0 siblings, 0 replies; 8+ messages in thread
From: Dan Williams @ 2018-10-03  1:52 UTC (permalink / raw)
  To: Vishal L Verma; +Cc: linux-nvdimm

On Mon, Oct 1, 2018 at 8:38 PM Vishal Verma <vishal.l.verma@intel.com> wrote:
>
> We don't need to check for the above flag for each badblock we're
> iterating over. Remove the check in the respective loops, and return
> early if it is not set.
>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  util/json.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/util/json.c b/util/json.c
> index 30b56c3..4dd5fc3 100644
> --- a/util/json.c
> +++ b/util/json.c
> @@ -457,7 +457,8 @@ struct json_object *util_region_badblocks_to_json(struct ndctl_region *region,
>                 jbbs = json_object_new_array();
>                 if (!jbbs)
>                         return NULL;
> -       }
> +       } else
> +               return NULL;
>
>         ndctl_region_badblock_foreach(region, bb) {
>                 struct json_object *jdimms;
> @@ -465,9 +466,6 @@ struct json_object *util_region_badblocks_to_json(struct ndctl_region *region,
>
>                 bbs += bb->len;
>
> -               if (!(flags & UTIL_JSON_MEDIA_ERRORS))
> -                       continue;
> -

The reason for this awkwardness is to account for the total badblocks
count even when the individual badblocks are not emitted. See that
"bbs += bb->len" that will now be skipped. Perhaps at least a comment
is needed because that trickery is not obvious.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

* Re: [ndctl PATCH 5/5] util/json: add a util_namespace_badblocks_to_json() helper
  2018-10-02  3:31 ` [ndctl PATCH 5/5] util/json: add a util_namespace_badblocks_to_json() helper Vishal Verma
@ 2018-10-03  1:58   ` Dan Williams
  0 siblings, 0 replies; 8+ messages in thread
From: Dan Williams @ 2018-10-03  1:58 UTC (permalink / raw)
  To: Vishal L Verma; +Cc: linux-nvdimm

On Mon, Oct 1, 2018 at 8:38 PM Vishal Verma <vishal.l.verma@intel.com> wrote:
>
> When ndctl list --media-errors is invoked, we rely upon the 'region'
> badblocks provided by the kernel, and reference them against the
> namespace start to report namespace badblocks. This can fail for
> non-root users, as the region and namespace resource information is
> root-only. However the kernel also provides block-device badblocks
> information in sysfs, and that is world readable. Provide a new helper
> to retrieve this information, and use it as a fallback when the region
> based badblocks are unavailable.
>
> Note that when falling back to the namespace based badblocks, we are
> unable to retrieve the DIMM on which the badblocks are present, since we
> still need root privileges for both getting the resource information,
> and for the address translation command.
>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>

Reviewed-by: Dan Williams <dan.j.williams@intel.com>
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

end of thread, other threads:[~2018-10-03  1:58 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-02  3:31 [ndctl PATCH 1/5] ndctl, lib: refactor badblocks retrieval routines Vishal Verma
2018-10-02  3:31 ` [ndctl PATCH 2/5] ndctl, lib: add APIs for retrieving namespace badblocks Vishal Verma
2018-10-02  3:31 ` [ndctl PATCH 3/5] util/json: fix an error check for region resource Vishal Verma
2018-10-03  1:47   ` Dan Williams
2018-10-02  3:31 ` [ndctl PATCH 4/5] util/json: consolidate check for the UTIL_JSON_MEDIA_ERRORS flag Vishal Verma
2018-10-03  1:52   ` Dan Williams
2018-10-02  3:31 ` [ndctl PATCH 5/5] util/json: add a util_namespace_badblocks_to_json() helper Vishal Verma
2018-10-03  1:58   ` Dan Williams

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).