[PATCH 1/2] Create Intel pmem shutdown latch rule

[PATCH 1/2] Create Intel pmem shutdown latch rule

[Keith Busch]

This patch provides a udev rule and the program it runs for Intel
nvdimms. The program the rule executes runs two commands to allow
applications to manage unsafe shutdowns.

The first command enables the shutdown latch. Without this, the last
shutdown status will remain fixed to the status set when the latch was
last enabled.

The second command retrieves the unclean shutdown counts and saves it
in a known location. Only root can access the health's shutdown count,
so we have to stash it somewhere accessible to non-privileged users. A
successful execution of the rule will write USC to the run time tmpfs
location. By default, the location will be set to:

  /run/ndctl/<dimm-id>/usc

A distro may change this location using the '--with-tmpfsdir=[DIR]'.

Reading the file will report the count observed when the dimm was
added.

Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 .gitignore                  |   1 +
 Makefile.am                 |   3 +
 configure.ac                |  10 ++++
 contrib/80-intel-pmem.rules |   1 +
 ndctl.spec.in               |   5 +-
 ndctl/Makefile.am           |   5 ++
 ndctl/latch-shutdown.c      | 140 ++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 164 insertions(+), 1 deletion(-)
 create mode 100644 contrib/80-intel-pmem.rules
 create mode 100644 ndctl/latch-shutdown.c

diff --git a/.gitignore b/.gitignore
index 1016b3b..1601738 100644
--- a/.gitignore
+++ b/.gitignore
@@ -25,6 +25,7 @@ daxctl/lib/libdaxctl.pc
 *.a
 ndctl/lib/libndctl.pc
 ndctl/ndctl
+ndctl/latch-shutdown
 rhel/
 sles/ndctl.spec
 util/log.lo
diff --git a/Makefile.am b/Makefile.am
index e0c463a..9d3913e 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -42,6 +42,9 @@ bashcompletiondir = $(BASH_COMPLETION_DIR)
 dist_bashcompletion_DATA = contrib/ndctl
 endif
 
+udevrulesdir = $(UDEVDIR)/rules.d
+dist_udevrules_DATA = contrib/80-intel-pmem.rules
+
 noinst_LIBRARIES = libccan.a
 libccan_a_SOURCES = \
 	ccan/str/str.h \
diff --git a/configure.ac b/configure.ac
index cf44260..15f336b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -143,7 +143,16 @@ AC_CHECK_FUNCS([ \
 	secure_getenv\
 ])
 
+AC_ARG_WITH([tmpfsdir],
+  [AS_HELP_STRING([--with-tmpfsdir=DIR], [Directory for temporary runtime files])],
+  [tmpfsdir=$withval],
+  [tmpfsdir="/run"])
+
+UDEVDIR="$(pkg-config udev --variable=udevdir)"
+AC_SUBST([UDEVDIR])
+
 my_CFLAGS="\
+-D DEF_TMPFS_DIR='\"${tmpfsdir}/ndctl\"' \
 -Wall \
 -Wchar-subscripts \
 -Wformat-security \
@@ -182,6 +191,7 @@ AC_MSG_RESULT([
 
         prefix:                 ${prefix}
         sysconfdir:             ${sysconfdir}
+        tmpfsdir:               ${tmpfsdir}
         libdir:                 ${libdir}
         includedir:             ${includedir}
 
diff --git a/contrib/80-intel-pmem.rules b/contrib/80-intel-pmem.rules
new file mode 100644
index 0000000..20fcef9
--- /dev/null
+++ b/contrib/80-intel-pmem.rules
@@ -0,0 +1 @@
+ACTION=="add", KERNEL=="nmem*", RUN+="latch-shutdown $kernel"
diff --git a/ndctl.spec.in b/ndctl.spec.in
index e2c879c..25edf7e 100644
--- a/ndctl.spec.in
+++ b/ndctl.spec.in
@@ -90,7 +90,7 @@ control API for these devices.
 %build
 echo %{version} > version
 ./autogen.sh
-%configure --disable-static --disable-silent-rules
+%configure --disable-static --disable-silent-rules --with-tmpfsdir=%{_tmpfilesdir}
 make %{?_smp_mflags}
 
 %install
@@ -109,6 +109,7 @@ 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)
@@ -116,6 +117,8 @@ make check
 %{_bindir}/ndctl
 %{_mandir}/man1/ndctl*
 %{bashcompdir}/
+%{_udevrulesdir}/80-intel-pmem.rules
+%{udevdir}/latch-shutdown
 
 %files -n daxctl
 %defattr(-,root,root)
diff --git a/ndctl/Makefile.am b/ndctl/Makefile.am
index 0f56871..155179b 100644
--- a/ndctl/Makefile.am
+++ b/ndctl/Makefile.am
@@ -41,3 +41,8 @@ ndctl_SOURCES += ../test/libndctl.c \
 		 ../test/core.c \
 		 test.c
 endif
+
+latch_shutdowndir = $(UDEVDIR)
+latch_shutdown_PROGRAMS = latch-shutdown
+latch_shutdown_SOURCES = latch-shutdown.c
+latch_shutdown_LDADD = lib/libndctl.la
diff --git a/ndctl/latch-shutdown.c b/ndctl/latch-shutdown.c
new file mode 100644
index 0000000..704fb8f
--- /dev/null
+++ b/ndctl/latch-shutdown.c
@@ -0,0 +1,140 @@
+/* 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, 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;
+}
+
+int main(int argc, char *argv[])
+{
+	struct ndctl_ctx *ctx;
+	struct ndctl_cmd *cmd;
+	struct ndctl_dimm *dimm = NULL;
+	char *path, *usc, count[16];
+	const char *id;
+	unsigned int shutdown;
+	int rc, fd;
+
+	if (argc < 2)
+		return EINVAL;
+
+	rc = ndctl_new(&ctx);
+	if (rc)
+		return ENOMEM;
+
+	dimm = find_dimm(ctx, argv[1]);
+	if (!dimm)
+		return ENODEV;
+
+	cmd = ndctl_dimm_cmd_new_ack_shutdown_count(dimm);
+	if (!cmd)
+		return ENOMEM;
+
+	rc = ndctl_cmd_submit(cmd);
+	if (rc)
+		return rc;
+	ndctl_cmd_unref(cmd);
+
+	id = ndctl_dimm_get_unique_id(dimm);
+	if (!id)
+		return ENXIO;
+
+	cmd = ndctl_dimm_cmd_new_smart(dimm);
+	if (!cmd)
+		return ENOMEM;
+
+	rc = ndctl_cmd_submit(cmd);
+	if (rc)
+		return rc;
+
+	shutdown = ndctl_cmd_smart_get_shutdown_count(cmd);
+	ndctl_cmd_unref(cmd);
+
+	rc = asprintf(&path, DEF_TMPFS_DIR "/%s", id);
+	if (rc < 0)
+		return ENOMEM;
+	ndctl_unref(ctx);
+
+	rc = mkdir_p(path, 0755);
+	if (rc)
+		return rc;
+
+	rc = asprintf(&usc, "%s/usc", path);
+	if (rc < 0)
+		return ENOMEM;
+	free(path);
+
+	fd = open(usc, O_WRONLY | O_CREAT, 0644);
+	if (!fd)
+		return errno;
+	free(usc);
+
+	rc = snprintf(count, sizeof(count), "%u\n", shutdown);
+	if (rc < 0)
+		return rc;
+
+	if (write(fd, count, strlen(count)))
+		return errno;
+	return 0;
+}
-- 
2.14.4

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

[PATCH 2/2] ndctl, intel: Fallback to smart cached shutdown_count

[Keith Busch]

A user space rule saves the unsafe shutdown count to a fixed location
for Intel dimms so that non-root users can read it. If the smart command
submission fails, this patch uses that saved value in a newly added
ndctl_cmd "handle_error" callback.  If the cached value is available,
the error handler will override the command status to success, and set
the valid flags for the shutdown count.

Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 ndctl/lib/intel.c    | 34 +++++++++++++++++++++++++++++++++-
 ndctl/lib/libndctl.c |  2 ++
 ndctl/lib/private.h  |  1 +
 3 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/ndctl/lib/intel.c b/ndctl/lib/intel.c
index 0abea1e..83f8203 100644
--- a/ndctl/lib/intel.c
+++ b/ndctl/lib/intel.c
@@ -56,6 +56,38 @@ 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;
+	char *path = NULL, shutdown_count[16] = {};
+	int fd, rc = cmd->status;
+
+	if (asprintf(&path, DEF_TMPFS_DIR "/%s/usc",
+		     ndctl_dimm_get_unique_id(dimm)) < 0)
+		return rc;
+
+	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);
+	return rc;
+}
+
 static struct ndctl_cmd *intel_dimm_cmd_new_smart(struct ndctl_dimm *dimm)
 {
 	struct ndctl_cmd *cmd;
@@ -67,7 +99,7 @@ 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 47e005e..d94321e 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -2651,6 +2651,8 @@ 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 a94f894..5812c85 100644
--- a/ndctl/lib/private.h
+++ b/ndctl/lib/private.h
@@ -255,6 +255,7 @@ 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];
-- 
2.14.4

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

Re: [PATCH 1/2] Create Intel pmem shutdown latch rule

[Verma, Vishal L]

On Tue, 2018-08-07 at 16:26 -0600, Keith Busch wrote:
> This patch provides a udev rule and the program it runs for Intel

Like we talked about, I think this should work for all NVDIMMs (at
least any that use the generic DIMM ops APIs that libndctl provides),
so we can remove any Intel specificity here.

> nvdimms. The program the rule executes runs two commands to allow
> applications to manage unsafe shutdowns.
> 
> The first command enables the shutdown latch. Without this, the last
> shutdown status will remain fixed to the status set when the latch was
> last enabled.
> 
> The second command retrieves the unclean shutdown counts and saves it
> in a known location. Only root can access the health's shutdown count,
> so we have to stash it somewhere accessible to non-privileged users. A
> successful execution of the rule will write USC to the run time tmpfs
> location. By default, the location will be set to:
> 
>   /run/ndctl/<dimm-id>/usc
> 
> A distro may change this location using the '--with-tmpfsdir=[DIR]'.

For consistency with the rpm macro, I'd suggest changing this to
--with-tmpfilesdir=

> 
> Reading the file will report the count observed when the dimm was
> added.
> 
> Signed-off-by: Keith Busch <keith.busch@intel.com>
> ---
>  .gitignore                  |   1 +
>  Makefile.am                 |   3 +
>  configure.ac                |  10 ++++
>  contrib/80-intel-pmem.rules |   1 +
>  ndctl.spec.in               |   5 +-
>  ndctl/Makefile.am           |   5 ++
>  ndctl/latch-shutdown.c      | 140 ++++++++++++++++++++++++++++++++++++++++++++

I think we can also rename this file to something more generic like
"ndctl-udev.c" so that any future udev actions can go here. We are
already doing two things in it, latching and saving the USCs.

>  7 files changed, 164 insertions(+), 1 deletion(-)
>  create mode 100644 contrib/80-intel-pmem.rules
>  create mode 100644 ndctl/latch-shutdown.c
> 
> diff --git a/.gitignore b/.gitignore
> index 1016b3b..1601738 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -25,6 +25,7 @@ daxctl/lib/libdaxctl.pc
>  *.a
>  ndctl/lib/libndctl.pc
>  ndctl/ndctl
> +ndctl/latch-shutdown
>  rhel/
>  sles/ndctl.spec
>  util/log.lo
> diff --git a/Makefile.am b/Makefile.am
> index e0c463a..9d3913e 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -42,6 +42,9 @@ bashcompletiondir = $(BASH_COMPLETION_DIR)
>  dist_bashcompletion_DATA = contrib/ndctl
>  endif
>  
> +udevrulesdir = $(UDEVDIR)/rules.d
> +dist_udevrules_DATA = contrib/80-intel-pmem.rules
> +
>  noinst_LIBRARIES = libccan.a
>  libccan_a_SOURCES = \
>  	ccan/str/str.h \
> diff --git a/configure.ac b/configure.ac
> index cf44260..15f336b 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -143,7 +143,16 @@ AC_CHECK_FUNCS([ \
>  	secure_getenv\
>  ])
>  
> +AC_ARG_WITH([tmpfsdir],
> +  [AS_HELP_STRING([--with-tmpfsdir=DIR], [Directory for temporary runtime files])],
> +  [tmpfsdir=$withval],
> +  [tmpfsdir="/run"])
> +
> +UDEVDIR="$(pkg-config udev --variable=udevdir)"
> +AC_SUBST([UDEVDIR])
> +
>  my_CFLAGS="\
> +-D DEF_TMPFS_DIR='\"${tmpfsdir}/ndctl\"' \
>  -Wall \
>  -Wchar-subscripts \
>  -Wformat-security \
> @@ -182,6 +191,7 @@ AC_MSG_RESULT([
>  
>          prefix:                 ${prefix}
>          sysconfdir:             ${sysconfdir}
> +        tmpfsdir:               ${tmpfsdir}
>          libdir:                 ${libdir}
>          includedir:             ${includedir}
>  
> diff --git a/contrib/80-intel-pmem.rules b/contrib/80-intel-pmem.rules
> new file mode 100644
> index 0000000..20fcef9
> --- /dev/null
> +++ b/contrib/80-intel-pmem.rules
> @@ -0,0 +1 @@
> +ACTION=="add", KERNEL=="nmem*", RUN+="latch-shutdown $kernel"
> diff --git a/ndctl.spec.in b/ndctl.spec.in
> index e2c879c..25edf7e 100644
> --- a/ndctl.spec.in
> +++ b/ndctl.spec.in
> @@ -90,7 +90,7 @@ control API for these devices.
>  %build
>  echo %{version} > version
>  ./autogen.sh
> -%configure --disable-static --disable-silent-rules
> +%configure --disable-static --disable-silent-rules --with-tmpfsdir=%{_tmpfilesdir}
>  make %{?_smp_mflags}
>  
>  %install
> @@ -109,6 +109,7 @@ 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)
> @@ -116,6 +117,8 @@ make check
>  %{_bindir}/ndctl
>  %{_mandir}/man1/ndctl*
>  %{bashcompdir}/
> +%{_udevrulesdir}/80-intel-pmem.rules
> +%{udevdir}/latch-shutdown
>  
>  %files -n daxctl
>  %defattr(-,root,root)
> diff --git a/ndctl/Makefile.am b/ndctl/Makefile.am
> index 0f56871..155179b 100644
> --- a/ndctl/Makefile.am
> +++ b/ndctl/Makefile.am
> @@ -41,3 +41,8 @@ ndctl_SOURCES += ../test/libndctl.c \
>  		 ../test/core.c \
>  		 test.c
>  endif
> +
> +latch_shutdowndir = $(UDEVDIR)
> +latch_shutdown_PROGRAMS = latch-shutdown
> +latch_shutdown_SOURCES = latch-shutdown.c
> +latch_shutdown_LDADD = lib/libndctl.la
> diff --git a/ndctl/latch-shutdown.c b/ndctl/latch-shutdown.c
> new file mode 100644
> index 0000000..704fb8f
> --- /dev/null
> +++ b/ndctl/latch-shutdown.c
> @@ -0,0 +1,140 @@
> +/* 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, 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;
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	struct ndctl_ctx *ctx;
> +	struct ndctl_cmd *cmd;
> +	struct ndctl_dimm *dimm = NULL;
> +	char *path, *usc, count[16];
> +	const char *id;
> +	unsigned int shutdown;
> +	int rc, fd;
> +
> +	if (argc < 2)
> +		return EINVAL;
> +
> +	rc = ndctl_new(&ctx);
> +	if (rc)
> +		return ENOMEM;
> +
> +	dimm = find_dimm(ctx, argv[1]);
> +	if (!dimm)
> +		return ENODEV;
> +
> +	cmd = ndctl_dimm_cmd_new_ack_shutdown_count(dimm);
> +	if (!cmd)
> +		return ENOMEM;
> +
> +	rc = ndctl_cmd_submit(cmd);
> +	if (rc)
> +		return rc;
> +	ndctl_cmd_unref(cmd);

I think we'd need to break any dependency between the two functions
here, latching and reading the USCs. We should read the USCs
irrespective of whether the latching succeeded and the other way round.

Let's refactor this a bit, splitting off each 'operation' into a
function, and then main just calls all the functions independently.
This should make any future additions cleaner.

> +
> +	id = ndctl_dimm_get_unique_id(dimm);
> +	if (!id)
> +		return ENXIO;

Let's just use the nmemX style name here. ndctl and sysfs both always
refer to dimms like that, so this can be the same.

> +
> +	cmd = ndctl_dimm_cmd_new_smart(dimm);
> +	if (!cmd)
> +		return ENOMEM;
> +
> +	rc = ndctl_cmd_submit(cmd);
> +	if (rc)
> +		return rc;
> +
> +	shutdown = ndctl_cmd_smart_get_shutdown_count(cmd);
> +	ndctl_cmd_unref(cmd);
> +
> +	rc = asprintf(&path, DEF_TMPFS_DIR "/%s", id);
> +	if (rc < 0)
> +		return ENOMEM;
> +	ndctl_unref(ctx);
> +
> +	rc = mkdir_p(path, 0755);
> +	if (rc)
> +		return rc;
> +
> +	rc = asprintf(&usc, "%s/usc", path);
> +	if (rc < 0)
> +		return ENOMEM;
> +	free(path);
> +
> +	fd = open(usc, O_WRONLY | O_CREAT, 0644);
> +	if (!fd)
> +		return errno;
> +	free(usc);
> +
> +	rc = snprintf(count, sizeof(count), "%u\n", shutdown);
> +	if (rc < 0)
> +		return rc;
> +
> +	if (write(fd, count, strlen(count)))
> +		return errno;
> +	return 0;
> +}
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH 2/2] ndctl, intel: Fallback to smart cached shutdown_count

[Verma, Vishal L]

On Tue, 2018-08-07 at 16:26 -0600, Keith Busch wrote:
> A user space rule saves the unsafe shutdown count to a fixed location
> for Intel dimms so that non-root users can read it. If the smart command
> submission fails, this patch uses that saved value in a newly added
> ndctl_cmd "handle_error" callback.  If the cached value is available,
> the error handler will override the command status to success, and set
> the valid flags for the shutdown count.
> 
> Signed-off-by: Keith Busch <keith.busch@intel.com>
> ---
>  ndctl/lib/intel.c    | 34 +++++++++++++++++++++++++++++++++-
>  ndctl/lib/libndctl.c |  2 ++
>  ndctl/lib/private.h  |  1 +
>  3 files changed, 36 insertions(+), 1 deletion(-)

Looks good to me!

> 
> diff --git a/ndctl/lib/intel.c b/ndctl/lib/intel.c
> index 0abea1e..83f8203 100644
> --- a/ndctl/lib/intel.c
> +++ b/ndctl/lib/intel.c
> @@ -56,6 +56,38 @@ 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;
> +	char *path = NULL, shutdown_count[16] = {};
> +	int fd, rc = cmd->status;
> +
> +	if (asprintf(&path, DEF_TMPFS_DIR "/%s/usc",
> +		     ndctl_dimm_get_unique_id(dimm)) < 0)
> +		return rc;
> +
> +	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);
> +	return rc;
> +}
> +
>  static struct ndctl_cmd *intel_dimm_cmd_new_smart(struct ndctl_dimm *dimm)
>  {
>  	struct ndctl_cmd *cmd;
> @@ -67,7 +99,7 @@ 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 47e005e..d94321e 100644
> --- a/ndctl/lib/libndctl.c
> +++ b/ndctl/lib/libndctl.c
> @@ -2651,6 +2651,8 @@ 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 a94f894..5812c85 100644
> --- a/ndctl/lib/private.h
> +++ b/ndctl/lib/private.h
> @@ -255,6 +255,7 @@ 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

Re: [PATCH 1/2] Create Intel pmem shutdown latch rule

[Verma, Vishal L]

On Tue, 2018-08-07 at 16:26 -0600, Keith Busch wrote:
> This patch provides a udev rule and the program it runs for Intel
> nvdimms. The program the rule executes runs two commands to allow
> applications to manage unsafe shutdowns.
> 
> The first command enables the shutdown latch. Without this, the last
> shutdown status will remain fixed to the status set when the latch was
> last enabled.
> 
> The second command retrieves the unclean shutdown counts and saves it
> in a known location. Only root can access the health's shutdown count,
> so we have to stash it somewhere accessible to non-privileged users. A
> successful execution of the rule will write USC to the run time tmpfs
> location. By default, the location will be set to:
> 
>   /run/ndctl/<dimm-id>/usc
> 
> A distro may change this location using the '--with-tmpfsdir=[DIR]'.
> 
> Reading the file will report the count observed when the dimm was
> added.
> 
> Signed-off-by: Keith Busch <keith.busch@intel.com>
> ---
>  .gitignore                  |   1 +
>  Makefile.am                 |   3 +
>  configure.ac                |  10 ++++
>  contrib/80-intel-pmem.rules |   1 +
>  ndctl.spec.in               |   5 +-
>  ndctl/Makefile.am           |   5 ++
>  ndctl/latch-shutdown.c      | 140 ++++++++++++++++++++++++++++++++++++++++++++
>  7 files changed, 164 insertions(+), 1 deletion(-)
>  create mode 100644 contrib/80-intel-pmem.rules
>  create mode 100644 ndctl/latch-shutdown.c
> 
> diff --git a/.gitignore b/.gitignore
> index 1016b3b..1601738 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -25,6 +25,7 @@ daxctl/lib/libdaxctl.pc
>  *.a
>  ndctl/lib/libndctl.pc
>  ndctl/ndctl
> +ndctl/latch-shutdown
>  rhel/
>  sles/ndctl.spec
>  util/log.lo
> diff --git a/Makefile.am b/Makefile.am
> index e0c463a..9d3913e 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -42,6 +42,9 @@ bashcompletiondir = $(BASH_COMPLETION_DIR)
>  dist_bashcompletion_DATA = contrib/ndctl
>  endif
>  
> +udevrulesdir = $(UDEVDIR)/rules.d
> +dist_udevrules_DATA = contrib/80-intel-pmem.rules
> +
>  noinst_LIBRARIES = libccan.a
>  libccan_a_SOURCES = \
>  	ccan/str/str.h \
> diff --git a/configure.ac b/configure.ac
> index cf44260..15f336b 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -143,7 +143,16 @@ AC_CHECK_FUNCS([ \
>  	secure_getenv\
>  ])
>  
> +AC_ARG_WITH([tmpfsdir],
> +  [AS_HELP_STRING([--with-tmpfsdir=DIR], [Directory for temporary runtime files])],
> +  [tmpfsdir=$withval],
> +  [tmpfsdir="/run"])
> +
> +UDEVDIR="$(pkg-config udev --variable=udevdir)"
> +AC_SUBST([UDEVDIR])
> +
>  my_CFLAGS="\
> +-D DEF_TMPFS_DIR='\"${tmpfsdir}/ndctl\"' \
>  -Wall \
>  -Wchar-subscripts \
>  -Wformat-security \
> @@ -182,6 +191,7 @@ AC_MSG_RESULT([
>  
>          prefix:                 ${prefix}
>          sysconfdir:             ${sysconfdir}
> +        tmpfsdir:               ${tmpfsdir}
>          libdir:                 ${libdir}
>          includedir:             ${includedir}
>  
> diff --git a/contrib/80-intel-pmem.rules b/contrib/80-intel-pmem.rules
> new file mode 100644
> index 0000000..20fcef9
> --- /dev/null
> +++ b/contrib/80-intel-pmem.rules
> @@ -0,0 +1 @@
> +ACTION=="add", KERNEL=="nmem*", RUN+="latch-shutdown $kernel"
> diff --git a/ndctl.spec.in b/ndctl.spec.in
> index e2c879c..25edf7e 100644
> --- a/ndctl.spec.in
> +++ b/ndctl.spec.in
> @@ -90,7 +90,7 @@ control API for these devices.
>  %build
>  echo %{version} > version
>  ./autogen.sh
> -%configure --disable-static --disable-silent-rules
> +%configure --disable-static --disable-silent-rules --with-tmpfsdir=%{_tmpfilesdir}

One more thing - looks like tmpfilesdir expands to:

$ rpm --eval "%{_tmpfilesdir}"
/usr/lib/tmpfiles.d

And using it requires a specific config file as described in the
following places:
https://fedoraproject.org/wiki/Packaging:Tmpfiles.d
http://0pointer.de/public/systemd-man/tmpfiles.d.html

So for this, I think we can simply drop the

	--with-tmpfsdir=%{_tmpfilesdir}

to configure above, and let autoconf use its default. If a distro
wanted to change that location, they still have the option of doing it
be passing something else to configure.

>  make %{?_smp_mflags}
>  
>  %install
> @@ -109,6 +109,7 @@ 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)
> @@ -116,6 +117,8 @@ make check
>  %{_bindir}/ndctl
>  %{_mandir}/man1/ndctl*
>  %{bashcompdir}/
> +%{_udevrulesdir}/80-intel-pmem.rules
> +%{udevdir}/latch-shutdown
>  
>  %files -n daxctl
>  %defattr(-,root,root)
> diff --git a/ndctl/Makefile.am b/ndctl/Makefile.am
> index 0f56871..155179b 100644
> --- a/ndctl/Makefile.am
> +++ b/ndctl/Makefile.am
> @@ -41,3 +41,8 @@ ndctl_SOURCES += ../test/libndctl.c \
>  		 ../test/core.c \
>  		 test.c
>  endif
> +
> +latch_shutdowndir = $(UDEVDIR)
> +latch_shutdown_PROGRAMS = latch-shutdown
> +latch_shutdown_SOURCES = latch-shutdown.c
> +latch_shutdown_LDADD = lib/libndctl.la
> diff --git a/ndctl/latch-shutdown.c b/ndctl/latch-shutdown.c
> new file mode 100644
> index 0000000..704fb8f
> --- /dev/null
> +++ b/ndctl/latch-shutdown.c
> @@ -0,0 +1,140 @@
> +/* 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, 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;
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	struct ndctl_ctx *ctx;
> +	struct ndctl_cmd *cmd;
> +	struct ndctl_dimm *dimm = NULL;
> +	char *path, *usc, count[16];
> +	const char *id;
> +	unsigned int shutdown;
> +	int rc, fd;
> +
> +	if (argc < 2)
> +		return EINVAL;
> +
> +	rc = ndctl_new(&ctx);
> +	if (rc)
> +		return ENOMEM;
> +
> +	dimm = find_dimm(ctx, argv[1]);
> +	if (!dimm)
> +		return ENODEV;
> +
> +	cmd = ndctl_dimm_cmd_new_ack_shutdown_count(dimm);
> +	if (!cmd)
> +		return ENOMEM;
> +
> +	rc = ndctl_cmd_submit(cmd);
> +	if (rc)
> +		return rc;
> +	ndctl_cmd_unref(cmd);
> +
> +	id = ndctl_dimm_get_unique_id(dimm);
> +	if (!id)
> +		return ENXIO;
> +
> +	cmd = ndctl_dimm_cmd_new_smart(dimm);
> +	if (!cmd)
> +		return ENOMEM;
> +
> +	rc = ndctl_cmd_submit(cmd);
> +	if (rc)
> +		return rc;
> +
> +	shutdown = ndctl_cmd_smart_get_shutdown_count(cmd);
> +	ndctl_cmd_unref(cmd);
> +
> +	rc = asprintf(&path, DEF_TMPFS_DIR "/%s", id);
> +	if (rc < 0)
> +		return ENOMEM;
> +	ndctl_unref(ctx);
> +
> +	rc = mkdir_p(path, 0755);
> +	if (rc)
> +		return rc;
> +
> +	rc = asprintf(&usc, "%s/usc", path);
> +	if (rc < 0)
> +		return ENOMEM;
> +	free(path);
> +
> +	fd = open(usc, O_WRONLY | O_CREAT, 0644);
> +	if (!fd)
> +		return errno;
> +	free(usc);
> +
> +	rc = snprintf(count, sizeof(count), "%u\n", shutdown);
> +	if (rc < 0)
> +		return rc;
> +
> +	if (write(fd, count, strlen(count)))
> +		return errno;
> +	return 0;
> +}
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm