nvdimm.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/4] libndctl: Use the supported_alignment attribute
@ 2018-11-19  8:00 Oliver O'Halloran
  2018-11-19  8:00 ` [PATCH 2/4] ndctl/namespace: Check for seed namespaces earlier Oliver O'Halloran
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Oliver O'Halloran @ 2018-11-19  8:00 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>
---
 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..4d0e58a22953 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 unsigned int ndctl_pfn_get_num_alignments(struct ndctl_pfn *pfn)
+{
+	return pfn->alignments.num;
+}
+
+NDCTL_EXPORT int 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 UINT_MAX;
+	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 unsigned int ndctl_dax_get_num_alignments(struct ndctl_dax *dax)
+{
+	return ndctl_pfn_get_num_alignments(&dax->pfn);
+}
+
+NDCTL_EXPORT int 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..4ff25c0a4783 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);
 
+unsigned int ndctl_pfn_get_num_alignments(struct ndctl_pfn *pfn);
+int ndctl_pfn_get_supported_alignment(struct ndctl_pfn *pfn, int i);
+
+unsigned int ndctl_dax_get_num_alignments(struct ndctl_dax *dax);
+int ndctl_dax_get_supported_alignment(struct ndctl_dax *dax, int i);
+
 #ifdef __cplusplus
 } /* extern "C" */
 #endif
-- 
2.17.2

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

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

* [PATCH 2/4] ndctl/namespace: Check for seed namespaces earlier
  2018-11-19  8:00 [PATCH 1/4] libndctl: Use the supported_alignment attribute Oliver O'Halloran
@ 2018-11-19  8:00 ` Oliver O'Halloran
  2018-11-25 20:12   ` Dan Williams
  2018-11-19  8:00 ` [PATCH 3/4] ndctl/namespace: Use seed alignment as the default Oliver O'Halloran
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 11+ messages in thread
From: Oliver O'Halloran @ 2018-11-19  8:00 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.

No functional changes.

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

diff --git a/ndctl/namespace.c b/ndctl/namespace.c
index b6f12306fe76..dc9a56609881 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,14 +523,28 @@ 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;
+		} else if (!pfn) {
+			error("Kernel does not support fsdax mode\n");
+			return -ENODEV;
+		}
+	} 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
-				&& (!pfn || !ndctl_pfn_has_align(pfn))) {
+				&& !ndctl_pfn_has_align(pfn)) {
 			/*
 			 * Initial pfn device support in the kernel
 			 * supported a 2M default alignment when
@@ -538,7 +554,7 @@ static int validate_namespace_options(struct ndctl_region *region,
 					region_name);
 			return -EAGAIN;
 		} else if (p->mode == NDCTL_NS_MODE_DAX
-				&& (!dax || !ndctl_dax_has_align(dax))) {
+				&& !ndctl_dax_has_align(dax)) {
 			/*
 			 * Unlike the pfn case, we require the kernel to
 			 * have 'align' support for device-dax.
@@ -705,31 +721,6 @@ 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;
-		}
-	}
-
-	/* 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;
 
 	return 0;
-- 
2.17.2

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

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

* [PATCH 3/4] ndctl/namespace: Use seed alignment as the default
  2018-11-19  8:00 [PATCH 1/4] libndctl: Use the supported_alignment attribute Oliver O'Halloran
  2018-11-19  8:00 ` [PATCH 2/4] ndctl/namespace: Check for seed namespaces earlier Oliver O'Halloran
@ 2018-11-19  8:00 ` Oliver O'Halloran
  2018-11-25 20:14   ` Dan Williams
  2018-11-19  8:00 ` [PATCH 4/4] ndctl/namespace: Validate alignment from the {pfn|dax} seed Oliver O'Halloran
  2018-11-25 20:06 ` [PATCH 1/4] libndctl: Use the supported_alignment attribute Dan Williams
  3 siblings, 1 reply; 11+ messages in thread
From: Oliver O'Halloran @ 2018-11-19  8:00 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 dc9a56609881..1e04f32c0e95 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
-				&& !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
-				&& !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 (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.17.2

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

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

* [PATCH 4/4] ndctl/namespace: Validate alignment from the {pfn|dax} seed
  2018-11-19  8:00 [PATCH 1/4] libndctl: Use the supported_alignment attribute Oliver O'Halloran
  2018-11-19  8:00 ` [PATCH 2/4] ndctl/namespace: Check for seed namespaces earlier Oliver O'Halloran
  2018-11-19  8:00 ` [PATCH 3/4] ndctl/namespace: Use seed alignment as the default Oliver O'Halloran
@ 2018-11-19  8:00 ` Oliver O'Halloran
  2018-11-25 20:18   ` Dan Williams
  2018-11-25 20:06 ` [PATCH 1/4] libndctl: Use the supported_alignment attribute Dan Williams
  3 siblings, 1 reply; 11+ messages in thread
From: Oliver O'Halloran @ 2018-11-19  8:00 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 | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/ndctl/namespace.c b/ndctl/namespace.c
index 1e04f32c0e95..6f8dca288527 100644
--- a/ndctl/namespace.c
+++ b/ndctl/namespace.c
@@ -537,8 +537,18 @@ 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:
+			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 +556,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.17.2

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

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

* Re: [PATCH 1/4] libndctl: Use the supported_alignment attribute
  2018-11-19  8:00 [PATCH 1/4] libndctl: Use the supported_alignment attribute Oliver O'Halloran
                   ` (2 preceding siblings ...)
  2018-11-19  8:00 ` [PATCH 4/4] ndctl/namespace: Validate alignment from the {pfn|dax} seed Oliver O'Halloran
@ 2018-11-25 20:06 ` Dan Williams
  3 siblings, 0 replies; 11+ messages in thread
From: Dan Williams @ 2018-11-25 20:06 UTC (permalink / raw)
  To: Oliver O'Halloran; +Cc: linux-nvdimm

On Mon, Nov 19, 2018 at 12:11 AM Oliver O'Halloran <oohall@gmail.com> 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>

Looks good to me,

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] 11+ messages in thread

* Re: [PATCH 2/4] ndctl/namespace: Check for seed namespaces earlier
  2018-11-19  8:00 ` [PATCH 2/4] ndctl/namespace: Check for seed namespaces earlier Oliver O'Halloran
@ 2018-11-25 20:12   ` Dan Williams
  2018-11-27 13:27     ` Oliver
  0 siblings, 1 reply; 11+ messages in thread
From: Dan Williams @ 2018-11-25 20:12 UTC (permalink / raw)
  To: Oliver O'Halloran; +Cc: linux-nvdimm

On Mon, Nov 19, 2018 at 12:11 AM Oliver O'Halloran <oohall@gmail.com> wrote:
>
> 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.
>
> No functional changes.

It does appear to have a functional change. do_setup_pfn() supports
the case of statically allocated namespaces in NDCTL_NS_MODE_MEMORY
mode. This is what one gets by default with "legacy" pmem defined as
E820-type-12 memory. In that case the kernel assumes that the
resulting memmap is always small enough to be allocated from DRAM and
there is no need to use a dynamic  pfn device. So, if I'm not
mistaken, the deletion of do_setup_pfn() loses that special case
handling.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

* Re: [PATCH 3/4] ndctl/namespace: Use seed alignment as the default
  2018-11-19  8:00 ` [PATCH 3/4] ndctl/namespace: Use seed alignment as the default Oliver O'Halloran
@ 2018-11-25 20:14   ` Dan Williams
  0 siblings, 0 replies; 11+ messages in thread
From: Dan Williams @ 2018-11-25 20:14 UTC (permalink / raw)
  To: Oliver O'Halloran; +Cc: linux-nvdimm

On Mon, Nov 19, 2018 at 12:11 AM Oliver O'Halloran <oohall@gmail.com> wrote:
>
> 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.

Looks ok, but I'd want to try it with the unit test suite after my
comment on patch-2 is resolved.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

* Re: [PATCH 4/4] ndctl/namespace: Validate alignment from the {pfn|dax} seed
  2018-11-19  8:00 ` [PATCH 4/4] ndctl/namespace: Validate alignment from the {pfn|dax} seed Oliver O'Halloran
@ 2018-11-25 20:18   ` Dan Williams
  0 siblings, 0 replies; 11+ messages in thread
From: Dan Williams @ 2018-11-25 20:18 UTC (permalink / raw)
  To: Oliver O'Halloran; +Cc: linux-nvdimm

On Mon, Nov 19, 2018 at 12:11 AM Oliver O'Halloran <oohall@gmail.com> wrote:
>
> 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 | 32 ++++++++++++++++++++++++--------
>  1 file changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/ndctl/namespace.c b/ndctl/namespace.c
> index 1e04f32c0e95..6f8dca288527 100644
> --- a/ndctl/namespace.c
> +++ b/ndctl/namespace.c
> @@ -537,8 +537,18 @@ 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:
> +                       alignments = ndctl_pfn_get_num_alignments(pfn);
> +                       break;
> +
> +               case NDCTL_NS_MODE_DAX:
> +                       alignments = ndctl_dax_get_num_alignments(dax);
> +                       break;
> +

This needs to be reworked after addressing the patch-2 comment because
the "(!pfn || !ndctl_pfn_has_align(pfn)))" was handling the
"memory-mode without pfn device" case.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

* Re: [PATCH 2/4] ndctl/namespace: Check for seed namespaces earlier
  2018-11-25 20:12   ` Dan Williams
@ 2018-11-27 13:27     ` Oliver
  2019-01-08  2:59       ` Dan Williams
  0 siblings, 1 reply; 11+ messages in thread
From: Oliver @ 2018-11-27 13:27 UTC (permalink / raw)
  To: Dan Williams; +Cc: linux-nvdimm

On Mon, Nov 26, 2018 at 7:12 AM Dan Williams <dan.j.williams@intel.com> wrote:
>
> On Mon, Nov 19, 2018 at 12:11 AM Oliver O'Halloran <oohall@gmail.com> wrote:
> >
> > 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.
> >
> > No functional changes.
>
> It does appear to have a functional change. do_setup_pfn() supports
> the case of statically allocated namespaces in NDCTL_NS_MODE_MEMORY
> mode.

Hmm, ok. Up until now I had assumed that as far as ndctl was concerned
NS_MODE_MEMORY was synonymous with a pfn namespace.

> This is what one gets by default with "legacy" pmem defined as
> E820-type-12 memory. In that case the kernel assumes that the
> resulting memmap is always small enough to be allocated from DRAM and
> there is no need to use a dynamic  pfn device. So, if I'm not
> mistaken, the deletion of do_setup_pfn() loses that special case
> handling.

>From what I see, the main difference is that ndctl would fail
validation when fsdax mode is specified and there's no pfn namespace
support in the kernel. I agree that's not great, but I'm not sure what
we should be doing here. The current behaviour will silently ignore -a
if "-m fsdax -M mem" is specified in the reconfigure case, which
doesn't seem great either.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

* Re: [PATCH 2/4] ndctl/namespace: Check for seed namespaces earlier
  2018-11-27 13:27     ` Oliver
@ 2019-01-08  2:59       ` Dan Williams
  2019-01-08 11:00         ` Oliver
  0 siblings, 1 reply; 11+ messages in thread
From: Dan Williams @ 2019-01-08  2:59 UTC (permalink / raw)
  To: Oliver; +Cc: linux-nvdimm

Sorry for letting this conversation go cold, lets try to revive it as
Vishal is looking to cut an ndctl v64 in the coming days and it would
be good to include this support.

On Tue, Nov 27, 2018 at 5:27 AM Oliver <oohall@gmail.com> wrote:
>
> On Mon, Nov 26, 2018 at 7:12 AM Dan Williams <dan.j.williams@intel.com> wrote:
> >
> > On Mon, Nov 19, 2018 at 12:11 AM Oliver O'Halloran <oohall@gmail.com> wrote:
> > >
> > > 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.
> > >
> > > No functional changes.
> >
> > It does appear to have a functional change. do_setup_pfn() supports
> > the case of statically allocated namespaces in NDCTL_NS_MODE_MEMORY
> > mode.
>
> Hmm, ok. Up until now I had assumed that as far as ndctl was concerned
> NS_MODE_MEMORY was synonymous with a pfn namespace.
>
> > This is what one gets by default with "legacy" pmem defined as
> > E820-type-12 memory. In that case the kernel assumes that the
> > resulting memmap is always small enough to be allocated from DRAM and
> > there is no need to use a dynamic  pfn device. So, if I'm not
> > mistaken, the deletion of do_setup_pfn() loses that special case
> > handling.
>
> From what I see, the main difference is that ndctl would fail
> validation when fsdax mode is specified and there's no pfn namespace
> support in the kernel. I agree that's not great, but I'm not sure what
> we should be doing here. The current behaviour will silently ignore -a
> if "-m fsdax -M mem" is specified in the reconfigure case, which
> doesn't seem great either.

Right, both issues need to be addressed. As far as I can see
do_setup_pfn() just needs to be extended to detect attempts to set a
non-default alignment, and then when setting the alignment require it
to be one of the supported values.

I expect we should also extend the region listing output to include
the supported alignments.

Does this clarify your concern enough to attempt a respin of the
series? If not just holler and Vishal and I will arm-wrestle to decide
who picks this up.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

* Re: [PATCH 2/4] ndctl/namespace: Check for seed namespaces earlier
  2019-01-08  2:59       ` Dan Williams
@ 2019-01-08 11:00         ` Oliver
  0 siblings, 0 replies; 11+ messages in thread
From: Oliver @ 2019-01-08 11:00 UTC (permalink / raw)
  To: Dan Williams; +Cc: linux-nvdimm

On Tue, Jan 8, 2019 at 1:59 PM Dan Williams <dan.j.williams@intel.com> wrote:
>
> Sorry for letting this conversation go cold, lets try to revive it as
> Vishal is looking to cut an ndctl v64 in the coming days and it would
> be good to include this support.
>
> On Tue, Nov 27, 2018 at 5:27 AM Oliver <oohall@gmail.com> wrote:
> >
> > On Mon, Nov 26, 2018 at 7:12 AM Dan Williams <dan.j.williams@intel.com> wrote:
> > >
> > > On Mon, Nov 19, 2018 at 12:11 AM Oliver O'Halloran <oohall@gmail.com> wrote:
> > > >
> > > > 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.
> > > >
> > > > No functional changes.
> > >
> > > It does appear to have a functional change. do_setup_pfn() supports
> > > the case of statically allocated namespaces in NDCTL_NS_MODE_MEMORY
> > > mode.
> >
> > Hmm, ok. Up until now I had assumed that as far as ndctl was concerned
> > NS_MODE_MEMORY was synonymous with a pfn namespace.
> >
> > > This is what one gets by default with "legacy" pmem defined as
> > > E820-type-12 memory. In that case the kernel assumes that the
> > > resulting memmap is always small enough to be allocated from DRAM and
> > > there is no need to use a dynamic  pfn device. So, if I'm not
> > > mistaken, the deletion of do_setup_pfn() loses that special case
> > > handling.
> >
> > From what I see, the main difference is that ndctl would fail
> > validation when fsdax mode is specified and there's no pfn namespace
> > support in the kernel. I agree that's not great, but I'm not sure what
> > we should be doing here. The current behaviour will silently ignore -a
> > if "-m fsdax -M mem" is specified in the reconfigure case, which
> > doesn't seem great either.
>
> Right, both issues need to be addressed. As far as I can see
> do_setup_pfn() just needs to be extended to detect attempts to set a
> non-default alignment, and then when setting the alignment require it
> to be one of the supported values.

Seems reasonable. I'll dust off the series and post a respin tomorrow.

> I expect we should also extend the region listing output to include
> the supported alignments.

Probably a good idea. I think even have a patch to do that somewhere...

> Does this clarify your concern enough to attempt a respin of the
> series? If not just holler and Vishal and I will arm-wrestle to decide
> who picks this up.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

end of thread, other threads:[~2019-01-08 11:00 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-19  8:00 [PATCH 1/4] libndctl: Use the supported_alignment attribute Oliver O'Halloran
2018-11-19  8:00 ` [PATCH 2/4] ndctl/namespace: Check for seed namespaces earlier Oliver O'Halloran
2018-11-25 20:12   ` Dan Williams
2018-11-27 13:27     ` Oliver
2019-01-08  2:59       ` Dan Williams
2019-01-08 11:00         ` Oliver
2018-11-19  8:00 ` [PATCH 3/4] ndctl/namespace: Use seed alignment as the default Oliver O'Halloran
2018-11-25 20:14   ` Dan Williams
2018-11-19  8:00 ` [PATCH 4/4] ndctl/namespace: Validate alignment from the {pfn|dax} seed Oliver O'Halloran
2018-11-25 20:18   ` Dan Williams
2018-11-25 20:06 ` [PATCH 1/4] libndctl: Use the supported_alignment attribute 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).