All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 1/6] libndctl: Use the supported_alignment attribute
@ 2019-01-16  9:49 Oliver O'Halloran
  2019-01-16  9:49 ` [PATCH v3 2/6] ndctl/namespace: Check for seed namespaces earlier Oliver O'Halloran
                   ` (5 more replies)
  0 siblings, 6 replies; 14+ messages in thread
From: Oliver O'Halloran @ 2019-01-16  9:49 UTC (permalink / raw)
  To: linux-nvdimm

Newer kernels provide the "supported_alignments" sysfs attribute that
indicates what alignments can be used with a PFN or DAX namespace. This
patch adds the plumbing inside of libndctl to allow users to query this
information through using:
	ndctl_{dax|pfn}_get_supported_alignment(), and
	ndctl_{dax|pfn}_get_num_alignments()

Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
v3: Changed the return type of the *_get_supported_alignment() functions
    to unsigned long to match the existing *_get_alignment() functions.
---
 ndctl/lib/libndctl.c   | 40 ++++++++++++++++++++++++++++++++++++++++
 ndctl/lib/libndctl.sym |  7 +++++++
 ndctl/libndctl.h       |  6 ++++++
 3 files changed, 53 insertions(+)

diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index 0c3a35e5bcc9..53369b5c9886 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -31,6 +31,7 @@
 #include <ccan/build_assert/build_assert.h>
 
 #include <ndctl.h>
+#include <util/size.h>
 #include <util/sysfs.h>
 #include <ndctl/libndctl.h>
 #include <ndctl/namespace.h>
@@ -237,6 +238,7 @@ struct ndctl_pfn {
 	int buf_len;
 	uuid_t uuid;
 	int id, generation;
+	struct ndctl_lbasize alignments;
 };
 
 struct ndctl_dax {
@@ -4781,6 +4783,18 @@ static void *__add_pfn(struct ndctl_pfn *pfn, const char *pfn_base)
 	else
 		pfn->size = strtoull(buf, NULL, 0);
 
+	/*
+	 * If the kernel doesn't provide the supported_alignments sysfs
+	 * attribute then it's safe to assume that we are running on x86
+	 * which will always support 2MB and 4KB alignments.
+	 */
+	sprintf(path, "%s/supported_alignments", pfn_base);
+	if (sysfs_read_attr(ctx, path, buf) < 0)
+		sprintf(buf, "%d %d", SZ_4K, SZ_2M);
+
+	if (parse_lbasize_supported(ctx, pfn_base, buf, &pfn->alignments) < 0)
+		goto err_read;
+
 	free(path);
 	return pfn;
 
@@ -5015,6 +5029,22 @@ NDCTL_EXPORT int ndctl_pfn_set_align(struct ndctl_pfn *pfn, unsigned long align)
 	return 0;
 }
 
+NDCTL_EXPORT int ndctl_pfn_get_num_alignments(struct ndctl_pfn *pfn)
+{
+	return pfn->alignments.num;
+}
+
+NDCTL_EXPORT unsigned long ndctl_pfn_get_supported_alignment(struct ndctl_pfn *pfn, int i)
+{
+	if (pfn->alignments.num == 0)
+		return 0;
+
+	if (i < 0 || i > pfn->alignments.num)
+		return -1;
+	else
+		return pfn->alignments.supported[i];
+}
+
 NDCTL_EXPORT int ndctl_pfn_set_namespace(struct ndctl_pfn *pfn,
 		struct ndctl_namespace *ndns)
 {
@@ -5237,6 +5267,16 @@ NDCTL_EXPORT unsigned long ndctl_dax_get_align(struct ndctl_dax *dax)
 	return ndctl_pfn_get_align(&dax->pfn);
 }
 
+NDCTL_EXPORT int ndctl_dax_get_num_alignments(struct ndctl_dax *dax)
+{
+	return ndctl_pfn_get_num_alignments(&dax->pfn);
+}
+
+NDCTL_EXPORT unsigned long ndctl_dax_get_supported_alignment(struct ndctl_dax *dax, int i)
+{
+	return ndctl_pfn_get_supported_alignment(&dax->pfn, i);
+}
+
 NDCTL_EXPORT int ndctl_dax_has_align(struct ndctl_dax *dax)
 {
 	return ndctl_pfn_has_align(&dax->pfn);
diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
index 6c4c8b4dfb8e..0103c1b71a1d 100644
--- a/ndctl/lib/libndctl.sym
+++ b/ndctl/lib/libndctl.sym
@@ -385,3 +385,10 @@ global:
 	ndctl_namespace_get_next_badblock;
 	ndctl_dimm_get_dirty_shutdown;
 } LIBNDCTL_17;
+
+LIBNDCTL_19 {
+	ndctl_pfn_get_supported_alignment;
+	ndctl_pfn_get_num_alignments;
+	ndctl_dax_get_supported_alignment;
+	ndctl_dax_get_num_alignments;
+} LIBNDCTL_18;
diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h
index 62cef9e82da3..0f1f66256c1d 100644
--- a/ndctl/libndctl.h
+++ b/ndctl/libndctl.h
@@ -681,6 +681,12 @@ enum ND_FW_STATUS ndctl_cmd_fw_xlat_firmware_status(struct ndctl_cmd *cmd);
 struct ndctl_cmd *ndctl_dimm_cmd_new_ack_shutdown_count(struct ndctl_dimm *dimm);
 int ndctl_dimm_fw_update_supported(struct ndctl_dimm *dimm);
 
+int ndctl_pfn_get_num_alignments(struct ndctl_pfn *pfn);
+unsigned long ndctl_pfn_get_supported_alignment(struct ndctl_pfn *pfn, int i);
+
+int ndctl_dax_get_num_alignments(struct ndctl_dax *dax);
+unsigned long ndctl_dax_get_supported_alignment(struct ndctl_dax *dax, int i);
+
 #ifdef __cplusplus
 } /* extern "C" */
 #endif
-- 
2.20.1

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

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

* [PATCH v3 2/6] ndctl/namespace: Check for seed namespaces earlier
  2019-01-16  9:49 [PATCH v3 1/6] libndctl: Use the supported_alignment attribute Oliver O'Halloran
@ 2019-01-16  9:49 ` Oliver O'Halloran
  2019-01-16  9:49 ` [PATCH v3 3/6] ndctl/namespace: Use seed alignment as the default Oliver O'Halloran
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Oliver O'Halloran @ 2019-01-16  9:49 UTC (permalink / raw)
  To: linux-nvdimm

When creating an fsdax or devdax namespace we need to verify that the
seed namespaces exist. This patch reworks the validation so that it's
done earlier to simplify the subsequent patches in the series.

Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
No functional changes, probably.
---
 ndctl/namespace.c | 48 +++++++++++++++++++++++------------------------
 1 file changed, 23 insertions(+), 25 deletions(-)

diff --git a/ndctl/namespace.c b/ndctl/namespace.c
index b6f12306fe76..efa8250456d9 100644
--- a/ndctl/namespace.c
+++ b/ndctl/namespace.c
@@ -465,6 +465,8 @@ static int validate_namespace_options(struct ndctl_region *region,
 {
 	const char *region_name = ndctl_region_get_devname(region);
 	unsigned long long size_align = SZ_4K, units = 1, resource;
+	struct ndctl_pfn *pfn = NULL;
+	struct ndctl_dax *dax = NULL;
 	unsigned int ways;
 	int rc = 0;
 
@@ -521,10 +523,24 @@ static int validate_namespace_options(struct ndctl_region *region,
 	} else if (ndns)
 		p->mode = ndctl_namespace_get_mode(ndns);
 
-	if (param.align) {
-		struct ndctl_pfn *pfn = ndctl_region_get_pfn_seed(region);
-		struct ndctl_dax *dax = ndctl_region_get_dax_seed(region);
+	if (p->mode == NDCTL_NS_MODE_MEMORY) {
+		pfn = ndctl_region_get_pfn_seed(region);
+		if (!pfn && param.mode_default) {
+			debug("%s fsdax mode not available\n", region_name);
+			p->mode = NDCTL_NS_MODE_RAW;
+		}
+		/*
+		 * NB: We only fail validation if a pfn-specific option is used
+		 */
+	} else if (p->mode == NDCTL_NS_MODE_DAX) {
+		dax = ndctl_region_get_dax_seed(region);
+		if (!dax) {
+			error("Kernel does not support devdax mode\n");
+			return -ENODEV;
+		}
+	}
 
+	if (param.align) {
 		p->align = parse_size64(param.align);
 
 		if (p->mode == NDCTL_NS_MODE_MEMORY && p->align != SZ_2M
@@ -705,30 +721,12 @@ static int validate_namespace_options(struct ndctl_region *region,
 			|| p->mode == NDCTL_NS_MODE_DAX)
 		p->loc = NDCTL_PFN_LOC_PMEM;
 
-	/* check if we need, and whether the kernel supports, pfn devices */
-	if (do_setup_pfn(ndns, p)) {
-		struct ndctl_pfn *pfn = ndctl_region_get_pfn_seed(region);
-
-		if (!pfn && param.mode_default) {
-			debug("%s fsdax mode not available\n", region_name);
-			p->mode = NDCTL_NS_MODE_RAW;
-		} else if (!pfn) {
-			error("operation failed, %s fsdax mode not available\n",
-					region_name);
-			return -EINVAL;
-		}
+	if (!pfn && do_setup_pfn(ndns, p)) {
+		error("operation failed, %s cannot support requested mode\n",
+			region_name);
+		return -EINVAL;
 	}
 
-	/* check if we need, and whether the kernel supports, dax devices */
-	if (p->mode == NDCTL_NS_MODE_DAX) {
-		struct ndctl_dax *dax = ndctl_region_get_dax_seed(region);
-
-		if (!dax) {
-			error("operation failed, %s devdax mode not available\n",
-					region_name);
-			return -EINVAL;
-		}
-	}
 
 	p->autolabel = param.autolabel;
 
-- 
2.20.1

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

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

* [PATCH v3 3/6] ndctl/namespace: Use seed alignment as the default
  2019-01-16  9:49 [PATCH v3 1/6] libndctl: Use the supported_alignment attribute Oliver O'Halloran
  2019-01-16  9:49 ` [PATCH v3 2/6] ndctl/namespace: Check for seed namespaces earlier Oliver O'Halloran
@ 2019-01-16  9:49 ` Oliver O'Halloran
  2019-01-16  9:49 ` [PATCH v3 4/6] ndctl/namespace: Validate alignment from the {pfn|dax} seed Oliver O'Halloran
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Oliver O'Halloran @ 2019-01-16  9:49 UTC (permalink / raw)
  To: linux-nvdimm

When creating a pfn or dax namespace ndctl uses a default alignment of 2MB
when the user does not explicitly supply one. This works on most systems
(x86, ARM, PPC64 with radix MMU), but it fails when the kernel does not
support a 2MB page size (PPC64 with hash MMU).

This patch makes ndctl use the alignment of the relevant seed namespace as
the default instead. The kernel will always pick a valid default alignment
so this should be a bit more portable.

Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
 ndctl/namespace.c | 96 +++++++++++++++++++++--------------------------
 1 file changed, 43 insertions(+), 53 deletions(-)

diff --git a/ndctl/namespace.c b/ndctl/namespace.c
index efa8250456d9..b27cc6967a68 100644
--- a/ndctl/namespace.c
+++ b/ndctl/namespace.c
@@ -39,7 +39,6 @@ static bool logfix;
 static struct parameters {
 	bool do_scan;
 	bool mode_default;
-	bool align_default;
 	bool autolabel;
 	const char *bus;
 	const char *map;
@@ -226,9 +225,6 @@ static int set_defaults(enum device_action mode)
 		error("failed to parse namespace alignment '%s'\n",
 				param.align);
 		rc = -EINVAL;
-	} else if (!param.align) {
-		param.align = "2M";
-		param.align_default = true;
 	}
 
 	if (param.uuid) {
@@ -464,7 +460,7 @@ static int validate_namespace_options(struct ndctl_region *region,
 		struct ndctl_namespace *ndns, struct parsed_parameters *p)
 {
 	const char *region_name = ndctl_region_get_devname(region);
-	unsigned long long size_align = SZ_4K, units = 1, resource;
+	unsigned long long size_align, units = 1, resource;
 	struct ndctl_pfn *pfn = NULL;
 	struct ndctl_dax *dax = NULL;
 	unsigned int ways;
@@ -541,53 +537,15 @@ static int validate_namespace_options(struct ndctl_region *region,
 	}
 
 	if (param.align) {
-		p->align = parse_size64(param.align);
-
-		if (p->mode == NDCTL_NS_MODE_MEMORY && p->align != SZ_2M
-				&& (!pfn || !ndctl_pfn_has_align(pfn))) {
-			/*
-			 * Initial pfn device support in the kernel
-			 * supported a 2M default alignment when
-			 * ndctl_pfn_has_align() returns false.
-			 */
-			debug("%s not support 'align' for fsdax mode\n",
-					region_name);
-			return -EAGAIN;
-		} else if (p->mode == NDCTL_NS_MODE_DAX
-				&& (!dax || !ndctl_dax_has_align(dax))) {
-			/*
-			 * Unlike the pfn case, we require the kernel to
-			 * have 'align' support for device-dax.
-			 */
-			debug("%s not support 'align' for devdax mode\n",
-					region_name);
-			return -EAGAIN;
-		} else if (!param.align_default
-				&& (p->mode == NDCTL_NS_MODE_SAFE
-					|| p->mode == NDCTL_NS_MODE_RAW)) {
-			/*
-			 * Specifying an alignment has no effect for
-			 * raw, or btt mode namespaces.
-			 */
+		if (p->mode != NDCTL_NS_MODE_MEMORY &&
+		    p->mode != NDCTL_NS_MODE_DAX) {
 			error("%s mode does not support setting an alignment\n",
 					p->mode == NDCTL_NS_MODE_SAFE
 					? "sector" : "raw");
 			return -ENXIO;
 		}
 
-		/*
-		 * Fallback to a 4K default alignment if the region is
-		 * not 2MB (typical default) aligned. This mainly helps
-		 * the nfit_test use case where it is backed by vmalloc
-		 * memory.
-		 */
-		resource = ndctl_region_get_resource(region);
-		if (param.align_default && resource < ULLONG_MAX
-				&& (resource & (SZ_2M - 1))) {
-			debug("%s: falling back to a 4K alignment\n",
-					region_name);
-			p->align = SZ_4K;
-		}
+		p->align = parse_size64(param.align);
 
 		switch (p->align) {
 		case SZ_4K:
@@ -598,16 +556,48 @@ static int validate_namespace_options(struct ndctl_region *region,
 			error("unsupported align: %s\n", param.align);
 			return -ENXIO;
 		}
+	} else {
+		/*
+		 * Use the seed namespace alignment as the default if we need
+		 * one. If we don't then use PAGE_SIZE so the size_align
+		 * checking works.
+		 */
+		if (p->mode == NDCTL_NS_MODE_MEMORY) {
+			/*
+			 * The initial pfn device support in the kernel didn't
+			 * have the 'align' sysfs attribute and assumed a 2MB
+			 * alignment. Fall back to that if we don't have the
+			 * attribute.
+			 */
+			if (pfn && ndctl_pfn_has_align(pfn))
+				p->align = ndctl_pfn_get_align(pfn);
+			else
+				p->align = SZ_2M;
+		} else if (p->mode == NDCTL_NS_MODE_DAX) {
+			/*
+			 * device dax mode was added after the align attribute
+			 * so checking for it is unnecessary.
+			 */
+			p->align = ndctl_dax_get_align(dax);
+		} else {
+			p->align = sysconf(_SC_PAGE_SIZE);
+		}
 
 		/*
-		 * 'raw' and 'sector' mode namespaces don't support an
-		 * alignment attribute.
+		 * Fallback to a page alignment if the region is not aligned
+		 * to the default. This is mainly useful for the nfit_test
+		 * use case where it is backed by vmalloc memory.
 		 */
-		if (p->mode == NDCTL_NS_MODE_MEMORY
-				|| p->mode == NDCTL_NS_MODE_DAX)
-			size_align = p->align;
+		resource = ndctl_region_get_resource(region);
+		if (resource < ULLONG_MAX && (resource & (p->align - 1))) {
+			debug("%s: falling back to a page alignment\n",
+					region_name);
+			p->align = sysconf(_SC_PAGE_SIZE);
+		}
 	}
 
+	size_align = p->align;
+
 	/* (re-)validate that the size satisfies the alignment */
 	ways = ndctl_region_get_interleave_ways(region);
 	if (p->size % (size_align * ways)) {
@@ -633,8 +623,8 @@ static int validate_namespace_options(struct ndctl_region *region,
 		p->size *= size_align;
 		p->size /= units;
 		error("'--size=' must align to interleave-width: %d and alignment: %ld\n"
-				"  did you intend --size=%lld%s?\n", ways, param.align
-				? p->align : SZ_4K, p->size, suffix);
+				"  did you intend --size=%lld%s?\n", ways, p->align,
+				p->size, suffix);
 		return -EINVAL;
 	}
 
-- 
2.20.1

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

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

* [PATCH v3 4/6] ndctl/namespace: Validate alignment from the {pfn|dax} seed
  2019-01-16  9:49 [PATCH v3 1/6] libndctl: Use the supported_alignment attribute Oliver O'Halloran
  2019-01-16  9:49 ` [PATCH v3 2/6] ndctl/namespace: Check for seed namespaces earlier Oliver O'Halloran
  2019-01-16  9:49 ` [PATCH v3 3/6] ndctl/namespace: Use seed alignment as the default Oliver O'Halloran
@ 2019-01-16  9:49 ` Oliver O'Halloran
  2019-01-16  9:49 ` [PATCH v3 5/6] ndctl: Add alignment to the namespace JSON output Oliver O'Halloran
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Oliver O'Halloran @ 2019-01-16  9:49 UTC (permalink / raw)
  To: linux-nvdimm

This patch adds support to the ndctl tool for validating that the
namespace alignment is valid.

Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
 ndctl/namespace.c | 37 +++++++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/ndctl/namespace.c b/ndctl/namespace.c
index b27cc6967a68..44fc91232ecb 100644
--- a/ndctl/namespace.c
+++ b/ndctl/namespace.c
@@ -537,8 +537,23 @@ static int validate_namespace_options(struct ndctl_region *region,
 	}
 
 	if (param.align) {
-		if (p->mode != NDCTL_NS_MODE_MEMORY &&
-		    p->mode != NDCTL_NS_MODE_DAX) {
+		int i, alignments;
+
+		switch (p->mode) {
+		case NDCTL_NS_MODE_MEMORY:
+			if (!pfn) {
+				error("Kernel does not support setting an alignment in fsdax mode\n");
+				return -EINVAL;
+			}
+
+			alignments = ndctl_pfn_get_num_alignments(pfn);
+			break;
+
+		case NDCTL_NS_MODE_DAX:
+			alignments = ndctl_dax_get_num_alignments(dax);
+			break;
+
+		default:
 			error("%s mode does not support setting an alignment\n",
 					p->mode == NDCTL_NS_MODE_SAFE
 					? "sector" : "raw");
@@ -546,13 +561,19 @@ static int validate_namespace_options(struct ndctl_region *region,
 		}
 
 		p->align = parse_size64(param.align);
+		for (i = 0; i < alignments; i++) {
+			uint64_t a;
 
-		switch (p->align) {
-		case SZ_4K:
-		case SZ_2M:
-		case SZ_1G:
-			break;
-		default:
+			if (p->mode == NDCTL_NS_MODE_MEMORY)
+				a = ndctl_pfn_get_supported_alignment(pfn, i);
+			else
+				a = ndctl_dax_get_supported_alignment(dax, i);
+
+			if (p->align == a)
+				break;
+		}
+
+		if (i >= alignments) {
 			error("unsupported align: %s\n", param.align);
 			return -ENXIO;
 		}
-- 
2.20.1

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

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

* [PATCH v3 5/6] ndctl: Add alignment to the namespace JSON output
  2019-01-16  9:49 [PATCH v3 1/6] libndctl: Use the supported_alignment attribute Oliver O'Halloran
                   ` (2 preceding siblings ...)
  2019-01-16  9:49 ` [PATCH v3 4/6] ndctl/namespace: Validate alignment from the {pfn|dax} seed Oliver O'Halloran
@ 2019-01-16  9:49 ` Oliver O'Halloran
  2019-01-16  9:49 ` [PATCH v3 6/6] ndctl: Add supported_alignments to the " Oliver O'Halloran
  2019-01-23 19:32 ` [PATCH v3 1/6] libndctl: Use the supported_alignment attribute Verma, Vishal L
  5 siblings, 0 replies; 14+ messages in thread
From: Oliver O'Halloran @ 2019-01-16  9:49 UTC (permalink / raw)
  To: linux-nvdimm

Automated tooling probably wants to know this and it's helpful output
for command line users for ndctl.

Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
 util/json.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/util/json.c b/util/json.c
index 5c3424e2a707..77c96fb53c27 100644
--- a/util/json.c
+++ b/util/json.c
@@ -753,6 +753,7 @@ struct json_object *util_namespace_to_json(struct ndctl_namespace *ndns,
 	struct ndctl_btt *btt;
 	struct ndctl_pfn *pfn;
 	struct ndctl_dax *dax;
+	unsigned long align = 0;
 	char buf[40];
 	uuid_t uuid;
 	int numa;
@@ -825,6 +826,7 @@ struct json_object *util_namespace_to_json(struct ndctl_namespace *ndns,
 		util_raw_uuid_to_json(ndns, flags, jndns);
 		bdev = ndctl_btt_get_block_device(btt);
 	} else if (pfn) {
+		align = ndctl_pfn_get_align(pfn);
 		ndctl_pfn_get_uuid(pfn, uuid);
 		uuid_unparse(uuid, buf);
 		jobj = json_object_new_string(buf);
@@ -837,6 +839,7 @@ struct json_object *util_namespace_to_json(struct ndctl_namespace *ndns,
 		struct daxctl_region *dax_region;
 
 		dax_region = ndctl_dax_get_daxctl_region(dax);
+		align = ndctl_dax_get_align(dax);
 		ndctl_dax_get_uuid(dax, uuid);
 		uuid_unparse(uuid, buf);
 		jobj = json_object_new_string(buf);
@@ -897,6 +900,13 @@ struct json_object *util_namespace_to_json(struct ndctl_namespace *ndns,
 		json_object_object_add(jndns, "sector_size", jobj);
 	}
 
+	if (align) {
+		jobj = json_object_new_int64(align);
+		if (!jobj)
+			goto err;
+		json_object_object_add(jndns, "align", jobj);
+	}
+
 	if (bdev && bdev[0]) {
 		jobj = json_object_new_string(bdev);
 		if (!jobj)
-- 
2.20.1

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

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

* [PATCH v3 6/6] ndctl: Add supported_alignments to the JSON output
  2019-01-16  9:49 [PATCH v3 1/6] libndctl: Use the supported_alignment attribute Oliver O'Halloran
                   ` (3 preceding siblings ...)
  2019-01-16  9:49 ` [PATCH v3 5/6] ndctl: Add alignment to the namespace JSON output Oliver O'Halloran
@ 2019-01-16  9:49 ` Oliver O'Halloran
  2019-01-29 14:40   ` Oliver
  2019-01-23 19:32 ` [PATCH v3 1/6] libndctl: Use the supported_alignment attribute Verma, Vishal L
  5 siblings, 1 reply; 14+ messages in thread
From: Oliver O'Halloran @ 2019-01-16  9:49 UTC (permalink / raw)
  To: linux-nvdimm

Add the list of supported alignemnts to PFN and DAX namespaces. Also add the
list of supported sector sizes to BTT namespaces.

Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
Not sure the namespace JSON blob are the best place to put these. The
region might be better, but slightly less accessable to users.
---
 util/json.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/util/json.c b/util/json.c
index 77c96fb53c27..6c66033bd312 100644
--- a/util/json.c
+++ b/util/json.c
@@ -303,6 +303,38 @@ struct json_object *util_daxctl_devs_to_list(struct daxctl_region *region,
 	return jdevs;
 }
 
+#define _SZ(get_max, get_elem, type) \
+static struct json_object *type##_build_size_array(struct type *arg)	\
+{								\
+	struct json_object *arr = json_object_new_array();	\
+	int i;							\
+								\
+	if (!arr)						\
+		return NULL;					\
+								\
+	for (i = 0; i < get_max(arg); i++) {			\
+		struct json_object *jobj;			\
+		int64_t align;					\
+								\
+		align = get_elem(arg, i);			\
+		jobj = json_object_new_int64(align);		\
+		if (!jobj)					\
+			goto err;				\
+		json_object_array_add(arr, jobj);		\
+	}							\
+								\
+	return arr;						\
+err:								\
+	json_object_put(arr);					\
+	return NULL;						\
+}
+#define SZ(type, kind) _SZ(ndctl_##type##_get_num_##kind##s, \
+			   ndctl_##type##_get_supported_##kind, ndctl_##type)
+SZ(pfn, alignment)
+SZ(dax, alignment)
+SZ(btt, sector_size)
+//SZ(namespace, sector_size)
+
 struct json_object *util_daxctl_region_to_json(struct daxctl_region *region,
 		const char *ident, unsigned long flags)
 {
@@ -739,7 +771,7 @@ struct json_object *util_namespace_to_json(struct ndctl_namespace *ndns,
 {
 	struct json_object *jndns = json_object_new_object();
 	enum ndctl_pfn_loc loc = NDCTL_PFN_LOC_NONE;
-	struct json_object *jobj, *jbbs = NULL;
+	struct json_object *jobj, *jbbs = NULL, *size_array = NULL;
 	const char *locations[] = {
 		[NDCTL_PFN_LOC_NONE] = "none",
 		[NDCTL_PFN_LOC_RAM] = "mem",
@@ -749,6 +781,7 @@ struct json_object *util_namespace_to_json(struct ndctl_namespace *ndns,
 	unsigned int sector_size = UINT_MAX;
 	enum ndctl_namespace_mode mode;
 	const char *bdev = NULL, *name;
+	const char *size_array_name;
 	unsigned int bb_count = 0;
 	struct ndctl_btt *btt;
 	struct ndctl_pfn *pfn;
@@ -936,6 +969,19 @@ struct json_object *util_namespace_to_json(struct ndctl_namespace *ndns,
 			json_object_object_add(jndns, "numa_node", jobj);
 	}
 
+	if (pfn) {
+		size_array_name = "supported_alignments";
+		size_array = ndctl_pfn_build_size_array(pfn);
+	} else if (dax) {
+		size_array_name = "supported_alignments";
+		size_array = ndctl_dax_build_size_array(dax);
+	} else if (btt) {
+		size_array_name = "supported sector sizes";
+		size_array = ndctl_btt_build_size_array(btt);
+	}
+	if (size_array)
+		json_object_object_add(jndns, size_array_name, size_array);
+
 	if (pfn)
 		jbbs = util_pfn_badblocks_to_json(pfn, &bb_count, flags);
 	else if (dax)
-- 
2.20.1

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

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

* Re: [PATCH v3 1/6] libndctl: Use the supported_alignment attribute
  2019-01-16  9:49 [PATCH v3 1/6] libndctl: Use the supported_alignment attribute Oliver O'Halloran
                   ` (4 preceding siblings ...)
  2019-01-16  9:49 ` [PATCH v3 6/6] ndctl: Add supported_alignments to the " Oliver O'Halloran
@ 2019-01-23 19:32 ` Verma, Vishal L
  2019-01-28 23:11   ` Oliver
  5 siblings, 1 reply; 14+ messages in thread
From: Verma, Vishal L @ 2019-01-23 19:32 UTC (permalink / raw)
  To: linux-nvdimm, oohall


On Wed, 2019-01-16 at 20:49 +1100, Oliver O'Halloran wrote:
> Newer kernels provide the "supported_alignments" sysfs attribute that
> indicates what alignments can be used with a PFN or DAX namespace. This
> patch adds the plumbing inside of libndctl to allow users to query this
> information through using:
> 	ndctl_{dax|pfn}_get_supported_alignment(), and
> 	ndctl_{dax|pfn}_get_num_alignments()
> 
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> ---
> v3: Changed the return type of the *_get_supported_alignment() functions
>     to unsigned long to match the existing *_get_alignment() functions.
> ---
>  ndctl/lib/libndctl.c   | 40 ++++++++++++++++++++++++++++++++++++++++
>  ndctl/lib/libndctl.sym |  7 +++++++
>  ndctl/libndctl.h       |  6 ++++++
>  3 files changed, 53 insertions(+)

Hi Oliver,

Thanks for resubmitting this series. I had a few comments below:

> 
> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> index 0c3a35e5bcc9..53369b5c9886 100644
> --- a/ndctl/lib/libndctl.c
> +++ b/ndctl/lib/libndctl.c
> @@ -31,6 +31,7 @@
>  #include <ccan/build_assert/build_assert.h>
>  
>  #include <ndctl.h>
> +#include <util/size.h>
>  #include <util/sysfs.h>
>  #include <ndctl/libndctl.h>
>  #include <ndctl/namespace.h>
> @@ -237,6 +238,7 @@ struct ndctl_pfn {
>  	int buf_len;
>  	uuid_t uuid;
>  	int id, generation;
> +	struct ndctl_lbasize alignments;
>  };
>  
>  struct ndctl_dax {
> @@ -4781,6 +4783,18 @@ static void *__add_pfn(struct ndctl_pfn *pfn, const char *pfn_base)
>  	else
>  		pfn->size = strtoull(buf, NULL, 0);
>  
> +	/*
> +	 * If the kernel doesn't provide the supported_alignments sysfs
> +	 * attribute then it's safe to assume that we are running on x86
> +	 * which will always support 2MB and 4KB alignments.
> +	 */
> +	sprintf(path, "%s/supported_alignments", pfn_base);
> +	if (sysfs_read_attr(ctx, path, buf) < 0)
> +		sprintf(buf, "%d %d", SZ_4K, SZ_2M);
> +
> +	if (parse_lbasize_supported(ctx, pfn_base, buf, &pfn->alignments) < 0)
> +		goto err_read;
> +
>  	free(path);
>  	return pfn;
>  
> @@ -5015,6 +5029,22 @@ NDCTL_EXPORT int ndctl_pfn_set_align(struct ndctl_pfn *pfn, unsigned long align)
>  	return 0;
>  }
>  
> +NDCTL_EXPORT int ndctl_pfn_get_num_alignments(struct ndctl_pfn *pfn)
> +{
> +	return pfn->alignments.num;
> +}
> +
> +NDCTL_EXPORT unsigned long ndctl_pfn_get_supported_alignment(struct ndctl_pfn *pfn, int i)

This and a few other spots exceed 80 char lines, could you fix that up
all over?

> +{
> +	if (pfn->alignments.num == 0)
> +		return 0;
> +
> +	if (i < 0 || i > pfn->alignments.num)
> +		return -1;

A bare '-1' return is unusual for exported functions. The convention in
libndctl is to return -ERRNO. In this case, -EINVAL seems the right
error to return?

> +	else
> +		return pfn->alignments.supported[i];
> +}
> +
>  NDCTL_EXPORT int ndctl_pfn_set_namespace(struct ndctl_pfn *pfn,
>  		struct ndctl_namespace *ndns)
>  {
> @@ -5237,6 +5267,16 @@ NDCTL_EXPORT unsigned long ndctl_dax_get_align(struct ndctl_dax *dax)
>  	return ndctl_pfn_get_align(&dax->pfn);
>  }
>  
> +NDCTL_EXPORT int ndctl_dax_get_num_alignments(struct ndctl_dax *dax)
> +{
> +	return ndctl_pfn_get_num_alignments(&dax->pfn);
> +}
> +
> +NDCTL_EXPORT unsigned long ndctl_dax_get_supported_alignment(struct ndctl_dax *dax, int i)

Another > 80 char line.

> +{
> +	return ndctl_pfn_get_supported_alignment(&dax->pfn, i);
> +}
> +
>  NDCTL_EXPORT int ndctl_dax_has_align(struct ndctl_dax *dax)
>  {
>  	return ndctl_pfn_has_align(&dax->pfn);
> diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
> index 6c4c8b4dfb8e..0103c1b71a1d 100644
> --- a/ndctl/lib/libndctl.sym
> +++ b/ndctl/lib/libndctl.sym
> @@ -385,3 +385,10 @@ global:
>  	ndctl_namespace_get_next_badblock;
>  	ndctl_dimm_get_dirty_shutdown;
>  } LIBNDCTL_17;
> +
> +LIBNDCTL_19 {
> +	ndctl_pfn_get_supported_alignment;
> +	ndctl_pfn_get_num_alignments;
> +	ndctl_dax_get_supported_alignment;
> +	ndctl_dax_get_num_alignments;
> +} LIBNDCTL_18;
> diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h
> index 62cef9e82da3..0f1f66256c1d 100644
> --- a/ndctl/libndctl.h
> +++ b/ndctl/libndctl.h
> @@ -681,6 +681,12 @@ enum ND_FW_STATUS ndctl_cmd_fw_xlat_firmware_status(struct ndctl_cmd *cmd);
>  struct ndctl_cmd *ndctl_dimm_cmd_new_ack_shutdown_count(struct ndctl_dimm *dimm);
>  int ndctl_dimm_fw_update_supported(struct ndctl_dimm *dimm);
>  
> +int ndctl_pfn_get_num_alignments(struct ndctl_pfn *pfn);
> +unsigned long ndctl_pfn_get_supported_alignment(struct ndctl_pfn *pfn, int i);
> +
> +int ndctl_dax_get_num_alignments(struct ndctl_dax *dax);
> +unsigned long ndctl_dax_get_supported_alignment(struct ndctl_dax *dax, int i);

For the libndctl.h definitions, instead of appending these at the
bottom, can you group the ndctl_pfn_ additions with the remaining
ndctl_pfn_ ones in the file? And same for ndctl_dax_*

> +
>  #ifdef __cplusplus
>  } /* extern "C" */
>  #endif

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

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

* Re: [PATCH v3 1/6] libndctl: Use the supported_alignment attribute
  2019-01-23 19:32 ` [PATCH v3 1/6] libndctl: Use the supported_alignment attribute Verma, Vishal L
@ 2019-01-28 23:11   ` Oliver
  2019-01-28 23:15     ` Verma, Vishal L
  0 siblings, 1 reply; 14+ messages in thread
From: Oliver @ 2019-01-28 23:11 UTC (permalink / raw)
  To: Verma, Vishal L; +Cc: linux-nvdimm

On Thu, Jan 24, 2019 at 6:32 AM Verma, Vishal L
<vishal.l.verma@intel.com> wrote:
>
>
> On Wed, 2019-01-16 at 20:49 +1100, Oliver O'Halloran wrote:
> > Newer kernels provide the "supported_alignments" sysfs attribute that
> > indicates what alignments can be used with a PFN or DAX namespace. This
> > patch adds the plumbing inside of libndctl to allow users to query this
> > information through using:
> >       ndctl_{dax|pfn}_get_supported_alignment(), and
> >       ndctl_{dax|pfn}_get_num_alignments()
> >
> > Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> > ---
> > v3: Changed the return type of the *_get_supported_alignment() functions
> >     to unsigned long to match the existing *_get_alignment() functions.
> > ---
> >  ndctl/lib/libndctl.c   | 40 ++++++++++++++++++++++++++++++++++++++++
> >  ndctl/lib/libndctl.sym |  7 +++++++
> >  ndctl/libndctl.h       |  6 ++++++
> >  3 files changed, 53 insertions(+)
>
> Hi Oliver,
>
> Thanks for resubmitting this series. I had a few comments below:

Hi Vishal,

Sorry about the late response. I've been out of the office for the last week.

>
> >
> > diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> > index 0c3a35e5bcc9..53369b5c9886 100644
> > --- a/ndctl/lib/libndctl.c
> > +++ b/ndctl/lib/libndctl.c
> > @@ -31,6 +31,7 @@
> >  #include <ccan/build_assert/build_assert.h>
> >
> >  #include <ndctl.h>
> > +#include <util/size.h>
> >  #include <util/sysfs.h>
> >  #include <ndctl/libndctl.h>
> >  #include <ndctl/namespace.h>
> > @@ -237,6 +238,7 @@ struct ndctl_pfn {
> >       int buf_len;
> >       uuid_t uuid;
> >       int id, generation;
> > +     struct ndctl_lbasize alignments;
> >  };
> >
> >  struct ndctl_dax {
> > @@ -4781,6 +4783,18 @@ static void *__add_pfn(struct ndctl_pfn *pfn, const char *pfn_base)
> >       else
> >               pfn->size = strtoull(buf, NULL, 0);
> >
> > +     /*
> > +      * If the kernel doesn't provide the supported_alignments sysfs
> > +      * attribute then it's safe to assume that we are running on x86
> > +      * which will always support 2MB and 4KB alignments.
> > +      */
> > +     sprintf(path, "%s/supported_alignments", pfn_base);
> > +     if (sysfs_read_attr(ctx, path, buf) < 0)
> > +             sprintf(buf, "%d %d", SZ_4K, SZ_2M);
> > +
> > +     if (parse_lbasize_supported(ctx, pfn_base, buf, &pfn->alignments) < 0)
> > +             goto err_read;
> > +
> >       free(path);
> >       return pfn;
> >
> > @@ -5015,6 +5029,22 @@ NDCTL_EXPORT int ndctl_pfn_set_align(struct ndctl_pfn *pfn, unsigned long align)
> >       return 0;
> >  }
> >
> > +NDCTL_EXPORT int ndctl_pfn_get_num_alignments(struct ndctl_pfn *pfn)
> > +{
> > +     return pfn->alignments.num;
> > +}
> > +
> > +NDCTL_EXPORT unsigned long ndctl_pfn_get_supported_alignment(struct ndctl_pfn *pfn, int i)
>
> This and a few other spots exceed 80 char lines, could you fix that up
> all over?

sure

>
> > +{
> > +     if (pfn->alignments.num == 0)
> > +             return 0;
> > +
> > +     if (i < 0 || i > pfn->alignments.num)
> > +             return -1;
>
> A bare '-1' return is unusual for exported functions. The convention in
> libndctl is to return -ERRNO. In this case, -EINVAL seems the right
> error to return?

ok

>
> > +     else
> > +             return pfn->alignments.supported[i];
> > +}
> > +
> >  NDCTL_EXPORT int ndctl_pfn_set_namespace(struct ndctl_pfn *pfn,
> >               struct ndctl_namespace *ndns)
> >  {
> > @@ -5237,6 +5267,16 @@ NDCTL_EXPORT unsigned long ndctl_dax_get_align(struct ndctl_dax *dax)
> >       return ndctl_pfn_get_align(&dax->pfn);
> >  }
> >
> > +NDCTL_EXPORT int ndctl_dax_get_num_alignments(struct ndctl_dax *dax)
> > +{
> > +     return ndctl_pfn_get_num_alignments(&dax->pfn);
> > +}
> > +
> > +NDCTL_EXPORT unsigned long ndctl_dax_get_supported_alignment(struct ndctl_dax *dax, int i)
>
> Another > 80 char line.
>
> > +{
> > +     return ndctl_pfn_get_supported_alignment(&dax->pfn, i);
> > +}
> > +
> >  NDCTL_EXPORT int ndctl_dax_has_align(struct ndctl_dax *dax)
> >  {
> >       return ndctl_pfn_has_align(&dax->pfn);
> > diff --git a/ndctl/lib/libndctl.sym b/ndctl/lib/libndctl.sym
> > index 6c4c8b4dfb8e..0103c1b71a1d 100644
> > --- a/ndctl/lib/libndctl.sym
> > +++ b/ndctl/lib/libndctl.sym
> > @@ -385,3 +385,10 @@ global:
> >       ndctl_namespace_get_next_badblock;
> >       ndctl_dimm_get_dirty_shutdown;
> >  } LIBNDCTL_17;
> > +
> > +LIBNDCTL_19 {
> > +     ndctl_pfn_get_supported_alignment;
> > +     ndctl_pfn_get_num_alignments;
> > +     ndctl_dax_get_supported_alignment;
> > +     ndctl_dax_get_num_alignments;
> > +} LIBNDCTL_18;
> > diff --git a/ndctl/libndctl.h b/ndctl/libndctl.h
> > index 62cef9e82da3..0f1f66256c1d 100644
> > --- a/ndctl/libndctl.h
> > +++ b/ndctl/libndctl.h
> > @@ -681,6 +681,12 @@ enum ND_FW_STATUS ndctl_cmd_fw_xlat_firmware_status(struct ndctl_cmd *cmd);
> >  struct ndctl_cmd *ndctl_dimm_cmd_new_ack_shutdown_count(struct ndctl_dimm *dimm);
> >  int ndctl_dimm_fw_update_supported(struct ndctl_dimm *dimm);
> >
> > +int ndctl_pfn_get_num_alignments(struct ndctl_pfn *pfn);
> > +unsigned long ndctl_pfn_get_supported_alignment(struct ndctl_pfn *pfn, int i);
> > +
> > +int ndctl_dax_get_num_alignments(struct ndctl_dax *dax);
> > +unsigned long ndctl_dax_get_supported_alignment(struct ndctl_dax *dax, int i);
>
> For the libndctl.h definitions, instead of appending these at the
> bottom, can you group the ndctl_pfn_ additions with the remaining
> ndctl_pfn_ ones in the file? And same for ndctl_dax_*

ok

>
> > +
> >  #ifdef __cplusplus
> >  } /* extern "C" */
> >  #endif
>
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

* Re: [PATCH v3 1/6] libndctl: Use the supported_alignment attribute
  2019-01-28 23:11   ` Oliver
@ 2019-01-28 23:15     ` Verma, Vishal L
  2019-01-28 23:20       ` Oliver
  0 siblings, 1 reply; 14+ messages in thread
From: Verma, Vishal L @ 2019-01-28 23:15 UTC (permalink / raw)
  To: oohall; +Cc: linux-nvdimm


On Tue, 2019-01-29 at 10:11 +1100, Oliver wrote:
> On Thu, Jan 24, 2019 at 6:32 AM Verma, Vishal L
> <vishal.l.verma@intel.com> wrote:
> > 
> > On Wed, 2019-01-16 at 20:49 +1100, Oliver O'Halloran wrote:
> > > Newer kernels provide the "supported_alignments" sysfs attribute that
> > > indicates what alignments can be used with a PFN or DAX namespace. This
> > > patch adds the plumbing inside of libndctl to allow users to query this
> > > information through using:
> > >       ndctl_{dax|pfn}_get_supported_alignment(), and
> > >       ndctl_{dax|pfn}_get_num_alignments()
> > > 
> > > Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> > > ---
> > > v3: Changed the return type of the *_get_supported_alignment() functions
> > >     to unsigned long to match the existing *_get_alignment() functions.
> > > ---
> > >  ndctl/lib/libndctl.c   | 40 ++++++++++++++++++++++++++++++++++++++++
> > >  ndctl/lib/libndctl.sym |  7 +++++++
> > >  ndctl/libndctl.h       |  6 ++++++
> > >  3 files changed, 53 insertions(+)
> > 
> > Hi Oliver,
> > 
> > Thanks for resubmitting this series. I had a few comments below:
> 
> Hi Vishal,
> 
> Sorry about the late response. I've been out of the office for the last week.

No worries. Also if you wouldn't mind, rebasing to the latest pending
branch on github would be helpful.

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

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

* Re: [PATCH v3 1/6] libndctl: Use the supported_alignment attribute
  2019-01-28 23:15     ` Verma, Vishal L
@ 2019-01-28 23:20       ` Oliver
  2019-01-28 23:22         ` Verma, Vishal L
  0 siblings, 1 reply; 14+ messages in thread
From: Oliver @ 2019-01-28 23:20 UTC (permalink / raw)
  To: Verma, Vishal L; +Cc: linux-nvdimm

On Tue, Jan 29, 2019 at 10:15 AM Verma, Vishal L
<vishal.l.verma@intel.com> wrote:
>
>
> On Tue, 2019-01-29 at 10:11 +1100, Oliver wrote:
> > On Thu, Jan 24, 2019 at 6:32 AM Verma, Vishal L
> > <vishal.l.verma@intel.com> wrote:
> > >
> > > On Wed, 2019-01-16 at 20:49 +1100, Oliver O'Halloran wrote:
> > > > Newer kernels provide the "supported_alignments" sysfs attribute that
> > > > indicates what alignments can be used with a PFN or DAX namespace. This
> > > > patch adds the plumbing inside of libndctl to allow users to query this
> > > > information through using:
> > > >       ndctl_{dax|pfn}_get_supported_alignment(), and
> > > >       ndctl_{dax|pfn}_get_num_alignments()
> > > >
> > > > Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> > > > ---
> > > > v3: Changed the return type of the *_get_supported_alignment() functions
> > > >     to unsigned long to match the existing *_get_alignment() functions.
> > > > ---
> > > >  ndctl/lib/libndctl.c   | 40 ++++++++++++++++++++++++++++++++++++++++
> > > >  ndctl/lib/libndctl.sym |  7 +++++++
> > > >  ndctl/libndctl.h       |  6 ++++++
> > > >  3 files changed, 53 insertions(+)
> > >
> > > Hi Oliver,
> > >
> > > Thanks for resubmitting this series. I had a few comments below:
> >
> > Hi Vishal,
> >
> > Sorry about the late response. I've been out of the office for the last week.
>
> No worries. Also if you wouldn't mind, rebasing to the latest pending
> branch on github would be helpful.

No problem.

One follow up question though. Does ndctl follow the kernel policy of
keeping format strings on one line even if they go over 80 chars or
should they be broken?

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

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

* Re: [PATCH v3 1/6] libndctl: Use the supported_alignment attribute
  2019-01-28 23:20       ` Oliver
@ 2019-01-28 23:22         ` Verma, Vishal L
  0 siblings, 0 replies; 14+ messages in thread
From: Verma, Vishal L @ 2019-01-28 23:22 UTC (permalink / raw)
  To: oohall; +Cc: linux-nvdimm


On Tue, 2019-01-29 at 10:20 +1100, Oliver wrote:
> On Tue, Jan 29, 2019 at 10:15 AM Verma, Vishal L
> <vishal.l.verma@intel.com> wrote:
> > 
> > On Tue, 2019-01-29 at 10:11 +1100, Oliver wrote:
> > > On Thu, Jan 24, 2019 at 6:32 AM Verma, Vishal L
> > > <vishal.l.verma@intel.com> wrote:
> > > > On Wed, 2019-01-16 at 20:49 +1100, Oliver O'Halloran wrote:
> > > > > Newer kernels provide the "supported_alignments" sysfs attribute that
> > > > > indicates what alignments can be used with a PFN or DAX namespace. This
> > > > > patch adds the plumbing inside of libndctl to allow users to query this
> > > > > information through using:
> > > > >       ndctl_{dax|pfn}_get_supported_alignment(), and
> > > > >       ndctl_{dax|pfn}_get_num_alignments()
> > > > > 
> > > > > Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> > > > > ---
> > > > > v3: Changed the return type of the *_get_supported_alignment() functions
> > > > >     to unsigned long to match the existing *_get_alignment() functions.
> > > > > ---
> > > > >  ndctl/lib/libndctl.c   | 40 ++++++++++++++++++++++++++++++++++++++++
> > > > >  ndctl/lib/libndctl.sym |  7 +++++++
> > > > >  ndctl/libndctl.h       |  6 ++++++
> > > > >  3 files changed, 53 insertions(+)
> > > > 
> > > > Hi Oliver,
> > > > 
> > > > Thanks for resubmitting this series. I had a few comments below:
> > > 
> > > Hi Vishal,
> > > 
> > > Sorry about the late response. I've been out of the office for the last week.
> > 
> > No worries. Also if you wouldn't mind, rebasing to the latest pending
> > branch on github would be helpful.
> 
> No problem.
> 
> One follow up question though. Does ndctl follow the kernel policy of
> keeping format strings on one line even if they go over 80 chars or
> should they be broken?

Yes we follow the kernel policy on that, so >80 char is ok for unbroken
format strings.


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

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

* Re: [PATCH v3 6/6] ndctl: Add supported_alignments to the JSON output
  2019-01-16  9:49 ` [PATCH v3 6/6] ndctl: Add supported_alignments to the " Oliver O'Halloran
@ 2019-01-29 14:40   ` Oliver
  2019-01-29 19:28     ` Verma, Vishal L
  0 siblings, 1 reply; 14+ messages in thread
From: Oliver @ 2019-01-29 14:40 UTC (permalink / raw)
  To: linux-nvdimm, Vishal L Verma, Dan Williams

On Wed, Jan 16, 2019 at 8:49 PM Oliver O'Halloran <oohall@gmail.com> wrote:
>
> Add the list of supported alignemnts to PFN and DAX namespaces. Also add the
> list of supported sector sizes to BTT namespaces.
>
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> ---
> Not sure the namespace JSON blob are the best place to put these. The
> region might be better, but slightly less accessable to users.
> ---
>  util/json.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 47 insertions(+), 1 deletion(-)
>
> diff --git a/util/json.c b/util/json.c
> index 77c96fb53c27..6c66033bd312 100644
> --- a/util/json.c
> +++ b/util/json.c
> @@ -303,6 +303,38 @@ struct json_object *util_daxctl_devs_to_list(struct daxctl_region *region,
>         return jdevs;
>  }
>
> +#define _SZ(get_max, get_elem, type) \
> +static struct json_object *type##_build_size_array(struct type *arg)   \
> +{                                                              \
> +       struct json_object *arr = json_object_new_array();      \
> +       int i;                                                  \
> +                                                               \
> +       if (!arr)                                               \
> +               return NULL;                                    \
> +                                                               \
> +       for (i = 0; i < get_max(arg); i++) {                    \
> +               struct json_object *jobj;                       \
> +               int64_t align;                                  \
> +                                                               \
> +               align = get_elem(arg, i);                       \
> +               jobj = json_object_new_int64(align);            \
> +               if (!jobj)                                      \
> +                       goto err;                               \
> +               json_object_array_add(arr, jobj);               \
> +       }                                                       \
> +                                                               \
> +       return arr;                                             \
> +err:                                                           \
> +       json_object_put(arr);                                   \
> +       return NULL;                                            \
> +}
> +#define SZ(type, kind) _SZ(ndctl_##type##_get_num_##kind##s, \
> +                          ndctl_##type##_get_supported_##kind, ndctl_##type)
> +SZ(pfn, alignment)
> +SZ(dax, alignment)
> +SZ(btt, sector_size)
> +//SZ(namespace, sector_size)
> +
>  struct json_object *util_daxctl_region_to_json(struct daxctl_region *region,
>                 const char *ident, unsigned long flags)
>  {
> @@ -739,7 +771,7 @@ struct json_object *util_namespace_to_json(struct ndctl_namespace *ndns,
>  {
>         struct json_object *jndns = json_object_new_object();
>         enum ndctl_pfn_loc loc = NDCTL_PFN_LOC_NONE;
> -       struct json_object *jobj, *jbbs = NULL;
> +       struct json_object *jobj, *jbbs = NULL, *size_array = NULL;
>         const char *locations[] = {
>                 [NDCTL_PFN_LOC_NONE] = "none",
>                 [NDCTL_PFN_LOC_RAM] = "mem",
> @@ -749,6 +781,7 @@ struct json_object *util_namespace_to_json(struct ndctl_namespace *ndns,
>         unsigned int sector_size = UINT_MAX;
>         enum ndctl_namespace_mode mode;
>         const char *bdev = NULL, *name;
> +       const char *size_array_name;
>         unsigned int bb_count = 0;
>         struct ndctl_btt *btt;
>         struct ndctl_pfn *pfn;
> @@ -936,6 +969,19 @@ struct json_object *util_namespace_to_json(struct ndctl_namespace *ndns,
>                         json_object_object_add(jndns, "numa_node", jobj);
>         }
>
> +       if (pfn) {
> +               size_array_name = "supported_alignments";
> +               size_array = ndctl_pfn_build_size_array(pfn);
> +       } else if (dax) {
> +               size_array_name = "supported_alignments";
> +               size_array = ndctl_dax_build_size_array(dax);
> +       } else if (btt) {
> +               size_array_name = "supported sector sizes";
> +               size_array = ndctl_btt_build_size_array(btt);
> +       }
> +       if (size_array)
> +               json_object_object_add(jndns, size_array_name, size_array);

So apparently I forgot to run the `make check` on v3 because this
completely breaks the unit tests.

The problem is that the tests rely on a sed script to parse the JSON
blob into shell variables. That works when the JSON blob only contains
scalars (and strings with no spaces), but it chokes when confronted
with a JSON array. I can't think of any simple ways to fix it other
than using a real JSON parser (jq?). Should I drop this patch for now?
I don't mind doing a real fix if you don't mind waiting a bit longer
:)

Oliver

> +
>         if (pfn)
>                 jbbs = util_pfn_badblocks_to_json(pfn, &bb_count, flags);
>         else if (dax)
> --
> 2.20.1
>
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

* Re: [PATCH v3 6/6] ndctl: Add supported_alignments to the JSON output
  2019-01-29 14:40   ` Oliver
@ 2019-01-29 19:28     ` Verma, Vishal L
  2019-01-29 19:43       ` Dan Williams
  0 siblings, 1 reply; 14+ messages in thread
From: Verma, Vishal L @ 2019-01-29 19:28 UTC (permalink / raw)
  To: Williams, Dan J, linux-nvdimm, oohall


On Wed, 2019-01-30 at 01:40 +1100, Oliver wrote:
> On Wed, Jan 16, 2019 at 8:49 PM Oliver O'Halloran <oohall@gmail.com> wrote:
> > Add the list of supported alignemnts to PFN and DAX namespaces. Also add the
> > list of supported sector sizes to BTT namespaces.
> > 
> > Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> > ---
> > Not sure the namespace JSON blob are the best place to put these. The
> > region might be better, but slightly less accessable to users.
> > ---
> >  util/json.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 47 insertions(+), 1 deletion(-)
> > 
> > diff --git a/util/json.c b/util/json.c
> > index 77c96fb53c27..6c66033bd312 100644
> > --- a/util/json.c
> > +++ b/util/json.c
> > @@ -303,6 +303,38 @@ struct json_object *util_daxctl_devs_to_list(struct daxctl_region *region,
> >         return jdevs;
> >  }
> > 
> > +#define _SZ(get_max, get_elem, type) \
> > +static struct json_object *type##_build_size_array(struct type *arg)   \
> > +{                                                              \
> > +       struct json_object *arr = json_object_new_array();      \
> > +       int i;                                                  \
> > +                                                               \
> > +       if (!arr)                                               \
> > +               return NULL;                                    \
> > +                                                               \
> > +       for (i = 0; i < get_max(arg); i++) {                    \
> > +               struct json_object *jobj;                       \
> > +               int64_t align;                                  \
> > +                                                               \
> > +               align = get_elem(arg, i);                       \
> > +               jobj = json_object_new_int64(align);            \
> > +               if (!jobj)                                      \
> > +                       goto err;                               \
> > +               json_object_array_add(arr, jobj);               \
> > +       }                                                       \
> > +                                                               \
> > +       return arr;                                             \
> > +err:                                                           \
> > +       json_object_put(arr);                                   \
> > +       return NULL;                                            \
> > +}
> > +#define SZ(type, kind) _SZ(ndctl_##type##_get_num_##kind##s, \
> > +                          ndctl_##type##_get_supported_##kind, ndctl_##type)
> > +SZ(pfn, alignment)
> > +SZ(dax, alignment)
> > +SZ(btt, sector_size)
> > +//SZ(namespace, sector_size)
> > +
> >  struct json_object *util_daxctl_region_to_json(struct daxctl_region *region,
> >                 const char *ident, unsigned long flags)
> >  {
> > @@ -739,7 +771,7 @@ struct json_object *util_namespace_to_json(struct ndctl_namespace *ndns,
> >  {
> >         struct json_object *jndns = json_object_new_object();
> >         enum ndctl_pfn_loc loc = NDCTL_PFN_LOC_NONE;
> > -       struct json_object *jobj, *jbbs = NULL;
> > +       struct json_object *jobj, *jbbs = NULL, *size_array = NULL;
> >         const char *locations[] = {
> >                 [NDCTL_PFN_LOC_NONE] = "none",
> >                 [NDCTL_PFN_LOC_RAM] = "mem",
> > @@ -749,6 +781,7 @@ struct json_object *util_namespace_to_json(struct ndctl_namespace *ndns,
> >         unsigned int sector_size = UINT_MAX;
> >         enum ndctl_namespace_mode mode;
> >         const char *bdev = NULL, *name;
> > +       const char *size_array_name;
> >         unsigned int bb_count = 0;
> >         struct ndctl_btt *btt;
> >         struct ndctl_pfn *pfn;
> > @@ -936,6 +969,19 @@ struct json_object *util_namespace_to_json(struct ndctl_namespace *ndns,
> >                         json_object_object_add(jndns, "numa_node", jobj);
> >         }
> > 
> > +       if (pfn) {
> > +               size_array_name = "supported_alignments";
> > +               size_array = ndctl_pfn_build_size_array(pfn);
> > +       } else if (dax) {
> > +               size_array_name = "supported_alignments";
> > +               size_array = ndctl_dax_build_size_array(dax);
> > +       } else if (btt) {
> > +               size_array_name = "supported sector sizes";
> > +               size_array = ndctl_btt_build_size_array(btt);
> > +       }
> > +       if (size_array)
> > +               json_object_object_add(jndns, size_array_name, size_array);
> 
> So apparently I forgot to run the `make check` on v3 because this
> completely breaks the unit tests.
> 
> The problem is that the tests rely on a sed script to parse the JSON
> blob into shell variables. That works when the JSON blob only contains
> scalars (and strings with no spaces), but it chokes when confronted
> with a JSON array. I can't think of any simple ways to fix it other
> than using a real JSON parser (jq?). Should I drop this patch for now?
> I don't mind doing a real fix if you don't mind waiting a bit longer
> :)

I think we should just fix the tests to properly parse json - we use jq
in several other places, and if something that didn't use jq breaks the
hackier sed/eval stuff, we should just take the chance to fix that in
the test. If this is a larger piece of work, I'm happy to defer it to
the next release. The other semi-related question might be, should we
always show the supported alignments array, or should hide it under one
or more -v verbosity levels. How often would a user need to know these?
If the array is now shown at the default non-verbose level, then maybe
the parsing problem goes away until a future time :)


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

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

* Re: [PATCH v3 6/6] ndctl: Add supported_alignments to the JSON output
  2019-01-29 19:28     ` Verma, Vishal L
@ 2019-01-29 19:43       ` Dan Williams
  0 siblings, 0 replies; 14+ messages in thread
From: Dan Williams @ 2019-01-29 19:43 UTC (permalink / raw)
  To: Verma, Vishal L; +Cc: linux-nvdimm

On Tue, Jan 29, 2019 at 11:28 AM Verma, Vishal L
<vishal.l.verma@intel.com> wrote:
>
>
> On Wed, 2019-01-30 at 01:40 +1100, Oliver wrote:
> > On Wed, Jan 16, 2019 at 8:49 PM Oliver O'Halloran <oohall@gmail.com> wrote:
> > > Add the list of supported alignemnts to PFN and DAX namespaces. Also add the
> > > list of supported sector sizes to BTT namespaces.
> > >
> > > Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> > > ---
> > > Not sure the namespace JSON blob are the best place to put these. The
> > > region might be better, but slightly less accessable to users.
> > > ---
> > >  util/json.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 47 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/util/json.c b/util/json.c
> > > index 77c96fb53c27..6c66033bd312 100644
> > > --- a/util/json.c
> > > +++ b/util/json.c
> > > @@ -303,6 +303,38 @@ struct json_object *util_daxctl_devs_to_list(struct daxctl_region *region,
> > >         return jdevs;
> > >  }
> > >
> > > +#define _SZ(get_max, get_elem, type) \
> > > +static struct json_object *type##_build_size_array(struct type *arg)   \
> > > +{                                                              \
> > > +       struct json_object *arr = json_object_new_array();      \
> > > +       int i;                                                  \
> > > +                                                               \
> > > +       if (!arr)                                               \
> > > +               return NULL;                                    \
> > > +                                                               \
> > > +       for (i = 0; i < get_max(arg); i++) {                    \
> > > +               struct json_object *jobj;                       \
> > > +               int64_t align;                                  \
> > > +                                                               \
> > > +               align = get_elem(arg, i);                       \
> > > +               jobj = json_object_new_int64(align);            \
> > > +               if (!jobj)                                      \
> > > +                       goto err;                               \
> > > +               json_object_array_add(arr, jobj);               \
> > > +       }                                                       \
> > > +                                                               \
> > > +       return arr;                                             \
> > > +err:                                                           \
> > > +       json_object_put(arr);                                   \
> > > +       return NULL;                                            \
> > > +}
> > > +#define SZ(type, kind) _SZ(ndctl_##type##_get_num_##kind##s, \
> > > +                          ndctl_##type##_get_supported_##kind, ndctl_##type)
> > > +SZ(pfn, alignment)
> > > +SZ(dax, alignment)
> > > +SZ(btt, sector_size)
> > > +//SZ(namespace, sector_size)
> > > +
> > >  struct json_object *util_daxctl_region_to_json(struct daxctl_region *region,
> > >                 const char *ident, unsigned long flags)
> > >  {
> > > @@ -739,7 +771,7 @@ struct json_object *util_namespace_to_json(struct ndctl_namespace *ndns,
> > >  {
> > >         struct json_object *jndns = json_object_new_object();
> > >         enum ndctl_pfn_loc loc = NDCTL_PFN_LOC_NONE;
> > > -       struct json_object *jobj, *jbbs = NULL;
> > > +       struct json_object *jobj, *jbbs = NULL, *size_array = NULL;
> > >         const char *locations[] = {
> > >                 [NDCTL_PFN_LOC_NONE] = "none",
> > >                 [NDCTL_PFN_LOC_RAM] = "mem",
> > > @@ -749,6 +781,7 @@ struct json_object *util_namespace_to_json(struct ndctl_namespace *ndns,
> > >         unsigned int sector_size = UINT_MAX;
> > >         enum ndctl_namespace_mode mode;
> > >         const char *bdev = NULL, *name;
> > > +       const char *size_array_name;
> > >         unsigned int bb_count = 0;
> > >         struct ndctl_btt *btt;
> > >         struct ndctl_pfn *pfn;
> > > @@ -936,6 +969,19 @@ struct json_object *util_namespace_to_json(struct ndctl_namespace *ndns,
> > >                         json_object_object_add(jndns, "numa_node", jobj);
> > >         }
> > >
> > > +       if (pfn) {
> > > +               size_array_name = "supported_alignments";
> > > +               size_array = ndctl_pfn_build_size_array(pfn);
> > > +       } else if (dax) {
> > > +               size_array_name = "supported_alignments";
> > > +               size_array = ndctl_dax_build_size_array(dax);
> > > +       } else if (btt) {
> > > +               size_array_name = "supported sector sizes";
> > > +               size_array = ndctl_btt_build_size_array(btt);
> > > +       }
> > > +       if (size_array)
> > > +               json_object_object_add(jndns, size_array_name, size_array);
> >
> > So apparently I forgot to run the `make check` on v3 because this
> > completely breaks the unit tests.
> >
> > The problem is that the tests rely on a sed script to parse the JSON
> > blob into shell variables. That works when the JSON blob only contains
> > scalars (and strings with no spaces), but it chokes when confronted
> > with a JSON array. I can't think of any simple ways to fix it other
> > than using a real JSON parser (jq?). Should I drop this patch for now?
> > I don't mind doing a real fix if you don't mind waiting a bit longer
> > :)
>
> I think we should just fix the tests to properly parse json - we use jq
> in several other places, and if something that didn't use jq breaks the
> hackier sed/eval stuff, we should just take the chance to fix that in
> the test. If this is a larger piece of work, I'm happy to defer it to
> the next release. The other semi-related question might be, should we
> always show the supported alignments array, or should hide it under one
> or more -v verbosity levels. How often would a user need to know these?
> If the array is now shown at the default non-verbose level, then maybe
> the parsing problem goes away until a future time :)
>

Yeah, if gating this property on UTIL_JSON_VERBOSE fixes the unit test
that seems like the fastest way forward for now.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

end of thread, other threads:[~2019-01-29 19:43 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-16  9:49 [PATCH v3 1/6] libndctl: Use the supported_alignment attribute Oliver O'Halloran
2019-01-16  9:49 ` [PATCH v3 2/6] ndctl/namespace: Check for seed namespaces earlier Oliver O'Halloran
2019-01-16  9:49 ` [PATCH v3 3/6] ndctl/namespace: Use seed alignment as the default Oliver O'Halloran
2019-01-16  9:49 ` [PATCH v3 4/6] ndctl/namespace: Validate alignment from the {pfn|dax} seed Oliver O'Halloran
2019-01-16  9:49 ` [PATCH v3 5/6] ndctl: Add alignment to the namespace JSON output Oliver O'Halloran
2019-01-16  9:49 ` [PATCH v3 6/6] ndctl: Add supported_alignments to the " Oliver O'Halloran
2019-01-29 14:40   ` Oliver
2019-01-29 19:28     ` Verma, Vishal L
2019-01-29 19:43       ` Dan Williams
2019-01-23 19:32 ` [PATCH v3 1/6] libndctl: Use the supported_alignment attribute Verma, Vishal L
2019-01-28 23:11   ` Oliver
2019-01-28 23:15     ` Verma, Vishal L
2019-01-28 23:20       ` Oliver
2019-01-28 23:22         ` Verma, Vishal L

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.