[ndctl PATCH v2 0/3] ndctl: Remove udev rule for latch and dirty-shutdown-count

[ndctl PATCH v2 0/3] ndctl: Remove udev rule for latch and dirty-shutdown-count

[Dan Williams]

Changes since v1:
* Add an error message for the not supported case (Vishal)
* Asciidoctor format fixup

---

The latch needs to be coordinated with writes to the namespace and that
makes it not suitable as a dimm-add-event udev rule.

Additionally, the dirty-shutdown-count is something that can live in
sysfs alongside the other health state flags in /sys/.../nmemX/nfit.
Otherwise, calling any of the libndctl apis from a udev script means the
entire udev queue can get blocked behind a ndctl_bus_wait_probe() call.
That's too much overhead for a non-default policy.

In other words, while the latch event remains in userspace as close as
possible to the application that wants to manage dimm-flush-failed
policy as anything other than fatal, dirty-shutdown-count caching should
move to sysfs where it is cheap to implement and compliments the
flush-failed health state flag.

---

Dan Williams (3):
      ndctl: Introduce dirty-dimm command
      ndctl: Revert "ndctl, intel: Fallback to smart cached shutdown_count"
      ndctl: Revert "ndctl: Create ndctl udev rules for dirty shutdown"


 .gitignore                               |    1 
 Documentation/ndctl/Makefile.am          |    1 
 Documentation/ndctl/ndctl-dirty-dimm.txt |   29 ++++++
 Makefile.am                              |    3 -
 builtin.h                                |    1 
 configure.ac                             |   10 --
 contrib/80-ndctl.rules                   |    3 -
 ndctl.spec.in                            |    3 -
 ndctl/Makefile.am                        |    5 -
 ndctl/dimm.c                             |   31 ++++++
 ndctl/lib/intel.c                        |   41 --------
 ndctl/lib/libndctl.c                     |    6 -
 ndctl/lib/private.h                      |    3 -
 ndctl/ndctl-udev.c                       |  150 ------------------------------
 ndctl/ndctl.c                            |    1 
 15 files changed, 64 insertions(+), 224 deletions(-)
 create mode 100644 Documentation/ndctl/ndctl-dirty-dimm.txt
 delete mode 100644 contrib/80-ndctl.rules
 delete mode 100644 ndctl/ndctl-udev.c
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[ndctl PATCH v2 1/3] ndctl: Introduce dirty-dimm command

[Dan Williams]

Some DIMMs provide a facility to track dirty-shutdown events. The
counter only rolls forward after the OS sets a latch. This allows the
agent tracking dirty shutdowns to ignore events that occur while the
capacity has not been written. For these DIMMs dirty-dimm will trigger
the counter to roll to the next state. The shutdown state can be
retrieved with 'ndctl list -DH'

Cc: Keith Busch <keith.busch@intel.com>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 Documentation/ndctl/Makefile.am          |    1 +
 Documentation/ndctl/ndctl-dirty-dimm.txt |   29 ++++++++++++++++++++++++++++
 builtin.h                                |    1 +
 ndctl/dimm.c                             |   31 ++++++++++++++++++++++++++++++
 ndctl/ndctl.c                            |    1 +
 5 files changed, 63 insertions(+)
 create mode 100644 Documentation/ndctl/ndctl-dirty-dimm.txt

diff --git a/Documentation/ndctl/Makefile.am b/Documentation/ndctl/Makefile.am
index a30b139ba3a3..1a826bb001be 100644
--- a/Documentation/ndctl/Makefile.am
+++ b/Documentation/ndctl/Makefile.am
@@ -38,6 +38,7 @@ man1_MANS = \
 	ndctl-disable-region.1 \
 	ndctl-enable-dimm.1 \
 	ndctl-disable-dimm.1 \
+	ndctl-dirty-dimm.1 \
 	ndctl-enable-namespace.1 \
 	ndctl-disable-namespace.1 \
 	ndctl-create-namespace.1 \
diff --git a/Documentation/ndctl/ndctl-dirty-dimm.txt b/Documentation/ndctl/ndctl-dirty-dimm.txt
new file mode 100644
index 000000000000..02a30fe85f53
--- /dev/null
+++ b/Documentation/ndctl/ndctl-dirty-dimm.txt
@@ -0,0 +1,29 @@
+// SPDX-License-Identifier: GPL-2.0
+
+ndctl-dirty-dimm(1)
+===================
+
+NAME
+----
+ndctl-dirty-dimm - set dimm to record the next dirty shutdown event
+
+SYNOPSIS
+--------
+[verse]
+'ndctl dirty-dimm <nmem0> [<nmem1>..<nmemN>]'
+
+Some NVDIMMs have the capability to detect 'flush failed' events whereby
+data that is pending in buffers at the time of system power loss fail to
+be flushed out to media. Some DIMMs go further to count how many times
+such fatal events occur, but only roll the count in response to a latch
+being set. The 'dirty-dimm' command sets this latch on those devices and
+is meant to be called in advance to any writes to media.
+
+OPTIONS
+-------
+<nmem>::
+include::xable-dimm-options.txt[]
+
+SEE ALSO
+--------
+http://pmem.io/documents/NVDIMM_DSM_Interface-V1.7.pdf[NVDIMM DSM Inteface]
diff --git a/builtin.h b/builtin.h
index 675a6ce79b9c..1157243cdf60 100644
--- a/builtin.h
+++ b/builtin.h
@@ -48,4 +48,5 @@ int cmd_bat(int argc, const char **argv, void *ctx);
 #endif
 int cmd_update_firmware(int argc, const char **argv, void *ctx);
 int cmd_inject_smart(int argc, const char **argv, void *ctx);
+int cmd_dirty_dimm(int argc, const char **argv, void *ctx);
 #endif /* _NDCTL_BUILTIN_H_ */
diff --git a/ndctl/dimm.c b/ndctl/dimm.c
index a4203f354000..32f8cb656d1d 100644
--- a/ndctl/dimm.c
+++ b/ndctl/dimm.c
@@ -61,6 +61,22 @@ static int action_zero(struct ndctl_dimm *dimm, struct action_context *actx)
 	return ndctl_dimm_zero_labels(dimm);
 }
 
+static int action_dirty(struct ndctl_dimm *dimm, struct action_context *actx)
+{
+	struct ndctl_cmd *cmd;
+
+	cmd = ndctl_dimm_cmd_new_ack_shutdown_count(dimm);
+	if (!cmd) {
+		fprintf(stderr, "%s: 'dirty-dimm' not supported\n",
+				ndctl_dimm_get_devname(dimm));
+		return -EOPNOTSUPP;
+	}
+	ndctl_cmd_submit(cmd);
+	ndctl_cmd_unref(cmd);
+
+	return 0;
+}
+
 static struct json_object *dump_label_json(struct ndctl_dimm *dimm,
 		struct ndctl_cmd *cmd_read, ssize_t size)
 {
@@ -943,6 +959,11 @@ static const struct option update_options[] = {
 	OPT_END(),
 };
 
+static const struct option dirty_options[] = {
+	BASE_OPTIONS(),
+	OPT_END(),
+};
+
 static const struct option base_options[] = {
 	BASE_OPTIONS(),
 	OPT_END(),
@@ -1181,3 +1202,13 @@ int cmd_update_firmware(int argc, const char **argv, void *ctx)
 			count > 1 ? "s" : "");
 	return count >= 0 ? 0 : EXIT_FAILURE;
 }
+
+int cmd_dirty_dimm(int argc, const char **argv, void *ctx)
+{
+	int count = dimm_action(argc, argv, ctx, action_dirty, dirty_options,
+			"ndctl dirty-dimm <nmem0> [<nmem1>..<nmemN>] [<options>]");
+
+	fprintf(stderr, "dirtied %d nmem%s.\n", count >= 0 ? count : 0,
+			count > 1 ? "s" : "");
+	return count >= 0 ? 0 : EXIT_FAILURE;
+}
diff --git a/ndctl/ndctl.c b/ndctl/ndctl.c
index 73dabfac3908..93d0d88a274c 100644
--- a/ndctl/ndctl.c
+++ b/ndctl/ndctl.c
@@ -83,6 +83,7 @@ static struct cmd_struct commands[] = {
 	{ "write-labels", cmd_write_labels },
 	{ "init-labels", cmd_init_labels },
 	{ "check-labels", cmd_check_labels },
+	{ "dirty-dimm", cmd_dirty_dimm },
 	{ "inject-error", cmd_inject_error },
 	{ "update-firmware", cmd_update_firmware },
 	{ "inject-smart", cmd_inject_smart },

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

[ndctl PATCH v2 2/3] ndctl: Revert "ndctl, intel: Fallback to smart cached shutdown_count"

[Dan Williams]

Static device attribute data, if it needs to be cached and exported to
unprivileged userspace, should live in sysfs.

Cc: Keith Busch <keith.busch@intel.com>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 ndctl/lib/intel.c    |   41 -----------------------------------------
 ndctl/lib/libndctl.c |    6 +-----
 ndctl/lib/private.h  |    3 ---
 3 files changed, 1 insertion(+), 49 deletions(-)

diff --git a/ndctl/lib/intel.c b/ndctl/lib/intel.c
index 15c5d1a60e34..0abea1e1f5ff 100644
--- a/ndctl/lib/intel.c
+++ b/ndctl/lib/intel.c
@@ -56,46 +56,6 @@ static struct ndctl_cmd *alloc_intel_cmd(struct ndctl_dimm *dimm,
 	return cmd;
 }
 
-/*
- * If provided, read the cached shutdown count in case the user does not have
- * access rights to run the smart command, and pretend the command was
- * successful with only the shutdown_count valid.
- */
-static int intel_smart_handle_error(struct ndctl_cmd *cmd)
-{
-	struct ndctl_dimm *dimm = cmd->dimm;
-	struct ndctl_bus *bus = cmd_to_bus(cmd);
-	struct ndctl_ctx *ctx = ndctl_bus_get_ctx(bus);
-	char *path = NULL, shutdown_count[16] = {};
-	int fd, rc = cmd->status;
-
-	if (!dimm)
-		goto out;
-
-	if (asprintf(&path, DEF_TMPFS_DIR "/%s/usc",
-		     ndctl_dimm_get_devname(dimm)) < 0)
-		goto out;
-
-	fd = open(path, O_RDONLY);
-	if (fd < 0)
-		goto free_path;
-
-	if (read(fd, shutdown_count, sizeof(shutdown_count)) < 0)
-		goto close;
-
-	cmd->intel->smart.flags = ND_INTEL_SMART_SHUTDOWN_COUNT_VALID;
-	cmd->intel->smart.shutdown_count = strtoull(shutdown_count, NULL, 0);
-	rc = cmd->status = 0;
- close:
-	close (fd);
- free_path:
-	free(path);
- out:
-	if (rc)
-		err(ctx, "failed: %s\n", strerror(errno));
-	return rc;
-}
-
 static struct ndctl_cmd *intel_dimm_cmd_new_smart(struct ndctl_dimm *dimm)
 {
 	struct ndctl_cmd *cmd;
@@ -107,7 +67,6 @@ static struct ndctl_cmd *intel_dimm_cmd_new_smart(struct ndctl_dimm *dimm)
 	if (!cmd)
 		return NULL;
 	cmd->firmware_status = &cmd->intel->smart.status;
-	cmd->handle_error = intel_smart_handle_error;
 
 	return cmd;
 }
diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index 481b110d2a18..5bc97f25607c 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -2753,9 +2753,7 @@ NDCTL_EXPORT int ndctl_cmd_submit(struct ndctl_cmd *cmd)
 
 	fd = open(path, O_RDWR);
 	if (fd < 0) {
-		if (!cmd->handle_error)
-			err(ctx, "failed to open %s: %s\n", path,
-				strerror(errno));
+		err(ctx, "failed to open %s: %s\n", path, strerror(errno));
 		rc = -errno;
 		goto out;
 	}
@@ -2771,8 +2769,6 @@ NDCTL_EXPORT int ndctl_cmd_submit(struct ndctl_cmd *cmd)
 	close(fd);
  out:
 	cmd->status = rc;
-	if (rc && cmd->handle_error)
-		rc = cmd->handle_error(cmd);
 	return rc;
 }
 
diff --git a/ndctl/lib/private.h b/ndctl/lib/private.h
index b6e438aff8c7..5ddc682b7514 100644
--- a/ndctl/lib/private.h
+++ b/ndctl/lib/private.h
@@ -227,8 +227,6 @@ struct ndctl_namespace {
  * @firmware_status: NFIT command output status code
  * @iter: iterator for multi-xfer commands
  * @source: source cmd of an inherited iter.total_buf
- * @handle_error: function pointer to handle a cmd error and override it to
- * 		  return alternative data (from a cache for example).
  *
  * For dynamically sized commands like 'get_config', 'set_config', or
  * 'vendor', @size encompasses the entire buffer for the command input
@@ -257,7 +255,6 @@ struct ndctl_cmd {
 		int dir;
 	} iter;
 	struct ndctl_cmd *source;
-	int (*handle_error)(struct ndctl_cmd *cmd);
 	union {
 		struct nd_cmd_ars_cap ars_cap[0];
 		struct nd_cmd_ars_start ars_start[0];

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

[ndctl PATCH v2 3/3] ndctl: Revert "ndctl: Create ndctl udev rules for dirty shutdown"

[Dan Williams]

With the introduction of the 'ndctl dirty-dimm' command,
environments that choose to implement dirty-shutdown mitigation can use
'dirty-dimm nmemX' and 'list --dimm --health nmemX' to acknowledge
and retrieve these details. However, it should be noted that this is not
the default. If the platform advertises support for "Memory Controller
Flush to NVDIMM Durability on Power Loss Capable" then the default Linux
policy is to trust that designation.

A platform owner can override the default Linux policy by delaying
namespace activation until after 'dirty-dimm' has been completed on all
DIMMs that contribute to a given interleave-set. Without coordinating
the latch with writes the dirty-shutdown counter could fail to reflect
the dirty-state relative to failed write-buffer flush.

Cc: Keith Busch <keith.busch@intel.com>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 .gitignore             |    1 
 Makefile.am            |    3 -
 configure.ac           |   10 ---
 contrib/80-ndctl.rules |    3 -
 ndctl.spec.in          |    3 -
 ndctl/Makefile.am      |    5 --
 ndctl/ndctl-udev.c     |  150 ------------------------------------------------
 7 files changed, 175 deletions(-)
 delete mode 100644 contrib/80-ndctl.rules
 delete mode 100644 ndctl/ndctl-udev.c

diff --git a/.gitignore b/.gitignore
index f13a7ef5a608..0baace4b457e 100644
--- a/.gitignore
+++ b/.gitignore
@@ -25,7 +25,6 @@ daxctl/lib/libdaxctl.pc
 *.a
 ndctl/lib/libndctl.pc
 ndctl/ndctl
-ndctl/ndctl-udev
 rhel/
 sles/ndctl.spec
 util/log.lo
diff --git a/Makefile.am b/Makefile.am
index 7880eef1c890..e0c463a3493a 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -42,9 +42,6 @@ bashcompletiondir = $(BASH_COMPLETION_DIR)
 dist_bashcompletion_DATA = contrib/ndctl
 endif
 
-udevrulesdir = $(UDEVDIR)/rules.d
-dist_udevrules_DATA = contrib/80-ndctl.rules
-
 noinst_LIBRARIES = libccan.a
 libccan_a_SOURCES = \
 	ccan/str/str.h \
diff --git a/configure.ac b/configure.ac
index 9dc11da194d1..bb6b03324ea8 100644
--- a/configure.ac
+++ b/configure.ac
@@ -153,17 +153,8 @@ fi
 AC_SUBST([systemd_unitdir])
 AM_CONDITIONAL([ENABLE_SYSTEMD_UNITS], [test "x$with_systemd" = "xyes"])
 
-AC_ARG_WITH([tmpfilesdir],
-	[AS_HELP_STRING([--with-tmpfilesdir=DIR], [Directory for temporary runtime files])],
-	[tmpfilesdir=$withval],
-	[tmpfilesdir="/run"])
-
-UDEVDIR="$(pkg-config udev --variable=udevdir)"
-AC_SUBST([UDEVDIR])
-
 my_CFLAGS="\
 -D DEF_CONF_FILE='\"${sysconfdir}/ndctl/monitor.conf\"' \
--D DEF_TMPFS_DIR='\"${tmpfilesdir}/ndctl\"' \
 -Wall \
 -Wchar-subscripts \
 -Wformat-security \
@@ -204,7 +195,6 @@ AC_MSG_RESULT([
         sysconfdir:             ${sysconfdir}
         libdir:                 ${libdir}
         includedir:             ${includedir}
-        tmpfilesdir:            ${tmpfilesdir}
 
         compiler:               ${CC}
         cflags:                 ${CFLAGS}
diff --git a/contrib/80-ndctl.rules b/contrib/80-ndctl.rules
deleted file mode 100644
index 54788c40ea9d..000000000000
--- a/contrib/80-ndctl.rules
+++ /dev/null
@@ -1,3 +0,0 @@
-# do not edit this file, it will be overwritten on update
-
-ACTION=="add", KERNEL=="nmem*", RUN+="ndctl-udev $kernel"
diff --git a/ndctl.spec.in b/ndctl.spec.in
index 98394a924030..26396d4abad7 100644
--- a/ndctl.spec.in
+++ b/ndctl.spec.in
@@ -110,7 +110,6 @@ make check
 %postun -n DAX_LNAME -p /sbin/ldconfig
 
 %define bashcompdir %(pkg-config --variable=completionsdir bash-completion)
-%define udevdir  %(pkg-config --variable=udevdir udev)
 
 %files
 %defattr(-,root,root)
@@ -120,8 +119,6 @@ make check
 %{bashcompdir}/
 %{_sysconfdir}/ndctl/monitor.conf
 %{_unitdir}/ndctl-monitor.service
-%{_udevrulesdir}/80-ndctl.rules
-%{udevdir}/ndctl-udev
 
 %files -n daxctl
 %defattr(-,root,root)
diff --git a/ndctl/Makefile.am b/ndctl/Makefile.am
index d6839e4a9fb6..8a5e5f87e6c5 100644
--- a/ndctl/Makefile.am
+++ b/ndctl/Makefile.am
@@ -51,8 +51,3 @@ EXTRA_DIST += $(monitor_config_file)
 if ENABLE_SYSTEMD_UNITS
 systemd_unit_DATA = ndctl-monitor.service
 endif
-
-ndctl_udevdir = $(UDEVDIR)
-ndctl_udev_PROGRAMS = ndctl-udev
-ndctl_udev_SOURCES = ndctl-udev.c
-ndctl_udev_LDADD = lib/libndctl.la
diff --git a/ndctl/ndctl-udev.c b/ndctl/ndctl-udev.c
deleted file mode 100644
index 7b174b61d8cd..000000000000
--- a/ndctl/ndctl-udev.c
+++ /dev/null
@@ -1,150 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-/* Copyright(c) 2018 Intel Corporation. All rights reserved. */
-
-#include <ctype.h>
-#include <errno.h>
-#include <fcntl.h>
-#include <stddef.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
-#include <sys/stat.h>
-#include <sys/types.h>
-#include <ndctl/libndctl.h>
-
-/**
- * mkdir_p
- *
- * Copied from util-linux lib/fileutils.c
- */
-static int mkdir_p(const char *path, mode_t mode)
-{
-	char *p, *dir;
-	int rc = 0;
-
-	if (!path || !*path)
-		return -EINVAL;
-
-	dir = p = strdup(path);
-	if (!dir)
-		return -ENOMEM;
-
-	if (*p == '/')
-		p++;
-
-	while (p && *p) {
-		char *e = strchr(p, '/');
-		if (e)
-			*e = '\0';
-		if (*p) {
-			rc = mkdir(dir, mode);
-			if (rc && errno != EEXIST)
-				break;
-			rc = 0;
-		}
-		if (!e)
-			break;
-		*e = '/';
-		p = e + 1;
-	}
-
-	free(dir);
-	return rc;
-}
-
-static struct ndctl_dimm *find_dimm(struct ndctl_ctx *ctx, const char *devname)
-{
-	struct ndctl_bus *bus;
-	struct ndctl_dimm *dimm;
-
-	ndctl_bus_foreach(ctx, bus) {
-		ndctl_dimm_foreach(bus, dimm) {
-			if (strcmp(ndctl_dimm_get_devname(dimm), devname) == 0)
-				return dimm;
-		}
-	}
-	return NULL;
-}
-
-static void ack_shutdown(struct ndctl_dimm *dimm)
-{
-	struct ndctl_cmd *cmd;
-
-	cmd = ndctl_dimm_cmd_new_ack_shutdown_count(dimm);
-	if (!cmd)
-		return;
-	ndctl_cmd_submit(cmd);
-	ndctl_cmd_unref(cmd);
-}
-
-static void save_unsafe_shutdown_count(struct ndctl_dimm *dimm,
-				       const char *devname)
-{
-	char *path, *usc, count[16];
-	unsigned int shutdown;
-	struct ndctl_cmd *cmd;
-	int fd;
-
-	cmd = ndctl_dimm_cmd_new_smart(dimm);
-	if (!cmd)
-		return;
-
-	if (ndctl_cmd_submit(cmd))
-		goto unref_cmd;
-
-	shutdown = ndctl_cmd_smart_get_shutdown_count(cmd);
-	if (shutdown == UINT_MAX)
-		goto unref_cmd;
-
-	if (asprintf(&path, DEF_TMPFS_DIR "/%s", devname) < 0)
-		goto unref_cmd;
-
-	if (mkdir_p(path, 0755))
-		goto free_path;
-
-	if (asprintf(&usc, "%s/usc", path) < 0)
-		goto free_path;
-
-	fd = open(usc, O_WRONLY | O_CREAT, 0644);
-	if (fd < 0)
-		goto free_usc;
-
-	if (snprintf(count, sizeof(count), "%u\n", shutdown) < 0)
-		goto close_fd;
-
-	if (write(fd, count, strlen(count)) < 0)
-		goto close_fd;
-
- close_fd:
-	close(fd);
- free_usc:
-	free(usc);
- free_path:
-	free(path);
- unref_cmd:
-	ndctl_cmd_unref(cmd);
-}
-
-int main(int argc, char *argv[])
-{
-	struct ndctl_ctx *ctx;
-	struct ndctl_dimm *dimm = NULL;
-	const char *devname;
-
-	if (argc < 2)
-		return EINVAL;
-
-	devname = argv[1];
-	if (ndctl_new(&ctx))
-		return ENOMEM;
-
-	dimm = find_dimm(ctx, devname);
-	if (!dimm)
-		return ENODEV;
-
-	ack_shutdown(dimm);
-	save_unsafe_shutdown_count(dimm, devname);
-
-	ndctl_unref(ctx);
-	return 0;
-}

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