[PATCH v2 00/20] multipath path classification

[PATCH v2 00/20] multipath path classification

[Martin Wilck]

Hello Christophe,

(This patch set is based on Ben's late series "multipath: new and rebased
patches" and "multipathd per-device waiter fixes". If preferred, I can of
course submit a series based on plain 0.7.6. There's only one minor conflict).

This patch set implements a new, improved path detection logic based
on the previous discussions in the dm-devel threads "Multipath path 
classification revisited" and "RFC: multipath path classification". I believe
that this is ready for upstream inclusion now. My testing has shown that this
is as close to "it just works" as we may be able to get. Changes wrt the RFC
submission are substantial, I've been trying hard to meet Ben's concerns, and
fixed various bugs that showed up in tests.

The ideas of this patch set are:

 1. Avoid boot failures due to "blocked" devices, in particular setting
   SYSTEMD_READY=0 for path devices while multipathd can't setup maps for
   some reason. To this end, a means to indicate mapping failure
   has been added in patch 9-11. This was not in the RFC series.
 2. Try to be as consistent as possible between udev rule processing
   (multipath -u) and multipathd. Patch 1-4 contain the necessary logic
   fixes. That alone doesn't fully avoid inconsistencies because udev rule
   processing and multipathd processing occur at different points in time.
   Therefore, Logic to retrigger uevents if multipathd detects inconsistencies
   has been added in patch 5 and 6.
 3. Disallow combinations of options that are dangerous or inconsistent, and
    generally simplify configuration. This is done by merging
   "find_multipaths", "multipath -i", and "multipathd -n" into a multi-value
   "find_multipaths" option in patch 7. This was not in the RFC series.
 4. Try to a achieve "it just works" autodetection, I've called it "smart".
    This requires waiting for siblings, which necessarily inflicts a certain
    delay on initial set-up (boot). To meet the criticism raised on the RFC
   series, I've tried to minimize these delays, in particular for the common
   "local SATA root" case. This is implemented in patch 12-17, implementation
   details have changed wrt the RFC series.
 5. Don't break compatibility with default setups of major distros. The
   semantics of "find_multipaths yes" and "find_multipaths no" was preserved
   for backwards compatibility (see commit message of patch 7 for details).

A side effect of this patch set is that the user visible output of "multipath
-u" and "multipath -c" has been changed to udev-friendly KEY="value" format.

Changes wrt RFC series not described above:
 - 03/17 "should_multipath: keep existing maps": don't check for empty
 pathvec; rather check if the mapping actually exists in device-mapper (Ben
 Marzinski)
 - added three minor patches at the end changing log levels (18-20)

Regards,
Martin

Benjamin Marzinski (1):
  libmultipath: trigger change uevent on new device creation

Martin Wilck (19):
  Revert "multipath: ignore -i if find_multipaths is set"
  Revert "multipathd: imply -n if find_multipaths is set"
  libmultipath: should_multipath: keep existing maps
  multipath -u -i: respect entries in WWIDs file
  libmultipath: trigger path uevent only when necessary
  libmultipath: change find_multipaths option to multi-value
  libmultipath: use const char* in open_file()
  libmultipath: functions to indicate mapping failure in /dev/shm
  libmultipath: indicate wwid failure in dm_addmap_create()
  libmultipath: don't try to set up failed wwids again
  multipath -u: common code path for result message
  multipath -u: change output to environment/key format
  multipath -u: add DM_MULTIPATH_DEVICE_PATH=2 for "maybe"
  libmultipath: implement find_multipaths_timeout
  libmultipath: enable find_multipaths "smart"
  multipath.rules: find_multipaths "smart" logic
  multipathd: decrease log level of "spurious uevent" message
  libmultipath: decrease log level of uevent filter/merge messages
  multipathd: decrease log level of waiter thread start/stop msgs

 libmultipath/config.h      |   3 +-
 libmultipath/configure.c   |  73 +++++++++++++++++++++++--
 libmultipath/configure.h   |   4 +-
 libmultipath/defaults.h    |   4 +-
 libmultipath/devmapper.c   |   8 ++-
 libmultipath/dict.c        |  54 ++++++++++++++++++-
 libmultipath/file.c        |   6 +--
 libmultipath/file.h        |   2 +-
 libmultipath/propsel.c     |  25 +++++++++
 libmultipath/propsel.h     |   1 +
 libmultipath/structs.h     |  28 ++++++++++
 libmultipath/uevent.c      |   4 +-
 libmultipath/wwids.c       | 129 ++++++++++++++++++++++++++++++++++++++++++---
 libmultipath/wwids.h       |   7 ++-
 multipath/main.c           |  79 ++++++++++++++++-----------
 multipath/multipath.8      |   9 +++-
 multipath/multipath.conf.5 |  75 +++++++++++++++++++-------
 multipath/multipath.rules  |  84 +++++++++++++++++++++++++++--
 multipathd/cli_handlers.c  |   5 +-
 multipathd/main.c          |  28 ++++------
 multipathd/main.h          |   8 +++
 multipathd/multipathd.8    |   8 +--
 multipathd/waiter.c        |   4 +-
 23 files changed, 544 insertions(+), 104 deletions(-)

-- 
2.16.1

[PATCH v2 01/20] Revert "multipath: ignore -i if find_multipaths is set"

[Martin Wilck]

This reverts commit ffbb886a8a16cb063d669cd76a1e656fd3ec8c4b.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipath/main.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/multipath/main.c b/multipath/main.c
index 716203eab56c..20f46acd924a 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -776,16 +776,6 @@ main (int argc, char *argv[])
 		}
 	}
 
-	/*
-	 * FIXME: new device detection with find_multipaths currently
-	 * doesn't work reliably.
-	 */
-	if (cmd ==  CMD_VALID_PATH &&
-	    conf->find_multipaths && conf->ignore_wwids) {
-		condlog(2, "ignoring -i flag because find_multipath is set in multipath.conf");
-		conf->ignore_wwids = 0;
-	}
-
 	if (getuid() != 0) {
 		fprintf(stderr, "need to be root\n");
 		exit(1);
-- 
2.16.1

[PATCH v2 02/20] Revert "multipathd: imply -n if find_multipaths is set"

[Martin Wilck]

This reverts commit 64e27ec066a001012f44550f095c93443e91d845.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 3ae04422a123..0435133fadfb 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2389,10 +2389,6 @@ reconfigure (struct vectors * vecs)
 		conf->verbosity = verbosity;
 	if (bindings_read_only)
 		conf->bindings_read_only = bindings_read_only;
-	if (conf->find_multipaths) {
-		condlog(2, "find_multipaths is set: -n is implied");
-		ignore_new_devs = 1;
-	}
 	if (ignore_new_devs)
 		conf->ignore_new_devs = ignore_new_devs;
 	uxsock_timeout = conf->uxsock_timeout;
-- 
2.16.1

[PATCH v2 03/20] libmultipath: should_multipath: keep existing maps

[Martin Wilck]

with find_multipaths "yes" and without the "-n" option to multipathd,
if a path is already multipathed, keep it. The same logic is applied by
"multipath -u -i".

To do this, we need to add a "mpvec" parameter to should_multipath().

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c |  2 +-
 libmultipath/wwids.c     | 12 +++++++++++-
 libmultipath/wwids.h     |  2 +-
 multipathd/main.c        |  2 +-
 4 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index fa6e21cb31af..16ce797c7d44 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -979,7 +979,7 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
 			continue;
 
 		/* If find_multipaths was selected check if the path is valid */
-		if (!refwwid && !should_multipath(pp1, pathvec)) {
+		if (!refwwid && !should_multipath(pp1, pathvec, curmp)) {
 			orphan_path(pp1, "only one path");
 			continue;
 		}
diff --git a/libmultipath/wwids.c b/libmultipath/wwids.c
index bc70a27409d3..cb6ab52aaa5b 100644
--- a/libmultipath/wwids.c
+++ b/libmultipath/wwids.c
@@ -15,6 +15,7 @@
 #include "wwids.h"
 #include "defaults.h"
 #include "config.h"
+#include "devmapper.h"
 
 /*
  * Copyright (c) 2010 Benjamin Marzinski, Redhat
@@ -271,7 +272,7 @@ out:
 }
 
 int
-should_multipath(struct path *pp1, vector pathvec)
+should_multipath(struct path *pp1, vector pathvec, vector mpvec)
 {
 	int i, ignore_new_devs;
 	struct path *pp2;
@@ -287,6 +288,15 @@ should_multipath(struct path *pp1, vector pathvec)
 
 	condlog(4, "checking if %s should be multipathed", pp1->dev);
 	if (!ignore_new_devs) {
+		char tmp_wwid[WWID_SIZE];
+		struct multipath *mp = find_mp_by_wwid(mpvec, pp1->wwid);
+
+		if (mp != NULL && dm_get_uuid(mp->alias, tmp_wwid) == 0 &&
+		    !strncmp(tmp_wwid, pp1->wwid, WWID_SIZE)) {
+			condlog(3, "wwid %s is already multipathed, keeping it",
+				pp1->wwid);
+			return 1;
+		}
 		vector_foreach_slot(pathvec, pp2, i) {
 			if (pp1->dev == pp2->dev)
 				continue;
diff --git a/libmultipath/wwids.h b/libmultipath/wwids.h
index 95270129daa0..d9a78b38ccf8 100644
--- a/libmultipath/wwids.h
+++ b/libmultipath/wwids.h
@@ -12,7 +12,7 @@
 "#\n" \
 "# Valid WWIDs:\n"
 
-int should_multipath(struct path *pp, vector pathvec);
+int should_multipath(struct path *pp, vector pathvec, vector mpvec);
 int remember_wwid(char *wwid);
 int check_wwids_file(char *wwid, int write_wwid);
 int remove_wwid(char *wwid);
diff --git a/multipathd/main.c b/multipathd/main.c
index 0435133fadfb..707245c67231 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -933,7 +933,7 @@ rescan:
 		mpp->action = ACT_RELOAD;
 		extract_hwe_from_path(mpp);
 	} else {
-		if (!should_multipath(pp, vecs->pathvec)) {
+		if (!should_multipath(pp, vecs->pathvec, vecs->mpvec)) {
 			orphan_path(pp, "only one path");
 			return 0;
 		}
-- 
2.16.1

[PATCH v2 04/20] multipath -u -i: respect entries in WWIDs file

[Martin Wilck]

Previously, if find_multipaths was set, devices listed in the WWIDs file
weren't classified as multipath members by "multipath -u -i" unless they also
met the "find_multipaths" criteria (at least two paths, or existing map with
this WWID). Now we classify all paths in the WWIDs file as multipath members, too.

The rationale for this patch is to match the logic that multipathd applies
by default (i.e. without "-n").

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipath/main.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/multipath/main.c b/multipath/main.c
index 20f46acd924a..4d45df3cdb83 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -437,16 +437,19 @@ configure (struct config *conf, enum mpath_cmds cmd,
 		 * set, you need to actually check if there are two available
 		 * paths to determine if this path should be multipathed. To
 		 * do this, we put off the check until after discovering all
-		 * the paths */
-		if (cmd == CMD_VALID_PATH &&
-		    (!conf->find_multipaths || !conf->ignore_wwids)) {
-			if (conf->ignore_wwids ||
+		 * the paths.
+		 * Paths listed in the wwids file are always considered valid.
+		 */
+		if (cmd == CMD_VALID_PATH) {
+			if ((!conf->find_multipaths && conf->ignore_wwids) ||
 			    check_wwids_file(refwwid, 0) == 0)
 				r = 0;
-
-			printf("%s %s a valid multipath device path\n",
-			       devpath, r == 0 ? "is" : "is not");
-			goto out;
+			if (r == 0 ||
+			    !conf->find_multipaths || !conf->ignore_wwids) {
+				printf("%s %s a valid multipath device path\n",
+				       devpath, r == 0 ? "is" : "is not");
+				goto out;
+			}
 		}
 	}
 
-- 
2.16.1

[PATCH v2 05/20] libmultipath: trigger change uevent on new device creation

[Martin Wilck]

From: Benjamin Marzinski <bmarzins@redhat.com>

When multipath first sees a path device with find_multipaths
enabled, it can't know if the device should be multipathed. This means
that it will not claim the device in udev.  If the device is eventually
multipathed, multipath should trigger a change uevent to update the udev
database to claim the device.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c | 26 ++++++++++++++++++++++++--
 libmultipath/configure.h |  1 +
 libmultipath/wwids.c     |  2 +-
 multipath/main.c         |  2 +-
 multipathd/main.c        |  3 ++-
 5 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 16ce797c7d44..245bd11672cb 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -442,6 +442,28 @@ trigger_udev_change(const struct multipath *mpp)
 	udev_device_unref(udd);
 }
 
+void
+trigger_paths_udev_change(const struct multipath *mpp)
+{
+	struct pathgroup * pgp;
+	struct path * pp;
+	int i, j;
+
+	if (!mpp || !mpp->pg)
+		return;
+
+	vector_foreach_slot (mpp->pg, pgp, i) {
+		if (!pgp->paths)
+			continue;
+		vector_foreach_slot(pgp->paths, pp, j) {
+			if (!pp->udev)
+				continue;
+			sysfs_attr_set_value(pp->udev, "uevent", "change",
+					     strlen("change"));
+		}
+	}
+}
+
 static int
 is_mpp_known_to_udev(const struct multipath *mpp)
 {
@@ -836,8 +858,8 @@ int domap(struct multipath *mpp, char *params, int is_daemon)
 		 * succeeded
 		 */
 		mpp->force_udev_reload = 0;
-		if (mpp->action == ACT_CREATE)
-			remember_wwid(mpp->wwid);
+		if (mpp->action == ACT_CREATE && remember_wwid(mpp->wwid) == 1)
+			trigger_paths_udev_change(mpp);
 		if (!is_daemon) {
 			/* multipath client mode */
 			dm_switchgroup(mpp->alias, mpp->bestpg);
diff --git a/libmultipath/configure.h b/libmultipath/configure.h
index 27a7e6f60a63..545cbc209793 100644
--- a/libmultipath/configure.h
+++ b/libmultipath/configure.h
@@ -37,3 +37,4 @@ int get_refwwid (enum mpath_cmds cmd, char * dev, enum devtypes dev_type,
 		 vector pathvec, char **wwid);
 int reload_map(struct vectors *vecs, struct multipath *mpp, int refresh, int is_daemon);
 struct udev_device *get_udev_device(const char *dev, enum devtypes dev_type);
+void trigger_paths_udev_change(const struct multipath *mpp);
diff --git a/libmultipath/wwids.c b/libmultipath/wwids.c
index cb6ab52aaa5b..ea56911b4607 100644
--- a/libmultipath/wwids.c
+++ b/libmultipath/wwids.c
@@ -329,5 +329,5 @@ remember_wwid(char *wwid)
 		condlog(3, "wrote wwid %s to wwids file", wwid);
 	else
 		condlog(4, "wwid %s already in wwids file", wwid);
-	return 0;
+	return ret;
 }
diff --git a/multipath/main.c b/multipath/main.c
index 4d45df3cdb83..19c729d8e058 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -425,7 +425,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
 		}
 		if (cmd == CMD_ADD_WWID) {
 			r = remember_wwid(refwwid);
-			if (r == 0)
+			if (r >= 0)
 				printf("wwid '%s' added\n", refwwid);
 			else
 				printf("failed adding '%s' to wwids file\n",
diff --git a/multipathd/main.c b/multipathd/main.c
index 707245c67231..da40595fc7c8 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2309,7 +2309,8 @@ configure (struct vectors * vecs)
 
 	sync_maps_state(mpvec);
 	vector_foreach_slot(mpvec, mpp, i){
-		remember_wwid(mpp->wwid);
+		if (remember_wwid(mpp->wwid) == 1)
+			trigger_paths_udev_change(mpp);
 		update_map_pr(mpp);
 	}
 
-- 
2.16.1

[PATCH v2 06/20] libmultipath: trigger path uevent only when necessary

[Martin Wilck]

Paths that are already classified as DM_MULTIPATH_DEVICE_PATH don't
need to be retriggered.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 245bd11672cb..838d145a5aa2 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -456,8 +456,20 @@ trigger_paths_udev_change(const struct multipath *mpp)
 		if (!pgp->paths)
 			continue;
 		vector_foreach_slot(pgp->paths, pp, j) {
+			const char *env;
+
 			if (!pp->udev)
 				continue;
+			/*
+			 * Paths that are already classified as multipath
+			 * members don't need another uevent.
+			 */
+			env = udev_device_get_property_value(
+				pp->udev, "DM_MULTIPATH_DEVICE_PATH");
+			if (env != NULL && !strcmp(env, "1"))
+					continue;
+
+			condlog(4, "triggering change uevent for %s", pp->dev);
 			sysfs_attr_set_value(pp->udev, "uevent", "change",
 					     strlen("change"));
 		}
-- 
2.16.1

[PATCH v2 07/20] libmultipath: change find_multipaths option to multi-value

[Martin Wilck]

Change the "find_multipaths" option from yes/no to multi-value. This
option now covers the effects of "find_multipaths" as it used to be,
plus the -i option to multipath (ignore_wwids) and the -n option to
multipathd (ignore_new_devs), excluding such combinations of the former
options that are dangerous or inconsistent.

The possible values for "find_multipaths" are now:

 - strict: strictly stick to the WWIDs file; never add new maps automatically
   (new default; upstream behaviour with with find_multipaths "yes" and
   commit 64e27ec "multipathd: imply -n if find_multipaths is set")
 - off|no: multipath like "strict", multipathd like "greedy" (previous
   upstream default)
 - on|yes: set up multipath if >1 paths are seen (current Red Hat/Ubuntu
   behavior with find_multipaths "yes")
 - greedy: set up multipath for all non-blacklisted devices (current SUSE
   default)
 - smart: Like "yes", but try to avoid inconsistencies between udev processing
   and multipathd processing by waiting for path siblings to
   appear (fully implemented in follow-up patches)

The default has been changed to "strict", because "no" may cause inconsistent
behavior between "multipath -u" and multipathd. This is deliberately a
conservative choice.

The patch also updates the related man pages.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/config.h      |  2 --
 libmultipath/defaults.h    |  2 +-
 libmultipath/dict.c        | 47 ++++++++++++++++++++++++++++++++++++++++++++--
 libmultipath/structs.h     | 26 +++++++++++++++++++++++++
 libmultipath/wwids.c       |  8 ++++----
 multipath/main.c           |  6 +++---
 multipath/multipath.8      |  9 ++++++++-
 multipath/multipath.conf.5 | 46 +++++++++++++++++++++++++--------------------
 multipathd/main.c          |  6 +-----
 multipathd/multipathd.8    |  8 +++++---
 10 files changed, 119 insertions(+), 41 deletions(-)

diff --git a/libmultipath/config.h b/libmultipath/config.h
index a20ac2aac296..21d8e72a2492 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -139,7 +139,6 @@ struct config {
 	int max_fds;
 	int force_reload;
 	int queue_without_daemon;
-	int ignore_wwids;
 	int checker_timeout;
 	int flush_on_last_del;
 	int attribute_flags;
@@ -168,7 +167,6 @@ struct config {
 	int strict_timing;
 	int retrigger_tries;
 	int retrigger_delay;
-	int ignore_new_devs;
 	int delayed_reconfig;
 	int uev_wait_timeout;
 	int skip_kpartx;
diff --git a/libmultipath/defaults.h b/libmultipath/defaults.h
index 2b270ca46f48..690182c368d9 100644
--- a/libmultipath/defaults.h
+++ b/libmultipath/defaults.h
@@ -17,7 +17,7 @@
 #define DEFAULT_NO_PATH_RETRY	NO_PATH_RETRY_UNDEF
 #define DEFAULT_VERBOSITY	2
 #define DEFAULT_REASSIGN_MAPS	0
-#define DEFAULT_FIND_MULTIPATHS	0
+#define DEFAULT_FIND_MULTIPATHS	FIND_MULTIPATHS_STRICT
 #define DEFAULT_FAST_IO_FAIL	5
 #define DEFAULT_DEV_LOSS_TMO	600
 #define DEFAULT_RETAIN_HWHANDLER RETAIN_HWHANDLER_ON
diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index ea273dd91962..a952c60a6f98 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -233,8 +233,51 @@ declare_def_snprint(multipath_dir, print_str)
 declare_def_handler(partition_delim, set_str)
 declare_def_snprint(partition_delim, print_str)
 
-declare_def_handler(find_multipaths, set_yes_no)
-declare_def_snprint(find_multipaths, print_yes_no)
+static const char *find_multipaths_optvals[] = {
+	[FIND_MULTIPATHS_OFF] = "off",
+	[FIND_MULTIPATHS_ON] = "on",
+	[FIND_MULTIPATHS_STRICT] = "strict",
+	[FIND_MULTIPATHS_GREEDY] = "greedy",
+};
+
+static int
+def_find_multipaths_handler(struct config *conf, vector strvec)
+{
+	char *buff;
+	int i;
+
+	if (set_yes_no_undef(strvec, &conf->find_multipaths) == 0 &&
+	    conf->find_multipaths != YNU_UNDEF)
+		return 0;
+
+	buff = set_value(strvec);
+	if (!buff)
+		return 1;
+
+	for (i = FIND_MULTIPATHS_OFF; i < __FIND_MULTIPATHS_LAST; i++) {
+		if (find_multipaths_optvals[i] != NULL &&
+		    !strcmp(buff, find_multipaths_optvals[i])) {
+			conf->find_multipaths = i;
+			break;
+		}
+	}
+
+	if (conf->find_multipaths == YNU_UNDEF) {
+		condlog(0, "illegal value for find_multipaths: %s", buff);
+		conf->find_multipaths = DEFAULT_FIND_MULTIPATHS;
+	}
+
+	FREE(buff);
+	return 0;
+}
+
+static int
+snprint_def_find_multipaths(struct config *conf, char *buff, int len,
+			    const void *data)
+{
+	return print_str(buff, len,
+			 find_multipaths_optvals[conf->find_multipaths]);
+}
 
 declare_def_handler(selector, set_str)
 declare_def_snprint_defstr(selector, print_str, DEFAULT_SELECTOR)
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 88a4b7862393..d6482f84f0e6 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -102,6 +102,32 @@ enum yes_no_undef_states {
 	YNU_YES,
 };
 
+#define _FIND_MULTIPATHS_F (1 << 1)
+#define _FIND_MULTIPATHS_I (1 << 2)
+#define _FIND_MULTIPATHS_N (1 << 3)
+/*
+ * _FIND_MULTIPATHS_F must have the same value as YNU_YES.
+ * Generate a compile time error if that isn't the case.
+ */
+char ___error1___[-(_FIND_MULTIPATHS_F != YNU_YES)];
+
+#define find_multipaths_on(conf) \
+	(!!((conf)->find_multipaths & _FIND_MULTIPATHS_F))
+#define ignore_wwids(conf) \
+	(!!((conf)->find_multipaths & _FIND_MULTIPATHS_I))
+#define ignore_new_devs(conf) \
+	(!!((conf)->find_multipaths & _FIND_MULTIPATHS_N))
+
+enum find_multipaths_states {
+	FIND_MULTIPATHS_UNDEF = YNU_UNDEF,
+	FIND_MULTIPATHS_OFF = YNU_NO,
+	FIND_MULTIPATHS_ON = _FIND_MULTIPATHS_F,
+	FIND_MULTIPATHS_STRICT = _FIND_MULTIPATHS_F|_FIND_MULTIPATHS_N,
+	FIND_MULTIPATHS_GREEDY = _FIND_MULTIPATHS_I,
+	FIND_MULTIPATHS_SMART = _FIND_MULTIPATHS_F|_FIND_MULTIPATHS_I,
+	__FIND_MULTIPATHS_LAST,
+};
+
 enum flush_states {
 	FLUSH_UNDEF = YNU_UNDEF,
 	FLUSH_DISABLED = YNU_NO,
diff --git a/libmultipath/wwids.c b/libmultipath/wwids.c
index ea56911b4607..5a2e86dcd7e2 100644
--- a/libmultipath/wwids.c
+++ b/libmultipath/wwids.c
@@ -274,20 +274,20 @@ out:
 int
 should_multipath(struct path *pp1, vector pathvec, vector mpvec)
 {
-	int i, ignore_new_devs;
+	int i, ignore_new;
 	struct path *pp2;
 	struct config *conf;
 
 	conf = get_multipath_config();
-	ignore_new_devs = conf->ignore_new_devs;
-	if (!conf->find_multipaths && !ignore_new_devs) {
+	ignore_new = ignore_new_devs(conf);
+	if (!find_multipaths_on(conf) && !ignore_new) {
 		put_multipath_config(conf);
 		return 1;
 	}
 	put_multipath_config(conf);
 
 	condlog(4, "checking if %s should be multipathed", pp1->dev);
-	if (!ignore_new_devs) {
+	if (!ignore_new) {
 		char tmp_wwid[WWID_SIZE];
 		struct multipath *mp = find_mp_by_wwid(mpvec, pp1->wwid);
 
diff --git a/multipath/main.c b/multipath/main.c
index 19c729d8e058..3944fd504de2 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -441,11 +441,11 @@ configure (struct config *conf, enum mpath_cmds cmd,
 		 * Paths listed in the wwids file are always considered valid.
 		 */
 		if (cmd == CMD_VALID_PATH) {
-			if ((!conf->find_multipaths && conf->ignore_wwids) ||
+			if ((!find_multipaths_on(conf) && ignore_wwids(conf)) ||
 			    check_wwids_file(refwwid, 0) == 0)
 				r = 0;
 			if (r == 0 ||
-			    !conf->find_multipaths || !conf->ignore_wwids) {
+			    !find_multipaths_on(conf) || !ignore_wwids(conf)) {
 				printf("%s %s a valid multipath device path\n",
 				       devpath, r == 0 ? "is" : "is not");
 				goto out;
@@ -737,7 +737,7 @@ main (int argc, char *argv[])
 			conf->force_reload = FORCE_RELOAD_YES;
 			break;
 		case 'i':
-			conf->ignore_wwids = 1;
+			conf->find_multipaths |= _FIND_MULTIPATHS_I;
 			break;
 		case 't':
 			r = dump_config(conf);
diff --git a/multipath/multipath.8 b/multipath/multipath.8
index 56f870351ee2..329658daeead 100644
--- a/multipath/multipath.8
+++ b/multipath/multipath.8
@@ -94,7 +94,14 @@ Force devmap reload.
 .
 .TP
 .B \-i
-Ignore WWIDs file when processing devices.
+Ignore WWIDs file when processing devices. If 
+\fIfind_multipaths strict\fR or \fIfind_multipaths no\fR is set in
+\fImultipath.conf\fR, multipath only considers devices that are
+listed in the WWIDs file. This option overrides that behavior. For other values
+of \fIfind_multipaths\fR, this option has no effect. See the description of
+\fIfind_multipaths\fR in 
+.BR multipath.conf (5).
+This option should only be used in rare circumstances.
 .
 .TP
 .B \-B
diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
index c4d0789475a3..6965dacb5c68 100644
--- a/multipath/multipath.conf.5
+++ b/multipath/multipath.conf.5
@@ -951,28 +951,34 @@ The default is: \fBno\fR
 .
 .TP
 .B find_multipaths
-If set to
-.I yes
-, instead of trying to create a multipath device for every non-blacklisted
-path, multipath will only create a device if one of three condidions are
-met.
-.I 1
-There are at least two non-blacklisted paths with the same WWID,
-.I 2
-the user manually forces the creation, by specifying a device with the multipath
-command, or
-.I 3
-a path has the same WWID as a multipath device that was previously created
-while find_multipaths was set (even if that multipath device doesn't currently
-exist).
-Whenever a multipath device is created with find_multipaths set, multipath will
-remember the WWID of the device, so that it will automatically create the
-device again, as soon as it sees a path with that WWID. This should allow most
-users to have multipath automatically choose the correct paths to make into
-multipath devices, without having to edit the blacklist.
+This option controls whether multipath and multipathd try to create multipath
+maps over non-blacklisted devices they encounter. This matters a) when a device is
+encountered by \fBmultipath -u\fR during udev rule processing (a device is
+blocked from further processing by higher layers - such as LVM - if and only
+if it\'s considered a valid multipath device path), and b) when multipathd
+detects a new device. The following values are possible:
 .RS
+.TP 10
+.I strict
+Both multipath and multipathd treat only such devices as multipath devices
+which have been part of a multipath map previously, and which are therefore
+listed in the \fBwwids_file\fR. Users can manually set up multipath maps using the
+\fBmultipathd add map\fR command. Once set up manually, the map is
+remembered in the wwids file and will be set up automatically in the future.
 .TP
-The default is: \fBno\fR
+.I no
+Multipath behaves like \fBstrict\fR. Multipathd behaves like \fBgreedy\fR.
+.TP
+.I yes
+Both multipathd and multipath treat a device as multipath device if the
+conditions for \fBstrict\fR are met, or if at least two non-blacklisted paths
+with the same WWID have been detected.
+.TP
+.I greedy
+Both multipathd and multipath treat every non-blacklisted device as multipath
+device path.
+.TP
+The default is: \fBstrict\fR
 .RE
 .
 .
diff --git a/multipathd/main.c b/multipathd/main.c
index da40595fc7c8..bfbe5f66b324 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2390,8 +2390,6 @@ reconfigure (struct vectors * vecs)
 		conf->verbosity = verbosity;
 	if (bindings_read_only)
 		conf->bindings_read_only = bindings_read_only;
-	if (ignore_new_devs)
-		conf->ignore_new_devs = ignore_new_devs;
 	uxsock_timeout = conf->uxsock_timeout;
 
 	old = rcu_dereference(multipath_conf);
@@ -2620,8 +2618,6 @@ child (void * param)
 		conf->verbosity = verbosity;
 	if (bindings_read_only)
 		conf->bindings_read_only = bindings_read_only;
-	if (ignore_new_devs)
-		conf->ignore_new_devs = ignore_new_devs;
 	uxsock_timeout = conf->uxsock_timeout;
 	rcu_assign_pointer(multipath_conf, conf);
 	if (init_checkers(conf->multipath_dir)) {
@@ -2975,7 +2971,7 @@ main (int argc, char *argv[])
 			bindings_read_only = 1;
 			break;
 		case 'n':
-			ignore_new_devs = 1;
+			condlog(0, "WARNING: ignoring deprecated option -n, use 'ignore_wwids = no' instead");
 			break;
 		case 'w':
 			poll_dmevents = 0;
diff --git a/multipathd/multipathd.8 b/multipathd/multipathd.8
index 5c96680c0514..e78ac9ed277f 100644
--- a/multipathd/multipathd.8
+++ b/multipathd/multipathd.8
@@ -25,7 +25,6 @@ multipathd \- Multipath daemon.
 .RB [\| \-v\ \c
 .IR verbosity \|]
 .RB [\| \-B \|]
-.RB [\| \-n \|]
 .
 .
 .\" ----------------------------------------------------------------------------
@@ -73,8 +72,11 @@ be viewed by entering '\fIhelp\fR'. When you are finished entering commands, pre
 .
 .TP
 .B \-n
-Ignore new devices. multipathd will not create a multipath device unless the
-WWID for the device is already listed in the WWIDs file.
+\fBIGNORED\fR. Use the option
+\fIfind_multipaths\fR to control the treatment of newly detected devices by
+multipathd. See
+.BR multipath.conf(5).
+.
 .
 .
 .\" ----------------------------------------------------------------------------
-- 
2.16.1

[PATCH v2 08/20] libmultipath: use const char* in open_file()

[Martin Wilck]

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/file.c | 6 +++---
 libmultipath/file.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/libmultipath/file.c b/libmultipath/file.c
index e4951c92eb67..d5165ec6709a 100644
--- a/libmultipath/file.c
+++ b/libmultipath/file.c
@@ -37,7 +37,7 @@
  */
 
 static int
-ensure_directories_exist(char *str, mode_t dir_mode)
+ensure_directories_exist(const char *str, mode_t dir_mode)
 {
 	char *pathname;
 	char *end;
@@ -80,7 +80,7 @@ sigalrm(int sig)
 }
 
 static int
-lock_file(int fd, char *file_name)
+lock_file(int fd, const char *file_name)
 {
 	struct sigaction act, oldact;
 	sigset_t set, oldset;
@@ -118,7 +118,7 @@ lock_file(int fd, char *file_name)
 }
 
 int
-open_file(char *file, int *can_write, char *header)
+open_file(const char *file, int *can_write, const char *header)
 {
 	int fd;
 	struct stat s;
diff --git a/libmultipath/file.h b/libmultipath/file.h
index 4f96dbf55e34..70bffa5301ad 100644
--- a/libmultipath/file.h
+++ b/libmultipath/file.h
@@ -6,6 +6,6 @@
 #define _FILE_H
 
 #define FILE_TIMEOUT 30
-int open_file(char *file, int *can_write, char *header);
+int open_file(const char *file, int *can_write, const char *header);
 
 #endif /* _FILE_H */
-- 
2.16.1

[PATCH v2 09/20] libmultipath: functions to indicate mapping failure in /dev/shm

[Martin Wilck]

Create a simple API that indicates failure to create a map for a
certain WWID. This will allow multipathd to indicate to other tools
(in particular, "multipath -u" during udev processing) that
an attempt to create a map for a certain wwid failed.

The indicator is simply the existence of a file under /dev/shm/multipath/failed_wwids.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/wwids.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++++++
 libmultipath/wwids.h |   4 +-
 2 files changed, 105 insertions(+), 1 deletion(-)

diff --git a/libmultipath/wwids.c b/libmultipath/wwids.c
index 5a2e86dcd7e2..e0fdcb34dbc6 100644
--- a/libmultipath/wwids.c
+++ b/libmultipath/wwids.c
@@ -5,6 +5,7 @@
 #include <limits.h>
 #include <stdio.h>
 #include <sys/types.h>
+#include <sys/stat.h>
 
 #include "checkers.h"
 #include "vector.h"
@@ -331,3 +332,104 @@ remember_wwid(char *wwid)
 		condlog(4, "wwid %s already in wwids file", wwid);
 	return ret;
 }
+
+static const char shm_dir[] = "/dev/shm/multipath/failed_wwids";
+static const char shm_lock[] = ".lock";
+static const char shm_header[] = "multipath shm lock file, don't edit";
+static char _shm_lock_path[sizeof(shm_dir)+sizeof(shm_lock)];
+static const char *shm_lock_path = &_shm_lock_path[0];
+
+static int multipath_shm_open(bool rw)
+{
+	int fd;
+	int can_write;
+
+	if (shm_lock_path[0] == '\0')
+		snprintf(_shm_lock_path, sizeof(_shm_lock_path),
+			 "%s/%s", shm_dir, shm_lock);
+	fd = open_file(shm_lock_path, &can_write, shm_header);
+
+	if (fd == 0 && rw && !can_write) {
+		close(fd);
+		condlog(1, "failed to open %s for writing", shm_dir);
+		return -1;
+	}
+
+	return fd;
+}
+
+void multipath_shm_close(void *arg)
+{
+	long fd = (long)arg;
+
+	close(fd);
+	unlink(shm_lock_path);
+}
+
+static int _failed_wwid_op(const char *wwid, bool rw,
+			   int(*func)(const char*), const char *msg)
+{
+	char path[PATH_MAX];
+	long lockfd;
+	int r = -1;
+
+	if (snprintf(path, sizeof(path), "%s/%s", shm_dir, wwid)
+	    >= sizeof(path)) {
+		condlog(1, "%s: path name overflow", __func__);
+		return -1;
+	}
+
+	lockfd = multipath_shm_open(rw);
+	if (lockfd == -1)
+		return -1;
+
+	pthread_cleanup_push(multipath_shm_close, (void*)lockfd);
+	r = func(path);
+	pthread_cleanup_pop(1);
+
+	if (r == -1)
+		condlog(1, "%s: %s: %s", msg, wwid, strerror(errno));
+	else if (rw && r == 1) /* r == 0 means nothing changed */
+		condlog(3, "%s: %s", msg, wwid);
+	else if (!rw)
+		condlog(4, "%s: %s is %s", msg, wwid, r ? "failed" : "good");
+
+	return r;
+}
+
+static int _is_failed(const char *path)
+{
+	struct stat st;
+
+	if (lstat(path, &st) == 0)
+		return 1;
+	else if (errno == ENOENT)
+		return 0;
+	else
+		return -1;
+}
+
+static int _mark_failed(const char *path)
+{
+	/* Called from _failed_wwid_op: we know that shm_lock_path exists */
+	if (_is_failed(path) == 1)
+		return 0;
+	return (link(shm_lock_path, path) == 0 ? 1 : -1);
+}
+
+static int _unmark_failed(const char *path)
+{
+	if (_is_failed(path) == 0)
+		return 0;
+	return (unlink(path) == 0 ? 1 : -1);
+}
+
+#define declare_failed_wwid_op(op, rw) \
+int op ## _wwid(const char *wwid) \
+{ \
+	return _failed_wwid_op(wwid, (rw), _ ## op, #op); \
+}
+
+declare_failed_wwid_op(is_failed, false)
+declare_failed_wwid_op(mark_failed, true)
+declare_failed_wwid_op(unmark_failed, true)
diff --git a/libmultipath/wwids.h b/libmultipath/wwids.h
index d9a78b38ccf8..d00e1f58137c 100644
--- a/libmultipath/wwids.h
+++ b/libmultipath/wwids.h
@@ -17,5 +17,7 @@ int remember_wwid(char *wwid);
 int check_wwids_file(char *wwid, int write_wwid);
 int remove_wwid(char *wwid);
 int replace_wwids(vector mp);
-
+int is_failed_wwid(const char *wwid);
+int mark_failed_wwid(const char *wwid);
+int unmark_failed_wwid(const char *wwid);
 #endif /* _WWIDS_H */
-- 
2.16.1

[PATCH v2 10/20] libmultipath: indicate wwid failure in dm_addmap_create()

[Martin Wilck]

dm_addmap_create() is where we actually try to set up a new
multipath map. Depending on the result, mark the wwid as
failed (or not), and re-trigger an uevent if necessary.
If a path changes from multipath to non-multipath, use an "add"
event to make sure LVM2 rules pick it up. Increase log level
of this event to 3.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c | 37 ++++++++++++++++++++++++++++---------
 libmultipath/configure.h |  2 +-
 libmultipath/devmapper.c |  8 +++++++-
 libmultipath/structs.h   |  1 +
 multipathd/main.c        |  2 +-
 5 files changed, 38 insertions(+), 12 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 838d145a5aa2..88e6687849f8 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -443,11 +443,18 @@ trigger_udev_change(const struct multipath *mpp)
 }
 
 void
-trigger_paths_udev_change(const struct multipath *mpp)
+trigger_paths_udev_change(struct multipath *mpp, bool is_mpath)
 {
 	struct pathgroup * pgp;
 	struct path * pp;
 	int i, j;
+	/*
+	 * If a path changes from multipath to non-multipath, we must
+	 * synthesize an artificial "add" event, otherwise the LVM2 rules
+	 * (69-lvm2-lvmetad.rules) won't pick it up. Otherwise, we'd just
+	 * irritate ourselves with an "add", so use "change".
+	 */
+	const char *action = is_mpath ? "change" : "add";
 
 	if (!mpp || !mpp->pg)
 		return;
@@ -466,14 +473,21 @@ trigger_paths_udev_change(const struct multipath *mpp)
 			 */
 			env = udev_device_get_property_value(
 				pp->udev, "DM_MULTIPATH_DEVICE_PATH");
-			if (env != NULL && !strcmp(env, "1"))
-					continue;
 
-			condlog(4, "triggering change uevent for %s", pp->dev);
-			sysfs_attr_set_value(pp->udev, "uevent", "change",
-					     strlen("change"));
+			if (is_mpath && env != NULL && !strcmp(env, "1"))
+				continue;
+			else if (!is_mpath &&
+				   (env == NULL || !strcmp(env, "0")))
+				continue;
+
+			condlog(3, "triggering %s uevent for %s (is %smultipath member)",
+				action, pp->dev, is_mpath ? "" : "no ");
+			sysfs_attr_set_value(pp->udev, "uevent",
+					     action, strlen(action));
 		}
 	}
+
+	mpp->needs_paths_uevent = 0;
 }
 
 static int
@@ -870,8 +884,10 @@ int domap(struct multipath *mpp, char *params, int is_daemon)
 		 * succeeded
 		 */
 		mpp->force_udev_reload = 0;
-		if (mpp->action == ACT_CREATE && remember_wwid(mpp->wwid) == 1)
-			trigger_paths_udev_change(mpp);
+		if (mpp->action == ACT_CREATE &&
+		    (remember_wwid(mpp->wwid) == 1 ||
+		     mpp->needs_paths_uevent))
+			trigger_paths_udev_change(mpp, true);
 		if (!is_daemon) {
 			/* multipath client mode */
 			dm_switchgroup(mpp->alias, mpp->bestpg);
@@ -896,7 +912,10 @@ int domap(struct multipath *mpp, char *params, int is_daemon)
 		}
 		dm_setgeometry(mpp);
 		return DOMAP_OK;
-	}
+	} else if (r == DOMAP_FAIL && mpp->action == ACT_CREATE &&
+		   mpp->needs_paths_uevent)
+		trigger_paths_udev_change(mpp, false);
+
 	return DOMAP_FAIL;
 }
 
diff --git a/libmultipath/configure.h b/libmultipath/configure.h
index 545cbc209793..8b56d33a1d0b 100644
--- a/libmultipath/configure.h
+++ b/libmultipath/configure.h
@@ -37,4 +37,4 @@ int get_refwwid (enum mpath_cmds cmd, char * dev, enum devtypes dev_type,
 		 vector pathvec, char **wwid);
 int reload_map(struct vectors *vecs, struct multipath *mpp, int refresh, int is_daemon);
 struct udev_device *get_udev_device(const char *dev, enum devtypes dev_type);
-void trigger_paths_udev_change(const struct multipath *mpp);
+void trigger_paths_udev_change(struct multipath *mpp, bool is_mpath);
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 9bafbc6a239a..bd595f4fa40b 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -22,6 +22,7 @@
 #include "devmapper.h"
 #include "sysfs.h"
 #include "config.h"
+#include "wwids.h"
 
 #include "log_pthread.h"
 #include <sys/types.h>
@@ -411,8 +412,11 @@ int dm_addmap_create (struct multipath *mpp, char * params)
 		int err;
 
 		if (dm_addmap(DM_DEVICE_CREATE, TGT_MPATH, mpp, params, ro,
-			      udev_flags))
+			      udev_flags)) {
+			if (unmark_failed_wwid(mpp->wwid) == 1)
+				mpp->needs_paths_uevent = 1;
 			return 1;
+		}
 		/*
 		 * DM_DEVICE_CREATE is actually DM_DEV_CREATE + DM_TABLE_LOAD.
 		 * Failing the second part leaves an empty map. Clean it up.
@@ -428,6 +432,8 @@ int dm_addmap_create (struct multipath *mpp, char * params)
 			break;
 		}
 	}
+	if (mark_failed_wwid(mpp->wwid) == 1)
+		mpp->needs_paths_uevent = 1;
 	return 0;
 }
 
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index d6482f84f0e6..32f4b2d0696c 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -322,6 +322,7 @@ struct multipath {
 	int max_sectors_kb;
 	int force_readonly;
 	int force_udev_reload;
+	int needs_paths_uevent;
 	int ghost_delay;
 	int ghost_delay_tick;
 	unsigned int dev_loss;
diff --git a/multipathd/main.c b/multipathd/main.c
index bfbe5f66b324..ea8c413f28c6 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2310,7 +2310,7 @@ configure (struct vectors * vecs)
 	sync_maps_state(mpvec);
 	vector_foreach_slot(mpvec, mpp, i){
 		if (remember_wwid(mpp->wwid) == 1)
-			trigger_paths_udev_change(mpp);
+			trigger_paths_udev_change(mpp, true);
 		update_map_pr(mpp);
 	}
 
-- 
2.16.1

[PATCH v2 11/20] libmultipath: don't try to set up failed wwids again

[Martin Wilck]

Once setting up a map has failed, don't try to set it up again.
This applies to "multipathd reconfigure" and even multipathd restart,
too. That's deliberate - if a WWID is marked as failed, we don't wont
to bother with it again, unless the admin explicitly tells us so.
Specifically, the exceptions are:

 1) multipathd add map $MAP
 2) multipathd add path $PATH
 3) multipath $PATH

In these cases, addmap() will eventually be called again, and the failed
flag will be set according to it's return status. Unless the reason for
the previous failure has been fixed, it may well be "failed" again.

Inofficially, it's also possible to manually remove a failed marker under
/dev/shm/multipath/failed_wwids and run "multipathd reconfigure".

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c  |  5 +++--
 libmultipath/configure.h  |  3 ++-
 libmultipath/wwids.c      |  7 ++++++-
 libmultipath/wwids.h      |  3 ++-
 multipath/main.c          | 12 +++++++++---
 multipathd/cli_handlers.c |  5 +++--
 multipathd/main.c         | 13 +++++++------
 multipathd/main.h         |  8 ++++++++
 8 files changed, 40 insertions(+), 16 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 88e6687849f8..98b80337d899 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -981,7 +981,7 @@ out:
  * reloaded in DM if there's a difference. This is useful during startup.
  */
 int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
-		    int force_reload, enum mpath_cmds cmd)
+		    int force_reload, bool retry_failed, enum mpath_cmds cmd)
 {
 	int r = 1;
 	int k, i;
@@ -1032,7 +1032,8 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
 			continue;
 
 		/* If find_multipaths was selected check if the path is valid */
-		if (!refwwid && !should_multipath(pp1, pathvec, curmp)) {
+		if (!refwwid && !should_multipath(pp1, pathvec, curmp,
+						  retry_failed)) {
 			orphan_path(pp1, "only one path");
 			continue;
 		}
diff --git a/libmultipath/configure.h b/libmultipath/configure.h
index 8b56d33a1d0b..7e175c890d25 100644
--- a/libmultipath/configure.h
+++ b/libmultipath/configure.h
@@ -32,7 +32,8 @@ int setup_map (struct multipath * mpp, char * params, int params_size,
 	       struct vectors *vecs );
 int domap (struct multipath * mpp, char * params, int is_daemon);
 int reinstate_paths (struct multipath *mpp);
-int coalesce_paths (struct vectors *vecs, vector curmp, char * refwwid, int force_reload, enum mpath_cmds cmd);
+int coalesce_paths (struct vectors *vecs, vector curmp, char * refwwid,
+		    int force_reload, bool retry_failed, enum mpath_cmds cmd);
 int get_refwwid (enum mpath_cmds cmd, char * dev, enum devtypes dev_type,
 		 vector pathvec, char **wwid);
 int reload_map(struct vectors *vecs, struct multipath *mpp, int refresh, int is_daemon);
diff --git a/libmultipath/wwids.c b/libmultipath/wwids.c
index e0fdcb34dbc6..288a9ad50c73 100644
--- a/libmultipath/wwids.c
+++ b/libmultipath/wwids.c
@@ -4,6 +4,7 @@
 #include <string.h>
 #include <limits.h>
 #include <stdio.h>
+#include <stdbool.h>
 #include <sys/types.h>
 #include <sys/stat.h>
 
@@ -273,12 +274,16 @@ out:
 }
 
 int
-should_multipath(struct path *pp1, vector pathvec, vector mpvec)
+should_multipath(struct path *pp1, vector pathvec, vector mpvec,
+		 bool retry_failed)
 {
 	int i, ignore_new;
 	struct path *pp2;
 	struct config *conf;
 
+	if (!retry_failed && is_failed_wwid(pp1->wwid))
+		return 0;
+
 	conf = get_multipath_config();
 	ignore_new = ignore_new_devs(conf);
 	if (!find_multipaths_on(conf) && !ignore_new) {
diff --git a/libmultipath/wwids.h b/libmultipath/wwids.h
index d00e1f58137c..911e8da720c5 100644
--- a/libmultipath/wwids.h
+++ b/libmultipath/wwids.h
@@ -12,7 +12,8 @@
 "#\n" \
 "# Valid WWIDs:\n"
 
-int should_multipath(struct path *pp, vector pathvec, vector mpvec);
+int should_multipath(struct path *pp, vector pathvec, vector mpvec,
+		     bool retry_failed);
 int remember_wwid(char *wwid);
 int check_wwids_file(char *wwid, int write_wwid);
 int remove_wwid(char *wwid);
diff --git a/multipath/main.c b/multipath/main.c
index 3944fd504de2..566404e56ef4 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -441,8 +441,12 @@ configure (struct config *conf, enum mpath_cmds cmd,
 		 * Paths listed in the wwids file are always considered valid.
 		 */
 		if (cmd == CMD_VALID_PATH) {
-			if ((!find_multipaths_on(conf) && ignore_wwids(conf)) ||
-			    check_wwids_file(refwwid, 0) == 0)
+			if (is_failed_wwid(refwwid)) {
+				r = 1;
+				goto print_valid;
+			} else if ((!find_multipaths_on(conf) &&
+				  ignore_wwids(conf)) ||
+				 check_wwids_file(refwwid, 0) == 0)
 				r = 0;
 			if (r == 0 ||
 			    !find_multipaths_on(conf) || !ignore_wwids(conf)) {
@@ -504,9 +508,11 @@ configure (struct config *conf, enum mpath_cmds cmd,
 
 	/*
 	 * core logic entry point
+	 * If refwwid != NULL, user has given a device to multipath,
+	 * so retry even if this ID has failed before.
 	 */
 	r = coalesce_paths(&vecs, NULL, refwwid,
-			   conf->force_reload, cmd);
+			   conf->force_reload, refwwid != NULL, cmd);
 
 out:
 	if (refwwid)
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 60ec48b9904a..202438795ee5 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -750,7 +750,7 @@ cli_add_path (void * v, char ** reply, int * len, void * data)
 		pp->checkint = conf->checkint;
 	}
 	put_multipath_config(conf);
-	return ev_add_path(pp, vecs, 1);
+	return ev_add_path(pp, vecs, ADD_PATH_DOMAP_FORCE);
 blacklisted:
 	*reply = strdup("blacklisted\n");
 	*len = strlen(*reply) + 1;
@@ -813,7 +813,8 @@ cli_add_map (void * v, char ** reply, int * len, void * data)
 					 vecs->pathvec, &refwwid);
 			if (refwwid) {
 				if (coalesce_paths(vecs, NULL, refwwid,
-						   FORCE_RELOAD_NONE, CMD_NONE))
+						   FORCE_RELOAD_NONE, true,
+						   CMD_NONE))
 					condlog(2, "%s: coalesce_paths failed",
 									param);
 				dm_lib_release();
diff --git a/multipathd/main.c b/multipathd/main.c
index ea8c413f28c6..8fd7d2b75cba 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -933,7 +933,8 @@ rescan:
 		mpp->action = ACT_RELOAD;
 		extract_hwe_from_path(mpp);
 	} else {
-		if (!should_multipath(pp, vecs->pathvec, vecs->mpvec)) {
+		if (!should_multipath(pp, vecs->pathvec, vecs->mpvec,
+				      need_do_map == ADD_PATH_DOMAP_FORCE)) {
 			orphan_path(pp, "only one path");
 			return 0;
 		}
@@ -953,7 +954,7 @@ rescan:
 	/* persistent reservation check*/
 	mpath_pr_event_handle(pp);
 
-	if (!need_do_map)
+	if (need_do_map == ADD_PATH_DOMAP_NO)
 		return 0;
 
 	if (!dm_map_present(mpp->alias)) {
@@ -1863,7 +1864,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
 			conf = get_multipath_config();
 			ret = pathinfo(pp, conf, DI_ALL | DI_BLACKLIST);
 			if (ret == PATHINFO_OK) {
-				ev_add_path(pp, vecs, 1);
+				ev_add_path(pp, vecs, ADD_PATH_DOMAP_YES);
 				pp->tick = 1;
 			} else if (ret == PATHINFO_SKIPPED) {
 				put_multipath_config(conf);
@@ -1989,7 +1990,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
 		}
 		if (!disable_reinstate && reinstate_path(pp, add_active)) {
 			condlog(3, "%s: reload map", pp->dev);
-			ev_add_path(pp, vecs, 1);
+			ev_add_path(pp, vecs, ADD_PATH_DOMAP_YES);
 			pp->tick = 1;
 			return 0;
 		}
@@ -2012,7 +2013,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
 			/* Clear IO errors */
 			if (reinstate_path(pp, 0)) {
 				condlog(3, "%s: reload map", pp->dev);
-				ev_add_path(pp, vecs, 1);
+				ev_add_path(pp, vecs, ADD_PATH_DOMAP_YES);
 				pp->tick = 1;
 				return 0;
 			}
@@ -2288,7 +2289,7 @@ configure (struct vectors * vecs)
 	 * superfluous ACT_RELOAD ioctls. Later calls are done
 	 * with FORCE_RELOAD_YES.
 	 */
-	ret = coalesce_paths(vecs, mpvec, NULL, force_reload, CMD_NONE);
+	ret = coalesce_paths(vecs, mpvec, NULL, force_reload, false, CMD_NONE);
 	if (force_reload == FORCE_RELOAD_WEAK)
 		force_reload = FORCE_RELOAD_YES;
 	if (ret) {
diff --git a/multipathd/main.h b/multipathd/main.h
index af395589ff93..a92650cd958b 100644
--- a/multipathd/main.h
+++ b/multipathd/main.h
@@ -22,6 +22,14 @@ void exit_daemon(void);
 const char * daemon_status(void);
 int need_to_delay_reconfig (struct vectors *);
 int reconfigure (struct vectors *);
+/*
+ * 3rd argument of ev_add_path()
+ */
+enum {
+	ADD_PATH_DOMAP_NO = 0,	  /* don't call domap */
+	ADD_PATH_DOMAP_YES = 1,	  /* call domap, don't retry failed */
+	ADD_PATH_DOMAP_FORCE = 2, /* call domap, retry previously failed */
+};
 int ev_add_path (struct path *, struct vectors *, int);
 int ev_remove_path (struct path *, struct vectors *, int);
 int ev_add_map (char *, const char *, struct vectors *);
-- 
2.16.1

[PATCH v2 12/20] multipath -u: common code path for result message

[Martin Wilck]

Print the result message in one place only. This simplifies
future changes. multipath -c is also affected.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipath/main.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/multipath/main.c b/multipath/main.c
index 566404e56ef4..ea4a3d44493a 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -350,6 +350,17 @@ out:
 	return r;
 }
 
+static void print_cmd_valid(const char *devpath, int k)
+{
+	const char *msg[] = { "is", "is not" };
+
+	if (k < 0 || k >= sizeof(msg))
+		return;
+
+	printf("%s %s a valid multipath device path\n",
+	       devpath, msg[k]);
+}
+
 /*
  * Return value:
  *  -1: Retry
@@ -391,10 +402,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
 	    cmd != CMD_REMOVE_WWID &&
 	    (filter_devnode(conf->blist_devnode,
 			    conf->elist_devnode, dev) > 0)) {
-		if (cmd == CMD_VALID_PATH)
-			printf("%s is not a valid multipath device path\n",
-			       devpath);
-		goto out;
+		goto print_valid;
 	}
 
 	/*
@@ -407,7 +415,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
 		if (!refwwid) {
 			condlog(4, "%s: failed to get wwid", devpath);
 			if (failed == 2 && cmd == CMD_VALID_PATH)
-				printf("%s is not a valid multipath device path\n", devpath);
+				goto print_valid;
 			else
 				condlog(3, "scope is null");
 			goto out;
@@ -450,9 +458,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
 				r = 0;
 			if (r == 0 ||
 			    !find_multipaths_on(conf) || !ignore_wwids(conf)) {
-				printf("%s %s a valid multipath device path\n",
-				       devpath, r == 0 ? "is" : "is not");
-				goto out;
+				goto print_valid;
 			}
 		}
 	}
@@ -496,9 +502,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
 		 * the refwwid, then the path is valid */
 		if (VECTOR_SIZE(curmp) != 0 || VECTOR_SIZE(pathvec) > 1)
 			r = 0;
-		printf("%s %s a valid multipath device path\n",
-		       devpath, r == 0 ? "is" : "is not");
-		goto out;
+		goto print_valid;
 	}
 
 	if (cmd != CMD_CREATE && cmd != CMD_DRY_RUN) {
@@ -514,6 +518,10 @@ configure (struct config *conf, enum mpath_cmds cmd,
 	r = coalesce_paths(&vecs, NULL, refwwid,
 			   conf->force_reload, refwwid != NULL, cmd);
 
+print_valid:
+	if (cmd == CMD_VALID_PATH)
+		print_cmd_valid(devpath, r);
+
 out:
 	if (refwwid)
 		FREE(refwwid);
-- 
2.16.1

[PATCH v2 13/20] multipath -u: change output to environment/key format

[Martin Wilck]

... instead of free format. This provides more flexibility
for udev rule processing for the future. Adapt code in multipath.rules.
The exit status remains as usual. This affects "multipath -c", too.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipath/main.c          | 11 +++++------
 multipath/multipath.rules |  6 +++---
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/multipath/main.c b/multipath/main.c
index ea4a3d44493a..f93cad2abde0 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -350,15 +350,14 @@ out:
 	return r;
 }
 
-static void print_cmd_valid(const char *devpath, int k)
+static void print_cmd_valid(int k)
 {
-	const char *msg[] = { "is", "is not" };
+	int vals[] = { 1, 0 };
 
-	if (k < 0 || k >= sizeof(msg))
+	if (k < 0 || k >= sizeof(vals))
 		return;
 
-	printf("%s %s a valid multipath device path\n",
-	       devpath, msg[k]);
+	printf("DM_MULTIPATH_DEVICE_PATH=\"%d\"\n", vals[k]);
 }
 
 /*
@@ -520,7 +519,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
 
 print_valid:
 	if (cmd == CMD_VALID_PATH)
-		print_cmd_valid(devpath, r);
+		print_cmd_valid(r);
 
 out:
 	if (refwwid)
diff --git a/multipath/multipath.rules b/multipath/multipath.rules
index 6f8ee2be0a58..aab64dc7182c 100644
--- a/multipath/multipath.rules
+++ b/multipath/multipath.rules
@@ -19,9 +19,9 @@ LABEL="test_dev"
 ENV{MPATH_SBIN_PATH}="/sbin"
 TEST!="$env{MPATH_SBIN_PATH}/multipath", ENV{MPATH_SBIN_PATH}="/usr/sbin"
 
-ENV{DM_MULTIPATH_DEVICE_PATH}!="1", \
-	PROGRAM=="$env{MPATH_SBIN_PATH}/multipath -u %k", \
-	ENV{DM_MULTIPATH_DEVICE_PATH}="1", ENV{ID_FS_TYPE}="mpath_member", \
+# multipath -u sets DM_MULTIPATH_DEVICE_PATH
+ENV{DM_MULTIPATH_DEVICE_PATH}!="1", IMPORT{program}="$env{MPATH_SBIN_PATH}/multipath -u %k"
+ENV{DM_MULTIPATH_DEVICE_PATH}=="1", ENV{ID_FS_TYPE}="mpath_member", \
 	ENV{SYSTEMD_READY}="0"
 
 LABEL="end_mpath"
-- 
2.16.1

[PATCH v2 14/20] multipath -u: add DM_MULTIPATH_DEVICE_PATH=2 for "maybe"

[Martin Wilck]

Use DM_MULTIPATH_DEVICE_PATH="2" to indicate that this might be a
valid path, but we aren't certain. This happens with find_multipaths
"smart", when the first path to a device (or a single-path
device) is encountered, and the device is neither blacklisted,
nor marked failed, nor whitelisted in the wwids file.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipath/main.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/multipath/main.c b/multipath/main.c
index f93cad2abde0..9b53c13b3a78 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -350,14 +350,15 @@ out:
 	return r;
 }
 
-static void print_cmd_valid(int k)
+static int print_cmd_valid(int k)
 {
-	int vals[] = { 1, 0 };
+	int vals[] = { 1, 0, 2 };
 
 	if (k < 0 || k >= sizeof(vals))
-		return;
+		return 1;
 
 	printf("DM_MULTIPATH_DEVICE_PATH=\"%d\"\n", vals[k]);
+	return k == 1;
 }
 
 /*
@@ -501,6 +502,9 @@ configure (struct config *conf, enum mpath_cmds cmd,
 		 * the refwwid, then the path is valid */
 		if (VECTOR_SIZE(curmp) != 0 || VECTOR_SIZE(pathvec) > 1)
 			r = 0;
+		else
+			/* Use r=2 as an indication for "maybe" */
+			r = 2;
 		goto print_valid;
 	}
 
@@ -519,7 +523,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
 
 print_valid:
 	if (cmd == CMD_VALID_PATH)
-		print_cmd_valid(r);
+		r = print_cmd_valid(r);
 
 out:
 	if (refwwid)
-- 
2.16.1

[PATCH v2 15/20] libmultipath: implement find_multipaths_timeout

[Martin Wilck]

This makes the timeout for "find_multipaths smart" configurable.
If the timeout has a negative value (default), it's applied only
to "known" hardware which is either in the hwtable or in a "device" section in
multipath.conf. For typical non-multipath hardware, which is not in the
hwtable, a short timeout of 1s is used, so that boot delays caused by
pointlessly waiting e.g. for SATA devices will be minimal.

It's expected that a "reasonable" timeout value depends less on the storage
hardware itself but on other properties of the data center such as network
latencies or distances. find_multipaths_timeout is therefore just a "defaults"
section setting.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/config.h      |  1 +
 libmultipath/defaults.h    |  2 ++
 libmultipath/dict.c        |  6 ++++++
 libmultipath/propsel.c     | 25 +++++++++++++++++++++++++
 libmultipath/propsel.h     |  1 +
 libmultipath/structs.h     |  1 +
 multipath/main.c           | 13 +++++++++++--
 multipath/multipath.conf.5 | 18 ++++++++++++++++++
 8 files changed, 65 insertions(+), 2 deletions(-)

diff --git a/libmultipath/config.h b/libmultipath/config.h
index 21d8e72a2492..72d66018893b 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -174,6 +174,7 @@ struct config {
 	int remove_retries;
 	int max_sectors_kb;
 	int ghost_delay;
+	int find_multipaths_timeout;
 	unsigned int version[3];
 
 	char * multipath_dir;
diff --git a/libmultipath/defaults.h b/libmultipath/defaults.h
index 690182c368d9..018eb01c1830 100644
--- a/libmultipath/defaults.h
+++ b/libmultipath/defaults.h
@@ -41,6 +41,8 @@
 #define DEFAULT_DISABLE_CHANGED_WWIDS 1
 #define DEFAULT_MAX_SECTORS_KB MAX_SECTORS_KB_UNDEF
 #define DEFAULT_GHOST_DELAY GHOST_DELAY_OFF
+#define DEFAULT_FIND_MULTIPATHS_TIMEOUT -10
+#define DEFAULT_UNKNOWN_FIND_MULTIPATHS_TIMEOUT 1
 
 #define DEFAULT_CHECKINT	5
 #define MAX_CHECKINT(a)		(a << 2)
diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index a952c60a6f98..d59fdc85a9cb 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -484,6 +484,10 @@ declare_hw_snprint(max_sectors_kb, print_nonzero)
 declare_mp_handler(max_sectors_kb, set_int)
 declare_mp_snprint(max_sectors_kb, print_nonzero)
 
+declare_def_handler(find_multipaths_timeout, set_int)
+declare_def_snprint_defint(find_multipaths_timeout, print_int,
+			   DEFAULT_FIND_MULTIPATHS_TIMEOUT)
+
 static int
 def_config_dir_handler(struct config *conf, vector strvec)
 {
@@ -1518,6 +1522,8 @@ init_keywords(vector keywords)
 	install_keyword("remove_retries", &def_remove_retries_handler, &snprint_def_remove_retries);
 	install_keyword("max_sectors_kb", &def_max_sectors_kb_handler, &snprint_def_max_sectors_kb);
 	install_keyword("ghost_delay", &def_ghost_delay_handler, &snprint_def_ghost_delay);
+	install_keyword("find_multipaths_timeout", &def_find_multipaths_timeout_handler,
+			&snprint_def_find_multipaths_timeout);
 	__deprecated install_keyword("default_selector", &def_selector_handler, NULL);
 	__deprecated install_keyword("default_path_grouping_policy", &def_pgpolicy_handler, NULL);
 	__deprecated install_keyword("default_uid_attribute", &def_uid_attribute_handler, NULL);
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index 58a6a42fe333..87d6865d17e6 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -911,3 +911,28 @@ out:
 	condlog(3, "%s: ghost_delay = %s %s", mp->alias, buff, origin);
 	return 0;
 }
+
+int select_find_multipaths_timeout(struct config *conf, struct path *pp)
+{
+	char *origin;
+
+	pp_set_conf(find_multipaths_timeout);
+	pp_set_default(find_multipaths_timeout,
+		       DEFAULT_FIND_MULTIPATHS_TIMEOUT);
+out:
+	/*
+	 * If configured value is negative, and this "unkown" hardware
+	 * (no hwentry), use very small timeout to avoid delays.
+	 */
+	if (pp->find_multipaths_timeout < 0) {
+		pp->find_multipaths_timeout = - pp->find_multipaths_timeout;
+		if (!pp->hwe) {
+			pp->find_multipaths_timeout =
+				DEFAULT_UNKNOWN_FIND_MULTIPATHS_TIMEOUT;
+			origin = "(default for unkown hardware)";
+		}
+	}
+	condlog(3, "%s: timeout for find_multipaths \"smart\" = %ds %s",
+		pp->dev, pp->find_multipaths_timeout, origin);
+	return 0;
+}
diff --git a/libmultipath/propsel.h b/libmultipath/propsel.h
index 136f90605b91..b6475d0d9c71 100644
--- a/libmultipath/propsel.h
+++ b/libmultipath/propsel.h
@@ -8,6 +8,7 @@ int select_hwhandler (struct config *conf, struct multipath * mp);
 int select_checker(struct config *conf, struct path *pp);
 int select_getuid (struct config *conf, struct path * pp);
 int select_prio (struct config *conf, struct path * pp);
+int select_find_multipaths_timeout(struct config *conf, struct path * pp);
 int select_no_path_retry(struct config *conf, struct multipath *mp);
 int select_flush_on_last_del(struct config *conf, struct multipath *mp);
 int select_minio(struct config *conf, struct multipath *mp);
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 32f4b2d0696c..d4e18df940ea 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -281,6 +281,7 @@ struct path {
 	int io_err_disable_reinstate;
 	int io_err_pathfail_cnt;
 	int io_err_pathfail_starttime;
+	int find_multipaths_timeout;
 	/* configlet pointers */
 	struct hwentry * hwe;
 	struct gen_path generic_path;
diff --git a/multipath/main.c b/multipath/main.c
index 9b53c13b3a78..b41621d267f7 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -60,6 +60,7 @@
 #include "uxsock.h"
 #include "mpath_cmd.h"
 #include "foreign.h"
+#include "propsel.h"
 
 int logsink;
 struct udev *udev;
@@ -350,7 +351,8 @@ out:
 	return r;
 }
 
-static int print_cmd_valid(int k)
+static int print_cmd_valid(int k, const vector pathvec,
+			   struct config *conf)
 {
 	int vals[] = { 1, 0, 2 };
 
@@ -358,6 +360,13 @@ static int print_cmd_valid(int k)
 		return 1;
 
 	printf("DM_MULTIPATH_DEVICE_PATH=\"%d\"\n", vals[k]);
+	if (vals[k] == 2 && VECTOR_SIZE(pathvec) > 0) {
+		struct path *pp = VECTOR_SLOT(pathvec, 0);
+
+		select_find_multipaths_timeout(conf, pp);
+		printf("FIND_MULTIPATHS_PATH_TMO=\"%d\"\n",
+		       pp->find_multipaths_timeout);
+	}
 	return k == 1;
 }
 
@@ -523,7 +532,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
 
 print_valid:
 	if (cmd == CMD_VALID_PATH)
-		r = print_cmd_valid(r);
+		r = print_cmd_valid(r, pathvec, conf);
 
 out:
 	if (refwwid)
diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
index 6965dacb5c68..94c419a078bf 100644
--- a/multipath/multipath.conf.5
+++ b/multipath/multipath.conf.5
@@ -983,6 +983,24 @@ The default is: \fBstrict\fR
 .
 .
 .TP
+.B find_multipaths_timeout
+Timeout, in seconds, to wait for additional paths after detecting the first
+one, if \fIfind_multipaths
+"smart"\fR (see above) is set. If the value is \fBpositive\fR, this timeout is used for all
+unkown, non-blacklisted devices encountered. If the value is \fBnegative\fR
+(recommended), it's only
+applied to "known" devices that have an entry in multipath's hardware table,
+either in the built-in table or in a \fIdevice\fR section; other ("unknown") devices will
+use a timeout of only 1 second to avoid booting delays. The value 0 means
+"use the built-in default". If \fIfind_multipath\fR has a value
+other than \fIsmart\fR, this option has no effect. 
+.RS
+.TP
+The default is: \fB-10\fR (10s for known and 1s for unknown hardware)
+.RE
+.
+.
+.TP
 .B uxsock_timeout
 CLI receive timeout in milliseconds. For larger systems CLI commands
 might timeout before the multipathd lock is released and the CLI command
-- 
2.16.1

[PATCH v2 16/20] libmultipath: enable find_multipaths "smart"

[Martin Wilck]

This activates "smart" path detection. This is similar to
"find_multipaths yes", but doesn't generally ignore single paths
that are not listed in the WWIDs file. Rather, such paths are
temporarily treated like multipath members. If no additional paths
are detected after a certain time, the paths are re-added to the
system as non-multipath. This needs support by the udev rules, to
be added in a follow-up patch.

If a multipath map is successfully successfully created, and paths are
in waiting state, trigger path uevents to update their status.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c   | 15 ++++++++++++---
 libmultipath/dict.c        |  1 +
 multipath/multipath.conf.5 | 13 +++++++++++++
 3 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 98b80337d899..ec1ad9c90f05 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -12,6 +12,7 @@
 #include <string.h>
 #include <sys/file.h>
 #include <errno.h>
+#include <ctype.h>
 #include <libdevmapper.h>
 #include <libudev.h>
 #include "mpath_cmd.h"
@@ -474,9 +475,17 @@ trigger_paths_udev_change(struct multipath *mpp, bool is_mpath)
 			env = udev_device_get_property_value(
 				pp->udev, "DM_MULTIPATH_DEVICE_PATH");
 
-			if (is_mpath && env != NULL && !strcmp(env, "1"))
-				continue;
-			else if (!is_mpath &&
+			if (is_mpath && env != NULL && !strcmp(env, "1")) {
+				/*
+				 * If FIND_MULTIPATHS_WAIT_UNTIL is a number,
+				 * path is in "maybe" state and timer is running
+				 * Send uevent now (see multipath.rules).
+				 */
+				env = udev_device_get_property_value(
+					pp->udev, "FIND_MULTIPATHS_WAIT_UNTIL");
+				if (env == NULL || !isdigit(env[0]))
+					continue;
+			} else if (!is_mpath &&
 				   (env == NULL || !strcmp(env, "0")))
 				continue;
 
diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index d59fdc85a9cb..1237c23ae7db 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -238,6 +238,7 @@ static const char *find_multipaths_optvals[] = {
 	[FIND_MULTIPATHS_ON] = "on",
 	[FIND_MULTIPATHS_STRICT] = "strict",
 	[FIND_MULTIPATHS_GREEDY] = "greedy",
+	[FIND_MULTIPATHS_SMART] = "smart",
 };
 
 static int
diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
index 94c419a078bf..641ba43ad158 100644
--- a/multipath/multipath.conf.5
+++ b/multipath/multipath.conf.5
@@ -978,6 +978,19 @@ with the same WWID have been detected.
 Both multipathd and multipath treat every non-blacklisted device as multipath
 device path.
 .TP
+.I smart
+This differs from \fIfind_multipaths yes\fR only in
+the way it treats new devices for which only one path has been
+detected yet. When such a device is first encounted in udev rules, it is
+treated as a multipath device. multipathd waits whether additional paths with
+the same WWID appears. If that happens, it sets up a multipath map. If it
+doesn\'t happen until a 
+timeout expires, or if setting up the map fails, a new uevent is triggered for
+the device; at second encounter in the udev rules, the device will be treated
+as non-multipath and passed on to upper layers.
+\fBNote:\fR this may cause delays during device detection if
+there are single-path devices which aren\'t blacklisted.
+.TP
 The default is: \fBstrict\fR
 .RE
 .
-- 
2.16.1

[PATCH v2 17/20] multipath.rules: find_multipaths "smart" logic

[Martin Wilck]

When the first path to a device appears, we don't know if more paths are going
to follow. find_multipath "smart" logic attempts to solve this dilemma by
waiting for additional paths for a configurable time before giving up
and releasing single paths to upper layers.

These rules apply only if both find_multipaths is set to "smart" in
multipath.conf. In this mode, multipath -u sets DM_MULTIPATH_DEVICE_PATH=2 if
there's no clear evidence wheteher a given device should be a multipath member
(not blacklisted, not listed as "failed", not in WWIDs file, not member of an
exisiting map, only one path seen yet).

In this case, pretend that the path is multipath member, disallow further
processing by systemd (allowing multipathd some time to grab the path),
and check again after some time. If the path is still not multipathed by then,
pass it on to systemd for further processing.

The timeout is controlled by the "find_multipaths_timeout" config option.
Note that delays caused by waiting don't "add up" during boot, because the
timers run concurrently.

Implementation note: This logic requires obtaining the current time. It's not
trivial to do this in udev rules in a portable way, because "/bin/date" is
often not available in restricted environments such as the initrd. I chose
the sysfs method, because /sys/class/rtc/rtc0 seems to be quite universally
available. I'm open for better suggestions if there are any.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipath/multipath.rules | 80 +++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 78 insertions(+), 2 deletions(-)

diff --git a/multipath/multipath.rules b/multipath/multipath.rules
index aab64dc7182c..32d33991db3d 100644
--- a/multipath/multipath.rules
+++ b/multipath/multipath.rules
@@ -21,7 +21,83 @@ TEST!="$env{MPATH_SBIN_PATH}/multipath", ENV{MPATH_SBIN_PATH}="/usr/sbin"
 
 # multipath -u sets DM_MULTIPATH_DEVICE_PATH
 ENV{DM_MULTIPATH_DEVICE_PATH}!="1", IMPORT{program}="$env{MPATH_SBIN_PATH}/multipath -u %k"
-ENV{DM_MULTIPATH_DEVICE_PATH}=="1", ENV{ID_FS_TYPE}="mpath_member", \
-	ENV{SYSTEMD_READY}="0"
+
+# case 1: this is definitely multipath
+ENV{DM_MULTIPATH_DEVICE_PATH}=="1", \
+	ENV{ID_FS_TYPE}="mpath_member", ENV{SYSTEMD_READY}="0", \
+	ENV{FIND_MULTIPATHS_WAIT_UNTIL}="finished", \
+	GOTO="end_mpath"
+
+# case 2: this is definitely not multipath
+ENV{DM_MULTIPATH_DEVICE_PATH}!="2", \
+	ENV{FIND_MULTIPATHS_WAIT_UNTIL}="finished", \
+	GOTO="end_mpath"
+
+# All code below here is only run in "smart" mode.
+
+# FIND_MULTIPATHS_WAIT_UNTIL is the timeout (in seconds after the
+# epoch). If waiting ends for any reason, it is set to "finished".
+IMPORT{db}="FIND_MULTIPATHS_WAIT_UNTIL"
+
+# At this point we know DM_MULTIPATH_DEVICE_PATH==2.
+# (multipath -u indicates this is "maybe" multipath)
+
+# case 3: waiting has already finished. Treat as non-multipath.
+ENV{FIND_MULTIPATHS_WAIT_UNTIL}=="finished", \
+	ENV{DM_MULTIPATH_DEVICE_PATH}="", GOTO="end_mpath"
+
+# The timeout should have been set by the multipath -u call above, set a default
+# value it that didn't happen for whatever reason
+ENV{FIND_MULTIPATHS_PATH_TMO}!="?*", ENV{FIND_MULTIPATHS_PATH_TMO}="5"
+
+PROGRAM="/bin/cat $sys/class/rtc/rtc0/since_epoch", \
+	ENV{.TIME_NOW}="$result"
+ENV{.TIME_NOW}!="?*", ENV{DM_MULTIPATH_DEVICE_PATH}="", \
+	ENV{FIND_MULTIPATHS_WAIT_UNTIL}="error", GOTO="end_mpath"
+
+ENV{FIND_MULTIPATHS_WAIT_UNTIL}!="?*", GOTO="start_wait"
+
+# At this point, we know that FIND_MULTIPATHS_WAIT_UNTIL is a timeout value.
+# If it's expired, give up waiting and assume this is non-multipath.
+PROGRAM="/bin/sh -c '[ $env{.TIME_NOW} -ge $env{FIND_MULTIPATHS_WAIT_UNTIL} ]'", \
+	ENV{DM_MULTIPATH_DEVICE_PATH}="", \
+	ENV{FIND_MULTIPATHS_WAIT_UNTIL}="finished", \
+	GOTO="end_mpath"
+
+# Timer not expired yet. The current uevent has not been triggered by the
+# systemd timer but something else, e.g. 60-block.rules. Continue pretending.
+ACTION!="add", GOTO="pretend_mpath"
+
+# Special case: Another "add" event happened before the timeout expired.
+# This can happen during "coldplug" after switching root FS, if a
+# systemd timer started during initramfs processing had been cancelled.
+# We need to start the timer again.
+
+LABEL="start_wait"
+
+# We are seeing this path for the first time, and it's "maybe" multipath.
+# Pretend that it is actually multipath, and set a timer to create another
+# uevent at FIND_MULTIPATHS_WAIT_UNTIL seconds after the epoch.
+ENV{FIND_MULTIPATHS_WAIT_UNTIL}!="?*", \
+	PROGRAM="/bin/sh -c 'echo $(( $env{.TIME_NOW} + $env{FIND_MULTIPATHS_PATH_TMO} ))'", \
+	ENV{FIND_MULTIPATHS_WAIT_UNTIL}="$result"
+
+# The actual start command for the timer.
+#
+# The purpose of this command is only to make sure we will receive another
+# uevent eventually. *Any* uevent may cause waiting to finish if it either ends
+# in case 1-3 above, or if it arrives after FIND_MULTIPATHS_WAIT_UNTIL.
+#
+# Note that this will try to activate multipathd if it isn't running yet.
+# If that fails, the unit starts and expires nonetheless. If multipathd
+# startup needs to wait for other services, this wait time will add up with
+# the --on-active timeout.
+#
+# We must trigger an "add" event because LVM2 will only act on those.
+RUN+="/usr/bin/systemd-run --unit=cancel-multipath-wait-$kernel --description 'cancel waiting for multipath siblings of $kernel' --no-block --timer-property DefaultDependencies=no --timer-property Conflicts=shutdown.target --timer-property Before=shutdown.target --timer-property AccuracySec=500ms --property DefaultDependencies=no --property Conflicts=shutdown.target --property Before=shutdown.target --property Wants=multipathd.service --property After=multipathd.service --on-active=$env{FIND_MULTIPATHS_PATH_TMO} /usr/bin/udevadm trigger --action=add $sys$devpath"
+
+LABEL="pretend_mpath"
+ENV{DM_MULTIPATH_DEVICE_PATH}="1"
+ENV{SYSTEMD_READY}="0"
 
 LABEL="end_mpath"
-- 
2.16.1

[PATCH v2 18/20] multipathd: decrease log level of "spurious uevent" message

[Martin Wilck]

We trigger such events under various conditions, there's no point
logging them at -v2.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 8fd7d2b75cba..ab6286f942ce 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -814,7 +814,7 @@ uev_add_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
 	if (pp) {
 		int r;
 
-		condlog(2, "%s: spurious uevent, path already in pathvec",
+		condlog(3, "%s: spurious uevent, path already in pathvec",
 			uev->kernel);
 		if (!pp->mpp && !strlen(pp->wwid)) {
 			condlog(3, "%s: reinitialize path", uev->kernel);
-- 
2.16.1

[PATCH v2 19/20] libmultipath: decrease log level of uevent filter/merge messages

[Martin Wilck]

There should be no need to log these at -v2. The messages are
informational.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/uevent.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index c6a9e8b485ab..7e2e0f6761f7 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -330,7 +330,7 @@ uevent_filter(struct uevent *later, struct list_head *tmpq)
 		 * by the later uevent
 		 */
 		if (uevent_can_filter(earlier, later)) {
-			condlog(2, "uevent: %s-%s has filtered by uevent: %s-%s",
+			condlog(3, "uevent: %s-%s has filtered by uevent: %s-%s",
 				earlier->kernel, earlier->action,
 				later->kernel, later->action);
 
@@ -354,7 +354,7 @@ uevent_merge(struct uevent *later, struct list_head *tmpq)
 		 * merge earlier uevents to the later uevent
 		 */
 		if (uevent_can_merge(earlier, later)) {
-			condlog(2, "merged uevent: %s-%s-%s with uevent: %s-%s-%s",
+			condlog(3, "merged uevent: %s-%s-%s with uevent: %s-%s-%s",
 				earlier->action, earlier->kernel, earlier->wwid,
 				later->action, later->kernel, later->wwid);
 
-- 
2.16.1

[PATCH v2 20/20] multipathd: decrease log level of waiter thread start/stop msgs

[Martin Wilck]

These rather unimportant messages account for a large portion
of multipathd's log messages with the default verbosity level (2).

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/waiter.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/multipathd/waiter.c b/multipathd/waiter.c
index 595c69a2d5ba..6c896f7fb7db 100644
--- a/multipathd/waiter.c
+++ b/multipathd/waiter.c
@@ -62,7 +62,7 @@ void stop_waiter_thread (struct multipath *mpp, struct vectors *vecs)
 	if (pthread_equal(mpp->waiter, pthread_self()))
 		return;
 
-	condlog(2, "%s: stop event checker thread (%lu)", mpp->alias,
+	condlog(3, "%s: stop event checker thread (%lu)", mpp->alias,
 		mpp->waiter);
 	thread = mpp->waiter;
 	mpp->waiter = (pthread_t)0;
@@ -219,7 +219,7 @@ int start_waiter_thread (struct multipath *mpp, struct vectors *vecs)
 		goto out1;
 	}
 	mpp->waiter = wp->thread;
-	condlog(2, "%s: event checker started", wp->mapname);
+	condlog(3, "%s: event checker started", wp->mapname);
 
 	return 0;
 out1:
-- 
2.16.1

Re: [PATCH v2 01/20] Revert "multipath: ignore -i if find_multipaths is set"

[Benjamin Marzinski]

On Mon, Mar 19, 2018 at 04:01:36PM +0100, Martin Wilck wrote:
> This reverts commit ffbb886a8a16cb063d669cd76a1e656fd3ec8c4b.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipath/main.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/multipath/main.c b/multipath/main.c
> index 716203eab56c..20f46acd924a 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -776,16 +776,6 @@ main (int argc, char *argv[])
>  		}
>  	}
>  
> -	/*
> -	 * FIXME: new device detection with find_multipaths currently
> -	 * doesn't work reliably.
> -	 */
> -	if (cmd ==  CMD_VALID_PATH &&
> -	    conf->find_multipaths && conf->ignore_wwids) {
> -		condlog(2, "ignoring -i flag because find_multipath is set in multipath.conf");
> -		conf->ignore_wwids = 0;
> -	}
> -
>  	if (getuid() != 0) {
>  		fprintf(stderr, "need to be root\n");
>  		exit(1);
> -- 
> 2.16.1

Re: [PATCH v2 02/20] Revert "multipathd: imply -n if find_multipaths is set"

[Benjamin Marzinski]

On Mon, Mar 19, 2018 at 04:01:37PM +0100, Martin Wilck wrote:
> This reverts commit 64e27ec066a001012f44550f095c93443e91d845.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipathd/main.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 3ae04422a123..0435133fadfb 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2389,10 +2389,6 @@ reconfigure (struct vectors * vecs)
>  		conf->verbosity = verbosity;
>  	if (bindings_read_only)
>  		conf->bindings_read_only = bindings_read_only;
> -	if (conf->find_multipaths) {
> -		condlog(2, "find_multipaths is set: -n is implied");
> -		ignore_new_devs = 1;
> -	}
>  	if (ignore_new_devs)
>  		conf->ignore_new_devs = ignore_new_devs;
>  	uxsock_timeout = conf->uxsock_timeout;
> -- 
> 2.16.1

Re: [PATCH v2 03/20] libmultipath: should_multipath: keep existing maps

[Benjamin Marzinski]

On Mon, Mar 19, 2018 at 04:01:38PM +0100, Martin Wilck wrote:
> with find_multipaths "yes" and without the "-n" option to multipathd,
> if a path is already multipathed, keep it. The same logic is applied by
> "multipath -u -i".
> 
> To do this, we need to add a "mpvec" parameter to should_multipath().
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/configure.c |  2 +-
>  libmultipath/wwids.c     | 12 +++++++++++-
>  libmultipath/wwids.h     |  2 +-
>  multipathd/main.c        |  2 +-
>  4 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index fa6e21cb31af..16ce797c7d44 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -979,7 +979,7 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
>  			continue;
>  
>  		/* If find_multipaths was selected check if the path is valid */
> -		if (!refwwid && !should_multipath(pp1, pathvec)) {
> +		if (!refwwid && !should_multipath(pp1, pathvec, curmp)) {
>  			orphan_path(pp1, "only one path");
>  			continue;
>  		}
> diff --git a/libmultipath/wwids.c b/libmultipath/wwids.c
> index bc70a27409d3..cb6ab52aaa5b 100644
> --- a/libmultipath/wwids.c
> +++ b/libmultipath/wwids.c
> @@ -15,6 +15,7 @@
>  #include "wwids.h"
>  #include "defaults.h"
>  #include "config.h"
> +#include "devmapper.h"
>  
>  /*
>   * Copyright (c) 2010 Benjamin Marzinski, Redhat
> @@ -271,7 +272,7 @@ out:
>  }
>  
>  int
> -should_multipath(struct path *pp1, vector pathvec)
> +should_multipath(struct path *pp1, vector pathvec, vector mpvec)
>  {
>  	int i, ignore_new_devs;
>  	struct path *pp2;
> @@ -287,6 +288,15 @@ should_multipath(struct path *pp1, vector pathvec)
>  
>  	condlog(4, "checking if %s should be multipathed", pp1->dev);
>  	if (!ignore_new_devs) {
> +		char tmp_wwid[WWID_SIZE];
> +		struct multipath *mp = find_mp_by_wwid(mpvec, pp1->wwid);
> +
> +		if (mp != NULL && dm_get_uuid(mp->alias, tmp_wwid) == 0 &&
> +		    !strncmp(tmp_wwid, pp1->wwid, WWID_SIZE)) {
> +			condlog(3, "wwid %s is already multipathed, keeping it",
> +				pp1->wwid);
> +			return 1;
> +		}
>  		vector_foreach_slot(pathvec, pp2, i) {
>  			if (pp1->dev == pp2->dev)
>  				continue;
> diff --git a/libmultipath/wwids.h b/libmultipath/wwids.h
> index 95270129daa0..d9a78b38ccf8 100644
> --- a/libmultipath/wwids.h
> +++ b/libmultipath/wwids.h
> @@ -12,7 +12,7 @@
>  "#\n" \
>  "# Valid WWIDs:\n"
>  
> -int should_multipath(struct path *pp, vector pathvec);
> +int should_multipath(struct path *pp, vector pathvec, vector mpvec);
>  int remember_wwid(char *wwid);
>  int check_wwids_file(char *wwid, int write_wwid);
>  int remove_wwid(char *wwid);
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 0435133fadfb..707245c67231 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -933,7 +933,7 @@ rescan:
>  		mpp->action = ACT_RELOAD;
>  		extract_hwe_from_path(mpp);
>  	} else {
> -		if (!should_multipath(pp, vecs->pathvec)) {
> +		if (!should_multipath(pp, vecs->pathvec, vecs->mpvec)) {
>  			orphan_path(pp, "only one path");
>  			return 0;
>  		}
> -- 
> 2.16.1

Re: [PATCH v2 04/20] multipath -u -i: respect entries in WWIDs file

[Benjamin Marzinski]

On Mon, Mar 19, 2018 at 04:01:39PM +0100, Martin Wilck wrote:
> Previously, if find_multipaths was set, devices listed in the WWIDs file
> weren't classified as multipath members by "multipath -u -i" unless they also
> met the "find_multipaths" criteria (at least two paths, or existing map with
> this WWID). Now we classify all paths in the WWIDs file as multipath members, too.
> 
> The rationale for this patch is to match the logic that multipathd applies
> by default (i.e. without "-n").
> 

Still calling the option ignore_wwids is a little confusing when you
aren't actually ignoring the wwids file (you're just not requiring it),
but at any rate...

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipath/main.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/multipath/main.c b/multipath/main.c
> index 20f46acd924a..4d45df3cdb83 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -437,16 +437,19 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  		 * set, you need to actually check if there are two available
>  		 * paths to determine if this path should be multipathed. To
>  		 * do this, we put off the check until after discovering all
> -		 * the paths */
> -		if (cmd == CMD_VALID_PATH &&
> -		    (!conf->find_multipaths || !conf->ignore_wwids)) {
> -			if (conf->ignore_wwids ||
> +		 * the paths.
> +		 * Paths listed in the wwids file are always considered valid.
> +		 */
> +		if (cmd == CMD_VALID_PATH) {
> +			if ((!conf->find_multipaths && conf->ignore_wwids) ||
>  			    check_wwids_file(refwwid, 0) == 0)
>  				r = 0;
> -
> -			printf("%s %s a valid multipath device path\n",
> -			       devpath, r == 0 ? "is" : "is not");
> -			goto out;
> +			if (r == 0 ||
> +			    !conf->find_multipaths || !conf->ignore_wwids) {
> +				printf("%s %s a valid multipath device path\n",
> +				       devpath, r == 0 ? "is" : "is not");
> +				goto out;
> +			}
>  		}
>  	}
>  
> -- 
> 2.16.1

Re: [PATCH v2 06/20] libmultipath: trigger path uevent only when necessary

[Benjamin Marzinski]

On Mon, Mar 19, 2018 at 04:01:41PM +0100, Martin Wilck wrote:
> Paths that are already classified as DM_MULTIPATH_DEVICE_PATH don't
> need to be retriggered.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/configure.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index 245bd11672cb..838d145a5aa2 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -456,8 +456,20 @@ trigger_paths_udev_change(const struct multipath *mpp)
>  		if (!pgp->paths)
>  			continue;
>  		vector_foreach_slot(pgp->paths, pp, j) {
> +			const char *env;
> +
>  			if (!pp->udev)
>  				continue;
> +			/*
> +			 * Paths that are already classified as multipath
> +			 * members don't need another uevent.
> +			 */
> +			env = udev_device_get_property_value(
> +				pp->udev, "DM_MULTIPATH_DEVICE_PATH");
> +			if (env != NULL && !strcmp(env, "1"))
> +					continue;
> +
> +			condlog(4, "triggering change uevent for %s", pp->dev);
>  			sysfs_attr_set_value(pp->udev, "uevent", "change",
>  					     strlen("change"));
>  		}
> -- 
> 2.16.1

Re: [PATCH v2 07/20] libmultipath: change find_multipaths option to multi-value

[Benjamin Marzinski]

On Mon, Mar 19, 2018 at 04:01:42PM +0100, Martin Wilck wrote:
> Change the "find_multipaths" option from yes/no to multi-value. This
> option now covers the effects of "find_multipaths" as it used to be,
> plus the -i option to multipath (ignore_wwids) and the -n option to
> multipathd (ignore_new_devs), excluding such combinations of the former
> options that are dangerous or inconsistent.
> 
> The possible values for "find_multipaths" are now:
> 
>  - strict: strictly stick to the WWIDs file; never add new maps automatically
>    (new default; upstream behaviour with with find_multipaths "yes" and
>    commit 64e27ec "multipathd: imply -n if find_multipaths is set")
>  - off|no: multipath like "strict", multipathd like "greedy" (previous
>    upstream default)
>  - on|yes: set up multipath if >1 paths are seen (current Red Hat/Ubuntu
>    behavior with find_multipaths "yes")
>  - greedy: set up multipath for all non-blacklisted devices (current SUSE
>    default)
>  - smart: Like "yes", but try to avoid inconsistencies between udev processing
>    and multipathd processing by waiting for path siblings to
>    appear (fully implemented in follow-up patches)
> 
> The default has been changed to "strict", because "no" may cause inconsistent
> behavior between "multipath -u" and multipathd. This is deliberately a
> conservative choice.
> 
> The patch also updates the related man pages.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

This patch conflicts with the "multipath: fix rcu thread cancellation
hang" patch I just sent.  I will be resending that patch anyways as part
of cleaning up my previous patchsets based on you comments, but we
apparently need to do a better job with managing the rcu read side
critical sections, between get_multipath_config() and
put_multipath_config().

> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/config.h      |  2 --
>  libmultipath/defaults.h    |  2 +-
>  libmultipath/dict.c        | 47 ++++++++++++++++++++++++++++++++++++++++++++--
>  libmultipath/structs.h     | 26 +++++++++++++++++++++++++
>  libmultipath/wwids.c       |  8 ++++----
>  multipath/main.c           |  6 +++---
>  multipath/multipath.8      |  9 ++++++++-
>  multipath/multipath.conf.5 | 46 +++++++++++++++++++++++++--------------------
>  multipathd/main.c          |  6 +-----
>  multipathd/multipathd.8    |  8 +++++---
>  10 files changed, 119 insertions(+), 41 deletions(-)
> 
> diff --git a/libmultipath/config.h b/libmultipath/config.h
> index a20ac2aac296..21d8e72a2492 100644
> --- a/libmultipath/config.h
> +++ b/libmultipath/config.h
> @@ -139,7 +139,6 @@ struct config {
>  	int max_fds;
>  	int force_reload;
>  	int queue_without_daemon;
> -	int ignore_wwids;
>  	int checker_timeout;
>  	int flush_on_last_del;
>  	int attribute_flags;
> @@ -168,7 +167,6 @@ struct config {
>  	int strict_timing;
>  	int retrigger_tries;
>  	int retrigger_delay;
> -	int ignore_new_devs;
>  	int delayed_reconfig;
>  	int uev_wait_timeout;
>  	int skip_kpartx;
> diff --git a/libmultipath/defaults.h b/libmultipath/defaults.h
> index 2b270ca46f48..690182c368d9 100644
> --- a/libmultipath/defaults.h
> +++ b/libmultipath/defaults.h
> @@ -17,7 +17,7 @@
>  #define DEFAULT_NO_PATH_RETRY	NO_PATH_RETRY_UNDEF
>  #define DEFAULT_VERBOSITY	2
>  #define DEFAULT_REASSIGN_MAPS	0
> -#define DEFAULT_FIND_MULTIPATHS	0
> +#define DEFAULT_FIND_MULTIPATHS	FIND_MULTIPATHS_STRICT
>  #define DEFAULT_FAST_IO_FAIL	5
>  #define DEFAULT_DEV_LOSS_TMO	600
>  #define DEFAULT_RETAIN_HWHANDLER RETAIN_HWHANDLER_ON
> diff --git a/libmultipath/dict.c b/libmultipath/dict.c
> index ea273dd91962..a952c60a6f98 100644
> --- a/libmultipath/dict.c
> +++ b/libmultipath/dict.c
> @@ -233,8 +233,51 @@ declare_def_snprint(multipath_dir, print_str)
>  declare_def_handler(partition_delim, set_str)
>  declare_def_snprint(partition_delim, print_str)
>  
> -declare_def_handler(find_multipaths, set_yes_no)
> -declare_def_snprint(find_multipaths, print_yes_no)
> +static const char *find_multipaths_optvals[] = {
> +	[FIND_MULTIPATHS_OFF] = "off",
> +	[FIND_MULTIPATHS_ON] = "on",
> +	[FIND_MULTIPATHS_STRICT] = "strict",
> +	[FIND_MULTIPATHS_GREEDY] = "greedy",
> +};
> +
> +static int
> +def_find_multipaths_handler(struct config *conf, vector strvec)
> +{
> +	char *buff;
> +	int i;
> +
> +	if (set_yes_no_undef(strvec, &conf->find_multipaths) == 0 &&
> +	    conf->find_multipaths != YNU_UNDEF)
> +		return 0;
> +
> +	buff = set_value(strvec);
> +	if (!buff)
> +		return 1;
> +
> +	for (i = FIND_MULTIPATHS_OFF; i < __FIND_MULTIPATHS_LAST; i++) {
> +		if (find_multipaths_optvals[i] != NULL &&
> +		    !strcmp(buff, find_multipaths_optvals[i])) {
> +			conf->find_multipaths = i;
> +			break;
> +		}
> +	}
> +
> +	if (conf->find_multipaths == YNU_UNDEF) {
> +		condlog(0, "illegal value for find_multipaths: %s", buff);
> +		conf->find_multipaths = DEFAULT_FIND_MULTIPATHS;
> +	}
> +
> +	FREE(buff);
> +	return 0;
> +}
> +
> +static int
> +snprint_def_find_multipaths(struct config *conf, char *buff, int len,
> +			    const void *data)
> +{
> +	return print_str(buff, len,
> +			 find_multipaths_optvals[conf->find_multipaths]);
> +}
>  
>  declare_def_handler(selector, set_str)
>  declare_def_snprint_defstr(selector, print_str, DEFAULT_SELECTOR)
> diff --git a/libmultipath/structs.h b/libmultipath/structs.h
> index 88a4b7862393..d6482f84f0e6 100644
> --- a/libmultipath/structs.h
> +++ b/libmultipath/structs.h
> @@ -102,6 +102,32 @@ enum yes_no_undef_states {
>  	YNU_YES,
>  };
>  
> +#define _FIND_MULTIPATHS_F (1 << 1)
> +#define _FIND_MULTIPATHS_I (1 << 2)
> +#define _FIND_MULTIPATHS_N (1 << 3)
> +/*
> + * _FIND_MULTIPATHS_F must have the same value as YNU_YES.
> + * Generate a compile time error if that isn't the case.
> + */
> +char ___error1___[-(_FIND_MULTIPATHS_F != YNU_YES)];
> +
> +#define find_multipaths_on(conf) \
> +	(!!((conf)->find_multipaths & _FIND_MULTIPATHS_F))
> +#define ignore_wwids(conf) \
> +	(!!((conf)->find_multipaths & _FIND_MULTIPATHS_I))
> +#define ignore_new_devs(conf) \
> +	(!!((conf)->find_multipaths & _FIND_MULTIPATHS_N))
> +
> +enum find_multipaths_states {
> +	FIND_MULTIPATHS_UNDEF = YNU_UNDEF,
> +	FIND_MULTIPATHS_OFF = YNU_NO,
> +	FIND_MULTIPATHS_ON = _FIND_MULTIPATHS_F,
> +	FIND_MULTIPATHS_STRICT = _FIND_MULTIPATHS_F|_FIND_MULTIPATHS_N,
> +	FIND_MULTIPATHS_GREEDY = _FIND_MULTIPATHS_I,
> +	FIND_MULTIPATHS_SMART = _FIND_MULTIPATHS_F|_FIND_MULTIPATHS_I,
> +	__FIND_MULTIPATHS_LAST,
> +};
> +
>  enum flush_states {
>  	FLUSH_UNDEF = YNU_UNDEF,
>  	FLUSH_DISABLED = YNU_NO,
> diff --git a/libmultipath/wwids.c b/libmultipath/wwids.c
> index ea56911b4607..5a2e86dcd7e2 100644
> --- a/libmultipath/wwids.c
> +++ b/libmultipath/wwids.c
> @@ -274,20 +274,20 @@ out:
>  int
>  should_multipath(struct path *pp1, vector pathvec, vector mpvec)
>  {
> -	int i, ignore_new_devs;
> +	int i, ignore_new;
>  	struct path *pp2;
>  	struct config *conf;
>  
>  	conf = get_multipath_config();
> -	ignore_new_devs = conf->ignore_new_devs;
> -	if (!conf->find_multipaths && !ignore_new_devs) {
> +	ignore_new = ignore_new_devs(conf);
> +	if (!find_multipaths_on(conf) && !ignore_new) {
>  		put_multipath_config(conf);
>  		return 1;
>  	}
>  	put_multipath_config(conf);
>  
>  	condlog(4, "checking if %s should be multipathed", pp1->dev);
> -	if (!ignore_new_devs) {
> +	if (!ignore_new) {
>  		char tmp_wwid[WWID_SIZE];
>  		struct multipath *mp = find_mp_by_wwid(mpvec, pp1->wwid);
>  
> diff --git a/multipath/main.c b/multipath/main.c
> index 19c729d8e058..3944fd504de2 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -441,11 +441,11 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  		 * Paths listed in the wwids file are always considered valid.
>  		 */
>  		if (cmd == CMD_VALID_PATH) {
> -			if ((!conf->find_multipaths && conf->ignore_wwids) ||
> +			if ((!find_multipaths_on(conf) && ignore_wwids(conf)) ||
>  			    check_wwids_file(refwwid, 0) == 0)
>  				r = 0;
>  			if (r == 0 ||
> -			    !conf->find_multipaths || !conf->ignore_wwids) {
> +			    !find_multipaths_on(conf) || !ignore_wwids(conf)) {
>  				printf("%s %s a valid multipath device path\n",
>  				       devpath, r == 0 ? "is" : "is not");
>  				goto out;
> @@ -737,7 +737,7 @@ main (int argc, char *argv[])
>  			conf->force_reload = FORCE_RELOAD_YES;
>  			break;
>  		case 'i':
> -			conf->ignore_wwids = 1;
> +			conf->find_multipaths |= _FIND_MULTIPATHS_I;
>  			break;
>  		case 't':
>  			r = dump_config(conf);
> diff --git a/multipath/multipath.8 b/multipath/multipath.8
> index 56f870351ee2..329658daeead 100644
> --- a/multipath/multipath.8
> +++ b/multipath/multipath.8
> @@ -94,7 +94,14 @@ Force devmap reload.
>  .
>  .TP
>  .B \-i
> -Ignore WWIDs file when processing devices.
> +Ignore WWIDs file when processing devices. If 
> +\fIfind_multipaths strict\fR or \fIfind_multipaths no\fR is set in
> +\fImultipath.conf\fR, multipath only considers devices that are
> +listed in the WWIDs file. This option overrides that behavior. For other values
> +of \fIfind_multipaths\fR, this option has no effect. See the description of
> +\fIfind_multipaths\fR in 
> +.BR multipath.conf (5).
> +This option should only be used in rare circumstances.
>  .
>  .TP
>  .B \-B
> diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
> index c4d0789475a3..6965dacb5c68 100644
> --- a/multipath/multipath.conf.5
> +++ b/multipath/multipath.conf.5
> @@ -951,28 +951,34 @@ The default is: \fBno\fR
>  .
>  .TP
>  .B find_multipaths
> -If set to
> -.I yes
> -, instead of trying to create a multipath device for every non-blacklisted
> -path, multipath will only create a device if one of three condidions are
> -met.
> -.I 1
> -There are at least two non-blacklisted paths with the same WWID,
> -.I 2
> -the user manually forces the creation, by specifying a device with the multipath
> -command, or
> -.I 3
> -a path has the same WWID as a multipath device that was previously created
> -while find_multipaths was set (even if that multipath device doesn't currently
> -exist).
> -Whenever a multipath device is created with find_multipaths set, multipath will
> -remember the WWID of the device, so that it will automatically create the
> -device again, as soon as it sees a path with that WWID. This should allow most
> -users to have multipath automatically choose the correct paths to make into
> -multipath devices, without having to edit the blacklist.
> +This option controls whether multipath and multipathd try to create multipath
> +maps over non-blacklisted devices they encounter. This matters a) when a device is
> +encountered by \fBmultipath -u\fR during udev rule processing (a device is
> +blocked from further processing by higher layers - such as LVM - if and only
> +if it\'s considered a valid multipath device path), and b) when multipathd
> +detects a new device. The following values are possible:
>  .RS
> +.TP 10
> +.I strict
> +Both multipath and multipathd treat only such devices as multipath devices
> +which have been part of a multipath map previously, and which are therefore
> +listed in the \fBwwids_file\fR. Users can manually set up multipath maps using the
> +\fBmultipathd add map\fR command. Once set up manually, the map is
> +remembered in the wwids file and will be set up automatically in the future.
>  .TP
> -The default is: \fBno\fR
> +.I no
> +Multipath behaves like \fBstrict\fR. Multipathd behaves like \fBgreedy\fR.
> +.TP
> +.I yes
> +Both multipathd and multipath treat a device as multipath device if the
> +conditions for \fBstrict\fR are met, or if at least two non-blacklisted paths
> +with the same WWID have been detected.
> +.TP
> +.I greedy
> +Both multipathd and multipath treat every non-blacklisted device as multipath
> +device path.
> +.TP
> +The default is: \fBstrict\fR
>  .RE
>  .
>  .
> diff --git a/multipathd/main.c b/multipathd/main.c
> index da40595fc7c8..bfbe5f66b324 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2390,8 +2390,6 @@ reconfigure (struct vectors * vecs)
>  		conf->verbosity = verbosity;
>  	if (bindings_read_only)
>  		conf->bindings_read_only = bindings_read_only;
> -	if (ignore_new_devs)
> -		conf->ignore_new_devs = ignore_new_devs;
>  	uxsock_timeout = conf->uxsock_timeout;
>  
>  	old = rcu_dereference(multipath_conf);
> @@ -2620,8 +2618,6 @@ child (void * param)
>  		conf->verbosity = verbosity;
>  	if (bindings_read_only)
>  		conf->bindings_read_only = bindings_read_only;
> -	if (ignore_new_devs)
> -		conf->ignore_new_devs = ignore_new_devs;
>  	uxsock_timeout = conf->uxsock_timeout;
>  	rcu_assign_pointer(multipath_conf, conf);
>  	if (init_checkers(conf->multipath_dir)) {
> @@ -2975,7 +2971,7 @@ main (int argc, char *argv[])
>  			bindings_read_only = 1;
>  			break;
>  		case 'n':
> -			ignore_new_devs = 1;
> +			condlog(0, "WARNING: ignoring deprecated option -n, use 'ignore_wwids = no' instead");
>  			break;
>  		case 'w':
>  			poll_dmevents = 0;
> diff --git a/multipathd/multipathd.8 b/multipathd/multipathd.8
> index 5c96680c0514..e78ac9ed277f 100644
> --- a/multipathd/multipathd.8
> +++ b/multipathd/multipathd.8
> @@ -25,7 +25,6 @@ multipathd \- Multipath daemon.
>  .RB [\| \-v\ \c
>  .IR verbosity \|]
>  .RB [\| \-B \|]
> -.RB [\| \-n \|]
>  .
>  .
>  .\" ----------------------------------------------------------------------------
> @@ -73,8 +72,11 @@ be viewed by entering '\fIhelp\fR'. When you are finished entering commands, pre
>  .
>  .TP
>  .B \-n
> -Ignore new devices. multipathd will not create a multipath device unless the
> -WWID for the device is already listed in the WWIDs file.
> +\fBIGNORED\fR. Use the option
> +\fIfind_multipaths\fR to control the treatment of newly detected devices by
> +multipathd. See
> +.BR multipath.conf(5).
> +.
>  .
>  .
>  .\" ----------------------------------------------------------------------------
> -- 
> 2.16.1

Re: [PATCH v2 08/20] libmultipath: use const char* in open_file()

[Benjamin Marzinski]

On Mon, Mar 19, 2018 at 04:01:43PM +0100, Martin Wilck wrote:

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/file.c | 6 +++---
>  libmultipath/file.h | 2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/libmultipath/file.c b/libmultipath/file.c
> index e4951c92eb67..d5165ec6709a 100644
> --- a/libmultipath/file.c
> +++ b/libmultipath/file.c
> @@ -37,7 +37,7 @@
>   */
>  
>  static int
> -ensure_directories_exist(char *str, mode_t dir_mode)
> +ensure_directories_exist(const char *str, mode_t dir_mode)
>  {
>  	char *pathname;
>  	char *end;
> @@ -80,7 +80,7 @@ sigalrm(int sig)
>  }
>  
>  static int
> -lock_file(int fd, char *file_name)
> +lock_file(int fd, const char *file_name)
>  {
>  	struct sigaction act, oldact;
>  	sigset_t set, oldset;
> @@ -118,7 +118,7 @@ lock_file(int fd, char *file_name)
>  }
>  
>  int
> -open_file(char *file, int *can_write, char *header)
> +open_file(const char *file, int *can_write, const char *header)
>  {
>  	int fd;
>  	struct stat s;
> diff --git a/libmultipath/file.h b/libmultipath/file.h
> index 4f96dbf55e34..70bffa5301ad 100644
> --- a/libmultipath/file.h
> +++ b/libmultipath/file.h
> @@ -6,6 +6,6 @@
>  #define _FILE_H
>  
>  #define FILE_TIMEOUT 30
> -int open_file(char *file, int *can_write, char *header);
> +int open_file(const char *file, int *can_write, const char *header);
>  
>  #endif /* _FILE_H */
> -- 
> 2.16.1

Re: [PATCH v2 10/20] libmultipath: indicate wwid failure in dm_addmap_create()

[Benjamin Marzinski]

On Mon, Mar 19, 2018 at 04:01:45PM +0100, Martin Wilck wrote:
> dm_addmap_create() is where we actually try to set up a new
> multipath map. Depending on the result, mark the wwid as
> failed (or not), and re-trigger an uevent if necessary.
> If a path changes from multipath to non-multipath, use an "add"
> event to make sure LVM2 rules pick it up. Increase log level
> of this event to 3.

I'm not sure of a specific problem with this patch, but I am a little
leery of sending out our own "add" events. Unless I am mistaken there is
usually there is only one add event per device, and there is extra work
done on add events that we might not want to do twice. I wonder if it
would be better to make other udev rules be able to respond to change
events from ex-multipath paths.  Have you looked into the implications
of sending out these add events?

-Ben

> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/configure.c | 37 ++++++++++++++++++++++++++++---------
>  libmultipath/configure.h |  2 +-
>  libmultipath/devmapper.c |  8 +++++++-
>  libmultipath/structs.h   |  1 +
>  multipathd/main.c        |  2 +-
>  5 files changed, 38 insertions(+), 12 deletions(-)
> 
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index 838d145a5aa2..88e6687849f8 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -443,11 +443,18 @@ trigger_udev_change(const struct multipath *mpp)
>  }
>  
>  void
> -trigger_paths_udev_change(const struct multipath *mpp)
> +trigger_paths_udev_change(struct multipath *mpp, bool is_mpath)
>  {
>  	struct pathgroup * pgp;
>  	struct path * pp;
>  	int i, j;
> +	/*
> +	 * If a path changes from multipath to non-multipath, we must
> +	 * synthesize an artificial "add" event, otherwise the LVM2 rules
> +	 * (69-lvm2-lvmetad.rules) won't pick it up. Otherwise, we'd just
> +	 * irritate ourselves with an "add", so use "change".
> +	 */
> +	const char *action = is_mpath ? "change" : "add";
>  
>  	if (!mpp || !mpp->pg)
>  		return;
> @@ -466,14 +473,21 @@ trigger_paths_udev_change(const struct multipath *mpp)
>  			 */
>  			env = udev_device_get_property_value(
>  				pp->udev, "DM_MULTIPATH_DEVICE_PATH");
> -			if (env != NULL && !strcmp(env, "1"))
> -					continue;
>  
> -			condlog(4, "triggering change uevent for %s", pp->dev);
> -			sysfs_attr_set_value(pp->udev, "uevent", "change",
> -					     strlen("change"));
> +			if (is_mpath && env != NULL && !strcmp(env, "1"))
> +				continue;
> +			else if (!is_mpath &&
> +				   (env == NULL || !strcmp(env, "0")))
> +				continue;
> +
> +			condlog(3, "triggering %s uevent for %s (is %smultipath member)",
> +				action, pp->dev, is_mpath ? "" : "no ");
> +			sysfs_attr_set_value(pp->udev, "uevent",
> +					     action, strlen(action));
>  		}
>  	}
> +
> +	mpp->needs_paths_uevent = 0;
>  }
>  
>  static int
> @@ -870,8 +884,10 @@ int domap(struct multipath *mpp, char *params, int is_daemon)
>  		 * succeeded
>  		 */
>  		mpp->force_udev_reload = 0;
> -		if (mpp->action == ACT_CREATE && remember_wwid(mpp->wwid) == 1)
> -			trigger_paths_udev_change(mpp);
> +		if (mpp->action == ACT_CREATE &&
> +		    (remember_wwid(mpp->wwid) == 1 ||
> +		     mpp->needs_paths_uevent))
> +			trigger_paths_udev_change(mpp, true);
>  		if (!is_daemon) {
>  			/* multipath client mode */
>  			dm_switchgroup(mpp->alias, mpp->bestpg);
> @@ -896,7 +912,10 @@ int domap(struct multipath *mpp, char *params, int is_daemon)
>  		}
>  		dm_setgeometry(mpp);
>  		return DOMAP_OK;
> -	}
> +	} else if (r == DOMAP_FAIL && mpp->action == ACT_CREATE &&
> +		   mpp->needs_paths_uevent)
> +		trigger_paths_udev_change(mpp, false);
> +
>  	return DOMAP_FAIL;
>  }
>  
> diff --git a/libmultipath/configure.h b/libmultipath/configure.h
> index 545cbc209793..8b56d33a1d0b 100644
> --- a/libmultipath/configure.h
> +++ b/libmultipath/configure.h
> @@ -37,4 +37,4 @@ int get_refwwid (enum mpath_cmds cmd, char * dev, enum devtypes dev_type,
>  		 vector pathvec, char **wwid);
>  int reload_map(struct vectors *vecs, struct multipath *mpp, int refresh, int is_daemon);
>  struct udev_device *get_udev_device(const char *dev, enum devtypes dev_type);
> -void trigger_paths_udev_change(const struct multipath *mpp);
> +void trigger_paths_udev_change(struct multipath *mpp, bool is_mpath);
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index 9bafbc6a239a..bd595f4fa40b 100644
> --- a/libmultipath/devmapper.c
> +++ b/libmultipath/devmapper.c
> @@ -22,6 +22,7 @@
>  #include "devmapper.h"
>  #include "sysfs.h"
>  #include "config.h"
> +#include "wwids.h"
>  
>  #include "log_pthread.h"
>  #include <sys/types.h>
> @@ -411,8 +412,11 @@ int dm_addmap_create (struct multipath *mpp, char * params)
>  		int err;
>  
>  		if (dm_addmap(DM_DEVICE_CREATE, TGT_MPATH, mpp, params, ro,
> -			      udev_flags))
> +			      udev_flags)) {
> +			if (unmark_failed_wwid(mpp->wwid) == 1)
> +				mpp->needs_paths_uevent = 1;
>  			return 1;
> +		}
>  		/*
>  		 * DM_DEVICE_CREATE is actually DM_DEV_CREATE + DM_TABLE_LOAD.
>  		 * Failing the second part leaves an empty map. Clean it up.
> @@ -428,6 +432,8 @@ int dm_addmap_create (struct multipath *mpp, char * params)
>  			break;
>  		}
>  	}
> +	if (mark_failed_wwid(mpp->wwid) == 1)
> +		mpp->needs_paths_uevent = 1;
>  	return 0;
>  }
>  
> diff --git a/libmultipath/structs.h b/libmultipath/structs.h
> index d6482f84f0e6..32f4b2d0696c 100644
> --- a/libmultipath/structs.h
> +++ b/libmultipath/structs.h
> @@ -322,6 +322,7 @@ struct multipath {
>  	int max_sectors_kb;
>  	int force_readonly;
>  	int force_udev_reload;
> +	int needs_paths_uevent;
>  	int ghost_delay;
>  	int ghost_delay_tick;
>  	unsigned int dev_loss;
> diff --git a/multipathd/main.c b/multipathd/main.c
> index bfbe5f66b324..ea8c413f28c6 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2310,7 +2310,7 @@ configure (struct vectors * vecs)
>  	sync_maps_state(mpvec);
>  	vector_foreach_slot(mpvec, mpp, i){
>  		if (remember_wwid(mpp->wwid) == 1)
> -			trigger_paths_udev_change(mpp);
> +			trigger_paths_udev_change(mpp, true);
>  		update_map_pr(mpp);
>  	}
>  
> -- 
> 2.16.1

Re: [PATCH v2 11/20] libmultipath: don't try to set up failed wwids again

[Benjamin Marzinski]

On Mon, Mar 19, 2018 at 04:01:46PM +0100, Martin Wilck wrote:
> Once setting up a map has failed, don't try to set it up again.
> This applies to "multipathd reconfigure" and even multipathd restart,
> too. That's deliberate - if a WWID is marked as failed, we don't wont
> to bother with it again, unless the admin explicitly tells us so.
> Specifically, the exceptions are:
> 
>  1) multipathd add map $MAP
>  2) multipathd add path $PATH
>  3) multipath $PATH
> 
> In these cases, addmap() will eventually be called again, and the failed
> flag will be set according to it's return status. Unless the reason for
> the previous failure has been fixed, it may well be "failed" again.
> 
> Inofficially, it's also possible to manually remove a failed marker under
> /dev/shm/multipath/failed_wwids and run "multipathd reconfigure".

The code looks fine, but I wonder why this is necessary.  You already
posted patches that let multipathd issue uevent to claim a path device
after if it was not previously claimed. I admit that you don't generally
see multipathd succeed in creating a device after failing to, but it's
easy to imagine situations where it could.  For instance, if a path
device appears and then disappears soon after, multipathd would fail to
create the device because when the path device finally reappeared for
good.

I see that this will keep multipathd from needlessly retrying in-use
devices whenever a path comes or goes, but I don't know of any harm that
has ever caused, and I can see this causing harm. Am I missing something
here?

-Ben

> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/configure.c  |  5 +++--
>  libmultipath/configure.h  |  3 ++-
>  libmultipath/wwids.c      |  7 ++++++-
>  libmultipath/wwids.h      |  3 ++-
>  multipath/main.c          | 12 +++++++++---
>  multipathd/cli_handlers.c |  5 +++--
>  multipathd/main.c         | 13 +++++++------
>  multipathd/main.h         |  8 ++++++++
>  8 files changed, 40 insertions(+), 16 deletions(-)
> 
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index 88e6687849f8..98b80337d899 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -981,7 +981,7 @@ out:
>   * reloaded in DM if there's a difference. This is useful during startup.
>   */
>  int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
> -		    int force_reload, enum mpath_cmds cmd)
> +		    int force_reload, bool retry_failed, enum mpath_cmds cmd)
>  {
>  	int r = 1;
>  	int k, i;
> @@ -1032,7 +1032,8 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
>  			continue;
>  
>  		/* If find_multipaths was selected check if the path is valid */
> -		if (!refwwid && !should_multipath(pp1, pathvec, curmp)) {
> +		if (!refwwid && !should_multipath(pp1, pathvec, curmp,
> +						  retry_failed)) {
>  			orphan_path(pp1, "only one path");
>  			continue;
>  		}
> diff --git a/libmultipath/configure.h b/libmultipath/configure.h
> index 8b56d33a1d0b..7e175c890d25 100644
> --- a/libmultipath/configure.h
> +++ b/libmultipath/configure.h
> @@ -32,7 +32,8 @@ int setup_map (struct multipath * mpp, char * params, int params_size,
>  	       struct vectors *vecs );
>  int domap (struct multipath * mpp, char * params, int is_daemon);
>  int reinstate_paths (struct multipath *mpp);
> -int coalesce_paths (struct vectors *vecs, vector curmp, char * refwwid, int force_reload, enum mpath_cmds cmd);
> +int coalesce_paths (struct vectors *vecs, vector curmp, char * refwwid,
> +		    int force_reload, bool retry_failed, enum mpath_cmds cmd);
>  int get_refwwid (enum mpath_cmds cmd, char * dev, enum devtypes dev_type,
>  		 vector pathvec, char **wwid);
>  int reload_map(struct vectors *vecs, struct multipath *mpp, int refresh, int is_daemon);
> diff --git a/libmultipath/wwids.c b/libmultipath/wwids.c
> index e0fdcb34dbc6..288a9ad50c73 100644
> --- a/libmultipath/wwids.c
> +++ b/libmultipath/wwids.c
> @@ -4,6 +4,7 @@
>  #include <string.h>
>  #include <limits.h>
>  #include <stdio.h>
> +#include <stdbool.h>
>  #include <sys/types.h>
>  #include <sys/stat.h>
>  
> @@ -273,12 +274,16 @@ out:
>  }
>  
>  int
> -should_multipath(struct path *pp1, vector pathvec, vector mpvec)
> +should_multipath(struct path *pp1, vector pathvec, vector mpvec,
> +		 bool retry_failed)
>  {
>  	int i, ignore_new;
>  	struct path *pp2;
>  	struct config *conf;
>  
> +	if (!retry_failed && is_failed_wwid(pp1->wwid))
> +		return 0;
> +
>  	conf = get_multipath_config();
>  	ignore_new = ignore_new_devs(conf);
>  	if (!find_multipaths_on(conf) && !ignore_new) {
> diff --git a/libmultipath/wwids.h b/libmultipath/wwids.h
> index d00e1f58137c..911e8da720c5 100644
> --- a/libmultipath/wwids.h
> +++ b/libmultipath/wwids.h
> @@ -12,7 +12,8 @@
>  "#\n" \
>  "# Valid WWIDs:\n"
>  
> -int should_multipath(struct path *pp, vector pathvec, vector mpvec);
> +int should_multipath(struct path *pp, vector pathvec, vector mpvec,
> +		     bool retry_failed);
>  int remember_wwid(char *wwid);
>  int check_wwids_file(char *wwid, int write_wwid);
>  int remove_wwid(char *wwid);
> diff --git a/multipath/main.c b/multipath/main.c
> index 3944fd504de2..566404e56ef4 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -441,8 +441,12 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  		 * Paths listed in the wwids file are always considered valid.
>  		 */
>  		if (cmd == CMD_VALID_PATH) {
> -			if ((!find_multipaths_on(conf) && ignore_wwids(conf)) ||
> -			    check_wwids_file(refwwid, 0) == 0)
> +			if (is_failed_wwid(refwwid)) {
> +				r = 1;
> +				goto print_valid;
> +			} else if ((!find_multipaths_on(conf) &&
> +				  ignore_wwids(conf)) ||
> +				 check_wwids_file(refwwid, 0) == 0)
>  				r = 0;
>  			if (r == 0 ||
>  			    !find_multipaths_on(conf) || !ignore_wwids(conf)) {
> @@ -504,9 +508,11 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  
>  	/*
>  	 * core logic entry point
> +	 * If refwwid != NULL, user has given a device to multipath,
> +	 * so retry even if this ID has failed before.
>  	 */
>  	r = coalesce_paths(&vecs, NULL, refwwid,
> -			   conf->force_reload, cmd);
> +			   conf->force_reload, refwwid != NULL, cmd);
>  
>  out:
>  	if (refwwid)
> diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> index 60ec48b9904a..202438795ee5 100644
> --- a/multipathd/cli_handlers.c
> +++ b/multipathd/cli_handlers.c
> @@ -750,7 +750,7 @@ cli_add_path (void * v, char ** reply, int * len, void * data)
>  		pp->checkint = conf->checkint;
>  	}
>  	put_multipath_config(conf);
> -	return ev_add_path(pp, vecs, 1);
> +	return ev_add_path(pp, vecs, ADD_PATH_DOMAP_FORCE);
>  blacklisted:
>  	*reply = strdup("blacklisted\n");
>  	*len = strlen(*reply) + 1;
> @@ -813,7 +813,8 @@ cli_add_map (void * v, char ** reply, int * len, void * data)
>  					 vecs->pathvec, &refwwid);
>  			if (refwwid) {
>  				if (coalesce_paths(vecs, NULL, refwwid,
> -						   FORCE_RELOAD_NONE, CMD_NONE))
> +						   FORCE_RELOAD_NONE, true,
> +						   CMD_NONE))
>  					condlog(2, "%s: coalesce_paths failed",
>  									param);
>  				dm_lib_release();
> diff --git a/multipathd/main.c b/multipathd/main.c
> index ea8c413f28c6..8fd7d2b75cba 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -933,7 +933,8 @@ rescan:
>  		mpp->action = ACT_RELOAD;
>  		extract_hwe_from_path(mpp);
>  	} else {
> -		if (!should_multipath(pp, vecs->pathvec, vecs->mpvec)) {
> +		if (!should_multipath(pp, vecs->pathvec, vecs->mpvec,
> +				      need_do_map == ADD_PATH_DOMAP_FORCE)) {
>  			orphan_path(pp, "only one path");
>  			return 0;
>  		}
> @@ -953,7 +954,7 @@ rescan:
>  	/* persistent reservation check*/
>  	mpath_pr_event_handle(pp);
>  
> -	if (!need_do_map)
> +	if (need_do_map == ADD_PATH_DOMAP_NO)
>  		return 0;
>  
>  	if (!dm_map_present(mpp->alias)) {
> @@ -1863,7 +1864,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
>  			conf = get_multipath_config();
>  			ret = pathinfo(pp, conf, DI_ALL | DI_BLACKLIST);
>  			if (ret == PATHINFO_OK) {
> -				ev_add_path(pp, vecs, 1);
> +				ev_add_path(pp, vecs, ADD_PATH_DOMAP_YES);
>  				pp->tick = 1;
>  			} else if (ret == PATHINFO_SKIPPED) {
>  				put_multipath_config(conf);
> @@ -1989,7 +1990,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
>  		}
>  		if (!disable_reinstate && reinstate_path(pp, add_active)) {
>  			condlog(3, "%s: reload map", pp->dev);
> -			ev_add_path(pp, vecs, 1);
> +			ev_add_path(pp, vecs, ADD_PATH_DOMAP_YES);
>  			pp->tick = 1;
>  			return 0;
>  		}
> @@ -2012,7 +2013,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
>  			/* Clear IO errors */
>  			if (reinstate_path(pp, 0)) {
>  				condlog(3, "%s: reload map", pp->dev);
> -				ev_add_path(pp, vecs, 1);
> +				ev_add_path(pp, vecs, ADD_PATH_DOMAP_YES);
>  				pp->tick = 1;
>  				return 0;
>  			}
> @@ -2288,7 +2289,7 @@ configure (struct vectors * vecs)
>  	 * superfluous ACT_RELOAD ioctls. Later calls are done
>  	 * with FORCE_RELOAD_YES.
>  	 */
> -	ret = coalesce_paths(vecs, mpvec, NULL, force_reload, CMD_NONE);
> +	ret = coalesce_paths(vecs, mpvec, NULL, force_reload, false, CMD_NONE);
>  	if (force_reload == FORCE_RELOAD_WEAK)
>  		force_reload = FORCE_RELOAD_YES;
>  	if (ret) {
> diff --git a/multipathd/main.h b/multipathd/main.h
> index af395589ff93..a92650cd958b 100644
> --- a/multipathd/main.h
> +++ b/multipathd/main.h
> @@ -22,6 +22,14 @@ void exit_daemon(void);
>  const char * daemon_status(void);
>  int need_to_delay_reconfig (struct vectors *);
>  int reconfigure (struct vectors *);
> +/*
> + * 3rd argument of ev_add_path()
> + */
> +enum {
> +	ADD_PATH_DOMAP_NO = 0,	  /* don't call domap */
> +	ADD_PATH_DOMAP_YES = 1,	  /* call domap, don't retry failed */
> +	ADD_PATH_DOMAP_FORCE = 2, /* call domap, retry previously failed */
> +};
>  int ev_add_path (struct path *, struct vectors *, int);
>  int ev_remove_path (struct path *, struct vectors *, int);
>  int ev_add_map (char *, const char *, struct vectors *);
> -- 
> 2.16.1

Re: [PATCH v2 12/20] multipath -u: common code path for result message

[Benjamin Marzinski]

On Mon, Mar 19, 2018 at 04:01:47PM +0100, Martin Wilck wrote:
> Print the result message in one place only. This simplifies
> future changes. multipath -c is also affected.
> 
Reviewed-by: Benjamin Marzinsk <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipath/main.c | 30 +++++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/multipath/main.c b/multipath/main.c
> index 566404e56ef4..ea4a3d44493a 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -350,6 +350,17 @@ out:
>  	return r;
>  }
>  
> +static void print_cmd_valid(const char *devpath, int k)
> +{
> +	const char *msg[] = { "is", "is not" };
> +
> +	if (k < 0 || k >= sizeof(msg))
> +		return;
> +
> +	printf("%s %s a valid multipath device path\n",
> +	       devpath, msg[k]);
> +}
> +
>  /*
>   * Return value:
>   *  -1: Retry
> @@ -391,10 +402,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  	    cmd != CMD_REMOVE_WWID &&
>  	    (filter_devnode(conf->blist_devnode,
>  			    conf->elist_devnode, dev) > 0)) {
> -		if (cmd == CMD_VALID_PATH)
> -			printf("%s is not a valid multipath device path\n",
> -			       devpath);
> -		goto out;
> +		goto print_valid;
>  	}
>  
>  	/*
> @@ -407,7 +415,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  		if (!refwwid) {
>  			condlog(4, "%s: failed to get wwid", devpath);
>  			if (failed == 2 && cmd == CMD_VALID_PATH)
> -				printf("%s is not a valid multipath device path\n", devpath);
> +				goto print_valid;
>  			else
>  				condlog(3, "scope is null");
>  			goto out;
> @@ -450,9 +458,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  				r = 0;
>  			if (r == 0 ||
>  			    !find_multipaths_on(conf) || !ignore_wwids(conf)) {
> -				printf("%s %s a valid multipath device path\n",
> -				       devpath, r == 0 ? "is" : "is not");
> -				goto out;
> +				goto print_valid;
>  			}
>  		}
>  	}
> @@ -496,9 +502,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  		 * the refwwid, then the path is valid */
>  		if (VECTOR_SIZE(curmp) != 0 || VECTOR_SIZE(pathvec) > 1)
>  			r = 0;
> -		printf("%s %s a valid multipath device path\n",
> -		       devpath, r == 0 ? "is" : "is not");
> -		goto out;
> +		goto print_valid;
>  	}
>  
>  	if (cmd != CMD_CREATE && cmd != CMD_DRY_RUN) {
> @@ -514,6 +518,10 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  	r = coalesce_paths(&vecs, NULL, refwwid,
>  			   conf->force_reload, refwwid != NULL, cmd);
>  
> +print_valid:
> +	if (cmd == CMD_VALID_PATH)
> +		print_cmd_valid(devpath, r);
> +
>  out:
>  	if (refwwid)
>  		FREE(refwwid);
> -- 
> 2.16.1

Re: [PATCH v2 13/20] multipath -u: change output to environment/key format

[Benjamin Marzinski]

On Mon, Mar 19, 2018 at 04:01:48PM +0100, Martin Wilck wrote:
> ... instead of free format. This provides more flexibility
> for udev rule processing for the future. Adapt code in multipath.rules.
> The exit status remains as usual. This affects "multipath -c", too.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipath/main.c          | 11 +++++------
>  multipath/multipath.rules |  6 +++---
>  2 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/multipath/main.c b/multipath/main.c
> index ea4a3d44493a..f93cad2abde0 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -350,15 +350,14 @@ out:
>  	return r;
>  }
>  
> -static void print_cmd_valid(const char *devpath, int k)
> +static void print_cmd_valid(int k)
>  {
> -	const char *msg[] = { "is", "is not" };
> +	int vals[] = { 1, 0 };
>  
> -	if (k < 0 || k >= sizeof(msg))
> +	if (k < 0 || k >= sizeof(vals))
>  		return;
>  
> -	printf("%s %s a valid multipath device path\n",
> -	       devpath, msg[k]);
> +	printf("DM_MULTIPATH_DEVICE_PATH=\"%d\"\n", vals[k]);
>  }
>  
>  /*
> @@ -520,7 +519,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  
>  print_valid:
>  	if (cmd == CMD_VALID_PATH)
> -		print_cmd_valid(devpath, r);
> +		print_cmd_valid(r);
>  
>  out:
>  	if (refwwid)
> diff --git a/multipath/multipath.rules b/multipath/multipath.rules
> index 6f8ee2be0a58..aab64dc7182c 100644
> --- a/multipath/multipath.rules
> +++ b/multipath/multipath.rules
> @@ -19,9 +19,9 @@ LABEL="test_dev"
>  ENV{MPATH_SBIN_PATH}="/sbin"
>  TEST!="$env{MPATH_SBIN_PATH}/multipath", ENV{MPATH_SBIN_PATH}="/usr/sbin"
>  
> -ENV{DM_MULTIPATH_DEVICE_PATH}!="1", \
> -	PROGRAM=="$env{MPATH_SBIN_PATH}/multipath -u %k", \
> -	ENV{DM_MULTIPATH_DEVICE_PATH}="1", ENV{ID_FS_TYPE}="mpath_member", \
> +# multipath -u sets DM_MULTIPATH_DEVICE_PATH
> +ENV{DM_MULTIPATH_DEVICE_PATH}!="1", IMPORT{program}="$env{MPATH_SBIN_PATH}/multipath -u %k"
> +ENV{DM_MULTIPATH_DEVICE_PATH}=="1", ENV{ID_FS_TYPE}="mpath_member", \
>  	ENV{SYSTEMD_READY}="0"
>  
>  LABEL="end_mpath"
> -- 
> 2.16.1

Re: [PATCH v2 10/20] libmultipath: indicate wwid failure in dm_addmap_create()

[Martin Wilck]

On Mon, 2018-03-26 at 12:52 -0500, Benjamin Marzinski wrote:
> On Mon, Mar 19, 2018 at 04:01:45PM +0100, Martin Wilck wrote:
> > dm_addmap_create() is where we actually try to set up a new
> > multipath map. Depending on the result, mark the wwid as
> > failed (or not), and re-trigger an uevent if necessary.
> > If a path changes from multipath to non-multipath, use an "add"
> > event to make sure LVM2 rules pick it up. Increase log level
> > of this event to 3.
> 
> I'm not sure of a specific problem with this patch, but I am a little
> leery of sending out our own "add" events. Unless I am mistaken there
> is
> usually there is only one add event per device, and there is extra
> work
> done on add events that we might not want to do twice. I wonder if it
> would be better to make other udev rules be able to respond to change
> events from ex-multipath paths.  Have you looked into the
> implications
> of sending out these add events?

Yes. I reviewed the udev rules on a few current systems. There are only
very few rules that act on "add" only. For those devices that matter
here (block devices, non-dm) I see only the following two:

60-block.rules: a rule that enables parameters/events_dfl_poll_msecs.
  => not critical, we just disable/enable the polling once
     more.
69-dm-lvm-metad.rules:ACTION!="add", GOTO="lvm_end"
  =>  this is the rule that forced me to synthesize an "add" event.

So indeed, I think that at least for the current state of affairs
sending an "add" event is safe.

Wrt "make other udev rules be able to respond to change events" - 
LVM2 commit 756bcab explains in detail why LVM2 switched to scanning on
"add" events only. This code is >5 years old. It might be possible to
distinguish different kinds of "change" events for lvmetad (e.g.
remembering "DM_MULTIPATH_DEVICE_PATH" and re-scanning when it has
changed from "1" to "0") but to be honest, I'd rather not want to
depend on such a change being merged in lvm2. I've submitted a minor,
IMO rather un-controversial fix for 69-dm-lvmetad.rules 3 monts ago
("LVM2: fix lvmetad udev rules for CHANGE events") and sent out
multiple reminders, without receiving a reaction.

Moreover, we're sending out these events for devices which have
DM_MULTIPATH_DEVICE_PATH=1 and SYSTEMD_READY=0 set. For everything
above the multipath layer, it's actually as if the device had just been
"add"ed.

I think this will either work with synthesized "add" events, or not at
all. Please correct me if I've overlooked something.

Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH v2 11/20] libmultipath: don't try to set up failed wwids again

[Martin Wilck]

On Mon, 2018-03-26 at 13:47 -0500, Benjamin Marzinski wrote:
> On Mon, Mar 19, 2018 at 04:01:46PM +0100, Martin Wilck wrote:
> > Once setting up a map has failed, don't try to set it up again.
> > This applies to "multipathd reconfigure" and even multipathd
> > restart,
> > too. That's deliberate - if a WWID is marked as failed, we don't
> > wont
> > to bother with it again, unless the admin explicitly tells us so.
> > Specifically, the exceptions are:
> > 
> >  1) multipathd add map $MAP
> >  2) multipathd add path $PATH
> >  3) multipath $PATH
> > 
> > In these cases, addmap() will eventually be called again, and the
> > failed
> > flag will be set according to it's return status. Unless the reason
> > for
> > the previous failure has been fixed, it may well be "failed" again.
> > 
> > Inofficially, it's also possible to manually remove a failed marker
> > under
> > /dev/shm/multipath/failed_wwids and run "multipathd reconfigure".
> 
> The code looks fine, but I wonder why this is necessary.  You already
> posted patches that let multipathd issue uevent to claim a path
> device
> after if it was not previously claimed. I admit that you don't
> generally
> see multipathd succeed in creating a device after failing to, but
> it's
> easy to imagine situations where it could.  For instance, if a path
> device appears and then disappears soon after, multipathd would fail
> to
> create the device because when the path device finally reappeared for
> good.
> 
> I see that this will keep multipathd from needlessly retrying in-use
> devices whenever a path comes or goes, but I don't know of any harm
> that
> has ever caused, and I can see this causing harm. Am I missing
> something
> here?

Hm. You've got a point there. The only code path where the "failed"
status _must_ really be honored is "multipath -u" (obviously, without
that, the whole purpose of the patch set would be forfeited). I'll give
it a try.

Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH v2 13/20] multipath -u: change output to environment/key format

[Benjamin Marzinski]

On Mon, Mar 19, 2018 at 04:01:48PM +0100, Martin Wilck wrote:
> ... instead of free format. This provides more flexibility
> for udev rule processing for the future. Adapt code in multipath.rules.
> The exit status remains as usual. This affects "multipath -c", too.

I assume that you meant to change the following code from
multipath/main.c: main() as well

        if (cmd == CMD_VALID_PATH &&
            dev_type == DEV_UEVENT) {
                int fd;

                fd = mpath_connect();
                if (fd == -1) {
                        condlog(3, "%s: daemon is not running", dev);
                        if (!systemd_service_enabled(dev)) {
                                printf("%s is not a valid "
                                       "multipath device path\n", dev);
                                goto out;
                        }
                } else  
                        mpath_disconnect(fd);
        }


> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipath/main.c          | 11 +++++------
>  multipath/multipath.rules |  6 +++---
>  2 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/multipath/main.c b/multipath/main.c
> index ea4a3d44493a..f93cad2abde0 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -350,15 +350,14 @@ out:
>  	return r;
>  }
>  
> -static void print_cmd_valid(const char *devpath, int k)
> +static void print_cmd_valid(int k)
>  {
> -	const char *msg[] = { "is", "is not" };
> +	int vals[] = { 1, 0 };
>  
> -	if (k < 0 || k >= sizeof(msg))
> +	if (k < 0 || k >= sizeof(vals))
>  		return;
>  
> -	printf("%s %s a valid multipath device path\n",
> -	       devpath, msg[k]);
> +	printf("DM_MULTIPATH_DEVICE_PATH=\"%d\"\n", vals[k]);
>  }
>  
>  /*
> @@ -520,7 +519,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  
>  print_valid:
>  	if (cmd == CMD_VALID_PATH)
> -		print_cmd_valid(devpath, r);
> +		print_cmd_valid(r);
>  
>  out:
>  	if (refwwid)
> diff --git a/multipath/multipath.rules b/multipath/multipath.rules
> index 6f8ee2be0a58..aab64dc7182c 100644
> --- a/multipath/multipath.rules
> +++ b/multipath/multipath.rules
> @@ -19,9 +19,9 @@ LABEL="test_dev"
>  ENV{MPATH_SBIN_PATH}="/sbin"
>  TEST!="$env{MPATH_SBIN_PATH}/multipath", ENV{MPATH_SBIN_PATH}="/usr/sbin"
>  
> -ENV{DM_MULTIPATH_DEVICE_PATH}!="1", \
> -	PROGRAM=="$env{MPATH_SBIN_PATH}/multipath -u %k", \
> -	ENV{DM_MULTIPATH_DEVICE_PATH}="1", ENV{ID_FS_TYPE}="mpath_member", \
> +# multipath -u sets DM_MULTIPATH_DEVICE_PATH
> +ENV{DM_MULTIPATH_DEVICE_PATH}!="1", IMPORT{program}="$env{MPATH_SBIN_PATH}/multipath -u %k"
> +ENV{DM_MULTIPATH_DEVICE_PATH}=="1", ENV{ID_FS_TYPE}="mpath_member", \
>  	ENV{SYSTEMD_READY}="0"
>  
>  LABEL="end_mpath"
> -- 
> 2.16.1

Re: [PATCH v2 14/20] multipath -u: add DM_MULTIPATH_DEVICE_PATH=2 for "maybe"

[Benjamin Marzinski]

On Mon, Mar 19, 2018 at 04:01:49PM +0100, Martin Wilck wrote:
> Use DM_MULTIPATH_DEVICE_PATH="2" to indicate that this might be a
> valid path, but we aren't certain. This happens with find_multipaths
> "smart", when the first path to a device (or a single-path
> device) is encountered, and the device is neither blacklisted,
> nor marked failed, nor whitelisted in the wwids file.
>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipath/main.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/multipath/main.c b/multipath/main.c
> index f93cad2abde0..9b53c13b3a78 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -350,14 +350,15 @@ out:
>  	return r;
>  }
>  
> -static void print_cmd_valid(int k)
> +static int print_cmd_valid(int k)
>  {
> -	int vals[] = { 1, 0 };
> +	int vals[] = { 1, 0, 2 };
>  
>  	if (k < 0 || k >= sizeof(vals))
> -		return;
> +		return 1;
>  
>  	printf("DM_MULTIPATH_DEVICE_PATH=\"%d\"\n", vals[k]);
> +	return k == 1;
>  }
>  
>  /*
> @@ -501,6 +502,9 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  		 * the refwwid, then the path is valid */
>  		if (VECTOR_SIZE(curmp) != 0 || VECTOR_SIZE(pathvec) > 1)
>  			r = 0;
> +		else
> +			/* Use r=2 as an indication for "maybe" */
> +			r = 2;
>  		goto print_valid;
>  	}
>  
> @@ -519,7 +523,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  
>  print_valid:
>  	if (cmd == CMD_VALID_PATH)
> -		print_cmd_valid(r);
> +		r = print_cmd_valid(r);
>  
>  out:
>  	if (refwwid)
> -- 
> 2.16.1

Re: [PATCH v2 15/20] libmultipath: implement find_multipaths_timeout

[Benjamin Marzinski]

On Mon, Mar 19, 2018 at 04:01:50PM +0100, Martin Wilck wrote:
> This makes the timeout for "find_multipaths smart" configurable.
> If the timeout has a negative value (default), it's applied only
> to "known" hardware which is either in the hwtable or in a "device" section in
> multipath.conf. For typical non-multipath hardware, which is not in the
> hwtable, a short timeout of 1s is used, so that boot delays caused by
> pointlessly waiting e.g. for SATA devices will be minimal.
> 
> It's expected that a "reasonable" timeout value depends less on the storage
> hardware itself but on other properties of the data center such as network
> latencies or distances. find_multipaths_timeout is therefore just a "defaults"
> section setting.
> 

I still dislike doing this every time we boot for devices that we aren't
supposed to multipath, although with the reduced timeout for devices
that don't have configs, I don't think that this will be a problem for
most users.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/config.h      |  1 +
>  libmultipath/defaults.h    |  2 ++
>  libmultipath/dict.c        |  6 ++++++
>  libmultipath/propsel.c     | 25 +++++++++++++++++++++++++
>  libmultipath/propsel.h     |  1 +
>  libmultipath/structs.h     |  1 +
>  multipath/main.c           | 13 +++++++++++--
>  multipath/multipath.conf.5 | 18 ++++++++++++++++++
>  8 files changed, 65 insertions(+), 2 deletions(-)
> 
> diff --git a/libmultipath/config.h b/libmultipath/config.h
> index 21d8e72a2492..72d66018893b 100644
> --- a/libmultipath/config.h
> +++ b/libmultipath/config.h
> @@ -174,6 +174,7 @@ struct config {
>  	int remove_retries;
>  	int max_sectors_kb;
>  	int ghost_delay;
> +	int find_multipaths_timeout;
>  	unsigned int version[3];
>  
>  	char * multipath_dir;
> diff --git a/libmultipath/defaults.h b/libmultipath/defaults.h
> index 690182c368d9..018eb01c1830 100644
> --- a/libmultipath/defaults.h
> +++ b/libmultipath/defaults.h
> @@ -41,6 +41,8 @@
>  #define DEFAULT_DISABLE_CHANGED_WWIDS 1
>  #define DEFAULT_MAX_SECTORS_KB MAX_SECTORS_KB_UNDEF
>  #define DEFAULT_GHOST_DELAY GHOST_DELAY_OFF
> +#define DEFAULT_FIND_MULTIPATHS_TIMEOUT -10
> +#define DEFAULT_UNKNOWN_FIND_MULTIPATHS_TIMEOUT 1
>  
>  #define DEFAULT_CHECKINT	5
>  #define MAX_CHECKINT(a)		(a << 2)
> diff --git a/libmultipath/dict.c b/libmultipath/dict.c
> index a952c60a6f98..d59fdc85a9cb 100644
> --- a/libmultipath/dict.c
> +++ b/libmultipath/dict.c
> @@ -484,6 +484,10 @@ declare_hw_snprint(max_sectors_kb, print_nonzero)
>  declare_mp_handler(max_sectors_kb, set_int)
>  declare_mp_snprint(max_sectors_kb, print_nonzero)
>  
> +declare_def_handler(find_multipaths_timeout, set_int)
> +declare_def_snprint_defint(find_multipaths_timeout, print_int,
> +			   DEFAULT_FIND_MULTIPATHS_TIMEOUT)
> +
>  static int
>  def_config_dir_handler(struct config *conf, vector strvec)
>  {
> @@ -1518,6 +1522,8 @@ init_keywords(vector keywords)
>  	install_keyword("remove_retries", &def_remove_retries_handler, &snprint_def_remove_retries);
>  	install_keyword("max_sectors_kb", &def_max_sectors_kb_handler, &snprint_def_max_sectors_kb);
>  	install_keyword("ghost_delay", &def_ghost_delay_handler, &snprint_def_ghost_delay);
> +	install_keyword("find_multipaths_timeout", &def_find_multipaths_timeout_handler,
> +			&snprint_def_find_multipaths_timeout);
>  	__deprecated install_keyword("default_selector", &def_selector_handler, NULL);
>  	__deprecated install_keyword("default_path_grouping_policy", &def_pgpolicy_handler, NULL);
>  	__deprecated install_keyword("default_uid_attribute", &def_uid_attribute_handler, NULL);
> diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
> index 58a6a42fe333..87d6865d17e6 100644
> --- a/libmultipath/propsel.c
> +++ b/libmultipath/propsel.c
> @@ -911,3 +911,28 @@ out:
>  	condlog(3, "%s: ghost_delay = %s %s", mp->alias, buff, origin);
>  	return 0;
>  }
> +
> +int select_find_multipaths_timeout(struct config *conf, struct path *pp)
> +{
> +	char *origin;
> +
> +	pp_set_conf(find_multipaths_timeout);
> +	pp_set_default(find_multipaths_timeout,
> +		       DEFAULT_FIND_MULTIPATHS_TIMEOUT);
> +out:
> +	/*
> +	 * If configured value is negative, and this "unkown" hardware
> +	 * (no hwentry), use very small timeout to avoid delays.
> +	 */
> +	if (pp->find_multipaths_timeout < 0) {
> +		pp->find_multipaths_timeout = - pp->find_multipaths_timeout;
> +		if (!pp->hwe) {
> +			pp->find_multipaths_timeout =
> +				DEFAULT_UNKNOWN_FIND_MULTIPATHS_TIMEOUT;
> +			origin = "(default for unkown hardware)";
> +		}
> +	}
> +	condlog(3, "%s: timeout for find_multipaths \"smart\" = %ds %s",
> +		pp->dev, pp->find_multipaths_timeout, origin);
> +	return 0;
> +}
> diff --git a/libmultipath/propsel.h b/libmultipath/propsel.h
> index 136f90605b91..b6475d0d9c71 100644
> --- a/libmultipath/propsel.h
> +++ b/libmultipath/propsel.h
> @@ -8,6 +8,7 @@ int select_hwhandler (struct config *conf, struct multipath * mp);
>  int select_checker(struct config *conf, struct path *pp);
>  int select_getuid (struct config *conf, struct path * pp);
>  int select_prio (struct config *conf, struct path * pp);
> +int select_find_multipaths_timeout(struct config *conf, struct path * pp);
>  int select_no_path_retry(struct config *conf, struct multipath *mp);
>  int select_flush_on_last_del(struct config *conf, struct multipath *mp);
>  int select_minio(struct config *conf, struct multipath *mp);
> diff --git a/libmultipath/structs.h b/libmultipath/structs.h
> index 32f4b2d0696c..d4e18df940ea 100644
> --- a/libmultipath/structs.h
> +++ b/libmultipath/structs.h
> @@ -281,6 +281,7 @@ struct path {
>  	int io_err_disable_reinstate;
>  	int io_err_pathfail_cnt;
>  	int io_err_pathfail_starttime;
> +	int find_multipaths_timeout;
>  	/* configlet pointers */
>  	struct hwentry * hwe;
>  	struct gen_path generic_path;
> diff --git a/multipath/main.c b/multipath/main.c
> index 9b53c13b3a78..b41621d267f7 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -60,6 +60,7 @@
>  #include "uxsock.h"
>  #include "mpath_cmd.h"
>  #include "foreign.h"
> +#include "propsel.h"
>  
>  int logsink;
>  struct udev *udev;
> @@ -350,7 +351,8 @@ out:
>  	return r;
>  }
>  
> -static int print_cmd_valid(int k)
> +static int print_cmd_valid(int k, const vector pathvec,
> +			   struct config *conf)
>  {
>  	int vals[] = { 1, 0, 2 };
>  
> @@ -358,6 +360,13 @@ static int print_cmd_valid(int k)
>  		return 1;
>  
>  	printf("DM_MULTIPATH_DEVICE_PATH=\"%d\"\n", vals[k]);
> +	if (vals[k] == 2 && VECTOR_SIZE(pathvec) > 0) {
> +		struct path *pp = VECTOR_SLOT(pathvec, 0);
> +
> +		select_find_multipaths_timeout(conf, pp);
> +		printf("FIND_MULTIPATHS_PATH_TMO=\"%d\"\n",
> +		       pp->find_multipaths_timeout);
> +	}
>  	return k == 1;
>  }
>  
> @@ -523,7 +532,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  
>  print_valid:
>  	if (cmd == CMD_VALID_PATH)
> -		r = print_cmd_valid(r);
> +		r = print_cmd_valid(r, pathvec, conf);
>  
>  out:
>  	if (refwwid)
> diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
> index 6965dacb5c68..94c419a078bf 100644
> --- a/multipath/multipath.conf.5
> +++ b/multipath/multipath.conf.5
> @@ -983,6 +983,24 @@ The default is: \fBstrict\fR
>  .
>  .
>  .TP
> +.B find_multipaths_timeout
> +Timeout, in seconds, to wait for additional paths after detecting the first
> +one, if \fIfind_multipaths
> +"smart"\fR (see above) is set. If the value is \fBpositive\fR, this timeout is used for all
> +unkown, non-blacklisted devices encountered. If the value is \fBnegative\fR
> +(recommended), it's only
> +applied to "known" devices that have an entry in multipath's hardware table,
> +either in the built-in table or in a \fIdevice\fR section; other ("unknown") devices will
> +use a timeout of only 1 second to avoid booting delays. The value 0 means
> +"use the built-in default". If \fIfind_multipath\fR has a value
> +other than \fIsmart\fR, this option has no effect. 
> +.RS
> +.TP
> +The default is: \fB-10\fR (10s for known and 1s for unknown hardware)
> +.RE
> +.
> +.
> +.TP
>  .B uxsock_timeout
>  CLI receive timeout in milliseconds. For larger systems CLI commands
>  might timeout before the multipathd lock is released and the CLI command
> -- 
> 2.16.1

Re: [PATCH v2 16/20] libmultipath: enable find_multipaths "smart"

[Benjamin Marzinski]

On Mon, Mar 19, 2018 at 04:01:51PM +0100, Martin Wilck wrote:
> This activates "smart" path detection. This is similar to
> "find_multipaths yes", but doesn't generally ignore single paths
> that are not listed in the WWIDs file. Rather, such paths are
> temporarily treated like multipath members. If no additional paths
> are detected after a certain time, the paths are re-added to the
> system as non-multipath. This needs support by the udev rules, to
> be added in a follow-up patch.
> 
> If a multipath map is successfully successfully created, and paths are
> in waiting state, trigger path uevents to update their status.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/configure.c   | 15 ++++++++++++---
>  libmultipath/dict.c        |  1 +
>  multipath/multipath.conf.5 | 13 +++++++++++++
>  3 files changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index 98b80337d899..ec1ad9c90f05 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -12,6 +12,7 @@
>  #include <string.h>
>  #include <sys/file.h>
>  #include <errno.h>
> +#include <ctype.h>
>  #include <libdevmapper.h>
>  #include <libudev.h>
>  #include "mpath_cmd.h"
> @@ -474,9 +475,17 @@ trigger_paths_udev_change(struct multipath *mpp, bool is_mpath)
>  			env = udev_device_get_property_value(
>  				pp->udev, "DM_MULTIPATH_DEVICE_PATH");
>  
> -			if (is_mpath && env != NULL && !strcmp(env, "1"))
> -				continue;
> -			else if (!is_mpath &&
> +			if (is_mpath && env != NULL && !strcmp(env, "1")) {
> +				/*
> +				 * If FIND_MULTIPATHS_WAIT_UNTIL is a number,
> +				 * path is in "maybe" state and timer is running
> +				 * Send uevent now (see multipath.rules).
> +				 */
> +				env = udev_device_get_property_value(
> +					pp->udev, "FIND_MULTIPATHS_WAIT_UNTIL");
> +				if (env == NULL || !isdigit(env[0]))
> +					continue;
> +			} else if (!is_mpath &&
>  				   (env == NULL || !strcmp(env, "0")))
>  				continue;
>  
> diff --git a/libmultipath/dict.c b/libmultipath/dict.c
> index d59fdc85a9cb..1237c23ae7db 100644
> --- a/libmultipath/dict.c
> +++ b/libmultipath/dict.c
> @@ -238,6 +238,7 @@ static const char *find_multipaths_optvals[] = {
>  	[FIND_MULTIPATHS_ON] = "on",
>  	[FIND_MULTIPATHS_STRICT] = "strict",
>  	[FIND_MULTIPATHS_GREEDY] = "greedy",
> +	[FIND_MULTIPATHS_SMART] = "smart",
>  };
>  
>  static int
> diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
> index 94c419a078bf..641ba43ad158 100644
> --- a/multipath/multipath.conf.5
> +++ b/multipath/multipath.conf.5
> @@ -978,6 +978,19 @@ with the same WWID have been detected.
>  Both multipathd and multipath treat every non-blacklisted device as multipath
>  device path.
>  .TP
> +.I smart
> +This differs from \fIfind_multipaths yes\fR only in
> +the way it treats new devices for which only one path has been
> +detected yet. When such a device is first encounted in udev rules, it is
> +treated as a multipath device. multipathd waits whether additional paths with
> +the same WWID appears. If that happens, it sets up a multipath map. If it
> +doesn\'t happen until a 
> +timeout expires, or if setting up the map fails, a new uevent is triggered for
> +the device; at second encounter in the udev rules, the device will be treated
> +as non-multipath and passed on to upper layers.
> +\fBNote:\fR this may cause delays during device detection if
> +there are single-path devices which aren\'t blacklisted.
> +.TP
>  The default is: \fBstrict\fR
>  .RE
>  .
> -- 
> 2.16.1

Re: [PATCH v2 17/20] multipath.rules: find_multipaths "smart" logic

[Benjamin Marzinski]

On Mon, Mar 19, 2018 at 04:01:52PM +0100, Martin Wilck wrote:
> When the first path to a device appears, we don't know if more paths are going
> to follow. find_multipath "smart" logic attempts to solve this dilemma by
> waiting for additional paths for a configurable time before giving up
> and releasing single paths to upper layers.
> 
> These rules apply only if both find_multipaths is set to "smart" in
> multipath.conf. In this mode, multipath -u sets DM_MULTIPATH_DEVICE_PATH=2 if
> there's no clear evidence wheteher a given device should be a multipath member
> (not blacklisted, not listed as "failed", not in WWIDs file, not member of an
> exisiting map, only one path seen yet).
> 
> In this case, pretend that the path is multipath member, disallow further
> processing by systemd (allowing multipathd some time to grab the path),
> and check again after some time. If the path is still not multipathed by then,
> pass it on to systemd for further processing.
> 
> The timeout is controlled by the "find_multipaths_timeout" config option.
> Note that delays caused by waiting don't "add up" during boot, because the
> timers run concurrently.
> 
> Implementation note: This logic requires obtaining the current time. It's not
> trivial to do this in udev rules in a portable way, because "/bin/date" is
> often not available in restricted environments such as the initrd. I chose
> the sysfs method, because /sys/class/rtc/rtc0 seems to be quite universally
> available. I'm open for better suggestions if there are any.

I have a couple of code issues, that I'll point out below, but I have an
overall question.  If multipath exists in the initramfs, and a device is
not claimed there, then after the pivot, multipath will not temporarily
claim it, correct?  I'm pretty sure, but not totally certain, that udev
database persists between the udev running in the initramfs and the
regular system. On the other hand, if multipth isn't in the initramfs
but it is in the regular system, then AFAICS, once the system pivots to
the regular fs, there is nothing to warn multipath that these devices
could already be in use, correct?  So, even if you don't need to
multipath any devices in your initramfs, you will need multipath in your
initramfs, or it could go setting devices to not ready. right?

> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipath/multipath.rules | 80 +++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 78 insertions(+), 2 deletions(-)
> 
> diff --git a/multipath/multipath.rules b/multipath/multipath.rules
> index aab64dc7182c..32d33991db3d 100644
> --- a/multipath/multipath.rules
> +++ b/multipath/multipath.rules
> @@ -21,7 +21,83 @@ TEST!="$env{MPATH_SBIN_PATH}/multipath", ENV{MPATH_SBIN_PATH}="/usr/sbin"
>  
>  # multipath -u sets DM_MULTIPATH_DEVICE_PATH
>  ENV{DM_MULTIPATH_DEVICE_PATH}!="1", IMPORT{program}="$env{MPATH_SBIN_PATH}/multipath -u %k"
> -ENV{DM_MULTIPATH_DEVICE_PATH}=="1", ENV{ID_FS_TYPE}="mpath_member", \
> -	ENV{SYSTEMD_READY}="0"
> +
> +# case 1: this is definitely multipath
> +ENV{DM_MULTIPATH_DEVICE_PATH}=="1", \
> +	ENV{ID_FS_TYPE}="mpath_member", ENV{SYSTEMD_READY}="0", \
> +	ENV{FIND_MULTIPATHS_WAIT_UNTIL}="finished", \
> +	GOTO="end_mpath"
> +
> +# case 2: this is definitely not multipath
> +ENV{DM_MULTIPATH_DEVICE_PATH}!="2", \
> +	ENV{FIND_MULTIPATHS_WAIT_UNTIL}="finished", \
> +	GOTO="end_mpath"
> +
> +# All code below here is only run in "smart" mode.
> +
> +# FIND_MULTIPATHS_WAIT_UNTIL is the timeout (in seconds after the
> +# epoch). If waiting ends for any reason, it is set to "finished".
> +IMPORT{db}="FIND_MULTIPATHS_WAIT_UNTIL"
> +
> +# At this point we know DM_MULTIPATH_DEVICE_PATH==2.
> +# (multipath -u indicates this is "maybe" multipath)
> +
> +# case 3: waiting has already finished. Treat as non-multipath.
> +ENV{FIND_MULTIPATHS_WAIT_UNTIL}=="finished", \
> +	ENV{DM_MULTIPATH_DEVICE_PATH}="", GOTO="end_mpath"
> +
> +# The timeout should have been set by the multipath -u call above, set a default
> +# value it that didn't happen for whatever reason
> +ENV{FIND_MULTIPATHS_PATH_TMO}!="?*", ENV{FIND_MULTIPATHS_PATH_TMO}="5"
> +

This code adds three more callouts.  I know that the udev people dislike
these, and they do eat up time that can cause udev to timeout on busy
systems.  To avoid the overhead of these execs, as well as to make the
rules simpler, what do you thing about moving the 

IMPORT{db}="FIND_MULTIPATHS_WAIT_UNTIL"

line before the "multipath -u" call, and passing that as a parameter if
present.  Then multipath could check the current time and compare it.
It could also return an updated FIND_MULTIPATHS_WAIT_UNTIL as a udev
environment variable, instead of returning FIND_MULTIPATHS_PATH_TMO, and
forcing udev to calculate the new timeout. That would remove the need
for the other PROGRAM calls.

-Ben

> +PROGRAM="/bin/cat $sys/class/rtc/rtc0/since_epoch", \
> +	ENV{.TIME_NOW}="$result"
> +ENV{.TIME_NOW}!="?*", ENV{DM_MULTIPATH_DEVICE_PATH}="", \
> +	ENV{FIND_MULTIPATHS_WAIT_UNTIL}="error", GOTO="end_mpath"
> +
> +ENV{FIND_MULTIPATHS_WAIT_UNTIL}!="?*", GOTO="start_wait"
> +
> +# At this point, we know that FIND_MULTIPATHS_WAIT_UNTIL is a timeout value.
> +# If it's expired, give up waiting and assume this is non-multipath.
> +PROGRAM="/bin/sh -c '[ $env{.TIME_NOW} -ge $env{FIND_MULTIPATHS_WAIT_UNTIL} ]'", \
> +	ENV{DM_MULTIPATH_DEVICE_PATH}="", \
> +	ENV{FIND_MULTIPATHS_WAIT_UNTIL}="finished", \
> +	GOTO="end_mpath"
> +
> +# Timer not expired yet. The current uevent has not been triggered by the
> +# systemd timer but something else, e.g. 60-block.rules. Continue pretending.
> +ACTION!="add", GOTO="pretend_mpath"
> +
> +# Special case: Another "add" event happened before the timeout expired.
> +# This can happen during "coldplug" after switching root FS, if a
> +# systemd timer started during initramfs processing had been cancelled.
> +# We need to start the timer again.
> +
> +LABEL="start_wait"
> +
> +# We are seeing this path for the first time, and it's "maybe" multipath.
> +# Pretend that it is actually multipath, and set a timer to create another
> +# uevent at FIND_MULTIPATHS_WAIT_UNTIL seconds after the epoch.
> +ENV{FIND_MULTIPATHS_WAIT_UNTIL}!="?*", \
> +	PROGRAM="/bin/sh -c 'echo $(( $env{.TIME_NOW} + $env{FIND_MULTIPATHS_PATH_TMO} ))'", \
> +	ENV{FIND_MULTIPATHS_WAIT_UNTIL}="$result"
> +
> +# The actual start command for the timer.
> +#
> +# The purpose of this command is only to make sure we will receive another
> +# uevent eventually. *Any* uevent may cause waiting to finish if it either ends
> +# in case 1-3 above, or if it arrives after FIND_MULTIPATHS_WAIT_UNTIL.
> +#
> +# Note that this will try to activate multipathd if it isn't running yet.
> +# If that fails, the unit starts and expires nonetheless. If multipathd
> +# startup needs to wait for other services, this wait time will add up with
> +# the --on-active timeout.
> +#
> +# We must trigger an "add" event because LVM2 will only act on those.
> +RUN+="/usr/bin/systemd-run --unit=cancel-multipath-wait-$kernel --description 'cancel waiting for multipath siblings of $kernel' --no-block --timer-property DefaultDependencies=no --timer-property Conflicts=shutdown.target --timer-property Before=shutdown.target --timer-property AccuracySec=500ms --property DefaultDependencies=no --property Conflicts=shutdown.target --property Before=shutdown.target --property Wants=multipathd.service --property After=multipathd.service --on-active=$env{FIND_MULTIPATHS_PATH_TMO} /usr/bin/udevadm trigger --action=add $sys$devpath"
> +
> +LABEL="pretend_mpath"
> +ENV{DM_MULTIPATH_DEVICE_PATH}="1"
> +ENV{SYSTEMD_READY}="0"
>  
>  LABEL="end_mpath"
> -- 
> 2.16.1

Re: [PATCH v2 17/20] multipath.rules: find_multipaths "smart" logic

[Martin Wilck]

On Tue, 2018-03-27 at 16:03 -0500, Benjamin Marzinski wrote:
> On Mon, Mar 19, 2018 at 04:01:52PM +0100, Martin Wilck wrote:
> > When the first path to a device appears, we don't know if more
> > paths are going
> > to follow. find_multipath "smart" logic attempts to solve this
> > dilemma by
> > waiting for additional paths for a configurable time before giving
> > up
> > and releasing single paths to upper layers.
> > 
> > These rules apply only if both find_multipaths is set to "smart" in
> > multipath.conf. In this mode, multipath -u sets
> > DM_MULTIPATH_DEVICE_PATH=2 if
> > there's no clear evidence wheteher a given device should be a
> > multipath member
> > (not blacklisted, not listed as "failed", not in WWIDs file, not
> > member of an
> > exisiting map, only one path seen yet).
> > 
> > In this case, pretend that the path is multipath member, disallow
> > further
> > processing by systemd (allowing multipathd some time to grab the
> > path),
> > and check again after some time. If the path is still not
> > multipathed by then,
> > pass it on to systemd for further processing.
> > 
> > The timeout is controlled by the "find_multipaths_timeout" config
> > option.
> > Note that delays caused by waiting don't "add up" during boot,
> > because the
> > timers run concurrently.
> > 
> > Implementation note: This logic requires obtaining the current
> > time. It's not
> > trivial to do this in udev rules in a portable way, because
> > "/bin/date" is
> > often not available in restricted environments such as the initrd.
> > I chose
> > the sysfs method, because /sys/class/rtc/rtc0 seems to be quite
> > universally
> > available. I'm open for better suggestions if there are any.
> 
> I have a couple of code issues, that I'll point out below, but I have
> an
> overall question.  If multipath exists in the initramfs, and a device
> is
> not claimed there, then after the pivot, multipath will not
> temporarily
> claim it, correct? 

Incorrect, it will do the temporary claim.

>  I'm pretty sure, but not totally certain, that udev
> database persists between the udev running in the initramfs and the
> regular system.

That's only true for devices that set OPTIONS+="db_persist", and dracut
sets this only for dm and md devices. For other devices,
/usr/lib/systemd/system/initrd-udevadm-cleanup-db.service cleans up the
udev data base, and devices are seen as "new" during coldplug. So, if
there's still only one path and no other information (e.g. wwids file)
after pivot, we'll wait.

>  On the other hand, if multipth isn't in the initramfs
> but it is in the regular system, then AFAICS, once the system pivots
> to
> the regular fs, there is nothing to warn multipath that these devices
> could already be in use, correct? 

Correct.

>  So, even if you don't need to
> multipath any devices in your initramfs, you will need multipath in
> your
> initramfs, or it could go setting devices to not ready. right?

The following happens: multipath -u temporarily claims the device. When
multipathd starts, it fails to set up the map, sets the "failed"
marker, and retriggers udev. The second time, multipath -u unclaims the
device because it recognizes it as failed.

I admit I haven't tested the default Red Hat setup with a very
restrictive multipath.conf in the initrd. But I'm pretty certain that
in that case, the same thing happens.
I'd be grateful if you could give it a try :-)

> 
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  multipath/multipath.rules | 80
> > +++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 78 insertions(+), 2 deletions(-)
> > 
> > diff --git a/multipath/multipath.rules b/multipath/multipath.rules
> > index aab64dc7182c..32d33991db3d 100644
> > --- a/multipath/multipath.rules
> > +++ b/multipath/multipath.rules
> > @@ -21,7 +21,83 @@ TEST!="$env{MPATH_SBIN_PATH}/multipath",
> > ENV{MPATH_SBIN_PATH}="/usr/sbin"
> >  
> >  # multipath -u sets DM_MULTIPATH_DEVICE_PATH
> >  ENV{DM_MULTIPATH_DEVICE_PATH}!="1",
> > IMPORT{program}="$env{MPATH_SBIN_PATH}/multipath -u %k"
> > -ENV{DM_MULTIPATH_DEVICE_PATH}=="1",
> > ENV{ID_FS_TYPE}="mpath_member", \
> > -	ENV{SYSTEMD_READY}="0"
> > +
> > +# case 1: this is definitely multipath
> > +ENV{DM_MULTIPATH_DEVICE_PATH}=="1", \
> > +	ENV{ID_FS_TYPE}="mpath_member", ENV{SYSTEMD_READY}="0", \
> > +	ENV{FIND_MULTIPATHS_WAIT_UNTIL}="finished", \
> > +	GOTO="end_mpath"
> > +
> > +# case 2: this is definitely not multipath
> > +ENV{DM_MULTIPATH_DEVICE_PATH}!="2", \
> > +	ENV{FIND_MULTIPATHS_WAIT_UNTIL}="finished", \
> > +	GOTO="end_mpath"
> > +
> > +# All code below here is only run in "smart" mode.
> > +
> > +# FIND_MULTIPATHS_WAIT_UNTIL is the timeout (in seconds after the
> > +# epoch). If waiting ends for any reason, it is set to "finished".
> > +IMPORT{db}="FIND_MULTIPATHS_WAIT_UNTIL"
> > +
> > +# At this point we know DM_MULTIPATH_DEVICE_PATH==2.
> > +# (multipath -u indicates this is "maybe" multipath)
> > +
> > +# case 3: waiting has already finished. Treat as non-multipath.
> > +ENV{FIND_MULTIPATHS_WAIT_UNTIL}=="finished", \
> > +	ENV{DM_MULTIPATH_DEVICE_PATH}="", GOTO="end_mpath"
> > +
> > +# The timeout should have been set by the multipath -u call above,
> > set a default
> > +# value it that didn't happen for whatever reason
> > +ENV{FIND_MULTIPATHS_PATH_TMO}!="?*",
> > ENV{FIND_MULTIPATHS_PATH_TMO}="5"
> > +
> 
> This code adds three more callouts.  I know that the udev people
> dislike
> these, and they do eat up time that can cause udev to timeout on busy
> systems.  To avoid the overhead of these execs, as well as to make
> the
> rules simpler, what do you thing about moving the 
> 
> IMPORT{db}="FIND_MULTIPATHS_WAIT_UNTIL"
> 
> line before the "multipath -u" call, and passing that as a parameter
> if
> present.  Then multipath could check the current time and compare it.
> It could also return an updated FIND_MULTIPATHS_WAIT_UNTIL as a udev
> environment variable, instead of returning FIND_MULTIPATHS_PATH_TMO,
> and
> forcing udev to calculate the new timeout. That would remove the need
> for the other PROGRAM calls.

That's a nice idea. Why didn't I have it?

Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH v2 17/20] multipath.rules: find_multipaths "smart" logic

[Benjamin Marzinski]

On Tue, Mar 27, 2018 at 11:34:00PM +0200, Martin Wilck wrote:
> On Tue, 2018-03-27 at 16:03 -0500, Benjamin Marzinski wrote:
> > On Mon, Mar 19, 2018 at 04:01:52PM +0100, Martin Wilck wrote:
> > > When the first path to a device appears, we don't know if more
> > > paths are going
> > > to follow. find_multipath "smart" logic attempts to solve this
> > > dilemma by
> > > waiting for additional paths for a configurable time before giving
> > > up
> > > and releasing single paths to upper layers.
> > > 
> > > These rules apply only if both find_multipaths is set to "smart" in
> > > multipath.conf. In this mode, multipath -u sets
> > > DM_MULTIPATH_DEVICE_PATH=2 if
> > > there's no clear evidence wheteher a given device should be a
> > > multipath member
> > > (not blacklisted, not listed as "failed", not in WWIDs file, not
> > > member of an
> > > exisiting map, only one path seen yet).
> > > 
> > > In this case, pretend that the path is multipath member, disallow
> > > further
> > > processing by systemd (allowing multipathd some time to grab the
> > > path),
> > > and check again after some time. If the path is still not
> > > multipathed by then,
> > > pass it on to systemd for further processing.
> > > 
> > > The timeout is controlled by the "find_multipaths_timeout" config
> > > option.
> > > Note that delays caused by waiting don't "add up" during boot,
> > > because the
> > > timers run concurrently.
> > > 
> > > Implementation note: This logic requires obtaining the current
> > > time. It's not
> > > trivial to do this in udev rules in a portable way, because
> > > "/bin/date" is
> > > often not available in restricted environments such as the initrd.
> > > I chose
> > > the sysfs method, because /sys/class/rtc/rtc0 seems to be quite
> > > universally
> > > available. I'm open for better suggestions if there are any.
> > 
> > I have a couple of code issues, that I'll point out below, but I have
> > an
> > overall question.  If multipath exists in the initramfs, and a device
> > is
> > not claimed there, then after the pivot, multipath will not
> > temporarily
> > claim it, correct? 
> 
> Incorrect, it will do the temporary claim.
> 
> >  I'm pretty sure, but not totally certain, that udev
> > database persists between the udev running in the initramfs and the
> > regular system.
> 
> That's only true for devices that set OPTIONS+="db_persist", and dracut
> sets this only for dm and md devices. For other devices,
> /usr/lib/systemd/system/initrd-udevadm-cleanup-db.service cleans up the
> udev data base, and devices are seen as "new" during coldplug. So, if
> there's still only one path and no other information (e.g. wwids file)
> after pivot, we'll wait.
> 
> >  On the other hand, if multipth isn't in the initramfs
> > but it is in the regular system, then AFAICS, once the system pivots
> > to
> > the regular fs, there is nothing to warn multipath that these devices
> > could already be in use, correct? 
> 
> Correct.
> 
> >  So, even if you don't need to
> > multipath any devices in your initramfs, you will need multipath in
> > your
> > initramfs, or it could go setting devices to not ready. right?
> 
> The following happens: multipath -u temporarily claims the device. When
> multipathd starts, it fails to set up the map, sets the "failed"
> marker, and retriggers udev. The second time, multipath -u unclaims the
> device because it recognizes it as failed.

But if that device is already in use because multipath didn't claim it
in the initramfs, and you suddenly mark it as ENV{SYSTEMD_READY}="0",
this can cause systemd to automatically unmount any filesystem on it.
This isn't just a problem with Red Hat's setup.  If it's not a
configured device type, there will only be a short timeout, but that's
still enough to mess with devices that are already in use.  I'm pretty
sure that the multipath temporary claiming is only safe the very first
time a device appears. Otherwise, it's possible that something else will
claim it first, and then multipath will claim it and mess with that
other user.

> I admit I haven't tested the default Red Hat setup with a very
> restrictive multipath.conf in the initrd. But I'm pretty certain that
> in that case, the same thing happens.
> I'd be grateful if you could give it a try :-)
> 
> > 
> > > 
> > > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > > ---
> > >  multipath/multipath.rules | 80
> > > +++++++++++++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 78 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/multipath/multipath.rules b/multipath/multipath.rules
> > > index aab64dc7182c..32d33991db3d 100644
> > > --- a/multipath/multipath.rules
> > > +++ b/multipath/multipath.rules
> > > @@ -21,7 +21,83 @@ TEST!="$env{MPATH_SBIN_PATH}/multipath",
> > > ENV{MPATH_SBIN_PATH}="/usr/sbin"
> > >  
> > >  # multipath -u sets DM_MULTIPATH_DEVICE_PATH
> > >  ENV{DM_MULTIPATH_DEVICE_PATH}!="1",
> > > IMPORT{program}="$env{MPATH_SBIN_PATH}/multipath -u %k"
> > > -ENV{DM_MULTIPATH_DEVICE_PATH}=="1",
> > > ENV{ID_FS_TYPE}="mpath_member", \
> > > -	ENV{SYSTEMD_READY}="0"
> > > +
> > > +# case 1: this is definitely multipath
> > > +ENV{DM_MULTIPATH_DEVICE_PATH}=="1", \
> > > +	ENV{ID_FS_TYPE}="mpath_member", ENV{SYSTEMD_READY}="0", \
> > > +	ENV{FIND_MULTIPATHS_WAIT_UNTIL}="finished", \
> > > +	GOTO="end_mpath"
> > > +
> > > +# case 2: this is definitely not multipath
> > > +ENV{DM_MULTIPATH_DEVICE_PATH}!="2", \
> > > +	ENV{FIND_MULTIPATHS_WAIT_UNTIL}="finished", \
> > > +	GOTO="end_mpath"
> > > +
> > > +# All code below here is only run in "smart" mode.
> > > +
> > > +# FIND_MULTIPATHS_WAIT_UNTIL is the timeout (in seconds after the
> > > +# epoch). If waiting ends for any reason, it is set to "finished".
> > > +IMPORT{db}="FIND_MULTIPATHS_WAIT_UNTIL"
> > > +
> > > +# At this point we know DM_MULTIPATH_DEVICE_PATH==2.
> > > +# (multipath -u indicates this is "maybe" multipath)
> > > +
> > > +# case 3: waiting has already finished. Treat as non-multipath.
> > > +ENV{FIND_MULTIPATHS_WAIT_UNTIL}=="finished", \
> > > +	ENV{DM_MULTIPATH_DEVICE_PATH}="", GOTO="end_mpath"
> > > +
> > > +# The timeout should have been set by the multipath -u call above,
> > > set a default
> > > +# value it that didn't happen for whatever reason
> > > +ENV{FIND_MULTIPATHS_PATH_TMO}!="?*",
> > > ENV{FIND_MULTIPATHS_PATH_TMO}="5"
> > > +
> > 
> > This code adds three more callouts.  I know that the udev people
> > dislike
> > these, and they do eat up time that can cause udev to timeout on busy
> > systems.  To avoid the overhead of these execs, as well as to make
> > the
> > rules simpler, what do you thing about moving the 
> > 
> > IMPORT{db}="FIND_MULTIPATHS_WAIT_UNTIL"
> > 
> > line before the "multipath -u" call, and passing that as a parameter
> > if
> > present.  Then multipath could check the current time and compare it.
> > It could also return an updated FIND_MULTIPATHS_WAIT_UNTIL as a udev
> > environment variable, instead of returning FIND_MULTIPATHS_PATH_TMO,
> > and
> > forcing udev to calculate the new timeout. That would remove the need
> > for the other PROGRAM calls.
> 
> That's a nice idea. Why didn't I have it?
> 
> Martin
> 
> -- 
> Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)

Re: [PATCH v2 17/20] multipath.rules: find_multipaths "smart" logic

[Martin Wilck]

On Tue, 2018-03-27 at 18:07 -0500, Benjamin Marzinski wrote:
> On Tue, Mar 27, 2018 at 11:34:00PM +0200, Martin Wilck wrote:
> > 
> > The following happens: multipath -u temporarily claims the device.
> > When
> > multipathd starts, it fails to set up the map, sets the "failed"
> > marker, and retriggers udev. The second time, multipath -u unclaims
> > the
> > device because it recognizes it as failed.
> 
> But if that device is already in use because multipath didn't claim
> it
> in the initramfs, and you suddenly mark it as ENV{SYSTEMD_READY}="0",
> this can cause systemd to automatically unmount any filesystem on it.
> This isn't just a problem with Red Hat's setup.  If it's not a
> configured device type, there will only be a short timeout, but
> that's
> still enough to mess with devices that are already in use.  I'm
> pretty
> sure that the multipath temporary claiming is only safe the very
> first
> time a device appears. Otherwise, it's possible that something else
> will
> claim it first, and then multipath will claim it and mess with that
> other user.
> 

Arrgh. Thanks for pointing that out. It's even worse because even if we
could figure out whether we're being called for the first or second
time, it wouldn't be sufficient. multipath.rules may not even be
present in the initrd, so the device may be present in the system, and
used, without multipath.rules having ever been called.

The only way I see to avoid this is to try calling open(O_EXCL) on the
path device in "multipath -u". So far we've avoided that because it's
not completely race-free. But we're not talking about a race here, but
a situation where some entity grabbed a device before pivot-root.
So we could attempt open(O_EXCL) only in the "is maybe a valid path"
case, and only return "maybe" if that open succeeds. Otherwise we'd
return "no", as we already checked that the device isn't currently part
of a multipath map.

Agreed?

Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH v2 17/20] multipath.rules: find_multipaths "smart" logic

[Benjamin Marzinski]

On Wed, Mar 28, 2018 at 04:51:12PM +0200, Martin Wilck wrote:
> On Tue, 2018-03-27 at 18:07 -0500, Benjamin Marzinski wrote:
> > On Tue, Mar 27, 2018 at 11:34:00PM +0200, Martin Wilck wrote:
> > > 
> > > The following happens: multipath -u temporarily claims the device.
> > > When
> > > multipathd starts, it fails to set up the map, sets the "failed"
> > > marker, and retriggers udev. The second time, multipath -u unclaims
> > > the
> > > device because it recognizes it as failed.
> > 
> > But if that device is already in use because multipath didn't claim
> > it
> > in the initramfs, and you suddenly mark it as ENV{SYSTEMD_READY}="0",
> > this can cause systemd to automatically unmount any filesystem on it.
> > This isn't just a problem with Red Hat's setup.  If it's not a
> > configured device type, there will only be a short timeout, but
> > that's
> > still enough to mess with devices that are already in use.  I'm
> > pretty
> > sure that the multipath temporary claiming is only safe the very
> > first
> > time a device appears. Otherwise, it's possible that something else
> > will
> > claim it first, and then multipath will claim it and mess with that
> > other user.
> > 
> 
> Arrgh. Thanks for pointing that out. It's even worse because even if we
> could figure out whether we're being called for the first or second
> time, it wouldn't be sufficient. multipath.rules may not even be
> present in the initrd, so the device may be present in the system, and
> used, without multipath.rules having ever been called.
> 
> The only way I see to avoid this is to try calling open(O_EXCL) on the
> path device in "multipath -u". So far we've avoided that because it's
> not completely race-free. But we're not talking about a race here, but
> a situation where some entity grabbed a device before pivot-root.
> So we could attempt open(O_EXCL) only in the "is maybe a valid path"
> case, and only return "maybe" if that open succeeds. Otherwise we'd
> return "no", as we already checked that the device isn't currently part
> of a multipath map.
> 

Something like that should work.  I think we need to be careful to only
ever do the exclusive open on an "add" event. In fact in probably
wouldn't hurt to add some checks to make sure that we never start
a temporary claim on a change event.  If we get a change event and
FIND_MULTIPATHS_WAIT_UNTIL is unset, we should just assume that we
missed the add event, and it should be set to "finished".

What would be really nice would be to be able to tell if we are doing a
cold-plug add after the pivot.  This is the only time where we actually
need to check if a device is in use. Otherwise nobody else will have had
a chance to use it yet.

-Ben

> Agreed?
> 
> Martin
> 
> -- 
> Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)

Re: [PATCH v2 17/20] multipath.rules: find_multipaths "smart" logic

[Hannes Reinecke]

On Wed, 28 Mar 2018 16:51:12 +0200
Martin Wilck <mwilck@suse.com> wrote:

> On Tue, 2018-03-27 at 18:07 -0500, Benjamin Marzinski wrote:
> > On Tue, Mar 27, 2018 at 11:34:00PM +0200, Martin Wilck wrote:  
> > > 
> > > The following happens: multipath -u temporarily claims the device.
> > > When
> > > multipathd starts, it fails to set up the map, sets the "failed"
> > > marker, and retriggers udev. The second time, multipath -u
> > > unclaims the
> > > device because it recognizes it as failed.  
> > 
> > But if that device is already in use because multipath didn't claim
> > it
> > in the initramfs, and you suddenly mark it as
> > ENV{SYSTEMD_READY}="0", this can cause systemd to automatically
> > unmount any filesystem on it. This isn't just a problem with Red
> > Hat's setup.  If it's not a configured device type, there will only
> > be a short timeout, but that's
> > still enough to mess with devices that are already in use.  I'm
> > pretty
> > sure that the multipath temporary claiming is only safe the very
> > first
> > time a device appears. Otherwise, it's possible that something else
> > will
> > claim it first, and then multipath will claim it and mess with that
> > other user.
> >   
> 
> Arrgh. Thanks for pointing that out. It's even worse because even if
> we could figure out whether we're being called for the first or second
> time, it wouldn't be sufficient. multipath.rules may not even be
> present in the initrd, so the device may be present in the system, and
> used, without multipath.rules having ever been called.
> 
> The only way I see to avoid this is to try calling open(O_EXCL) on the
> path device in "multipath -u". So far we've avoided that because it's
> not completely race-free. But we're not talking about a race here, but
> a situation where some entity grabbed a device before pivot-root.
> So we could attempt open(O_EXCL) only in the "is maybe a valid path"
> case, and only return "maybe" if that open succeeds. Otherwise we'd
> return "no", as we already checked that the device isn't currently
> part of a multipath map.
> 
Ho-hum.

Watch out when you do this; systemd will generate another event
whenever you close this fd, and suddenly you find yourself in a middle
of an event storm ...

Cheers,

Hannes

Re: [PATCH v2 17/20] multipath.rules: find_multipaths "smart" logic

[Martin Wilck]

On Thu, 2018-03-29 at 08:04 +0200, Hannes Reinecke wrote:
> 
> > The only way I see to avoid this is to try calling open(O_EXCL) on
> > the
> > path device in "multipath -u". So far we've avoided that because
> > it's
> > not completely race-free. But we're not talking about a race here,
> > but
> > a situation where some entity grabbed a device before pivot-root.
> > So we could attempt open(O_EXCL) only in the "is maybe a valid
> > path"
> > case, and only return "maybe" if that open succeeds. Otherwise we'd
> > return "no", as we already checked that the device isn't currently
> > part of a multipath map.
> > 
> 
> Ho-hum.
> 
> Watch out when you do this; systemd will generate another event
> whenever you close this fd, and suddenly you find yourself in a
> middle
> of an event storm ...

Thanks for mentioning that.

Udev listens for inotify events for "close after write". We will do the
test with O_RDONLY|O_EXCL, so no event should be generated. Moreover,
events should only occur if udev is watching the device, which
shouldn't be the case while an event is processed (but I'm not 100%
certain about that). Anyway, I'll test this.
 
Cheers,
Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel