nvdimm.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
* [ndctl PATCH 0/7] Policy based reconfiguration for daxctl
@ 2021-08-31  9:04 Vishal Verma
  2021-08-31  9:04 ` [ndctl PATCH 1/7] ndctl: Update ndctl.spec.in for 'ndctl.conf' Vishal Verma
                   ` (7 more replies)
  0 siblings, 8 replies; 20+ messages in thread
From: Vishal Verma @ 2021-08-31  9:04 UTC (permalink / raw)
  To: nvdimm; +Cc: Dan Williams, QI Fuli, fenghua.hu, Vishal Verma

These patches add policy (config file) support to daxctl. The
introductory user is daxctl-reconfigure-device. Sysadmins may wish to
use daxctl devices as system-ram, but it may be cumbersome to automate
the reconfiguration step for every device upon boot.

Introduce a new option for daxctl-reconfigure-device, --check-config.
This is at the heart of policy based reconfiguration, as it allows
daxctl to look up reconfiguration parameters for a given device from the
config system instead of the command line.

Some systemd and udev glue then automates this for every new dax device
that shows up, providing a way for the administrator to simply list all
the 'system-ram' UUIDs in a config file, and not have to worry about
anything else.

An example config file can be:

  # cat /etc/ndctl/daxctl.conf

  [auto-online unique_identifier_foo]
  uuid = 48d8e42c-a2f0-4312-9e70-a837faafe862
  mode = system-ram
  online = true
  movable = false

Any file under '/etc/ndctl/' can be used - all files with a '.conf' suffix
will be considered when looking for matches.

These patches depend on the initial config file support from Qi Fuli[1].
A branch containing these patches is available at [2].

[1]: https://lore.kernel.org/nvdimm/20210824095106.104808-1-qi.fuli@fujitsu.com/
[2]: https://github.com/pmem/ndctl/tree/vv/daxctl_config

Vishal Verma (7):
  ndctl: Update ndctl.spec.in for 'ndctl.conf'
  daxctl: Documentation updates for persistent reconfiguration
  util/parse-config: refactor filter_conf_files into util/
  daxctl: add basic config parsing support
  util/parse-configs: add a key/value search helper
  daxctl/device.c: add an option for getting params from a config file
  daxctl: add systemd service and udev rule for auto-onlining

 .../daxctl/daxctl-reconfigure-device.txt      |  67 +++++++++
 configure.ac                                  |   9 +-
 daxctl/lib/libdaxctl.c                        |  37 +++++
 ndctl/lib/libndctl.c                          |  19 +--
 daxctl/libdaxctl.h                            |   2 +
 util/parse-configs.h                          |  19 +++
 daxctl/daxctl.c                               |   1 +
 daxctl/device.c                               | 141 +++++++++++++++++-
 util/parse-configs.c                          |  67 +++++++++
 daxctl/90-daxctl-device.rules                 |   1 +
 daxctl/Makefile.am                            |  12 ++
 daxctl/daxdev-auto-reconfigure.sh             |   3 +
 daxctl/daxdev-reconfigure@.service            |   8 +
 daxctl/lib/Makefile.am                        |   6 +
 daxctl/lib/libdaxctl.sym                      |   2 +
 ndctl.spec.in                                 |   4 +
 ndctl/lib/Makefile.am                         |   2 +
 17 files changed, 381 insertions(+), 19 deletions(-)
 create mode 100644 daxctl/90-daxctl-device.rules
 create mode 100755 daxctl/daxdev-auto-reconfigure.sh
 create mode 100644 daxctl/daxdev-reconfigure@.service


base-commit: 5f1026ef3ad108f3f5aa889ef15edae92cb5de43
-- 
2.31.1


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

* [ndctl PATCH 1/7] ndctl: Update ndctl.spec.in for 'ndctl.conf'
  2021-08-31  9:04 [ndctl PATCH 0/7] Policy based reconfiguration for daxctl Vishal Verma
@ 2021-08-31  9:04 ` Vishal Verma
  2021-09-02 12:15   ` qi.fuli
  2021-08-31  9:04 ` [ndctl PATCH 2/7] daxctl: Documentation updates for persistent reconfiguration Vishal Verma
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 20+ messages in thread
From: Vishal Verma @ 2021-08-31  9:04 UTC (permalink / raw)
  To: nvdimm; +Cc: Dan Williams, QI Fuli, fenghua.hu, Vishal Verma, QI Fuli

The new config system introduces and installs a sample config file
called ndctl.conf. Update the RPM spec to include this in the %files
section for ndctl.

Cc: QI Fuli <qi.fuli@fujitsu.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 ndctl.spec.in | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ndctl.spec.in b/ndctl.spec.in
index 0563b2d..07c36ec 100644
--- a/ndctl.spec.in
+++ b/ndctl.spec.in
@@ -118,6 +118,7 @@ make check
 %{_sysconfdir}/modprobe.d/nvdimm-security.conf
 
 %config(noreplace) %{_sysconfdir}/ndctl/monitor.conf
+%config(noreplace) %{_sysconfdir}/ndctl/ndctl.conf
 
 %files -n daxctl
 %defattr(-,root,root)
-- 
2.31.1


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

* [ndctl PATCH 2/7] daxctl: Documentation updates for persistent reconfiguration
  2021-08-31  9:04 [ndctl PATCH 0/7] Policy based reconfiguration for daxctl Vishal Verma
  2021-08-31  9:04 ` [ndctl PATCH 1/7] ndctl: Update ndctl.spec.in for 'ndctl.conf' Vishal Verma
@ 2021-08-31  9:04 ` Vishal Verma
  2021-09-16 22:47   ` Dan Williams
  2021-08-31  9:04 ` [ndctl PATCH 3/7] util/parse-config: refactor filter_conf_files into util/ Vishal Verma
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 20+ messages in thread
From: Vishal Verma @ 2021-08-31  9:04 UTC (permalink / raw)
  To: nvdimm; +Cc: Dan Williams, QI Fuli, fenghua.hu, Vishal Verma

Add a man page update describing how daxctl-reconfigure-device(1) can
be used for persistent reconfiguration of a daxctl device using a
config file.

Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 .../daxctl/daxctl-reconfigure-device.txt      | 67 +++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/Documentation/daxctl/daxctl-reconfigure-device.txt b/Documentation/daxctl/daxctl-reconfigure-device.txt
index f112b3c..1e2d380 100644
--- a/Documentation/daxctl/daxctl-reconfigure-device.txt
+++ b/Documentation/daxctl/daxctl-reconfigure-device.txt
@@ -162,6 +162,15 @@ include::region-option.txt[]
 	brought online automatically and immediately with the 'online_movable'
 	policy. Use this option to disable the automatic onlining behavior.
 
+-C::
+--check-config::
+	Get reconfiguration parameters from the global daxctl config file.
+	This is typically used when daxctl-reconfigure-device is called from
+	a systemd-udevd device unit file. The reconfiguration proceeds only
+	if the UUID of the dax device passed in on the command line matches
+	a UUID listed in the auto-online section of the config. See the
+	'PERSISTENT RECONFIGURATION' section for more details.
+
 include::movable-options.txt[]
 
 -f::
@@ -183,6 +192,64 @@ include::human-option.txt[]
 
 include::verbose-option.txt[]
 
+PERSISTENT RECONFIGURATION
+--------------------------
+
+The 'mode' of a daxctl device is not persistent across reboots by default. This
+is because the device itself may hold any metadata that hints at what mode it
+was set to, or is intended to be used in. The default mode for such a device
+is 'devdax', and on reboot, that is the mode devices appear in by default.
+
+The administrator may desire to configure the system in a way that certain
+dax devices are always reconfigured into a certain mode every time on boot.
+This is accomplished via a daxctl config file located at [location TBD].
+
+The config file may have multiple sections influencing different aspects of
+daxctl operation. The section of interest for persistent reconfiguration is
+'auto-online'. The format of this is as follows:
+
+----
+[auto-online <subsection_name>]
+uuid = <namespace uuid>
+mode = <desired reconfiguration mode> (default: system-ram)
+online = <true|false> (default: true)
+movable = <true|false> (default: true)
+----
+
+Here is an example of a config snippet for managing three devdax namespaces,
+one is left in devdax mode, the second is changed to system-ram mode with
+default options (online, movable), and the third is set to system-ram mode,
+the memory is onlined, but not movable.
+
+Note that the 'subsection name' can be arbitrary, and is only used to
+identify a specific config section. It does not have to match the 'device
+name' (e.g. 'dax0.0' etc).
+
+----
+[auto-online dax0]
+uuid = ed93e918-e165-49d8-921d-383d7b9660c5
+mode = devdax
+
+[auto-online dax1]
+uuid = f36d02ff-1d9f-4fb9-a5b9-8ceb10a00fe3
+mode = system-ram
+
+[auto-online dax2]
+uuid = f36d02ff-1d9f-4fb9-a5b9-8ceb10a00fe3
+mode = system-ram
+online = true
+movable = false
+----
+
+The following example can be used to create a devdax mode namespace, and
+simultaneously add the newly created namespace to the config file for
+system-ram conversion.
+
+----
+ndctl create-namespace --mode=devdax | \
+	jq -r "\"[auto-online $(uuidgen)]\", \"uuid = \(.uuid)\", \"mode = system-ram\"" >> $config_path
+----
+
 include::../copyright.txt[]
 
 SEE ALSO
-- 
2.31.1


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

* [ndctl PATCH 3/7] util/parse-config: refactor filter_conf_files into util/
  2021-08-31  9:04 [ndctl PATCH 0/7] Policy based reconfiguration for daxctl Vishal Verma
  2021-08-31  9:04 ` [ndctl PATCH 1/7] ndctl: Update ndctl.spec.in for 'ndctl.conf' Vishal Verma
  2021-08-31  9:04 ` [ndctl PATCH 2/7] daxctl: Documentation updates for persistent reconfiguration Vishal Verma
@ 2021-08-31  9:04 ` Vishal Verma
  2021-09-02 12:17   ` qi.fuli
  2021-09-16 22:54   ` Dan Williams
  2021-08-31  9:04 ` [ndctl PATCH 4/7] daxctl: add basic config parsing support Vishal Verma
                   ` (4 subsequent siblings)
  7 siblings, 2 replies; 20+ messages in thread
From: Vishal Verma @ 2021-08-31  9:04 UTC (permalink / raw)
  To: nvdimm; +Cc: Dan Williams, QI Fuli, fenghua.hu, Vishal Verma, QI Fuli

Move filter_conf() into util/parse-configs.c as filter_conf_files() so
that it can be reused by the config parser in daxctl.

Cc: QI Fuli <qi.fuli@fujitsu.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 ndctl/lib/libndctl.c   | 19 ++-----------------
 util/parse-configs.h   |  4 ++++
 util/parse-configs.c   | 16 ++++++++++++++++
 daxctl/lib/Makefile.am |  2 ++
 ndctl/lib/Makefile.am  |  2 ++
 5 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index db2e38b..a01ac80 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -25,6 +25,7 @@
 #include <util/size.h>
 #include <util/sysfs.h>
 #include <util/strbuf.h>
+#include <util/parse-configs.h>
 #include <ndctl/libndctl.h>
 #include <ndctl/namespace.h>
 #include <daxctl/libdaxctl.h>
@@ -266,22 +267,6 @@ NDCTL_EXPORT void ndctl_set_userdata(struct ndctl_ctx *ctx, void *userdata)
 	ctx->userdata = userdata;
 }
 
-static int filter_conf(const struct dirent *dir)
-{
-	if (!dir)
-		return 0;
-
-	if (dir->d_type == DT_REG) {
-		const char *ext = strrchr(dir->d_name, '.');
-		if ((!ext) || (ext == dir->d_name))
-			return 0;
-		if (strcmp(ext, ".conf") == 0)
-			return 1;
-	}
-
-	return 0;
-}
-
 NDCTL_EXPORT void ndctl_set_configs(struct ndctl_ctx **ctx, char *conf_dir)
 {
 	struct dirent **namelist;
@@ -291,7 +276,7 @@ NDCTL_EXPORT void ndctl_set_configs(struct ndctl_ctx **ctx, char *conf_dir)
 	if ((!ctx) || (!conf_dir))
 		return;
 
-	rc = scandir(conf_dir, &namelist, filter_conf, alphasort);
+	rc = scandir(conf_dir, &namelist, filter_conf_files, alphasort);
 	if (rc == -1) {
 		perror("scandir");
 		return;
diff --git a/util/parse-configs.h b/util/parse-configs.h
index f70f58f..491aebb 100644
--- a/util/parse-configs.h
+++ b/util/parse-configs.h
@@ -1,8 +1,10 @@
 // SPDX-License-Identifier: GPL-2.0
 // Copyright (C) 2021, FUJITSU LIMITED. ALL rights reserved.
 
+#include <dirent.h>
 #include <stdbool.h>
 #include <stdint.h>
+#include <string.h>
 #include <util/util.h>
 
 enum parse_conf_type {
@@ -11,6 +13,8 @@ enum parse_conf_type {
 	MONITOR_CALLBACK,
 };
 
+int filter_conf_files(const struct dirent *dir);
+
 struct config;
 typedef int parse_conf_cb(const struct config *, const char *config_file);
 
diff --git a/util/parse-configs.c b/util/parse-configs.c
index 44dcff4..72c4913 100644
--- a/util/parse-configs.c
+++ b/util/parse-configs.c
@@ -6,6 +6,22 @@
 #include <util/strbuf.h>
 #include <util/iniparser.h>
 
+int filter_conf_files(const struct dirent *dir)
+{
+	if (!dir)
+		return 0;
+
+	if (dir->d_type == DT_REG) {
+		const char *ext = strrchr(dir->d_name, '.');
+		if ((!ext) || (ext == dir->d_name))
+			return 0;
+		if (strcmp(ext, ".conf") == 0)
+			return 1;
+	}
+
+	return 0;
+}
+
 static void set_str_val(const char **value, const char *val)
 {
 	struct strbuf buf = STRBUF_INIT;
diff --git a/daxctl/lib/Makefile.am b/daxctl/lib/Makefile.am
index 25efd83..db2351e 100644
--- a/daxctl/lib/Makefile.am
+++ b/daxctl/lib/Makefile.am
@@ -15,6 +15,8 @@ libdaxctl_la_SOURCES =\
 	../../util/sysfs.h \
 	../../util/log.c \
 	../../util/log.h \
+	../../util/parse-configs.h \
+	../../util/parse-configs.c \
 	libdaxctl.c
 
 libdaxctl_la_LIBADD =\
diff --git a/ndctl/lib/Makefile.am b/ndctl/lib/Makefile.am
index f741c44..8020eb4 100644
--- a/ndctl/lib/Makefile.am
+++ b/ndctl/lib/Makefile.am
@@ -19,6 +19,8 @@ libndctl_la_SOURCES =\
 	../../util/wrapper.c \
 	../../util/usage.c \
 	../../util/fletcher.h \
+	../../util/parse-configs.h \
+	../../util/parse-configs.c \
 	dimm.c \
 	inject.c \
 	nfit.c \
-- 
2.31.1


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

* [ndctl PATCH 4/7] daxctl: add basic config parsing support
  2021-08-31  9:04 [ndctl PATCH 0/7] Policy based reconfiguration for daxctl Vishal Verma
                   ` (2 preceding siblings ...)
  2021-08-31  9:04 ` [ndctl PATCH 3/7] util/parse-config: refactor filter_conf_files into util/ Vishal Verma
@ 2021-08-31  9:04 ` Vishal Verma
  2021-09-02 12:19   ` qi.fuli
  2021-09-16 22:58   ` Dan Williams
  2021-08-31  9:04 ` [ndctl PATCH 5/7] util/parse-configs: add a key/value search helper Vishal Verma
                   ` (3 subsequent siblings)
  7 siblings, 2 replies; 20+ messages in thread
From: Vishal Verma @ 2021-08-31  9:04 UTC (permalink / raw)
  To: nvdimm; +Cc: Dan Williams, QI Fuli, fenghua.hu, Vishal Verma, QI Fuli

Add support similar to ndctl and libndctl for parsing config files. This
allows storing a config file path/list in the daxctl_ctx, and adds APIs
for setting and retrieving it.

Cc: QI Fuli <qi.fuli@fujitsu.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 daxctl/lib/libdaxctl.c   | 37 +++++++++++++++++++++++++++++++++++++
 daxctl/libdaxctl.h       |  2 ++
 daxctl/Makefile.am       |  1 +
 daxctl/lib/Makefile.am   |  4 ++++
 daxctl/lib/libdaxctl.sym |  2 ++
 5 files changed, 46 insertions(+)

diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c
index 860bd9c..659d2fe 100644
--- a/daxctl/lib/libdaxctl.c
+++ b/daxctl/lib/libdaxctl.c
@@ -17,6 +17,8 @@
 #include <util/log.h>
 #include <util/sysfs.h>
 #include <util/iomem.h>
+#include <util/strbuf.h>
+#include <util/parse-configs.h>
 #include <daxctl/libdaxctl.h>
 #include "libdaxctl-private.h"
 
@@ -37,6 +39,7 @@ struct daxctl_ctx {
 	struct log_ctx ctx;
 	int refcount;
 	void *userdata;
+	const char *configs;
 	int regions_init;
 	struct list_head regions;
 	struct kmod_ctx *kmod_ctx;
@@ -68,6 +71,40 @@ DAXCTL_EXPORT void daxctl_set_userdata(struct daxctl_ctx *ctx, void *userdata)
 	ctx->userdata = userdata;
 }
 
+DAXCTL_EXPORT void daxctl_set_configs(struct daxctl_ctx **ctx, char *conf_dir)
+{
+	struct dirent **namelist;
+	struct strbuf value = STRBUF_INIT;
+	int rc;
+
+	if ((!ctx) || (!conf_dir))
+		return;
+
+	rc = scandir(conf_dir, &namelist, filter_conf_files, alphasort);
+	if (rc == -1) {
+		perror("scandir");
+		return;
+	}
+
+	while (rc--) {
+		if (value.len)
+			strbuf_addstr(&value, " ");
+		strbuf_addstr(&value, conf_dir);
+		strbuf_addstr(&value, "/");
+		strbuf_addstr(&value, namelist[rc]->d_name);
+		free(namelist[rc]);
+	}
+	(*ctx)->configs = strbuf_detach(&value, NULL);
+	free(namelist);
+}
+
+DAXCTL_EXPORT const char *daxctl_get_configs(struct daxctl_ctx *ctx)
+{
+	if (ctx == NULL)
+		return NULL;
+	return ctx->configs;
+}
+
 /**
  * daxctl_new - instantiate a new library context
  * @ctx: context to establish
diff --git a/daxctl/libdaxctl.h b/daxctl/libdaxctl.h
index 683ae9c..9388f85 100644
--- a/daxctl/libdaxctl.h
+++ b/daxctl/libdaxctl.h
@@ -28,6 +28,8 @@ int daxctl_get_log_priority(struct daxctl_ctx *ctx);
 void daxctl_set_log_priority(struct daxctl_ctx *ctx, int priority);
 void daxctl_set_userdata(struct daxctl_ctx *ctx, void *userdata);
 void *daxctl_get_userdata(struct daxctl_ctx *ctx);
+void daxctl_set_configs(struct daxctl_ctx **ctx, char *conf_dir);
+const char *daxctl_get_configs(struct daxctl_ctx *ctx);
 
 struct daxctl_region;
 struct daxctl_region *daxctl_new_region(struct daxctl_ctx *ctx, int id,
diff --git a/daxctl/Makefile.am b/daxctl/Makefile.am
index 9b1313a..a9845a0 100644
--- a/daxctl/Makefile.am
+++ b/daxctl/Makefile.am
@@ -10,6 +10,7 @@ config.h: $(srcdir)/Makefile.am
 		"$(daxctl_modprobe_datadir)/$(daxctl_modprobe_data)"' >>$@ && \
 	echo '#define DAXCTL_MODPROBE_INSTALL \
 		"$(sysconfdir)/modprobe.d/$(daxctl_modprobe_data)"' >>$@
+	$(AM_V_GEN) echo '#define DAXCTL_CONF_DIR  "$(ndctl_confdir)"' >>$@
 
 daxctl_SOURCES =\
 		daxctl.c \
diff --git a/daxctl/lib/Makefile.am b/daxctl/lib/Makefile.am
index db2351e..7a53598 100644
--- a/daxctl/lib/Makefile.am
+++ b/daxctl/lib/Makefile.am
@@ -13,6 +13,10 @@ libdaxctl_la_SOURCES =\
 	../../util/iomem.h \
 	../../util/sysfs.c \
 	../../util/sysfs.h \
+	../../util/strbuf.h \
+	../../util/strbuf.c \
+	../../util/wrapper.c \
+	../../util/usage.c \
 	../../util/log.c \
 	../../util/log.h \
 	../../util/parse-configs.h \
diff --git a/daxctl/lib/libdaxctl.sym b/daxctl/lib/libdaxctl.sym
index a13e93d..190b605 100644
--- a/daxctl/lib/libdaxctl.sym
+++ b/daxctl/lib/libdaxctl.sym
@@ -96,4 +96,6 @@ LIBDAXCTL_9 {
 global:
 	daxctl_dev_will_auto_online_memory;
 	daxctl_dev_has_online_memory;
+	daxctl_set_configs;
+	daxctl_get_configs;
 } LIBDAXCTL_8;
-- 
2.31.1


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

* [ndctl PATCH 5/7] util/parse-configs: add a key/value search helper
  2021-08-31  9:04 [ndctl PATCH 0/7] Policy based reconfiguration for daxctl Vishal Verma
                   ` (3 preceding siblings ...)
  2021-08-31  9:04 ` [ndctl PATCH 4/7] daxctl: add basic config parsing support Vishal Verma
@ 2021-08-31  9:04 ` Vishal Verma
  2021-09-02 13:12   ` qi.fuli
  2021-09-16 23:54   ` Dan Williams
  2021-08-31  9:04 ` [ndctl PATCH 6/7] daxctl/device.c: add an option for getting params from a config file Vishal Verma
                   ` (2 subsequent siblings)
  7 siblings, 2 replies; 20+ messages in thread
From: Vishal Verma @ 2021-08-31  9:04 UTC (permalink / raw)
  To: nvdimm; +Cc: Dan Williams, QI Fuli, fenghua.hu, Vishal Verma, QI Fuli

Add a new config query type called CONFIG_SEARCH_SECTION, which searches
all loaded config files based on a query criteria of: specified section
name, specified key/value pair within that section, and can return other
key/values from the section that matched the search criteria.

This allows for multiple named subsections, where a subsection name is
of the type: '[section subsection]'.

Cc: QI Fuli <qi.fuli@fujitsu.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 util/parse-configs.h | 15 +++++++++++++
 util/parse-configs.c | 51 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+)

diff --git a/util/parse-configs.h b/util/parse-configs.h
index 491aebb..6dcc01c 100644
--- a/util/parse-configs.h
+++ b/util/parse-configs.h
@@ -9,6 +9,7 @@
 
 enum parse_conf_type {
 	CONFIG_STRING,
+	CONFIG_SEARCH_SECTION,
 	CONFIG_END,
 	MONITOR_CALLBACK,
 };
@@ -20,6 +21,10 @@ typedef int parse_conf_cb(const struct config *, const char *config_file);
 
 struct config {
 	enum parse_conf_type type;
+	const char *section;
+	const char *search_key;
+	const char *search_val;
+	const char *get_key;
 	const char *key;
 	void *value;
 	void *defval;
@@ -31,6 +36,16 @@ struct config {
 #define CONF_END() { .type = CONFIG_END }
 #define CONF_STR(k,v,d) \
 	{ .type = CONFIG_STRING, .key = (k), .value = check_vtype(v, const char **), .defval = (d) }
+#define CONF_SEARCH(s, sk, sv, gk, v, d)	\
+{						\
+	.type = CONFIG_SEARCH_SECTION,		\
+	.section = (s),				\
+	.search_key = (sk),			\
+	.search_val = (sv),			\
+	.get_key = (gk),			\
+	.value = check_vtype(v, const char **),	\
+	.defval = (d)				\
+}
 #define CONF_MONITOR(k,f) \
 	{ .type = MONITOR_CALLBACK, .key = (k), .callback = (f)}
 
diff --git a/util/parse-configs.c b/util/parse-configs.c
index 72c4913..8eabe3d 100644
--- a/util/parse-configs.c
+++ b/util/parse-configs.c
@@ -38,6 +38,54 @@ static void set_str_val(const char **value, const char *val)
 	*value = strbuf_detach(&buf, NULL);
 }
 
+static const char *search_section_kv(dictionary *d, const struct config *c)
+{
+	int i;
+
+	for (i = 0; i < iniparser_getnsec(d); i++) {
+		const char *cur_sec_full = iniparser_getsecname(d, i);
+		char *cur_sec = strdup(cur_sec_full);
+		const char *search_val, *ret_val;
+		const char *delim = " \t\n\r";
+		char *save, *cur, *query;
+
+		if (!cur_sec)
+			return NULL;
+		if (!c->section || !c->search_key || !c->search_val || !c->get_key) {
+			fprintf(stderr, "warning: malformed config query, skipping\n");
+			return NULL;
+		}
+
+		cur = strtok_r(cur_sec, delim, &save);
+		if ((cur == NULL) || (strcmp(cur, c->section) != 0))
+			goto out_sec;
+
+		if (asprintf(&query, "%s:%s", cur_sec_full, c->search_key) < 0)
+			goto out_sec;
+		search_val = iniparser_getstring(d, query, NULL);
+		if (!search_val)
+			goto out_query;
+		if (strcmp(search_val, c->search_val) != 0)
+			goto out_query;
+
+		/* we're now in a matching section */
+		free(query);
+		if (asprintf(&query, "%s:%s", cur_sec_full, c->get_key) < 0)
+			goto out_sec;
+		ret_val = iniparser_getstring(d, query, NULL);
+		free(query);
+		free(cur_sec);
+		return ret_val;
+
+out_query:
+		free(query);
+out_sec:
+		free(cur_sec);
+	}
+
+	return NULL;
+}
+
 static int parse_config_file(const char *config_file,
 			const struct config *configs)
 {
@@ -54,6 +102,9 @@ static int parse_config_file(const char *config_file,
 					iniparser_getstring(dic,
 					configs->key, configs->defval));
 			break;
+		case CONFIG_SEARCH_SECTION:
+			set_str_val((const char **)configs->value,
+					search_section_kv(dic, configs));
 		case MONITOR_CALLBACK:
 		case CONFIG_END:
 			break;
-- 
2.31.1


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

* [ndctl PATCH 6/7] daxctl/device.c: add an option for getting params from a config file
  2021-08-31  9:04 [ndctl PATCH 0/7] Policy based reconfiguration for daxctl Vishal Verma
                   ` (4 preceding siblings ...)
  2021-08-31  9:04 ` [ndctl PATCH 5/7] util/parse-configs: add a key/value search helper Vishal Verma
@ 2021-08-31  9:04 ` Vishal Verma
  2021-09-17  1:59   ` Dan Williams
  2021-08-31  9:04 ` [ndctl PATCH 7/7] daxctl: add systemd service and udev rule for auto-onlining Vishal Verma
  2021-09-16 22:12 ` [ndctl PATCH 0/7] Policy based reconfiguration for daxctl Dan Williams
  7 siblings, 1 reply; 20+ messages in thread
From: Vishal Verma @ 2021-08-31  9:04 UTC (permalink / raw)
  To: nvdimm; +Cc: Dan Williams, QI Fuli, fenghua.hu, Vishal Verma, QI Fuli

Add a new option to daxctl-reconfigure-device that allows it to
comprehend the new global config system in ndctl/daxctl. With this, the
reconfigure-device command can query the config to match a specific
device UUID, and operate using the parameters supplied in that INI
section.

This is in preparation to make daxctl device reconfiguration (usually
as system-ram) policy based, so that reconfiguration can happen
automatically on boot.

Cc: QI Fuli <qi.fuli@fujitsu.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 daxctl/daxctl.c    |   1 +
 daxctl/device.c    | 141 ++++++++++++++++++++++++++++++++++++++++++++-
 daxctl/Makefile.am |   1 +
 3 files changed, 142 insertions(+), 1 deletion(-)

diff --git a/daxctl/daxctl.c b/daxctl/daxctl.c
index 928814c..dc7ac5f 100644
--- a/daxctl/daxctl.c
+++ b/daxctl/daxctl.c
@@ -95,6 +95,7 @@ int main(int argc, const char **argv)
 	rc = daxctl_new(&ctx);
 	if (rc)
 		goto out;
+	daxctl_set_configs(&ctx, DAXCTL_CONF_DIR);
 	main_handle_internal_command(argc, argv, ctx, commands,
 			ARRAY_SIZE(commands), PROG_DAXCTL);
 	daxctl_unref(ctx);
diff --git a/daxctl/device.c b/daxctl/device.c
index a427b7d..99a4548 100644
--- a/daxctl/device.c
+++ b/daxctl/device.c
@@ -14,8 +14,10 @@
 #include <util/filter.h>
 #include <json-c/json.h>
 #include <json-c/json_util.h>
+#include <ndctl/libndctl.h>
 #include <daxctl/libdaxctl.h>
 #include <util/parse-options.h>
+#include <util/parse-configs.h>
 #include <ccan/array_size/array_size.h>
 
 static struct {
@@ -25,6 +27,7 @@ static struct {
 	const char *size;
 	const char *align;
 	const char *input;
+	bool check_config;
 	bool no_online;
 	bool no_movable;
 	bool force;
@@ -75,7 +78,9 @@ OPT_STRING('m', "mode", &param.mode, "mode", "mode to switch the device to"), \
 OPT_BOOLEAN('N', "no-online", &param.no_online, \
 	"don't auto-online memory sections"), \
 OPT_BOOLEAN('f', "force", &param.force, \
-		"attempt to offline memory sections before reconfiguration")
+		"attempt to offline memory sections before reconfiguration"), \
+OPT_BOOLEAN('C', "check-config", &param.check_config, \
+		"use config files to determine parameters for the operation")
 
 #define CREATE_OPTIONS() \
 OPT_STRING('s', "size", &param.size, "size", "size to switch the device to"), \
@@ -218,6 +223,130 @@ err:
 	return rc;
 }
 
+static int conf_string_to_bool(const char *str)
+{
+	if (!str)
+		return INT_MAX;
+	if (strncmp(str, "t", 1) == 0 ||
+			strncmp(str, "T", 1) == 0 ||
+			strncmp(str, "y", 1) == 0 ||
+			strncmp(str, "Y", 1) == 0 ||
+			strncmp(str, "1", 1) == 0)
+		return true;
+	if (strncmp(str, "f", 1) == 0 ||
+			strncmp(str, "F", 1) == 0 ||
+			strncmp(str, "n", 1) == 0 ||
+			strncmp(str, "N", 1) == 0 ||
+			strncmp(str, "0", 1) == 0)
+		return false;
+	return INT_MAX;
+}
+
+#define conf_assign_inverted_bool(p, conf_var) \
+do { \
+	if (conf_string_to_bool(conf_var) != INT_MAX) \
+		param.p = !conf_string_to_bool(conf_var); \
+} while(0)
+
+static int parse_config_reconfig_set_params(struct daxctl_ctx *ctx, const char *device,
+					    const char *uuid)
+{
+	const char *conf_online = NULL, *conf_movable = NULL;
+	const struct config configs[] = {
+		CONF_SEARCH("auto-online", "uuid", uuid, "mode", &param.mode, NULL),
+		CONF_SEARCH("auto-online", "uuid", uuid, "online", &conf_online, NULL),
+		CONF_SEARCH("auto-online", "uuid", uuid, "movable", &conf_movable, NULL),
+		CONF_END(),
+	};
+	const char *prefix = "./";
+	int rc;
+
+	rc = parse_configs_prefix(daxctl_get_configs(ctx), prefix, configs);
+	if (rc < 0)
+		return rc;
+
+	conf_assign_inverted_bool(no_online, conf_online);
+	conf_assign_inverted_bool(no_movable, conf_movable);
+
+	return 0;
+}
+
+static bool daxctl_ndns_has_device(struct ndctl_namespace *ndns,
+				    const char *device)
+{
+	struct daxctl_region *dax_region;
+	struct ndctl_dax *dax;
+
+	dax = ndctl_namespace_get_dax(ndns);
+	if (!dax)
+		return false;
+
+	dax_region = ndctl_dax_get_daxctl_region(dax);
+	if (dax_region) {
+		struct daxctl_dev *dev;
+
+		dev = daxctl_dev_get_first(dax_region);
+		if (dev) {
+			if (strcmp(daxctl_dev_get_devname(dev), device) == 0)
+				return true;
+		}
+	}
+	return false;
+}
+
+static int parse_config_reconfig(struct daxctl_ctx *ctx, const char *device)
+{
+	struct ndctl_namespace *ndns;
+	struct ndctl_ctx *ndctl_ctx;
+	struct ndctl_region *region;
+	struct ndctl_bus *bus;
+	struct ndctl_dax *dax;
+	int rc, found = 0;
+	char uuid_buf[40];
+	uuid_t uuid;
+
+	if (strcmp(device, "all") == 0)
+		return 0;
+
+	rc = ndctl_new(&ndctl_ctx);
+	if (rc < 0)
+		return rc;
+
+        ndctl_bus_foreach(ndctl_ctx, bus) {
+		ndctl_region_foreach(bus, region) {
+			ndctl_namespace_foreach(region, ndns) {
+				if (daxctl_ndns_has_device(ndns, device)) {
+					dax = ndctl_namespace_get_dax(ndns);
+					if (!dax)
+						continue;
+					ndctl_dax_get_uuid(dax, uuid);
+					found = 1;
+				}
+			}
+		}
+	}
+
+	if (!found) {
+		fprintf(stderr, "no UUID match for %s found in config files\n",
+			device);
+		return 0;
+	}
+
+	uuid_unparse(uuid, uuid_buf);
+	return parse_config_reconfig_set_params(ctx, device, uuid_buf);
+}
+
+static int parse_device_config(struct daxctl_ctx *ctx, const char *device,
+			       enum device_action action)
+{
+	switch (action) {
+	case ACTION_RECONFIG:
+		return parse_config_reconfig(ctx, device);
+	default:
+		return 0;
+	}
+}
+
 static const char *parse_device_options(int argc, const char **argv,
 		enum device_action action, const struct option *options,
 		const char *usage, struct daxctl_ctx *ctx)
@@ -279,6 +408,16 @@ static const char *parse_device_options(int argc, const char **argv,
 	if (param.human)
 		flags |= UTIL_JSON_HUMAN;
 
+	/* Scan config file(s) for options. This sets param.foo accordingly */
+	if (param.check_config) {
+		rc = parse_device_config(ctx, argv[0], action);
+		if (rc) {
+			fprintf(stderr, "error parsing config file: %s\n",
+				strerror(-rc));
+			return NULL;
+		}
+	}
+
 	/* Handle action-specific options */
 	switch (action) {
 	case ACTION_RECONFIG:
diff --git a/daxctl/Makefile.am b/daxctl/Makefile.am
index a9845a0..f30c485 100644
--- a/daxctl/Makefile.am
+++ b/daxctl/Makefile.am
@@ -23,6 +23,7 @@ daxctl_SOURCES =\
 
 daxctl_LDADD =\
 	lib/libdaxctl.la \
+	../ndctl/lib/libndctl.la \
 	../libutil.a \
 	$(UUID_LIBS) \
 	$(KMOD_LIBS) \
-- 
2.31.1


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

* [ndctl PATCH 7/7] daxctl: add systemd service and udev rule for auto-onlining
  2021-08-31  9:04 [ndctl PATCH 0/7] Policy based reconfiguration for daxctl Vishal Verma
                   ` (5 preceding siblings ...)
  2021-08-31  9:04 ` [ndctl PATCH 6/7] daxctl/device.c: add an option for getting params from a config file Vishal Verma
@ 2021-08-31  9:04 ` Vishal Verma
  2021-09-03  0:56   ` qi.fuli
  2021-09-17 18:10   ` Dan Williams
  2021-09-16 22:12 ` [ndctl PATCH 0/7] Policy based reconfiguration for daxctl Dan Williams
  7 siblings, 2 replies; 20+ messages in thread
From: Vishal Verma @ 2021-08-31  9:04 UTC (permalink / raw)
  To: nvdimm; +Cc: Dan Williams, QI Fuli, fenghua.hu, Vishal Verma, QI Fuli

Install a helper script that calls daxctl-reconfigure-device with the
new 'check-config' option for a given device. This is meant to be called
via a systemd service.

Install a systemd service that calls the above wrapper script with a
daxctl device passed in to it via the environment.

Install a udev rule that is triggered for every daxctl device, and
triggers the above oneshot systemd service.

Together, these three things work such that upon boot, whenever a daxctl
device is found, udev triggers a device-specific systemd service called,
for example:

  daxdev-reconfigure@-dev-dax0.0.service

This initiates a daxctl-reconfigure-device with a config lookup for the
'dax0.0' device. If the config has an '[auto-online <unique_id>]'
section, it uses the information in that to set the operating mode of
the device.

If any device is in an unexpected status, 'journalctl' can be used to
view the reconfiguration log for that device, for example:

  journalctl --unit daxdev-reconfigure@-dev-dax0.0.service

Update the RPM spec file to include the newly added files to the RPM
build.

Cc: QI Fuli <qi.fuli@fujitsu.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
---
 configure.ac                       |  9 ++++++++-
 daxctl/90-daxctl-device.rules      |  1 +
 daxctl/Makefile.am                 | 10 ++++++++++
 daxctl/daxdev-auto-reconfigure.sh  |  3 +++
 daxctl/daxdev-reconfigure@.service |  8 ++++++++
 ndctl.spec.in                      |  3 +++
 6 files changed, 33 insertions(+), 1 deletion(-)
 create mode 100644 daxctl/90-daxctl-device.rules
 create mode 100755 daxctl/daxdev-auto-reconfigure.sh
 create mode 100644 daxctl/daxdev-reconfigure@.service

diff --git a/configure.ac b/configure.ac
index 9e1c6db..df6ab10 100644
--- a/configure.ac
+++ b/configure.ac
@@ -160,7 +160,7 @@ AC_CHECK_FUNCS([ \
 
 AC_ARG_WITH([systemd],
 	AS_HELP_STRING([--with-systemd],
-		[Enable systemd functionality (monitor). @<:@default=yes@:>@]),
+		[Enable systemd functionality. @<:@default=yes@:>@]),
 	[], [with_systemd=yes])
 
 if test "x$with_systemd" = "xyes"; then
@@ -183,6 +183,13 @@ daxctl_modprobe_data=daxctl.conf
 AC_SUBST([daxctl_modprobe_datadir])
 AC_SUBST([daxctl_modprobe_data])
 
+AC_ARG_WITH(udevrulesdir,
+    [AS_HELP_STRING([--with-udevrulesdir=DIR], [udev rules.d directory])],
+    [UDEVRULESDIR="$withval"],
+    [UDEVRULESDIR='${prefix}/lib/udev/rules.d']
+)
+AC_SUBST(UDEVRULESDIR)
+
 AC_ARG_WITH([keyutils],
 	    AS_HELP_STRING([--with-keyutils],
 			[Enable keyutils functionality (security).  @<:@default=yes@:>@]), [], [with_keyutils=yes])
diff --git a/daxctl/90-daxctl-device.rules b/daxctl/90-daxctl-device.rules
new file mode 100644
index 0000000..ee0670f
--- /dev/null
+++ b/daxctl/90-daxctl-device.rules
@@ -0,0 +1 @@
+ACTION=="add", SUBSYSTEM=="dax", TAG+="systemd", ENV{SYSTEMD_WANTS}="daxdev-reconfigure@$env{DEVNAME}.service"
diff --git a/daxctl/Makefile.am b/daxctl/Makefile.am
index f30c485..d53bdcf 100644
--- a/daxctl/Makefile.am
+++ b/daxctl/Makefile.am
@@ -28,3 +28,13 @@ daxctl_LDADD =\
 	$(UUID_LIBS) \
 	$(KMOD_LIBS) \
 	$(JSON_LIBS)
+
+bin_SCRIPTS = daxdev-auto-reconfigure.sh
+CLEANFILES = $(bin_SCRIPTS)
+
+udevrulesdir = $(UDEVRULESDIR)
+udevrules_DATA = 90-daxctl-device.rules
+
+if ENABLE_SYSTEMD_UNITS
+systemd_unit_DATA = daxdev-reconfigure@.service
+endif
diff --git a/daxctl/daxdev-auto-reconfigure.sh b/daxctl/daxdev-auto-reconfigure.sh
new file mode 100755
index 0000000..f6da43f
--- /dev/null
+++ b/daxctl/daxdev-auto-reconfigure.sh
@@ -0,0 +1,3 @@
+#!/bin/bash
+
+daxctl reconfigure-device --check-config "${1##*/}"
diff --git a/daxctl/daxdev-reconfigure@.service b/daxctl/daxdev-reconfigure@.service
new file mode 100644
index 0000000..451fef1
--- /dev/null
+++ b/daxctl/daxdev-reconfigure@.service
@@ -0,0 +1,8 @@
+[Unit]
+Description=Automatic daxctl device reconfiguration
+Documentation=man:daxctl-reconfigure-device(1)
+
+[Service]
+Type=forking
+GuessMainPID=false
+ExecStart=/bin/sh -c "exec daxdev-auto-reconfigure.sh %I"
diff --git a/ndctl.spec.in b/ndctl.spec.in
index 07c36ec..fd1a5ff 100644
--- a/ndctl.spec.in
+++ b/ndctl.spec.in
@@ -124,8 +124,11 @@ make check
 %defattr(-,root,root)
 %license LICENSES/preferred/GPL-2.0 LICENSES/other/MIT LICENSES/other/CC0-1.0
 %{_bindir}/daxctl
+%{_bindir}/daxdev-auto-reconfigure.sh
 %{_mandir}/man1/daxctl*
 %{_datadir}/daxctl/daxctl.conf
+%{_unitdir}/daxdev-reconfigure@.service
+%config %{_udevrulesdir}/90-daxctl-device.rules
 
 %files -n LNAME
 %defattr(-,root,root)
-- 
2.31.1


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

* RE: [ndctl PATCH 1/7] ndctl: Update ndctl.spec.in for 'ndctl.conf'
  2021-08-31  9:04 ` [ndctl PATCH 1/7] ndctl: Update ndctl.spec.in for 'ndctl.conf' Vishal Verma
@ 2021-09-02 12:15   ` qi.fuli
  0 siblings, 0 replies; 20+ messages in thread
From: qi.fuli @ 2021-09-02 12:15 UTC (permalink / raw)
  To: 'Vishal Verma', nvdimm; +Cc: Dan Williams, fenghua.hu

> Subject: [ndctl PATCH 1/7] ndctl: Update ndctl.spec.in for 'ndctl.conf'
> 
> The new config system introduces and installs a sample config file called
> ndctl.conf. Update the RPM spec to include this in the %files section for ndctl.
> 
> Cc: QI Fuli <qi.fuli@fujitsu.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  ndctl.spec.in | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/ndctl.spec.in b/ndctl.spec.in index 0563b2d..07c36ec 100644
> --- a/ndctl.spec.in
> +++ b/ndctl.spec.in
> @@ -118,6 +118,7 @@ make check
>  %{_sysconfdir}/modprobe.d/nvdimm-security.conf
> 
>  %config(noreplace) %{_sysconfdir}/ndctl/monitor.conf
> +%config(noreplace) %{_sysconfdir}/ndctl/ndctl.conf
> 
>  %files -n daxctl
>  %defattr(-,root,root)
> --
> 2.31.1

Looks good to me.
Thank you very much.

QI

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

* RE: [ndctl PATCH 3/7] util/parse-config: refactor filter_conf_files into util/
  2021-08-31  9:04 ` [ndctl PATCH 3/7] util/parse-config: refactor filter_conf_files into util/ Vishal Verma
@ 2021-09-02 12:17   ` qi.fuli
  2021-09-16 22:54   ` Dan Williams
  1 sibling, 0 replies; 20+ messages in thread
From: qi.fuli @ 2021-09-02 12:17 UTC (permalink / raw)
  To: 'Vishal Verma', nvdimm; +Cc: Dan Williams, fenghua.hu

> Subject: [ndctl PATCH 3/7] util/parse-config: refactor filter_conf_files into util/
> 
> Move filter_conf() into util/parse-configs.c as filter_conf_files() so that it can be
> reused by the config parser in daxctl.
> 
> Cc: QI Fuli <qi.fuli@fujitsu.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  ndctl/lib/libndctl.c   | 19 ++-----------------
>  util/parse-configs.h   |  4 ++++
>  util/parse-configs.c   | 16 ++++++++++++++++
>  daxctl/lib/Makefile.am |  2 ++
>  ndctl/lib/Makefile.am  |  2 ++
>  5 files changed, 26 insertions(+), 17 deletions(-)
> 
> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c index db2e38b..a01ac80
> 100644
> --- a/ndctl/lib/libndctl.c
> +++ b/ndctl/lib/libndctl.c
> @@ -25,6 +25,7 @@
>  #include <util/size.h>
>  #include <util/sysfs.h>
>  #include <util/strbuf.h>
> +#include <util/parse-configs.h>
>  #include <ndctl/libndctl.h>
>  #include <ndctl/namespace.h>
>  #include <daxctl/libdaxctl.h>
> @@ -266,22 +267,6 @@ NDCTL_EXPORT void ndctl_set_userdata(struct
> ndctl_ctx *ctx, void *userdata)
>  	ctx->userdata = userdata;
>  }
> 
> -static int filter_conf(const struct dirent *dir) -{
> -	if (!dir)
> -		return 0;
> -
> -	if (dir->d_type == DT_REG) {
> -		const char *ext = strrchr(dir->d_name, '.');
> -		if ((!ext) || (ext == dir->d_name))
> -			return 0;
> -		if (strcmp(ext, ".conf") == 0)
> -			return 1;
> -	}
> -
> -	return 0;
> -}
> -
>  NDCTL_EXPORT void ndctl_set_configs(struct ndctl_ctx **ctx, char *conf_dir)
> {
>  	struct dirent **namelist;
> @@ -291,7 +276,7 @@ NDCTL_EXPORT void ndctl_set_configs(struct ndctl_ctx
> **ctx, char *conf_dir)
>  	if ((!ctx) || (!conf_dir))
>  		return;
> 
> -	rc = scandir(conf_dir, &namelist, filter_conf, alphasort);
> +	rc = scandir(conf_dir, &namelist, filter_conf_files, alphasort);
>  	if (rc == -1) {
>  		perror("scandir");
>  		return;
> diff --git a/util/parse-configs.h b/util/parse-configs.h index f70f58f..491aebb
> 100644
> --- a/util/parse-configs.h
> +++ b/util/parse-configs.h
> @@ -1,8 +1,10 @@
>  // SPDX-License-Identifier: GPL-2.0
>  // Copyright (C) 2021, FUJITSU LIMITED. ALL rights reserved.
> 
> +#include <dirent.h>
>  #include <stdbool.h>
>  #include <stdint.h>
> +#include <string.h>
>  #include <util/util.h>
> 
>  enum parse_conf_type {
> @@ -11,6 +13,8 @@ enum parse_conf_type {
>  	MONITOR_CALLBACK,
>  };
> 
> +int filter_conf_files(const struct dirent *dir);
> +
>  struct config;
>  typedef int parse_conf_cb(const struct config *, const char *config_file);
> 
> diff --git a/util/parse-configs.c b/util/parse-configs.c index 44dcff4..72c4913
> 100644
> --- a/util/parse-configs.c
> +++ b/util/parse-configs.c
> @@ -6,6 +6,22 @@
>  #include <util/strbuf.h>
>  #include <util/iniparser.h>
> 
> +int filter_conf_files(const struct dirent *dir) {
> +	if (!dir)
> +		return 0;
> +
> +	if (dir->d_type == DT_REG) {
> +		const char *ext = strrchr(dir->d_name, '.');
> +		if ((!ext) || (ext == dir->d_name))
> +			return 0;
> +		if (strcmp(ext, ".conf") == 0)
> +			return 1;
> +	}
> +
> +	return 0;
> +}
> +
>  static void set_str_val(const char **value, const char *val)  {
>  	struct strbuf buf = STRBUF_INIT;
> diff --git a/daxctl/lib/Makefile.am b/daxctl/lib/Makefile.am index
> 25efd83..db2351e 100644
> --- a/daxctl/lib/Makefile.am
> +++ b/daxctl/lib/Makefile.am
> @@ -15,6 +15,8 @@ libdaxctl_la_SOURCES =\
>  	../../util/sysfs.h \
>  	../../util/log.c \
>  	../../util/log.h \
> +	../../util/parse-configs.h \
> +	../../util/parse-configs.c \
>  	libdaxctl.c
> 
>  libdaxctl_la_LIBADD =\
> diff --git a/ndctl/lib/Makefile.am b/ndctl/lib/Makefile.am index
> f741c44..8020eb4 100644
> --- a/ndctl/lib/Makefile.am
> +++ b/ndctl/lib/Makefile.am
> @@ -19,6 +19,8 @@ libndctl_la_SOURCES =\
>  	../../util/wrapper.c \
>  	../../util/usage.c \
>  	../../util/fletcher.h \
> +	../../util/parse-configs.h \
> +	../../util/parse-configs.c \
>  	dimm.c \
>  	inject.c \
>  	nfit.c \
> --
> 2.31.1

Looks good to me.
Thank you very much.

QI

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

* RE: [ndctl PATCH 4/7] daxctl: add basic config parsing support
  2021-08-31  9:04 ` [ndctl PATCH 4/7] daxctl: add basic config parsing support Vishal Verma
@ 2021-09-02 12:19   ` qi.fuli
  2021-09-16 22:58   ` Dan Williams
  1 sibling, 0 replies; 20+ messages in thread
From: qi.fuli @ 2021-09-02 12:19 UTC (permalink / raw)
  To: 'Vishal Verma', nvdimm; +Cc: Dan Williams, fenghua.hu

> Subject: [ndctl PATCH 4/7] daxctl: add basic config parsing support
> 
> Add support similar to ndctl and libndctl for parsing config files. This allows
> storing a config file path/list in the daxctl_ctx, and adds APIs for setting and
> retrieving it.
> 
> Cc: QI Fuli <qi.fuli@fujitsu.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  daxctl/lib/libdaxctl.c   | 37
> +++++++++++++++++++++++++++++++++++++
>  daxctl/libdaxctl.h       |  2 ++
>  daxctl/Makefile.am       |  1 +
>  daxctl/lib/Makefile.am   |  4 ++++
>  daxctl/lib/libdaxctl.sym |  2 ++
>  5 files changed, 46 insertions(+)
> 
> diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c index 860bd9c..659d2fe
> 100644
> --- a/daxctl/lib/libdaxctl.c
> +++ b/daxctl/lib/libdaxctl.c
> @@ -17,6 +17,8 @@
>  #include <util/log.h>
>  #include <util/sysfs.h>
>  #include <util/iomem.h>
> +#include <util/strbuf.h>
> +#include <util/parse-configs.h>
>  #include <daxctl/libdaxctl.h>
>  #include "libdaxctl-private.h"
> 
> @@ -37,6 +39,7 @@ struct daxctl_ctx {
>  	struct log_ctx ctx;
>  	int refcount;
>  	void *userdata;
> +	const char *configs;
>  	int regions_init;
>  	struct list_head regions;
>  	struct kmod_ctx *kmod_ctx;
> @@ -68,6 +71,40 @@ DAXCTL_EXPORT void daxctl_set_userdata(struct
> daxctl_ctx *ctx, void *userdata)
>  	ctx->userdata = userdata;
>  }
> 
> +DAXCTL_EXPORT void daxctl_set_configs(struct daxctl_ctx **ctx, char
> +*conf_dir) {
> +	struct dirent **namelist;
> +	struct strbuf value = STRBUF_INIT;
> +	int rc;
> +
> +	if ((!ctx) || (!conf_dir))
> +		return;
> +
> +	rc = scandir(conf_dir, &namelist, filter_conf_files, alphasort);
> +	if (rc == -1) {
> +		perror("scandir");
> +		return;
> +	}
> +
> +	while (rc--) {
> +		if (value.len)
> +			strbuf_addstr(&value, " ");
> +		strbuf_addstr(&value, conf_dir);
> +		strbuf_addstr(&value, "/");
> +		strbuf_addstr(&value, namelist[rc]->d_name);
> +		free(namelist[rc]);
> +	}
> +	(*ctx)->configs = strbuf_detach(&value, NULL);
> +	free(namelist);
> +}
> +
> +DAXCTL_EXPORT const char *daxctl_get_configs(struct daxctl_ctx *ctx) {
> +	if (ctx == NULL)
> +		return NULL;
> +	return ctx->configs;
> +}
> +
>  /**
>   * daxctl_new - instantiate a new library context
>   * @ctx: context to establish
> diff --git a/daxctl/libdaxctl.h b/daxctl/libdaxctl.h index 683ae9c..9388f85 100644
> --- a/daxctl/libdaxctl.h
> +++ b/daxctl/libdaxctl.h
> @@ -28,6 +28,8 @@ int daxctl_get_log_priority(struct daxctl_ctx *ctx);  void
> daxctl_set_log_priority(struct daxctl_ctx *ctx, int priority);  void
> daxctl_set_userdata(struct daxctl_ctx *ctx, void *userdata);  void
> *daxctl_get_userdata(struct daxctl_ctx *ctx);
> +void daxctl_set_configs(struct daxctl_ctx **ctx, char *conf_dir); const
> +char *daxctl_get_configs(struct daxctl_ctx *ctx);
> 
>  struct daxctl_region;
>  struct daxctl_region *daxctl_new_region(struct daxctl_ctx *ctx, int id, diff --git
> a/daxctl/Makefile.am b/daxctl/Makefile.am index 9b1313a..a9845a0 100644
> --- a/daxctl/Makefile.am
> +++ b/daxctl/Makefile.am
> @@ -10,6 +10,7 @@ config.h: $(srcdir)/Makefile.am
>  		"$(daxctl_modprobe_datadir)/$(daxctl_modprobe_data)"' >>$@
> && \
>  	echo '#define DAXCTL_MODPROBE_INSTALL \
>  		"$(sysconfdir)/modprobe.d/$(daxctl_modprobe_data)"' >>$@
> +	$(AM_V_GEN) echo '#define DAXCTL_CONF_DIR  "$(ndctl_confdir)"'
> >>$@
> 
>  daxctl_SOURCES =\
>  		daxctl.c \
> diff --git a/daxctl/lib/Makefile.am b/daxctl/lib/Makefile.am index
> db2351e..7a53598 100644
> --- a/daxctl/lib/Makefile.am
> +++ b/daxctl/lib/Makefile.am
> @@ -13,6 +13,10 @@ libdaxctl_la_SOURCES =\
>  	../../util/iomem.h \
>  	../../util/sysfs.c \
>  	../../util/sysfs.h \
> +	../../util/strbuf.h \
> +	../../util/strbuf.c \
> +	../../util/wrapper.c \
> +	../../util/usage.c \
>  	../../util/log.c \
>  	../../util/log.h \
>  	../../util/parse-configs.h \
> diff --git a/daxctl/lib/libdaxctl.sym b/daxctl/lib/libdaxctl.sym index
> a13e93d..190b605 100644
> --- a/daxctl/lib/libdaxctl.sym
> +++ b/daxctl/lib/libdaxctl.sym
> @@ -96,4 +96,6 @@ LIBDAXCTL_9 {
>  global:
>  	daxctl_dev_will_auto_online_memory;
>  	daxctl_dev_has_online_memory;
> +	daxctl_set_configs;
> +	daxctl_get_configs;
>  } LIBDAXCTL_8;
> --
> 2.31.1

Looks good to me.
Thank you very much.

QI

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

* RE: [ndctl PATCH 5/7] util/parse-configs: add a key/value search helper
  2021-08-31  9:04 ` [ndctl PATCH 5/7] util/parse-configs: add a key/value search helper Vishal Verma
@ 2021-09-02 13:12   ` qi.fuli
  2021-09-16 23:54   ` Dan Williams
  1 sibling, 0 replies; 20+ messages in thread
From: qi.fuli @ 2021-09-02 13:12 UTC (permalink / raw)
  To: 'Vishal Verma', nvdimm; +Cc: Dan Williams, fenghua.hu

> Subject: [ndctl PATCH 5/7] util/parse-configs: add a key/value search helper
> 
> Add a new config query type called CONFIG_SEARCH_SECTION, which searches
> all loaded config files based on a query criteria of: specified section name,
> specified key/value pair within that section, and can return other key/values from
> the section that matched the search criteria.
> 
> This allows for multiple named subsections, where a subsection name is of the
> type: '[section subsection]'.
> 
> Cc: QI Fuli <qi.fuli@fujitsu.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  util/parse-configs.h | 15 +++++++++++++  util/parse-configs.c | 51
> ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 66 insertions(+)
> 
> diff --git a/util/parse-configs.h b/util/parse-configs.h index 491aebb..6dcc01c
> 100644
> --- a/util/parse-configs.h
> +++ b/util/parse-configs.h
> @@ -9,6 +9,7 @@
> 
>  enum parse_conf_type {
>  	CONFIG_STRING,
> +	CONFIG_SEARCH_SECTION,
>  	CONFIG_END,
>  	MONITOR_CALLBACK,
>  };
> @@ -20,6 +21,10 @@ typedef int parse_conf_cb(const struct config *, const char
> *config_file);
> 
>  struct config {
>  	enum parse_conf_type type;
> +	const char *section;
> +	const char *search_key;
> +	const char *search_val;
> +	const char *get_key;
>  	const char *key;
>  	void *value;
>  	void *defval;
> @@ -31,6 +36,16 @@ struct config {
>  #define CONF_END() { .type = CONFIG_END }  #define CONF_STR(k,v,d) \
>  	{ .type = CONFIG_STRING, .key = (k), .value = check_vtype(v, const char
> **), .defval = (d) }
> +#define CONF_SEARCH(s, sk, sv, gk, v, d)	\
> +{						\
> +	.type = CONFIG_SEARCH_SECTION,		\
> +	.section = (s),				\
> +	.search_key = (sk),			\
> +	.search_val = (sv),			\
> +	.get_key = (gk),			\
> +	.value = check_vtype(v, const char **),	\
> +	.defval = (d)				\
> +}
>  #define CONF_MONITOR(k,f) \
>  	{ .type = MONITOR_CALLBACK, .key = (k), .callback = (f)}
> 
> diff --git a/util/parse-configs.c b/util/parse-configs.c index 72c4913..8eabe3d
> 100644
> --- a/util/parse-configs.c
> +++ b/util/parse-configs.c
> @@ -38,6 +38,54 @@ static void set_str_val(const char **value, const char *val)
>  	*value = strbuf_detach(&buf, NULL);
>  }
> 
> +static const char *search_section_kv(dictionary *d, const struct config
> +*c) {
> +	int i;
> +
> +	for (i = 0; i < iniparser_getnsec(d); i++) {
> +		const char *cur_sec_full = iniparser_getsecname(d, i);
> +		char *cur_sec = strdup(cur_sec_full);
> +		const char *search_val, *ret_val;
> +		const char *delim = " \t\n\r";
> +		char *save, *cur, *query;
> +
> +		if (!cur_sec)
> +			return NULL;
> +		if (!c->section || !c->search_key || !c->search_val
> || !c->get_key) {
> +			fprintf(stderr, "warning: malformed config query,
> skipping\n");
> +			return NULL;
> +		}
> +
> +		cur = strtok_r(cur_sec, delim, &save);
> +		if ((cur == NULL) || (strcmp(cur, c->section) != 0))
> +			goto out_sec;
> +
> +		if (asprintf(&query, "%s:%s", cur_sec_full, c->search_key) < 0)
> +			goto out_sec;
> +		search_val = iniparser_getstring(d, query, NULL);
> +		if (!search_val)
> +			goto out_query;
> +		if (strcmp(search_val, c->search_val) != 0)
> +			goto out_query;
> +
> +		/* we're now in a matching section */
> +		free(query);
> +		if (asprintf(&query, "%s:%s", cur_sec_full, c->get_key) < 0)
> +			goto out_sec;
> +		ret_val = iniparser_getstring(d, query, NULL);
> +		free(query);
> +		free(cur_sec);
> +		return ret_val;
> +
> +out_query:
> +		free(query);
> +out_sec:
> +		free(cur_sec);
> +	}
> +
> +	return NULL;
> +}
> +
>  static int parse_config_file(const char *config_file,
>  			const struct config *configs)
>  {
> @@ -54,6 +102,9 @@ static int parse_config_file(const char *config_file,
>  					iniparser_getstring(dic,
>  					configs->key, configs->defval));
>  			break;
> +		case CONFIG_SEARCH_SECTION:
> +			set_str_val((const char **)configs->value,
> +					search_section_kv(dic, configs));
>  		case MONITOR_CALLBACK:
>  		case CONFIG_END:
>  			break;
> --
> 2.31.1

Thank you very much.
This looks good to me.

QI


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

* RE: [ndctl PATCH 7/7] daxctl: add systemd service and udev rule for auto-onlining
  2021-08-31  9:04 ` [ndctl PATCH 7/7] daxctl: add systemd service and udev rule for auto-onlining Vishal Verma
@ 2021-09-03  0:56   ` qi.fuli
  2021-09-17 18:10   ` Dan Williams
  1 sibling, 0 replies; 20+ messages in thread
From: qi.fuli @ 2021-09-03  0:56 UTC (permalink / raw)
  To: 'Vishal Verma', nvdimm; +Cc: Dan Williams, fenghua.hu

> Subject: [ndctl PATCH 7/7] daxctl: add systemd service and udev rule for
> auto-onlining
> 
> Install a helper script that calls daxctl-reconfigure-device with the
> new 'check-config' option for a given device. This is meant to be called
> via a systemd service.
> 
> Install a systemd service that calls the above wrapper script with a
> daxctl device passed in to it via the environment.
> 
> Install a udev rule that is triggered for every daxctl device, and
> triggers the above oneshot systemd service.
> 
> Together, these three things work such that upon boot, whenever a daxctl
> device is found, udev triggers a device-specific systemd service called,
> for example:
> 
>   daxdev-reconfigure@-dev-dax0.0.service
> 
> This initiates a daxctl-reconfigure-device with a config lookup for the
> 'dax0.0' device. If the config has an '[auto-online <unique_id>]'
> section, it uses the information in that to set the operating mode of
> the device.
> 
> If any device is in an unexpected status, 'journalctl' can be used to
> view the reconfiguration log for that device, for example:
> 
>   journalctl --unit daxdev-reconfigure@-dev-dax0.0.service
> 
> Update the RPM spec file to include the newly added files to the RPM
> build.
> 
> Cc: QI Fuli <qi.fuli@fujitsu.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  configure.ac                       |  9 ++++++++-
>  daxctl/90-daxctl-device.rules      |  1 +
>  daxctl/Makefile.am                 | 10 ++++++++++
>  daxctl/daxdev-auto-reconfigure.sh  |  3 +++
>  daxctl/daxdev-reconfigure@.service |  8 ++++++++
>  ndctl.spec.in                      |  3 +++
>  6 files changed, 33 insertions(+), 1 deletion(-)
>  create mode 100644 daxctl/90-daxctl-device.rules
>  create mode 100755 daxctl/daxdev-auto-reconfigure.sh
>  create mode 100644 daxctl/daxdev-reconfigure@.service
> 
> diff --git a/configure.ac b/configure.ac
> index 9e1c6db..df6ab10 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -160,7 +160,7 @@ AC_CHECK_FUNCS([ \
> 
>  AC_ARG_WITH([systemd],
>  	AS_HELP_STRING([--with-systemd],
> -		[Enable systemd functionality (monitor).
> @<:@default=yes@:>@]),
> +		[Enable systemd functionality. @<:@default=yes@:>@]),
>  	[], [with_systemd=yes])
> 
>  if test "x$with_systemd" = "xyes"; then
> @@ -183,6 +183,13 @@ daxctl_modprobe_data=daxctl.conf
>  AC_SUBST([daxctl_modprobe_datadir])
>  AC_SUBST([daxctl_modprobe_data])
> 
> +AC_ARG_WITH(udevrulesdir,
> +    [AS_HELP_STRING([--with-udevrulesdir=DIR], [udev rules.d directory])],
> +    [UDEVRULESDIR="$withval"],
> +    [UDEVRULESDIR='${prefix}/lib/udev/rules.d']
> +)
> +AC_SUBST(UDEVRULESDIR)
> +
>  AC_ARG_WITH([keyutils],
>  	    AS_HELP_STRING([--with-keyutils],
>  			[Enable keyutils functionality (security).
> @<:@default=yes@:>@]), [], [with_keyutils=yes])
> diff --git a/daxctl/90-daxctl-device.rules b/daxctl/90-daxctl-device.rules
> new file mode 100644
> index 0000000..ee0670f
> --- /dev/null
> +++ b/daxctl/90-daxctl-device.rules
> @@ -0,0 +1 @@
> +ACTION=="add", SUBSYSTEM=="dax", TAG+="systemd",
> ENV{SYSTEMD_WANTS}="daxdev-reconfigure@$env{DEVNAME}.service"
> diff --git a/daxctl/Makefile.am b/daxctl/Makefile.am
> index f30c485..d53bdcf 100644
> --- a/daxctl/Makefile.am
> +++ b/daxctl/Makefile.am
> @@ -28,3 +28,13 @@ daxctl_LDADD =\
>  	$(UUID_LIBS) \
>  	$(KMOD_LIBS) \
>  	$(JSON_LIBS)
> +
> +bin_SCRIPTS = daxdev-auto-reconfigure.sh
> +CLEANFILES = $(bin_SCRIPTS)

Hi Vishal,

Thank you for the patch.
I got some warnings in my test.

$ ./autogen.sh 
daxctl/Makefile.am:33: warning: CLEANFILES multiply defined in condition TRUE ...
Makefile.am.in:2: ... 'CLEANFILES' previously defined here
daxctl/Makefile.am:1:   'Makefile.am.in' included from here

----------------------------------------------------------------
Initialized build system. For a common configuration please run:
----------------------------------------------------------------

./configure CFLAGS='-g -O2' --prefix=/usr --sysconfdir=/etc --libdir=/usr/lib64

QI

> +
> +udevrulesdir = $(UDEVRULESDIR)
> +udevrules_DATA = 90-daxctl-device.rules
> +
> +if ENABLE_SYSTEMD_UNITS
> +systemd_unit_DATA = daxdev-reconfigure@.service
> +endif
> diff --git a/daxctl/daxdev-auto-reconfigure.sh
> b/daxctl/daxdev-auto-reconfigure.sh
> new file mode 100755
> index 0000000..f6da43f
> --- /dev/null
> +++ b/daxctl/daxdev-auto-reconfigure.sh
> @@ -0,0 +1,3 @@
> +#!/bin/bash
> +
> +daxctl reconfigure-device --check-config "${1##*/}"
> diff --git a/daxctl/daxdev-reconfigure@.service
> b/daxctl/daxdev-reconfigure@.service
> new file mode 100644
> index 0000000..451fef1
> --- /dev/null
> +++ b/daxctl/daxdev-reconfigure@.service
> @@ -0,0 +1,8 @@
> +[Unit]
> +Description=Automatic daxctl device reconfiguration
> +Documentation=man:daxctl-reconfigure-device(1)
> +
> +[Service]
> +Type=forking
> +GuessMainPID=false
> +ExecStart=/bin/sh -c "exec daxdev-auto-reconfigure.sh %I"
> diff --git a/ndctl.spec.in b/ndctl.spec.in
> index 07c36ec..fd1a5ff 100644
> --- a/ndctl.spec.in
> +++ b/ndctl.spec.in
> @@ -124,8 +124,11 @@ make check
>  %defattr(-,root,root)
>  %license LICENSES/preferred/GPL-2.0 LICENSES/other/MIT
> LICENSES/other/CC0-1.0
>  %{_bindir}/daxctl
> +%{_bindir}/daxdev-auto-reconfigure.sh
>  %{_mandir}/man1/daxctl*
>  %{_datadir}/daxctl/daxctl.conf
> +%{_unitdir}/daxdev-reconfigure@.service
> +%config %{_udevrulesdir}/90-daxctl-device.rules
> 
>  %files -n LNAME
>  %defattr(-,root,root)
> --
> 2.31.1


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

* Re: [ndctl PATCH 0/7] Policy based reconfiguration for daxctl
  2021-08-31  9:04 [ndctl PATCH 0/7] Policy based reconfiguration for daxctl Vishal Verma
                   ` (6 preceding siblings ...)
  2021-08-31  9:04 ` [ndctl PATCH 7/7] daxctl: add systemd service and udev rule for auto-onlining Vishal Verma
@ 2021-09-16 22:12 ` Dan Williams
  7 siblings, 0 replies; 20+ messages in thread
From: Dan Williams @ 2021-09-16 22:12 UTC (permalink / raw)
  To: Vishal Verma; +Cc: Linux NVDIMM, QI Fuli, Hu, Fenghua

On Tue, Aug 31, 2021 at 2:05 AM Vishal Verma <vishal.l.verma@intel.com> wrote:
>
> These patches add policy (config file) support to daxctl. The
> introductory user is daxctl-reconfigure-device. Sysadmins may wish to
> use daxctl devices as system-ram, but it may be cumbersome to automate
> the reconfiguration step for every device upon boot.
>
> Introduce a new option for daxctl-reconfigure-device, --check-config.
> This is at the heart of policy based reconfiguration, as it allows
> daxctl to look up reconfiguration parameters for a given device from the
> config system instead of the command line.
>
> Some systemd and udev glue then automates this for every new dax device
> that shows up, providing a way for the administrator to simply list all
> the 'system-ram' UUIDs in a config file, and not have to worry about
> anything else.
>
> An example config file can be:
>
>   # cat /etc/ndctl/daxctl.conf

Take these comments as provisional until I read through the rest, but
this is just a reaction to the proposed ini format.

>
>   [auto-online unique_identifier_foo]

I am thinking this section name should be "reconfigure-device
unique_identifier_foo" if only because resize might also be something
someone wants to do, and if other commands get config automation it
makes it clearer which config snippets apply to which command.

>   uuid = 48d8e42c-a2f0-4312-9e70-a837faafe862

I think this should be called:

"nvdimm.uuid"

...or something like that to make it clear this depends on dax devices
emitted by libnvdimm, and not those that come from "soft-reserved"
memory. It also helps distinguish if we ever get UUIDs in the HMAT
which is something I have been meaning to propose.

>   mode = system-ram

I can see this being "mode = devdax" if feature was being used to
change size or alignment.

>   online = true
>   movable = false

I wonder if these keys should be prefixed by the mode name:

system-ram.online = true
system-ram.movable = false

...so it's a bit more self documenting about which parameters are
sub-options, and delineates them from generic options like size.

> Any file under '/etc/ndctl/' can be used - all files with a '.conf' suffix
> will be considered when looking for matches.

any concern about name collisions between ndctl, daxctl, and cxl-cli
section names?

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

* Re: [ndctl PATCH 2/7] daxctl: Documentation updates for persistent reconfiguration
  2021-08-31  9:04 ` [ndctl PATCH 2/7] daxctl: Documentation updates for persistent reconfiguration Vishal Verma
@ 2021-09-16 22:47   ` Dan Williams
  0 siblings, 0 replies; 20+ messages in thread
From: Dan Williams @ 2021-09-16 22:47 UTC (permalink / raw)
  To: Vishal Verma; +Cc: Linux NVDIMM, QI Fuli, Hu, Fenghua

On Tue, Aug 31, 2021 at 2:05 AM Vishal Verma <vishal.l.verma@intel.com> wrote:
>
> Add a man page update describing how daxctl-reconfigure-device(1) can
> be used for persistent reconfiguration of a daxctl device using a
> config file.
>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  .../daxctl/daxctl-reconfigure-device.txt      | 67 +++++++++++++++++++
>  1 file changed, 67 insertions(+)
>
> diff --git a/Documentation/daxctl/daxctl-reconfigure-device.txt b/Documentation/daxctl/daxctl-reconfigure-device.txt
> index f112b3c..1e2d380 100644
> --- a/Documentation/daxctl/daxctl-reconfigure-device.txt
> +++ b/Documentation/daxctl/daxctl-reconfigure-device.txt
> @@ -162,6 +162,15 @@ include::region-option.txt[]
>         brought online automatically and immediately with the 'online_movable'
>         policy. Use this option to disable the automatic onlining behavior.
>
> +-C::
> +--check-config::
> +       Get reconfiguration parameters from the global daxctl config file.
> +       This is typically used when daxctl-reconfigure-device is called from
> +       a systemd-udevd device unit file. The reconfiguration proceeds only
> +       if the UUID of the dax device passed in on the command line matches
> +       a UUID listed in the auto-online section of the config. See the
> +       'PERSISTENT RECONFIGURATION' section for more details.

There's going to be other match parameters in the future, so this can
probably say something like:

"The reconfiguration proceeds only if the match parameters in a
'reconfigure-device' section of the config match dax device specified
on the command line"

> +
>  include::movable-options.txt[]
>
>  -f::
> @@ -183,6 +192,64 @@ include::human-option.txt[]
>
>  include::verbose-option.txt[]
>
> +PERSISTENT RECONFIGURATION
> +--------------------------
> +
> +The 'mode' of a daxctl device is not persistent across reboots by default. This
> +is because the device itself may hold any metadata that hints at what mode it

s/may hold any/does not hold/

> +was set to, or is intended to be used in. The default mode for such a device

s/in//

> +is 'devdax', and on reboot, that is the mode devices appear in by default.

s/, that is the mode devices appear in by default//

i.e. redundant.

> +
> +The administrator may desire to configure the system in a way that certain

How about:

"The administrator may set policy such that certain dax devices are
always reconfigured into a target configuration every boot."

> +dax devices are always reconfigured into a certain mode every time on boot.
> +This is accomplished via a daxctl config file located at [location TBD].
> +
> +The config file may have multiple sections influencing different aspects of
> +daxctl operation. The section of interest for persistent reconfiguration is
> +'auto-online'. The format of this is as follows:
> +
> +----
> +[auto-online <subsection_name>]
> +uuid = <namespace uuid>
> +mode = <desired reconfiguration mode> (default: system-ram)
> +online = <true|false> (default: true)
> +movable = <true|false> (default: true)
> +----
> +
> +Here is an example of a config snippet for managing three devdax namespaces,
> +one is left in devdax mode, the second is changed to system-ram mode with
> +default options (online, movable), and the third is set to system-ram mode,
> +the memory is onlined, but not movable.
> +
> +Note that the 'subsection name' can be arbitrary, and is only used to
> +identify a specific config section. It does not have to match the 'device
> +name' (e.g. 'dax0.0' etc).
> +
> +----
> +[auto-online dax0]
> +uuid = ed93e918-e165-49d8-921d-383d7b9660c5
> +mode = devdax
> +
> +[auto-online dax1]
> +uuid = f36d02ff-1d9f-4fb9-a5b9-8ceb10a00fe3
> +mode = system-ram
> +
> +[auto-online dax2]
> +uuid = f36d02ff-1d9f-4fb9-a5b9-8ceb10a00fe3
> +mode = system-ram
> +online = true
> +movable = false

One of the tasks I envisioned with persistent configurations was
recalling resize and create device operations. Not saying that needs
to be included now, but I can assume that these reconfiguration steps
are performed in order... Hmm, as I ask that I realize it may depend
on device arrival. Ok, assuming the devices have all arrived by the
time this runs is there a guarantee that these are processed in order?

> +----
> +
> +The following example can be used to create a devdax mode namespace, and
> +simultaneously add the newly created namespace to the config file for
> +system-ram conversion.
> +
> +----
> +ndctl create-namespace --mode=devdax | \
> +       jq -r "\"[auto-online $(uuidgen)]\", \"uuid = \(.uuid)\", \"mode = system-ram\"" >> $config_path
> +----
> +
>  include::../copyright.txt[]
>
>  SEE ALSO
> --
> 2.31.1
>

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

* Re: [ndctl PATCH 3/7] util/parse-config: refactor filter_conf_files into util/
  2021-08-31  9:04 ` [ndctl PATCH 3/7] util/parse-config: refactor filter_conf_files into util/ Vishal Verma
  2021-09-02 12:17   ` qi.fuli
@ 2021-09-16 22:54   ` Dan Williams
  1 sibling, 0 replies; 20+ messages in thread
From: Dan Williams @ 2021-09-16 22:54 UTC (permalink / raw)
  To: Vishal Verma; +Cc: Linux NVDIMM, QI Fuli, Hu, Fenghua, QI Fuli

On Tue, Aug 31, 2021 at 2:05 AM Vishal Verma <vishal.l.verma@intel.com> wrote:
>
> Move filter_conf() into util/parse-configs.c as filter_conf_files() so
> that it can be reused by the config parser in daxctl.
>
> Cc: QI Fuli <qi.fuli@fujitsu.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  ndctl/lib/libndctl.c   | 19 ++-----------------
>  util/parse-configs.h   |  4 ++++
>  util/parse-configs.c   | 16 ++++++++++++++++
>  daxctl/lib/Makefile.am |  2 ++
>  ndctl/lib/Makefile.am  |  2 ++
>  5 files changed, 26 insertions(+), 17 deletions(-)
>
> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> index db2e38b..a01ac80 100644
> --- a/ndctl/lib/libndctl.c
> +++ b/ndctl/lib/libndctl.c
> @@ -25,6 +25,7 @@
>  #include <util/size.h>
>  #include <util/sysfs.h>
>  #include <util/strbuf.h>
> +#include <util/parse-configs.h>
>  #include <ndctl/libndctl.h>
>  #include <ndctl/namespace.h>
>  #include <daxctl/libdaxctl.h>
> @@ -266,22 +267,6 @@ NDCTL_EXPORT void ndctl_set_userdata(struct ndctl_ctx *ctx, void *userdata)
>         ctx->userdata = userdata;
>  }
>
> -static int filter_conf(const struct dirent *dir)
> -{
> -       if (!dir)
> -               return 0;
> -
> -       if (dir->d_type == DT_REG) {
> -               const char *ext = strrchr(dir->d_name, '.');
> -               if ((!ext) || (ext == dir->d_name))
> -                       return 0;
> -               if (strcmp(ext, ".conf") == 0)
> -                       return 1;
> -       }
> -
> -       return 0;
> -}
> -
>  NDCTL_EXPORT void ndctl_set_configs(struct ndctl_ctx **ctx, char *conf_dir)
>  {
>         struct dirent **namelist;
> @@ -291,7 +276,7 @@ NDCTL_EXPORT void ndctl_set_configs(struct ndctl_ctx **ctx, char *conf_dir)
>         if ((!ctx) || (!conf_dir))
>                 return;
>
> -       rc = scandir(conf_dir, &namelist, filter_conf, alphasort);
> +       rc = scandir(conf_dir, &namelist, filter_conf_files, alphasort);
>         if (rc == -1) {
>                 perror("scandir");
>                 return;
> diff --git a/util/parse-configs.h b/util/parse-configs.h
> index f70f58f..491aebb 100644
> --- a/util/parse-configs.h
> +++ b/util/parse-configs.h
> @@ -1,8 +1,10 @@
>  // SPDX-License-Identifier: GPL-2.0
>  // Copyright (C) 2021, FUJITSU LIMITED. ALL rights reserved.
>
> +#include <dirent.h>
>  #include <stdbool.h>
>  #include <stdint.h>
> +#include <string.h>
>  #include <util/util.h>
>
>  enum parse_conf_type {
> @@ -11,6 +13,8 @@ enum parse_conf_type {
>         MONITOR_CALLBACK,
>  };
>
> +int filter_conf_files(const struct dirent *dir);
> +
>  struct config;
>  typedef int parse_conf_cb(const struct config *, const char *config_file);
>
> diff --git a/util/parse-configs.c b/util/parse-configs.c
> index 44dcff4..72c4913 100644
> --- a/util/parse-configs.c
> +++ b/util/parse-configs.c
> @@ -6,6 +6,22 @@
>  #include <util/strbuf.h>
>  #include <util/iniparser.h>
>
> +int filter_conf_files(const struct dirent *dir)
> +{
> +       if (!dir)
> +               return 0;
> +
> +       if (dir->d_type == DT_REG) {
> +               const char *ext = strrchr(dir->d_name, '.');

Darn, I guess clang-format does not know how to do line-break after
declarations, but... line break after declarations please. Otherwise
it looks good.

> +               if ((!ext) || (ext == dir->d_name))
> +                       return 0;
> +               if (strcmp(ext, ".conf") == 0)
> +                       return 1;
> +       }
> +
> +       return 0;
> +}
> +
>  static void set_str_val(const char **value, const char *val)
>  {
>         struct strbuf buf = STRBUF_INIT;
> diff --git a/daxctl/lib/Makefile.am b/daxctl/lib/Makefile.am
> index 25efd83..db2351e 100644
> --- a/daxctl/lib/Makefile.am
> +++ b/daxctl/lib/Makefile.am
> @@ -15,6 +15,8 @@ libdaxctl_la_SOURCES =\
>         ../../util/sysfs.h \
>         ../../util/log.c \
>         ../../util/log.h \
> +       ../../util/parse-configs.h \
> +       ../../util/parse-configs.c \
>         libdaxctl.c
>
>  libdaxctl_la_LIBADD =\
> diff --git a/ndctl/lib/Makefile.am b/ndctl/lib/Makefile.am
> index f741c44..8020eb4 100644
> --- a/ndctl/lib/Makefile.am
> +++ b/ndctl/lib/Makefile.am
> @@ -19,6 +19,8 @@ libndctl_la_SOURCES =\
>         ../../util/wrapper.c \
>         ../../util/usage.c \
>         ../../util/fletcher.h \
> +       ../../util/parse-configs.h \
> +       ../../util/parse-configs.c \
>         dimm.c \
>         inject.c \
>         nfit.c \
> --
> 2.31.1
>

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

* Re: [ndctl PATCH 4/7] daxctl: add basic config parsing support
  2021-08-31  9:04 ` [ndctl PATCH 4/7] daxctl: add basic config parsing support Vishal Verma
  2021-09-02 12:19   ` qi.fuli
@ 2021-09-16 22:58   ` Dan Williams
  1 sibling, 0 replies; 20+ messages in thread
From: Dan Williams @ 2021-09-16 22:58 UTC (permalink / raw)
  To: Vishal Verma; +Cc: Linux NVDIMM, QI Fuli, Hu, Fenghua, QI Fuli

On Tue, Aug 31, 2021 at 2:05 AM Vishal Verma <vishal.l.verma@intel.com> wrote:
>
> Add support similar to ndctl and libndctl for parsing config files. This
> allows storing a config file path/list in the daxctl_ctx, and adds APIs
> for setting and retrieving it.
>
> Cc: QI Fuli <qi.fuli@fujitsu.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  daxctl/lib/libdaxctl.c   | 37 +++++++++++++++++++++++++++++++++++++
>  daxctl/libdaxctl.h       |  2 ++
>  daxctl/Makefile.am       |  1 +
>  daxctl/lib/Makefile.am   |  4 ++++
>  daxctl/lib/libdaxctl.sym |  2 ++
>  5 files changed, 46 insertions(+)
>
> diff --git a/daxctl/lib/libdaxctl.c b/daxctl/lib/libdaxctl.c
> index 860bd9c..659d2fe 100644
> --- a/daxctl/lib/libdaxctl.c
> +++ b/daxctl/lib/libdaxctl.c
> @@ -17,6 +17,8 @@
>  #include <util/log.h>
>  #include <util/sysfs.h>
>  #include <util/iomem.h>
> +#include <util/strbuf.h>
> +#include <util/parse-configs.h>
>  #include <daxctl/libdaxctl.h>
>  #include "libdaxctl-private.h"
>
> @@ -37,6 +39,7 @@ struct daxctl_ctx {
>         struct log_ctx ctx;
>         int refcount;
>         void *userdata;
> +       const char *configs;
>         int regions_init;
>         struct list_head regions;
>         struct kmod_ctx *kmod_ctx;
> @@ -68,6 +71,40 @@ DAXCTL_EXPORT void daxctl_set_userdata(struct daxctl_ctx *ctx, void *userdata)
>         ctx->userdata = userdata;
>  }
>
> +DAXCTL_EXPORT void daxctl_set_configs(struct daxctl_ctx **ctx, char *conf_dir)
> +{
> +       struct dirent **namelist;
> +       struct strbuf value = STRBUF_INIT;
> +       int rc;
> +
> +       if ((!ctx) || (!conf_dir))
> +               return;
> +
> +       rc = scandir(conf_dir, &namelist, filter_conf_files, alphasort);
> +       if (rc == -1) {
> +               perror("scandir");
> +               return;
> +       }
> +
> +       while (rc--) {
> +               if (value.len)
> +                       strbuf_addstr(&value, " ");
> +               strbuf_addstr(&value, conf_dir);
> +               strbuf_addstr(&value, "/");
> +               strbuf_addstr(&value, namelist[rc]->d_name);
> +               free(namelist[rc]);
> +       }
> +       (*ctx)->configs = strbuf_detach(&value, NULL);
> +       free(namelist);
> +}
> +
> +DAXCTL_EXPORT const char *daxctl_get_configs(struct daxctl_ctx *ctx)
> +{
> +       if (ctx == NULL)
> +               return NULL;
> +       return ctx->configs;
> +}
> +
>  /**
>   * daxctl_new - instantiate a new library context
>   * @ctx: context to establish
> diff --git a/daxctl/libdaxctl.h b/daxctl/libdaxctl.h
> index 683ae9c..9388f85 100644
> --- a/daxctl/libdaxctl.h
> +++ b/daxctl/libdaxctl.h
> @@ -28,6 +28,8 @@ int daxctl_get_log_priority(struct daxctl_ctx *ctx);
>  void daxctl_set_log_priority(struct daxctl_ctx *ctx, int priority);
>  void daxctl_set_userdata(struct daxctl_ctx *ctx, void *userdata);
>  void *daxctl_get_userdata(struct daxctl_ctx *ctx);
> +void daxctl_set_configs(struct daxctl_ctx **ctx, char *conf_dir);
> +const char *daxctl_get_configs(struct daxctl_ctx *ctx);
>
>  struct daxctl_region;
>  struct daxctl_region *daxctl_new_region(struct daxctl_ctx *ctx, int id,
> diff --git a/daxctl/Makefile.am b/daxctl/Makefile.am
> index 9b1313a..a9845a0 100644
> --- a/daxctl/Makefile.am
> +++ b/daxctl/Makefile.am
> @@ -10,6 +10,7 @@ config.h: $(srcdir)/Makefile.am
>                 "$(daxctl_modprobe_datadir)/$(daxctl_modprobe_data)"' >>$@ && \
>         echo '#define DAXCTL_MODPROBE_INSTALL \
>                 "$(sysconfdir)/modprobe.d/$(daxctl_modprobe_data)"' >>$@
> +       $(AM_V_GEN) echo '#define DAXCTL_CONF_DIR  "$(ndctl_confdir)"' >>$@

This gets back to my namespace question about collisions between
daxctl, ndctl, and cxl-cli conf snippets. I think they should each get
their own directory in /etc, then we don't need to encode any prefixes
into section names. What do you think?

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

* Re: [ndctl PATCH 5/7] util/parse-configs: add a key/value search helper
  2021-08-31  9:04 ` [ndctl PATCH 5/7] util/parse-configs: add a key/value search helper Vishal Verma
  2021-09-02 13:12   ` qi.fuli
@ 2021-09-16 23:54   ` Dan Williams
  1 sibling, 0 replies; 20+ messages in thread
From: Dan Williams @ 2021-09-16 23:54 UTC (permalink / raw)
  To: Vishal Verma; +Cc: Linux NVDIMM, QI Fuli, Hu, Fenghua, QI Fuli

On Tue, Aug 31, 2021 at 2:05 AM Vishal Verma <vishal.l.verma@intel.com> wrote:
>
> Add a new config query type called CONFIG_SEARCH_SECTION, which searches
> all loaded config files based on a query criteria of: specified section
> name, specified key/value pair within that section, and can return other
> key/values from the section that matched the search criteria.
>
> This allows for multiple named subsections, where a subsection name is
> of the type: '[section subsection]'.

Presumably in the future this could be used to search by subsection as
well, but for now daxctl does not need that and the subsection is
essentially a comment?

Perhaps that should be called out in a sample / comment only
daxctl.conf file that gets installed by default. Where it clarifies
that everything after the first whitespace in a section name is
treated as a subsection.

This looks good to me.

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

* Re: [ndctl PATCH 6/7] daxctl/device.c: add an option for getting params from a config file
  2021-08-31  9:04 ` [ndctl PATCH 6/7] daxctl/device.c: add an option for getting params from a config file Vishal Verma
@ 2021-09-17  1:59   ` Dan Williams
  0 siblings, 0 replies; 20+ messages in thread
From: Dan Williams @ 2021-09-17  1:59 UTC (permalink / raw)
  To: Vishal Verma; +Cc: Linux NVDIMM, QI Fuli, Hu, Fenghua, QI Fuli

On Tue, Aug 31, 2021 at 2:05 AM Vishal Verma <vishal.l.verma@intel.com> wrote:
>
> Add a new option to daxctl-reconfigure-device that allows it to
> comprehend the new global config system in ndctl/daxctl. With this, the
> reconfigure-device command can query the config to match a specific
> device UUID, and operate using the parameters supplied in that INI
> section.
>
> This is in preparation to make daxctl device reconfiguration (usually
> as system-ram) policy based, so that reconfiguration can happen
> automatically on boot.
>
> Cc: QI Fuli <qi.fuli@fujitsu.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  daxctl/daxctl.c    |   1 +
>  daxctl/device.c    | 141 ++++++++++++++++++++++++++++++++++++++++++++-
>  daxctl/Makefile.am |   1 +
>  3 files changed, 142 insertions(+), 1 deletion(-)
>
> diff --git a/daxctl/daxctl.c b/daxctl/daxctl.c
> index 928814c..dc7ac5f 100644
> --- a/daxctl/daxctl.c
> +++ b/daxctl/daxctl.c
> @@ -95,6 +95,7 @@ int main(int argc, const char **argv)
>         rc = daxctl_new(&ctx);
>         if (rc)
>                 goto out;
> +       daxctl_set_configs(&ctx, DAXCTL_CONF_DIR);

Now that I see how this is used, I think this wants to be called:

daxctl_ctx_add_configs()

...because the operation is populating the context with all the
configuration fragments found in the given directory.

I would also have an option to specify an override for the config
directory, maybe that could be an environment variable?

>         main_handle_internal_command(argc, argv, ctx, commands,
>                         ARRAY_SIZE(commands), PROG_DAXCTL);
>         daxctl_unref(ctx);
> diff --git a/daxctl/device.c b/daxctl/device.c
> index a427b7d..99a4548 100644
> --- a/daxctl/device.c
> +++ b/daxctl/device.c
> @@ -14,8 +14,10 @@
>  #include <util/filter.h>
>  #include <json-c/json.h>
>  #include <json-c/json_util.h>
> +#include <ndctl/libndctl.h>
>  #include <daxctl/libdaxctl.h>
>  #include <util/parse-options.h>
> +#include <util/parse-configs.h>
>  #include <ccan/array_size/array_size.h>
>
>  static struct {
> @@ -25,6 +27,7 @@ static struct {
>         const char *size;
>         const char *align;
>         const char *input;
> +       bool check_config;
>         bool no_online;
>         bool no_movable;
>         bool force;
> @@ -75,7 +78,9 @@ OPT_STRING('m', "mode", &param.mode, "mode", "mode to switch the device to"), \
>  OPT_BOOLEAN('N', "no-online", &param.no_online, \
>         "don't auto-online memory sections"), \
>  OPT_BOOLEAN('f', "force", &param.force, \
> -               "attempt to offline memory sections before reconfiguration")
> +               "attempt to offline memory sections before reconfiguration"), \
> +OPT_BOOLEAN('C', "check-config", &param.check_config, \
> +               "use config files to determine parameters for the operation")
>
>  #define CREATE_OPTIONS() \
>  OPT_STRING('s', "size", &param.size, "size", "size to switch the device to"), \
> @@ -218,6 +223,130 @@ err:
>         return rc;
>  }
>
> +static int conf_string_to_bool(const char *str)
> +{
> +       if (!str)
> +               return INT_MAX;
> +       if (strncmp(str, "t", 1) == 0 ||
> +                       strncmp(str, "T", 1) == 0 ||
> +                       strncmp(str, "y", 1) == 0 ||
> +                       strncmp(str, "Y", 1) == 0 ||
> +                       strncmp(str, "1", 1) == 0)
> +               return true;
> +       if (strncmp(str, "f", 1) == 0 ||
> +                       strncmp(str, "F", 1) == 0 ||
> +                       strncmp(str, "n", 1) == 0 ||
> +                       strncmp(str, "N", 1) == 0 ||
> +                       strncmp(str, "0", 1) == 0)
> +               return false;

Doesn't iniparser_getboolean() already do this conversion for you?

> +       return INT_MAX;
> +}
> +
> +#define conf_assign_inverted_bool(p, conf_var) \
> +do { \
> +       if (conf_string_to_bool(conf_var) != INT_MAX) \
> +               param.p = !conf_string_to_bool(conf_var); \
> +} while(0)
> +
> +static int parse_config_reconfig_set_params(struct daxctl_ctx *ctx, const char *device,
> +                                           const char *uuid)
> +{
> +       const char *conf_online = NULL, *conf_movable = NULL;
> +       const struct config configs[] = {
> +               CONF_SEARCH("auto-online", "uuid", uuid, "mode", &param.mode, NULL),
> +               CONF_SEARCH("auto-online", "uuid", uuid, "online", &conf_online, NULL),
> +               CONF_SEARCH("auto-online", "uuid", uuid, "movable", &conf_movable, NULL),
> +               CONF_END(),
> +       };
> +       const char *prefix = "./";
> +       int rc;
> +
> +       rc = parse_configs_prefix(daxctl_get_configs(ctx), prefix, configs);
> +       if (rc < 0)
> +               return rc;
> +
> +       conf_assign_inverted_bool(no_online, conf_online);
> +       conf_assign_inverted_bool(no_movable, conf_movable);
> +
> +       return 0;
> +}
> +
> +static bool daxctl_ndns_has_device(struct ndctl_namespace *ndns,
> +                                   const char *device)
> +{
> +       struct daxctl_region *dax_region;
> +       struct ndctl_dax *dax;
> +
> +       dax = ndctl_namespace_get_dax(ndns);
> +       if (!dax)
> +               return false;
> +
> +       dax_region = ndctl_dax_get_daxctl_region(dax);
> +       if (dax_region) {
> +               struct daxctl_dev *dev;
> +
> +               dev = daxctl_dev_get_first(dax_region);
> +               if (dev) {
> +                       if (strcmp(daxctl_dev_get_devname(dev), device) == 0)
> +                               return true;
> +               }
> +       }
> +       return false;
> +}
> +
> +static int parse_config_reconfig(struct daxctl_ctx *ctx, const char *device)
> +{
> +       struct ndctl_namespace *ndns;
> +       struct ndctl_ctx *ndctl_ctx;
> +       struct ndctl_region *region;
> +       struct ndctl_bus *bus;
> +       struct ndctl_dax *dax;
> +       int rc, found = 0;
> +       char uuid_buf[40];
> +       uuid_t uuid;
> +
> +       if (strcmp(device, "all") == 0)
> +               return 0;
> +
> +       rc = ndctl_new(&ndctl_ctx);
> +       if (rc < 0)
> +               return rc;
> +
> +        ndctl_bus_foreach(ndctl_ctx, bus) {
> +               ndctl_region_foreach(bus, region) {
> +                       ndctl_namespace_foreach(region, ndns) {
> +                               if (daxctl_ndns_has_device(ndns, device)) {
> +                                       dax = ndctl_namespace_get_dax(ndns);
> +                                       if (!dax)
> +                                               continue;
> +                                       ndctl_dax_get_uuid(dax, uuid);
> +                                       found = 1;
> +                               }
> +                       }
> +               }
> +       }
> +
> +       if (!found) {
> +               fprintf(stderr, "no UUID match for %s found in config files\n",
> +                       device);
> +               return 0;
> +       }
> +
> +       uuid_unparse(uuid, uuid_buf);
> +       return parse_config_reconfig_set_params(ctx, device, uuid_buf);
> +}
> +
> +static int parse_device_config(struct daxctl_ctx *ctx, const char *device,
> +                              enum device_action action)
> +{
> +       switch (action) {
> +       case ACTION_RECONFIG:
> +               return parse_config_reconfig(ctx, device);
> +       default:
> +               return 0;
> +       }
> +}
> +
>  static const char *parse_device_options(int argc, const char **argv,
>                 enum device_action action, const struct option *options,
>                 const char *usage, struct daxctl_ctx *ctx)
> @@ -279,6 +408,16 @@ static const char *parse_device_options(int argc, const char **argv,
>         if (param.human)
>                 flags |= UTIL_JSON_HUMAN;
>
> +       /* Scan config file(s) for options. This sets param.foo accordingly */
> +       if (param.check_config) {
> +               rc = parse_device_config(ctx, argv[0], action);

What happens if someone does:

daxctl reconfigure-device -C --no-online /dev/dax0.0

...and it matches an entry in the configuration file that has
"system-ram.online = true".

My expectation is that precedence ordering is:

built-in defaults
configuration file settings
command line options

Where later entries in that list override settings from the previous
entry. That said, I don't see an easy way to achieve this with parse
options, so might need to fail if anything but -C is specified as an
option until we can fix that conflict.

> +               if (rc) {
> +                       fprintf(stderr, "error parsing config file: %s\n",
> +                               strerror(-rc));
> +                       return NULL;
> +               }
> +       }
> +
>         /* Handle action-specific options */
>         switch (action) {
>         case ACTION_RECONFIG:
> diff --git a/daxctl/Makefile.am b/daxctl/Makefile.am
> index a9845a0..f30c485 100644
> --- a/daxctl/Makefile.am
> +++ b/daxctl/Makefile.am
> @@ -23,6 +23,7 @@ daxctl_SOURCES =\
>
>  daxctl_LDADD =\
>         lib/libdaxctl.la \
> +       ../ndctl/lib/libndctl.la \
>         ../libutil.a \
>         $(UUID_LIBS) \
>         $(KMOD_LIBS) \
> --
> 2.31.1
>

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

* Re: [ndctl PATCH 7/7] daxctl: add systemd service and udev rule for auto-onlining
  2021-08-31  9:04 ` [ndctl PATCH 7/7] daxctl: add systemd service and udev rule for auto-onlining Vishal Verma
  2021-09-03  0:56   ` qi.fuli
@ 2021-09-17 18:10   ` Dan Williams
  1 sibling, 0 replies; 20+ messages in thread
From: Dan Williams @ 2021-09-17 18:10 UTC (permalink / raw)
  To: Vishal Verma; +Cc: Linux NVDIMM, QI Fuli, Hu, Fenghua, QI Fuli

On Tue, Aug 31, 2021 at 2:05 AM Vishal Verma <vishal.l.verma@intel.com> wrote:
>
> Install a helper script that calls daxctl-reconfigure-device with the
> new 'check-config' option for a given device. This is meant to be called
> via a systemd service.
>
> Install a systemd service that calls the above wrapper script with a
> daxctl device passed in to it via the environment.
>
> Install a udev rule that is triggered for every daxctl device, and
> triggers the above oneshot systemd service.
>
> Together, these three things work such that upon boot, whenever a daxctl
> device is found, udev triggers a device-specific systemd service called,
> for example:
>
>   daxdev-reconfigure@-dev-dax0.0.service

I'm thinking the service would be called daxdev-add, because it is
servicing KOBJ_ADD events, or is the convention to name the service
after what it does vs what it services?

Also, I'm curious why would "dax0.0" be in the service name, shouldn't
this be scanning all dax devices and internally matching based on the
config file?

>
> This initiates a daxctl-reconfigure-device with a config lookup for the
> 'dax0.0' device. If the config has an '[auto-online <unique_id>]'
> section, it uses the information in that to set the operating mode of
> the device.
>
> If any device is in an unexpected status, 'journalctl' can be used to
> view the reconfiguration log for that device, for example:
>
>   journalctl --unit daxdev-reconfigure@-dev-dax0.0.service

There will be a log per-device, or only if there is a service per
device? My assumption was that this service is firing off for all
devices so you would need to filter the log by the device-name if you
know it... or maybe I'm misunderstanding how this udev service works.

>
> Update the RPM spec file to include the newly added files to the RPM
> build.
>
> Cc: QI Fuli <qi.fuli@fujitsu.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  configure.ac                       |  9 ++++++++-
>  daxctl/90-daxctl-device.rules      |  1 +
>  daxctl/Makefile.am                 | 10 ++++++++++
>  daxctl/daxdev-auto-reconfigure.sh  |  3 +++
>  daxctl/daxdev-reconfigure@.service |  8 ++++++++
>  ndctl.spec.in                      |  3 +++
>  6 files changed, 33 insertions(+), 1 deletion(-)
>  create mode 100644 daxctl/90-daxctl-device.rules
>  create mode 100755 daxctl/daxdev-auto-reconfigure.sh
>  create mode 100644 daxctl/daxdev-reconfigure@.service
>
> diff --git a/configure.ac b/configure.ac
> index 9e1c6db..df6ab10 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -160,7 +160,7 @@ AC_CHECK_FUNCS([ \
>
>  AC_ARG_WITH([systemd],
>         AS_HELP_STRING([--with-systemd],
> -               [Enable systemd functionality (monitor). @<:@default=yes@:>@]),
> +               [Enable systemd functionality. @<:@default=yes@:>@]),
>         [], [with_systemd=yes])
>
>  if test "x$with_systemd" = "xyes"; then
> @@ -183,6 +183,13 @@ daxctl_modprobe_data=daxctl.conf
>  AC_SUBST([daxctl_modprobe_datadir])
>  AC_SUBST([daxctl_modprobe_data])
>
> +AC_ARG_WITH(udevrulesdir,
> +    [AS_HELP_STRING([--with-udevrulesdir=DIR], [udev rules.d directory])],
> +    [UDEVRULESDIR="$withval"],
> +    [UDEVRULESDIR='${prefix}/lib/udev/rules.d']
> +)
> +AC_SUBST(UDEVRULESDIR)
> +
>  AC_ARG_WITH([keyutils],
>             AS_HELP_STRING([--with-keyutils],
>                         [Enable keyutils functionality (security).  @<:@default=yes@:>@]), [], [with_keyutils=yes])
> diff --git a/daxctl/90-daxctl-device.rules b/daxctl/90-daxctl-device.rules
> new file mode 100644
> index 0000000..ee0670f
> --- /dev/null
> +++ b/daxctl/90-daxctl-device.rules
> @@ -0,0 +1 @@
> +ACTION=="add", SUBSYSTEM=="dax", TAG+="systemd", ENV{SYSTEMD_WANTS}="daxdev-reconfigure@$env{DEVNAME}.service"
> diff --git a/daxctl/Makefile.am b/daxctl/Makefile.am
> index f30c485..d53bdcf 100644
> --- a/daxctl/Makefile.am
> +++ b/daxctl/Makefile.am
> @@ -28,3 +28,13 @@ daxctl_LDADD =\
>         $(UUID_LIBS) \
>         $(KMOD_LIBS) \
>         $(JSON_LIBS)
> +
> +bin_SCRIPTS = daxdev-auto-reconfigure.sh
> +CLEANFILES = $(bin_SCRIPTS)
> +
> +udevrulesdir = $(UDEVRULESDIR)
> +udevrules_DATA = 90-daxctl-device.rules
> +
> +if ENABLE_SYSTEMD_UNITS
> +systemd_unit_DATA = daxdev-reconfigure@.service
> +endif
> diff --git a/daxctl/daxdev-auto-reconfigure.sh b/daxctl/daxdev-auto-reconfigure.sh
> new file mode 100755
> index 0000000..f6da43f
> --- /dev/null
> +++ b/daxctl/daxdev-auto-reconfigure.sh
> @@ -0,0 +1,3 @@
> +#!/bin/bash
> +
> +daxctl reconfigure-device --check-config "${1##*/}"
> diff --git a/daxctl/daxdev-reconfigure@.service b/daxctl/daxdev-reconfigure@.service
> new file mode 100644
> index 0000000..451fef1
> --- /dev/null
> +++ b/daxctl/daxdev-reconfigure@.service
> @@ -0,0 +1,8 @@
> +[Unit]
> +Description=Automatic daxctl device reconfiguration
> +Documentation=man:daxctl-reconfigure-device(1)
> +
> +[Service]
> +Type=forking
> +GuessMainPID=false
> +ExecStart=/bin/sh -c "exec daxdev-auto-reconfigure.sh %I"

Why is the daxdev-auto-reconfigure.sh indirection needed? Can this not be:

ExecStart=daxctl reconfigure-device -C %I

...if the format of %l is the issue I think it would be good for
reconfigure-device to be tolerant of this format.

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

end of thread, other threads:[~2021-09-17 18:11 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-31  9:04 [ndctl PATCH 0/7] Policy based reconfiguration for daxctl Vishal Verma
2021-08-31  9:04 ` [ndctl PATCH 1/7] ndctl: Update ndctl.spec.in for 'ndctl.conf' Vishal Verma
2021-09-02 12:15   ` qi.fuli
2021-08-31  9:04 ` [ndctl PATCH 2/7] daxctl: Documentation updates for persistent reconfiguration Vishal Verma
2021-09-16 22:47   ` Dan Williams
2021-08-31  9:04 ` [ndctl PATCH 3/7] util/parse-config: refactor filter_conf_files into util/ Vishal Verma
2021-09-02 12:17   ` qi.fuli
2021-09-16 22:54   ` Dan Williams
2021-08-31  9:04 ` [ndctl PATCH 4/7] daxctl: add basic config parsing support Vishal Verma
2021-09-02 12:19   ` qi.fuli
2021-09-16 22:58   ` Dan Williams
2021-08-31  9:04 ` [ndctl PATCH 5/7] util/parse-configs: add a key/value search helper Vishal Verma
2021-09-02 13:12   ` qi.fuli
2021-09-16 23:54   ` Dan Williams
2021-08-31  9:04 ` [ndctl PATCH 6/7] daxctl/device.c: add an option for getting params from a config file Vishal Verma
2021-09-17  1:59   ` Dan Williams
2021-08-31  9:04 ` [ndctl PATCH 7/7] daxctl: add systemd service and udev rule for auto-onlining Vishal Verma
2021-09-03  0:56   ` qi.fuli
2021-09-17 18:10   ` Dan Williams
2021-09-16 22:12 ` [ndctl PATCH 0/7] Policy based reconfiguration for daxctl 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).