[ndctl PATCH 0/2] xaction-namespace counting reworks

[ndctl PATCH 0/2] xaction-namespace counting reworks

[Vishal Verma]

There have been several reports of the counts printed for namespace
actions such as enable, disable, and especially destroy being incorrect
and confusing. These patches are an attempt to clean up and bring
greater consistency to these operations.

Patch 1 lays some groundwork for testing whether a namespace (under
destroy-namespace) that is supposed to have metadata due to its
personality (btt, pfn, dev-dax) has already had its info-blocks zeroed.
If that is the case, then allow for skipping the zeroing, and
communicate the fact. This is really only applicable for legacy
namespaces as in the labeled case, the label would have been
inconsistent if the metadata had been previously wiped.

Patch 2 builds on the above by creating a harder separation between the
result of an action vs. the number of objects (namespaces) the action
actually processed. Making the result independent of the number
processed allows us to make a cleaner decision on what exactly was done,
and print errors and/or counts accordingly.

Vishal Verma (2):
  ndctl, destroy-namespace: check for an already-zeroed info block
  ndctl, namespace: rework namespace action accounting

 ndctl/namespace.c | 197 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 116 insertions(+), 81 deletions(-)

-- 
2.14.4

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

[ndctl PATCH 1/2] ndctl, destroy-namespace: check for an already-zeroed info block

[Vishal Verma]

in zero_info_block, check if the info block is already all-zeroes, and
if so, don't write zeroes. This is in preparation for the fixes to
destroy-namespace accounting where the number of namespaces 'destroyed'
is often incorrect.

Cc: Steve Scargall <steve.scargall@intel.com>
Link: https://github.com/pmem/ndctl/issues/41
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 ndctl/namespace.c | 43 ++++++++++++++++++++++++++++++++++---------
 1 file changed, 34 insertions(+), 9 deletions(-)

diff --git a/ndctl/namespace.c b/ndctl/namespace.c
index 510553c..65e8f7c 100644
--- a/ndctl/namespace.c
+++ b/ndctl/namespace.c
@@ -14,6 +14,7 @@
 #include <fcntl.h>
 #include <errno.h>
 #include <stdlib.h>
+#include <string.h>
 #include <unistd.h>
 #include <limits.h>
 #include <syslog.h>
@@ -786,23 +787,35 @@ static int namespace_create(struct ndctl_region *region)
 	return setup_namespace(region, ndns, &p);
 }
 
+/*
+ * Return convention:
+ * rc < 0 : Error while zeroing, propagate forward
+ * rc == 0 : Successfully cleared the info block, report as destroyed
+ * rc > 0 : skipped, do not count
+ */
 static int zero_info_block(struct ndctl_namespace *ndns)
 {
 	const char *devname = ndctl_namespace_get_devname(ndns);
-	int fd, rc = -ENXIO;
-	void *buf = NULL;
+	int fd, rc = -ENXIO, info_size = 8192;
+	void *buf = NULL, *read_buf = NULL;
 	char path[50];
 
 	ndctl_namespace_set_raw_mode(ndns, 1);
 	rc = ndctl_namespace_enable(ndns);
 	if (rc < 0) {
 		debug("%s failed to enable for zeroing, continuing\n", devname);
-		rc = 0;
+		rc = 1;
 		goto out;
 	}
 
-	if (posix_memalign(&buf, 4096, 4096) != 0)
-		return -ENXIO;
+	if (posix_memalign(&buf, 4096, info_size) != 0) {
+		rc = -ENOMEM;
+		goto out;
+	}
+	if (posix_memalign(&read_buf, 4096, info_size) != 0) {
+		rc = -ENOMEM;
+		goto out;
+	}
 
 	sprintf(path, "/dev/%s", ndctl_namespace_get_block_device(ndns));
 	fd = open(path, O_RDWR|O_DIRECT|O_EXCL);
@@ -812,18 +825,30 @@ static int zero_info_block(struct ndctl_namespace *ndns)
 		goto out;
 	}
 
-	memset(buf, 0, 4096);
-	rc = pwrite(fd, buf, 4096, 4096);
-	if (rc < 4096) {
+	memset(buf, 0, info_size);
+	rc = pread(fd, read_buf, info_size, 0);
+	if (rc < info_size) {
+		debug("%s: failed to read info block, continuing\n",
+			devname);
+	}
+	if (memcmp(buf, read_buf, info_size) == 0) {
+		rc = 1;
+		goto out_close;
+	}
+
+	rc = pwrite(fd, buf, info_size, 0);
+	if (rc < info_size) {
 		debug("%s: failed to zero info block %s\n",
 				devname, path);
 		rc = -ENXIO;
 	} else
 		rc = 0;
+ out_close:
 	close(fd);
  out:
 	ndctl_namespace_set_raw_mode(ndns, 0);
 	ndctl_namespace_disable_invalidate(ndns);
+	free(read_buf);
 	free(buf);
 	return rc;
 }
@@ -857,7 +882,7 @@ static int namespace_destroy(struct ndctl_region *region,
 
 	if (pfn || btt || dax) {
 		rc = zero_info_block(ndns);
-		if (rc)
+		if (rc < 0)
 			return rc;
 	}
 
-- 
2.14.4

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

[ndctl PATCH 2/2] ndctl, namespace: rework namespace action accounting

[Vishal Verma]

Make the count calculations consistent for all namespace actions by
introducing a new parameter to do_xaction_namespace() which explicitly
keeps a count of successful operations.

For destroy-namespace, correctly propagate errors so that the above
accounting works as expected. For legacy (labelless or emulated)
namespaces, if we perform a destructive action such as writing zeroes to
the info block area, acccount for them in the destroy count, but
otherwise consider them as 'skipped' since they cannot be truly
'destroyed'.

Cc: Steve Scargall <steve.scargall@intel.com>
Link: https://github.com/pmem/ndctl/issues/41
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 ndctl/namespace.c | 154 +++++++++++++++++++++++++++++-------------------------
 1 file changed, 82 insertions(+), 72 deletions(-)

diff --git a/ndctl/namespace.c b/ndctl/namespace.c
index 65e8f7c..b6f1230 100644
--- a/ndctl/namespace.c
+++ b/ndctl/namespace.c
@@ -860,6 +860,7 @@ static int namespace_destroy(struct ndctl_region *region,
 	struct ndctl_pfn *pfn = ndctl_namespace_get_pfn(ndns);
 	struct ndctl_dax *dax = ndctl_namespace_get_dax(ndns);
 	struct ndctl_btt *btt = ndctl_namespace_get_btt(ndns);
+	bool did_zero = false;
 	int rc;
 
 	if (ndctl_region_get_ro(region)) {
@@ -884,13 +885,33 @@ static int namespace_destroy(struct ndctl_region *region,
 		rc = zero_info_block(ndns);
 		if (rc < 0)
 			return rc;
+		if (rc == 0)
+			did_zero = true;
+	}
+
+	switch (ndctl_namespace_get_type(ndns)) {
+        case ND_DEVICE_NAMESPACE_PMEM:
+        case ND_DEVICE_NAMESPACE_BLK:
+		break;
+	default:
+		/*
+		 * for legacy namespaces, we we did any info block
+		 * zeroing, we need "processed" to be incremented
+		 * but otherwise we are skipping in the count
+		 */
+		if (did_zero)
+			rc = 0;
+		else
+			rc = 1;
+		goto out;
 	}
 
 	rc = ndctl_namespace_delete(ndns);
 	if (rc)
 		debug("%s: failed to reclaim\n", devname);
 
-	return 0;
+out:
+	return rc;
 }
 
 static int enable_labels(struct ndctl_region *region)
@@ -987,7 +1008,7 @@ static int namespace_reconfig(struct ndctl_region *region,
 		return rc;
 
 	rc = namespace_destroy(region, ndns);
-	if (rc)
+	if (rc < 0)
 		return rc;
 
 	/* check if we can enable labels on this region */
@@ -1012,13 +1033,16 @@ int namespace_check(struct ndctl_namespace *ndns, bool verbose, bool force,
 		bool repair, bool logfix);
 
 static int do_xaction_namespace(const char *namespace,
-		enum device_action action, struct ndctl_ctx *ctx)
+		enum device_action action, struct ndctl_ctx *ctx,
+		int *processed)
 {
 	struct ndctl_namespace *ndns, *_n;
-	int rc = -ENXIO, success = 0;
 	struct ndctl_region *region;
 	const char *ndns_name;
 	struct ndctl_bus *bus;
+	int rc = -ENXIO;
+
+	*processed = 0;
 
 	if (!namespace && action != ACTION_CREATE)
 		return rc;
@@ -1052,7 +1076,7 @@ static int do_xaction_namespace(const char *namespace,
 				if (rc == -EAGAIN)
 					continue;
 				if (rc == 0)
-					rc = 1;
+					*processed = 1;
 				return rc;
 			}
 			ndctl_namespace_foreach_safe(region, ndns, _n) {
@@ -1064,36 +1088,43 @@ static int do_xaction_namespace(const char *namespace,
 				switch (action) {
 				case ACTION_DISABLE:
 					rc = ndctl_namespace_disable_safe(ndns);
+					if (rc == 0)
+						(*processed)++;
 					break;
 				case ACTION_ENABLE:
 					rc = ndctl_namespace_enable(ndns);
+					if (rc >= 0) {
+						(*processed)++;
+						rc = 0;
+					}
 					break;
 				case ACTION_DESTROY:
 					rc = namespace_destroy(region, ndns);
+					if (rc == 0)
+						(*processed)++;
+					/* return success if skipped */
+					if (rc > 0)
+						rc = 0;
 					break;
 				case ACTION_CHECK:
 					rc = namespace_check(ndns, verbose,
 							force, repair, logfix);
-					if (rc < 0)
-						return rc;
+					if (rc == 0)
+						(*processed)++;
 					break;
 				case ACTION_CREATE:
 					rc = namespace_reconfig(region, ndns);
-					if (rc < 0)
-						return rc;
-					return 1;
+					if (rc == 0)
+						*processed = 1;
+					return rc;
 				default:
 					rc = -EINVAL;
 					break;
 				}
-				if (rc >= 0)
-					success++;
 			}
 		}
 	}
 
-	if (success)
-		return success;
 	return rc;
 }
 
@@ -1102,20 +1133,16 @@ int cmd_disable_namespace(int argc, const char **argv, void *ctx)
 	char *xable_usage = "ndctl disable-namespace <namespace> [<options>]";
 	const char *namespace = parse_namespace_options(argc, argv,
 			ACTION_DISABLE, base_options, xable_usage);
-	int disabled = do_xaction_namespace(namespace, ACTION_DISABLE, ctx);
+	int disabled, rc;
 
-	if (disabled < 0) {
+	rc = do_xaction_namespace(namespace, ACTION_DISABLE, ctx, &disabled);
+	if (rc < 0)
 		fprintf(stderr, "error disabling namespaces: %s\n",
-				strerror(-disabled));
-		return disabled;
-	} else if (disabled == 0) {
-		fprintf(stderr, "disabled 0 namespaces\n");
-		return -ENXIO;
-	} else {
-		fprintf(stderr, "disabled %d namespace%s\n", disabled,
-				disabled > 1 ? "s" : "");
-		return 0;
-	}
+				strerror(-rc));
+
+	fprintf(stderr, "disabled %d namespace%s\n", disabled,
+			disabled == 1 ? "" : "s");
+	return rc;
 }
 
 int cmd_enable_namespace(int argc, const char **argv, void *ctx)
@@ -1123,20 +1150,15 @@ int cmd_enable_namespace(int argc, const char **argv, void *ctx)
 	char *xable_usage = "ndctl enable-namespace <namespace> [<options>]";
 	const char *namespace = parse_namespace_options(argc, argv,
 			ACTION_ENABLE, base_options, xable_usage);
-	int enabled = do_xaction_namespace(namespace, ACTION_ENABLE, ctx);
+	int enabled, rc;
 
-	if (enabled < 0) {
+	rc = do_xaction_namespace(namespace, ACTION_ENABLE, ctx, &enabled);
+	if (rc < 0)
 		fprintf(stderr, "error enabling namespaces: %s\n",
-				strerror(-enabled));
-		return enabled;
-	} else if (enabled == 0) {
-		fprintf(stderr, "enabled 0 namespaces\n");
-		return 0;
-	} else {
-		fprintf(stderr, "enabled %d namespace%s\n", enabled,
-				enabled > 1 ? "s" : "");
-		return 0;
-	}
+				strerror(-rc));
+	fprintf(stderr, "enabled %d namespace%s\n", enabled,
+			enabled == 1 ? "" : "s");
+	return rc;
 }
 
 int cmd_create_namespace(int argc, const char **argv, void *ctx)
@@ -1144,9 +1166,10 @@ int cmd_create_namespace(int argc, const char **argv, void *ctx)
 	char *xable_usage = "ndctl create-namespace [<options>]";
 	const char *namespace = parse_namespace_options(argc, argv,
 			ACTION_CREATE, create_options, xable_usage);
-	int created = do_xaction_namespace(namespace, ACTION_CREATE, ctx);
+	int created, rc;
 
-	if (created < 1 && param.do_scan) {
+	rc = do_xaction_namespace(namespace, ACTION_CREATE, ctx, &created);
+	if (rc < 0 && created < 1 && param.do_scan) {
 		/*
 		 * In the default scan case we try pmem first and then
 		 * fallback to blk before giving up.
@@ -1154,19 +1177,17 @@ int cmd_create_namespace(int argc, const char **argv, void *ctx)
 		memset(&param, 0, sizeof(param));
 		param.type = "blk";
 		set_defaults(ACTION_CREATE);
-		created = do_xaction_namespace(NULL, ACTION_CREATE, ctx);
+		rc = do_xaction_namespace(NULL, ACTION_CREATE, ctx, &created);
 	}
 
-	if (created < 0 || (!namespace && created < 1)) {
+	if (rc < 0 || (!namespace && created < 1)) {
 		fprintf(stderr, "failed to %s namespace: %s\n", namespace
-				? "reconfigure" : "create", strerror(-created));
+				? "reconfigure" : "create", strerror(-rc));
 		if (!namespace)
-			created = -ENODEV;
+			rc = -ENODEV;
 	}
 
-	if (created < 0)
-		return created;
-	return 0;
+	return rc;
 }
 
 int cmd_destroy_namespace(int argc , const char **argv, void *ctx)
@@ -1174,20 +1195,15 @@ int cmd_destroy_namespace(int argc , const char **argv, void *ctx)
 	char *xable_usage = "ndctl destroy-namespace <namespace> [<options>]";
 	const char *namespace = parse_namespace_options(argc, argv,
 			ACTION_DESTROY, destroy_options, xable_usage);
-	int destroyed = do_xaction_namespace(namespace, ACTION_DESTROY, ctx);
+	int destroyed, rc;
 
-	if (destroyed < 0) {
+	rc = do_xaction_namespace(namespace, ACTION_DESTROY, ctx, &destroyed);
+	if (rc < 0)
 		fprintf(stderr, "error destroying namespaces: %s\n",
-				strerror(-destroyed));
-		return destroyed;
-	} else if (destroyed == 0) {
-		fprintf(stderr, "destroyed 0 namespaces\n");
-		return 0;
-	} else {
-		fprintf(stderr, "destroyed %d namespace%s\n", destroyed,
-				destroyed > 1 ? "s" : "");
-		return 0;
-	}
+				strerror(-rc));
+	fprintf(stderr, "destroyed %d namespace%s\n", destroyed,
+			destroyed == 1 ? "" : "s");
+	return rc;
 }
 
 int cmd_check_namespace(int argc , const char **argv, void *ctx)
@@ -1195,19 +1211,13 @@ int cmd_check_namespace(int argc , const char **argv, void *ctx)
 	char *xable_usage = "ndctl check-namespace <namespace> [<options>]";
 	const char *namespace = parse_namespace_options(argc, argv,
 			ACTION_CHECK, check_options, xable_usage);
-	int checked;
+	int checked, rc;
 
-	checked = do_xaction_namespace(namespace, ACTION_CHECK, ctx);
-	if (checked < 0) {
+	rc = do_xaction_namespace(namespace, ACTION_CHECK, ctx, &checked);
+	if (rc < 0)
 		fprintf(stderr, "error checking namespaces: %s\n",
-				strerror(-checked));
-		return checked;
-	} else if (checked == 0) {
-		fprintf(stderr, "checked 0 namespaces\n");
-		return 0;
-	} else {
-		fprintf(stderr, "checked %d namespace%s\n", checked,
-				checked > 1 ? "s" : "");
-		return 0;
-	}
+				strerror(-rc));
+	fprintf(stderr, "checked %d namespace%s\n", checked,
+			checked == 1 ? "" : "s");
+	return rc;
 }
-- 
2.14.4

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