[PATCH v3 00/20] multipath path classification

[PATCH v3 00/20] multipath path classification

[Martin Wilck]

Hello Christophe,

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", as well
as Ben's review of v2 of this series. Up to 08/20, this series is the same
as v2. Patch 11/20ff are substantially changed from v2. I left out the log
level changes I'd included in the v2 series, they will be submitted as a
separate series later.

(This set is based on Ben's late series "multipath: new and rebased
patches" and "multipathd per-device waiter fixes".)

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-10. This was not in the RFC series.
   In contrast the v2 series, failure is not considered permanent;
   multipath and multipathd retry to setup the map under normal operations.
   Only "multipath -u/-c" checks and honors the failed flag always.
 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 11-20, 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 v2->v3:

Almost all of these are direct or indirect outcomes of Ben Marzinski's review
of the v2 series. Kudos to Ben for the excellent review!

 - 09/20: use symbolic return codes for the helper functions for map failure
 - 09/20: use pthread_once() for path initialization
 - 11/20: added one more common code path invocation I'd forgotten in v2. 
 - 13/20 "multipath -u: treat failed wwids as invalid" replaces v2 11/20
   "don't try to setup failed wwids again" (we do try now, just not in
   "multipath -u").
 - 16/20 and 19-20/20 change the way the udev rules communicate with multipath -u.
   In contrast to v2, we directly calculate the expiry time in "multipath -u" itself
   now, saving several callouts in the udev rules. Moreover, we remember the
   timeouts as timestamps under /dev/shm, allowing to pass this information
   across pivot-root.
 - 17/20 introduces a test if a path is busy, addressing Ben's valid concern that
   a file system mounted on a path device in the initrd might be unmounted
   because rules setting SYSTEMD_READY=0 on the device, permanently or
   temporarily. 
 - 18/20 is an new addition, a speed-up for some cases where multipath -u would
   otherwise need to scan all paths and dm maps.
 - In 20/20, added the code path to multipath.rules for
   cancelling a running systemd timer and thus avoiding a spurious "add" event.
 - dropped v2 18/20-20/20.

Patch 1-8 and 12, 14, 15, 19 are identical or only minimally modified wrt
their counterparts in v2 (numbering changed from 11 on, though).

Changes RFC->v2 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)

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()
  multipath -u: common code path for result message
  multipath -u: change output to environment/key format
  multipath -u: treat failed wwids as invalid
  multipath -u: add DM_MULTIPATH_DEVICE_PATH=2 for "maybe"
  libmultipath: implement find_multipaths_timeout
  multipath -u : set FIND_MULTIPATHS_WAIT_UNTIL from /dev/shm
  multipath -u: test if path is busy
  multipath -u: quick check if path is multipathed
  libmultipath: enable find_multipaths "smart"
  multipath.rules: find_multipaths "smart" logic

 libmultipath/config.h      |   3 +-
 libmultipath/configure.c   |  70 ++++++++++++-
 libmultipath/configure.h   |   1 +
 libmultipath/defaults.h    |   5 +-
 libmultipath/devmapper.c   |   9 +-
 libmultipath/dict.c        |  54 +++++++++-
 libmultipath/file.c        |   8 +-
 libmultipath/file.h        |   3 +-
 libmultipath/propsel.c     |  25 +++++
 libmultipath/propsel.h     |   1 +
 libmultipath/structs.h     |  28 ++++++
 libmultipath/sysfs.c       |  66 ++++++++++++
 libmultipath/sysfs.h       |   2 +
 libmultipath/wwids.c       | 132 ++++++++++++++++++++++--
 libmultipath/wwids.h       |  13 ++-
 multipath/main.c           | 246 +++++++++++++++++++++++++++++++++++++++------
 multipath/multipath.8      |   9 +-
 multipath/multipath.conf.5 |  75 ++++++++++----
 multipath/multipath.rules  |  62 +++++++++++-
 multipathd/main.c          |  15 +--
 multipathd/multipathd.8    |   8 +-
 21 files changed, 744 insertions(+), 91 deletions(-)

-- 
2.16.1

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

[Martin Wilck]

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 716203e..20f46ac 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 v3 02/20] Revert "multipathd: imply -n if find_multipaths is set"

[Martin Wilck]

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 3ae0442..0435133 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 v3 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().

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 fa6e21c..16ce797 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 bc70a27..cb6ab52 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 9527012..d9a78b3 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 0435133..707245c 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 v3 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").

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 20f46ac..4d45df3 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 v3 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>
Reviewed-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 16ce797..245bd11 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 27a7e6f..545cbc2 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 cb6ab52..ea56911 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 4d45df3..19c729d 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 707245c..da40595 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 v3 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.

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 245bd11..838d145 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 v3 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.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
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 a20ac2a..21d8e72 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 2b270ca..690182c 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 ea273dd..a952c60 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 88a4b78..d6482f8 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 ea56911..5a2e86d 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 19c729d..3944fd5 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 56f8703..329658d 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 c4d0789..6965dac 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 da40595..bfbe5f6 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 5c96680..e78ac9e 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 v3 08/20] libmultipath: use const char* in open_file()

[Martin Wilck]

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 e4951c9..d5165ec 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 4f96dbf..70bffa5 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 v3 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/defaults.h |   1 +
 libmultipath/wwids.c    | 110 ++++++++++++++++++++++++++++++++++++++++++++++++
 libmultipath/wwids.h    |  11 +++++
 3 files changed, 122 insertions(+)

diff --git a/libmultipath/defaults.h b/libmultipath/defaults.h
index 690182c..19ad2bf 100644
--- a/libmultipath/defaults.h
+++ b/libmultipath/defaults.h
@@ -53,5 +53,6 @@
 #define DEFAULT_WWIDS_FILE	"/etc/multipath/wwids"
 #define DEFAULT_PRKEYS_FILE    "/etc/multipath/prkeys"
 #define DEFAULT_CONFIG_DIR	"/etc/multipath/conf.d"
+#define MULTIPATH_SHM_BASE	"/dev/shm/multipath/"
 
 char * set_default (char * str);
diff --git a/libmultipath/wwids.c b/libmultipath/wwids.c
index 5a2e86d..5351e3c 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,112 @@ remember_wwid(char *wwid)
 		condlog(4, "wwid %s already in wwids file", wwid);
 	return ret;
 }
+
+static const char shm_dir[] = MULTIPATH_SHM_BASE "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 void init_shm_paths(void)
+{
+	snprintf(_shm_lock_path, sizeof(_shm_lock_path),
+		 "%s/%s", shm_dir, shm_lock);
+}
+
+static pthread_once_t shm_path_once = PTHREAD_ONCE_INIT;
+
+static int multipath_shm_open(bool rw)
+{
+	int fd;
+	int can_write;
+
+	pthread_once(&shm_path_once, init_shm_paths);
+	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;
+}
+
+static 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 == WWID_FAILED_ERROR)
+		condlog(1, "%s: %s: %s", msg, wwid, strerror(errno));
+	else if (r == WWID_FAILED_CHANGED)
+		condlog(3, "%s: %s", msg, wwid);
+	else if (!rw)
+		condlog(4, "%s: %s is %s", msg, wwid,
+			r == WWID_IS_FAILED ? "failed" : "good");
+
+	return r;
+}
+
+static int _is_failed(const char *path)
+{
+	struct stat st;
+
+	if (lstat(path, &st) == 0)
+		return WWID_IS_FAILED;
+	else if (errno == ENOENT)
+		return WWID_IS_NOT_FAILED;
+	else
+		return WWID_FAILED_ERROR;
+}
+
+static int _mark_failed(const char *path)
+{
+	/* Called from _failed_wwid_op: we know that shm_lock_path exists */
+	if (_is_failed(path) == WWID_IS_FAILED)
+		return WWID_FAILED_UNCHANGED;
+	return (link(shm_lock_path, path) == 0 ? WWID_FAILED_CHANGED :
+		WWID_FAILED_ERROR);
+}
+
+static int _unmark_failed(const char *path)
+{
+	if (_is_failed(path) == WWID_IS_NOT_FAILED)
+		return WWID_FAILED_UNCHANGED;
+	return (unlink(path) == 0 ? WWID_FAILED_CHANGED : WWID_FAILED_ERROR);
+}
+
+#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 d9a78b3..0c6ee54 100644
--- a/libmultipath/wwids.h
+++ b/libmultipath/wwids.h
@@ -18,4 +18,15 @@ int check_wwids_file(char *wwid, int write_wwid);
 int remove_wwid(char *wwid);
 int replace_wwids(vector mp);
 
+enum {
+	WWID_IS_NOT_FAILED = 0,
+	WWID_IS_FAILED,
+	WWID_FAILED_UNCHANGED,
+	WWID_FAILED_CHANGED,
+	WWID_FAILED_ERROR = -1,
+};
+
+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 v3 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 |  9 ++++++++-
 libmultipath/structs.h   |  1 +
 multipathd/main.c        |  2 +-
 5 files changed, 39 insertions(+), 12 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 838d145..88e6687 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 545cbc2..8b56d33 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 9bafbc6..d36e256 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,12 @@ 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) ==
+			    WWID_FAILED_CHANGED)
+				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 +433,8 @@ int dm_addmap_create (struct multipath *mpp, char * params)
 			break;
 		}
 	}
+	if (mark_failed_wwid(mpp->wwid) == WWID_FAILED_CHANGED)
+		mpp->needs_paths_uevent = 1;
 	return 0;
 }
 
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index d6482f8..32f4b2d 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 bfbe5f6..ea8c413 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 v3 11/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 | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/multipath/main.c b/multipath/main.c
index 3944fd5..587111b 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -350,6 +350,16 @@ out:
 	return r;
 }
 
+static int print_cmd_valid(const char *devpath, int k)
+{
+	if (k < 0 || k > 1)
+		return 1;
+
+	printf("%s is%s a valid multipath device path\n",
+	       devpath, k ? "" : " not");
+	return k == 1;
+}
+
 /*
  * Return value:
  *  -1: Retry
@@ -391,10 +401,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 +414,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;
@@ -446,9 +453,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;
 			}
 		}
 	}
@@ -492,9 +497,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) {
@@ -508,6 +511,10 @@ configure (struct config *conf, enum mpath_cmds cmd,
 	r = coalesce_paths(&vecs, NULL, refwwid,
 			   conf->force_reload, cmd);
 
+print_valid:
+	if (cmd == CMD_VALID_PATH)
+		r = print_cmd_valid(devpath, r);
+
 out:
 	if (refwwid)
 		FREE(refwwid);
@@ -843,8 +850,7 @@ main (int argc, char *argv[])
 		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);
+				r = print_cmd_valid(dev, 1);
 				goto out;
 			}
 		} else
-- 
2.16.1

[PATCH v3 12/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.

The parameters "pathvec" and "conf" for print_cmd_valid are currently
unused, but will be in follow-up patches.

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

diff --git a/multipath/main.c b/multipath/main.c
index 587111b..dbe908e 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -350,13 +350,15 @@ out:
 	return r;
 }
 
-static int print_cmd_valid(const char *devpath, int k)
+static int print_cmd_valid(int k, const vector pathvec,
+			   struct config *conf)
 {
-	if (k < 0 || k > 1)
+	static const int vals[] = { 1, 0 };
+
+	if (k < 0 || k >= sizeof(vals))
 		return 1;
 
-	printf("%s is%s a valid multipath device path\n",
-	       devpath, k ? "" : " not");
+	printf("DM_MULTIPATH_DEVICE_PATH=\"%d\"\n", vals[k]);
 	return k == 1;
 }
 
@@ -513,7 +515,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
 
 print_valid:
 	if (cmd == CMD_VALID_PATH)
-		r = print_cmd_valid(devpath, r);
+		r = print_cmd_valid(r, pathvec, conf);
 
 out:
 	if (refwwid)
@@ -850,7 +852,7 @@ main (int argc, char *argv[])
 		if (fd == -1) {
 			condlog(3, "%s: daemon is not running", dev);
 			if (!systemd_service_enabled(dev)) {
-				r = print_cmd_valid(dev, 1);
+				r = print_cmd_valid(1, NULL, conf);
 				goto out;
 			}
 		} else
diff --git a/multipath/multipath.rules b/multipath/multipath.rules
index 6f8ee2b..aab64dc 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 v3 13/20] multipath -u: treat failed wwids as invalid

[Martin Wilck]

If a WWID has been marked as "failed", don't treat it as "valid multipath
device path" in multipath -c/-u. This is key to achieve consistency between
multipathd and udev rule processing.

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

diff --git a/multipath/main.c b/multipath/main.c
index dbe908e..88495f8 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -450,8 +450,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) == WWID_IS_FAILED) {
+				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)) {
-- 
2.16.1

[PATCH v3 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.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipath/main.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/multipath/main.c b/multipath/main.c
index 88495f8..650e2bb 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -353,7 +353,7 @@ out:
 static int print_cmd_valid(int k, const vector pathvec,
 			   struct config *conf)
 {
-	static const int vals[] = { 1, 0 };
+	static const int vals[] = { 1, 0, 2 };
 
 	if (k < 0 || k >= sizeof(vals))
 		return 1;
@@ -503,6 +503,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;
 	}
 
-- 
2.16.1

[PATCH v3 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/multipath.conf.5 | 18 ++++++++++++++++++
 7 files changed, 54 insertions(+)

diff --git a/libmultipath/config.h b/libmultipath/config.h
index 21d8e72..72d6601 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 19ad2bf..d7b87b4 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 a952c60..d59fdc8 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 58a6a42..87d6865 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 136f906..b6475d0 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 32f4b2d..d4e18df 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/multipath.conf.5 b/multipath/multipath.conf.5
index 6965dac..94c419a 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 v3 16/20] multipath -u : set FIND_MULTIPATHS_WAIT_UNTIL from /dev/shm

[Martin Wilck]

In "find_multipaths smart" mode, use time stamps under
/dev/shm/multipath/find_multipaths to track waiting for multipath
siblings. When a path is first encountered and is "maybe" multipath, create a
file under /dev/shm, set its modification time to the expiry time of the
timer, and set the FIND_MULTIPATHS_WAIT_UNTIL variable. On later calls, also set
FIND_MULTIPATHS_WAIT_UNTIL to the expiry time (but don't change the time
stamp) if it's not expired yet, or 0 if it is expired. Set
FIND_MULTIPATHS_WAIT_UNTIL even if enough evidence becomes available to decide
if the path needs to be multipathed - this enables the udev rules to detect
that this is a device a timer has been started for, and stop it. By using
/dev/shm, we share information about "smart" timers between initrd and root
file system, and thus only calculate the timeout once.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/file.c |   2 +-
 libmultipath/file.h |   1 +
 multipath/main.c    | 133 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 135 insertions(+), 1 deletion(-)

diff --git a/libmultipath/file.c b/libmultipath/file.c
index d5165ec..8727f16 100644
--- a/libmultipath/file.c
+++ b/libmultipath/file.c
@@ -36,7 +36,7 @@
  * See the file COPYING included with this distribution for more details.
  */
 
-static int
+int
 ensure_directories_exist(const char *str, mode_t dir_mode)
 {
 	char *pathname;
diff --git a/libmultipath/file.h b/libmultipath/file.h
index 70bffa5..29520c7 100644
--- a/libmultipath/file.h
+++ b/libmultipath/file.h
@@ -6,6 +6,7 @@
 #define _FILE_H
 
 #define FILE_TIMEOUT 30
+int ensure_directories_exist(const char *str, mode_t dir_mode);
 int open_file(const char *file, int *can_write, const char *header);
 
 #endif /* _FILE_H */
diff --git a/multipath/main.c b/multipath/main.c
index 650e2bb..d09f117 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -29,6 +29,7 @@
 #include <ctype.h>
 #include <libudev.h>
 #include <syslog.h>
+#include <fcntl.h>
 
 #include "checkers.h"
 #include "prio.h"
@@ -60,6 +61,9 @@
 #include "uxsock.h"
 #include "mpath_cmd.h"
 #include "foreign.h"
+#include "propsel.h"
+#include "time-util.h"
+#include "file.h"
 
 int logsink;
 struct udev *udev;
@@ -350,14 +354,143 @@ out:
 	return r;
 }
 
+enum {
+	FIND_MULTIPATHS_WAIT_DONE = 0,
+	FIND_MULTIPATHS_WAITING = 1,
+	FIND_MULTIPATHS_ERROR = -1,
+	FIND_MULTIPATHS_NEVER = -2,
+};
+
+static const char shm_find_mp_dir[] = MULTIPATH_SHM_BASE "find_multipaths";
+static void close_fd(void* arg)
+{
+	close((long)arg);
+}
+
+/**
+ * find_multipaths_check_timeout(wwid, tmo)
+ * Helper for "find_multipaths smart"
+ *
+ * @param[in] pp: path to check / record
+ * @param[in] tmo: configured timeout for this WWID, or value <= 0 for checking
+ * @param[out] until: timestamp until we must wait, CLOCK_REALTIME, if return
+ *             value is FIND_MULTIPATHS_WAITING
+ * @returns: FIND_MULTIPATHS_WAIT_DONE, if waiting has finished
+ * @returns: FIND_MULTIPATHS_ERROR, if internal error occured
+ * @returns: FIND_MULTIPATHS_NEVER, if tmo is 0 and we didn't wait for this
+ *           device
+ * @returns: FIND_MULTIPATHS_WAITING, if timeout hasn't expired
+ */
+static int find_multipaths_check_timeout(const struct path *pp, long tmo,
+					 struct timespec *until)
+{
+	char path[PATH_MAX];
+	struct timespec now, ftimes[2], tdiff;
+	struct stat st;
+	long fd;
+	int r, err, retries = 0;
+
+	clock_gettime(CLOCK_REALTIME, &now);
+
+	if (snprintf(path, sizeof(path), "%s/%s", shm_find_mp_dir, pp->dev_t)
+	    >= sizeof(path)) {
+		condlog(1, "%s: path name overflow", __func__);
+		return -1;
+	}
+
+	if (ensure_directories_exist(path, 0700)) {
+		condlog(1, "%s: error creating directories", __func__);
+		return FIND_MULTIPATHS_ERROR;
+	}
+
+retry:
+	fd = open(path, O_RDONLY);
+	if (fd != -1) {
+		pthread_cleanup_push(close_fd, (void*)fd);
+		r = fstat(fd, &st);
+		if (r != 0)
+			err = errno;
+		pthread_cleanup_pop(1);
+
+	} else if (tmo > 0) {
+		if (errno == ENOENT)
+			fd = open(path, O_RDWR|O_EXCL|O_CREAT, 0644);
+		if (fd == -1) {
+			if (errno == EEXIST && !retries++)
+				/* We could have raced with another process */
+				goto retry;
+			condlog(1, "%s: error opening %s: %s",
+				__func__, path, strerror(errno));
+			return FIND_MULTIPATHS_ERROR;
+		};
+
+		pthread_cleanup_push(close_fd, (void*)fd);
+		/*
+		 * We just created the file. Set st_mtim to our desired
+		 * expiry time.
+		 */
+		ftimes[0].tv_sec = 0;
+		ftimes[0].tv_nsec = UTIME_OMIT;
+		ftimes[1].tv_sec = now.tv_sec + tmo;
+		ftimes[1].tv_nsec = now.tv_nsec;
+		if (futimens(fd, ftimes) != 0) {
+			condlog(1, "%s: error in futimens(%s): %s", __func__,
+				path, strerror(errno));
+		}
+		r = fstat(fd, &st);
+		if (r != 0)
+			err = errno;
+		pthread_cleanup_pop(1);
+	} else
+		return FIND_MULTIPATHS_NEVER;
+
+	if (r != 0) {
+		condlog(1, "%s: error in fstat for %s: %s", __func__,
+			path, strerror(err));
+		return FIND_MULTIPATHS_ERROR;
+	}
+
+	timespecsub(&st.st_mtim, &now, &tdiff);
+
+	if (tdiff.tv_sec <= 0)
+		return FIND_MULTIPATHS_WAIT_DONE;
+	else {
+		*until = tdiff;
+		return FIND_MULTIPATHS_WAITING;
+	}
+}
+
 static int print_cmd_valid(int k, const vector pathvec,
 			   struct config *conf)
 {
 	static const int vals[] = { 1, 0, 2 };
+	int wait = FIND_MULTIPATHS_NEVER;
+	struct timespec until;
+	struct path *pp;
 
 	if (k < 0 || k >= sizeof(vals))
 		return 1;
 
+	if (k == 2) {
+		/*
+		 * Caller ensures that pathvec[0] is the path to
+		 * examine.
+		 */
+		pp = VECTOR_SLOT(pathvec, 0);
+		select_find_multipaths_timeout(conf, pp);
+		wait = find_multipaths_check_timeout(
+			pp, pp->find_multipaths_timeout, &until);
+		if (wait != FIND_MULTIPATHS_WAITING)
+			k = 1;
+	} else if (pathvec != NULL) {
+		pp = VECTOR_SLOT(pathvec, 0);
+		wait = find_multipaths_check_timeout(pp, 0, &until);
+	}
+	if (wait == FIND_MULTIPATHS_WAITING)
+		printf("FIND_MULTIPATHS_WAIT_UNTIL=\"%ld.%06ld\"\n",
+			       until.tv_sec, until.tv_nsec/1000);
+	else if (wait == FIND_MULTIPATHS_WAIT_DONE)
+		printf("FIND_MULTIPATHS_WAIT_UNTIL=\"0\"\n");
 	printf("DM_MULTIPATH_DEVICE_PATH=\"%d\"\n", vals[k]);
 	return k == 1;
 }
-- 
2.16.1

[PATCH v3 17/20] multipath -u: test if path is busy

[Martin Wilck]

For "find_multipaths smart", check if a path is already in use
before setting DM_MULTIPATH_DEVICE_PATH to 1 or 2 (and thus,
SYSTEMD_READY=0). If we don't do this, a device which has already been
mounted (e.g. during initrd processing) may be unmounted by systemd, causing
havoc to the boot process.

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

diff --git a/multipath/main.c b/multipath/main.c
index d09f117..392d5f0 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -629,16 +629,45 @@ configure (struct config *conf, enum mpath_cmds cmd,
 
 
 	if (cmd == CMD_VALID_PATH) {
+		struct path *pp;
+		int fd;
+
 		/* This only happens if find_multipaths and
 		 * ignore_wwids is set.
 		 * If there is currently a multipath device matching
 		 * the refwwid, or there is more than one path matching
 		 * the refwwid, then the path is valid */
-		if (VECTOR_SIZE(curmp) != 0 || VECTOR_SIZE(pathvec) > 1)
+		if (VECTOR_SIZE(curmp) != 0) {
+			r = 0;
+			goto print_valid;
+		} else if (VECTOR_SIZE(pathvec) > 1)
 			r = 0;
 		else
 			/* Use r=2 as an indication for "maybe" */
 			r = 2;
+
+		/*
+		 * If opening the path with O_EXCL fails, the path
+		 * is in use (e.g. mounted during initramfs processing).
+		 * We know that it's not used by dm-multipath.
+		 * We may not set SYSTEMD_READY=0 on such devices, it
+		 * might cause systemd to umount the device.
+		 * Use O_RDONLY, because udevd would trigger another
+		 * uevent for close-after-write.
+		 *
+		 * get_refwwid() above stores the path we examine in slot 0.
+		 */
+		pp = VECTOR_SLOT(pathvec, 0);
+		fd = open(udev_device_get_devnode(pp->udev),
+			  O_RDONLY|O_EXCL);
+		if (fd >= 0)
+			close(fd);
+		else {
+			condlog(3, "%s: path %s is in use: %s",
+				__func__, pp->dev,
+				strerror(errno));
+			r = 1;
+		}
 		goto print_valid;
 	}
 
-- 
2.16.1

[PATCH v3 18/20] multipath -u: quick check if path is multipathed

[Martin Wilck]

With "find_multipaths smart", we accept paths as valid if they are
already part of a multipath map. This patch avoids doing a full path
and device-mapper map scan for this case, speeding up "multipath -u"
considerably.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/sysfs.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 libmultipath/sysfs.h |  2 ++
 multipath/main.c     | 14 ++++++++++-
 3 files changed, 81 insertions(+), 1 deletion(-)

diff --git a/libmultipath/sysfs.c b/libmultipath/sysfs.c
index 97e0997..589eeaf 100644
--- a/libmultipath/sysfs.c
+++ b/libmultipath/sysfs.c
@@ -27,6 +27,7 @@
 #include <string.h>
 #include <dirent.h>
 #include <libudev.h>
+#include <fnmatch.h>
 
 #include "checkers.h"
 #include "vector.h"
@@ -287,3 +288,68 @@ int sysfs_check_holders(char * check_devt, char * new_devt)
 
 	return 0;
 }
+
+static int select_dm_devs(const struct dirent *di)
+{
+	return fnmatch("dm-*", di->d_name, FNM_FILE_NAME) == 0;
+}
+
+static void close_fd(void *arg)
+{
+	close((long)arg);
+}
+
+bool sysfs_is_multipathed(const struct path *pp)
+{
+	char pathbuf[PATH_MAX];
+	struct dirent **di;
+	int n, r, i;
+	bool found = false;
+
+	n = snprintf(pathbuf, sizeof(pathbuf), "/sys/block/%s/holders",
+		     pp->dev);
+
+	if (n >= sizeof(pathbuf)) {
+		condlog(1, "%s: pathname overflow", __func__);
+		return false;
+	}
+
+	r = scandir(pathbuf, &di, select_dm_devs, alphasort);
+	if (r == 0)
+		return false;
+	else if (r < 0) {
+		condlog(1, "%s: error scanning %s", __func__, pathbuf);
+		return false;
+	}
+
+	pthread_cleanup_push(free, di);
+	for (i = 0; i < r && !found; i++) {
+		long fd;
+		int nr;
+		char uuid[6];
+
+		if (snprintf(pathbuf + n, sizeof(pathbuf) - n,
+			     "/%s/dm/uuid", di[i]->d_name)
+		    >= sizeof(pathbuf) - n)
+			continue;
+
+		fd = open(pathbuf, O_RDONLY);
+		if (fd == -1) {
+			condlog(1, "%s: error opening %s", __func__, pathbuf);
+			continue;
+		}
+
+		pthread_cleanup_push(close_fd, (void*)fd);
+		nr = read(fd, uuid, sizeof(uuid));
+		if (nr == sizeof(uuid) && !memcmp(uuid, "mpath-", sizeof(uuid)))
+			found = true;
+		else if (nr < 0) {
+			condlog(1, "%s: error reading from %s: %s",
+				__func__, pathbuf, strerror(errno));
+		}
+		pthread_cleanup_pop(1);
+	}
+	pthread_cleanup_pop(1);
+
+	return found;
+}
diff --git a/libmultipath/sysfs.h b/libmultipath/sysfs.h
index 75c0f9c..9ae30b3 100644
--- a/libmultipath/sysfs.h
+++ b/libmultipath/sysfs.h
@@ -4,6 +4,7 @@
 
 #ifndef _LIBMULTIPATH_SYSFS_H
 #define _LIBMULTIPATH_SYSFS_H
+#include <stdbool.h>
 
 ssize_t sysfs_attr_set_value(struct udev_device *dev, const char *attr_name,
 			     const char * value, size_t value_len);
@@ -13,4 +14,5 @@ ssize_t sysfs_bin_attr_get_value(struct udev_device *dev, const char *attr_name,
 				 unsigned char * value, size_t value_len);
 int sysfs_get_size (struct path *pp, unsigned long long * size);
 int sysfs_check_holders(char * check_devt, char * new_devt);
+bool sysfs_is_multipathed(const struct path *pp);
 #endif
diff --git a/multipath/main.c b/multipath/main.c
index 392d5f0..c21ee28 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -594,6 +594,15 @@ configure (struct config *conf, enum mpath_cmds cmd,
 			    !find_multipaths_on(conf) || !ignore_wwids(conf)) {
 				goto print_valid;
 			}
+			/*
+			 * Shortcut for find_multipaths smart:
+			 * Quick check if path is already multipathed.
+			 */
+			if (ignore_wwids(conf) &&
+			    sysfs_is_multipathed(VECTOR_SLOT(pathvec, 0))) {
+				r = 0;
+				goto print_valid;
+			}
 		}
 	}
 
@@ -666,7 +675,10 @@ configure (struct config *conf, enum mpath_cmds cmd,
 			condlog(3, "%s: path %s is in use: %s",
 				__func__, pp->dev,
 				strerror(errno));
-			r = 1;
+			/*
+			 * Check if we raced with multipathd
+			 */
+			r = !sysfs_is_multipathed(VECTOR_SLOT(pathvec, 0));
 		}
 		goto print_valid;
 	}
-- 
2.16.1

[PATCH v3 19/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 88e6687..2d28ded 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 not "0",
+				 * 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 || !strcmp(env, "0"))
+					continue;
+			} else if (!is_mpath &&
 				   (env == NULL || !strcmp(env, "0")))
 				continue;
 
diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index d59fdc8..1237c23 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 94c419a..641ba43 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 v3 20/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
existing map, only one path seen yet). In this case, "multipath -u" also sets the
variable FIND_MULTIPATHS_WAIT_UNIL to a relative time stamp (we need to use
relative "monotonic" time stamps because this may be triggered early during
boot, before system "calendar" time is correctly initialized).

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

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

diff --git a/multipath/multipath.rules b/multipath/multipath.rules
index aab64dc..77b9f16 100644
--- a/multipath/multipath.rules
+++ b/multipath/multipath.rules
@@ -19,9 +19,65 @@ LABEL="test_dev"
 ENV{MPATH_SBIN_PATH}="/sbin"
 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"
+# FIND_MULTIPATHS_WAIT_UNTIL is the timeout (in seconds after the
+# epoch).
+IMPORT{db}="FIND_MULTIPATHS_WAIT_UNTIL"
+ENV{.SAVED_FM_WAIT_UNTIL}="$env{FIND_MULTIPATHS_WAIT_UNTIL}"
+
+# multipath -u sets DM_MULTIPATH_DEVICE_PATH and,
+# if "find_multipaths smart", also FIND_MULTIPATHS_WAIT_UNTIL.
+ENV{DM_MULTIPATH_DEVICE_PATH}!="1", \
+	IMPORT{program}="$env{MPATH_SBIN_PATH}/multipath -u %k"
+
+# case 1: this is definitely multipath
+ENV{DM_MULTIPATH_DEVICE_PATH}=="1", \
+	ENV{ID_FS_TYPE}="mpath_member", ENV{SYSTEMD_READY}="0", \
+	GOTO="stop_wait"
+
+# case 2: this is definitely not multipath, or timeout has expired
+ENV{DM_MULTIPATH_DEVICE_PATH}!="2", \
+	GOTO="stop_wait"
+
+# Code below here is only run in "smart" mode.
+# multipath -u has indicated this is "maybe" multipath.
+
+# This shouldn't happen, just in case.
+ENV{FIND_MULTIPATHS_WAIT_UNTIL}!="?*", GOTO="end_mpath"
+
+# Be careful not to start the timer twice.
+ACTION!="add", GOTO="pretend_mpath"
+ENV{.SAVED_FM_WAIT_UNTIL}=="?*", GOTO="pretend_mpath"
+
+# At this point, we are seeing this path for the first time, and it's "maybe" multipath.
+
+# 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_WAIT_UNTIL} /usr/bin/udevadm trigger --action=add $sys$devpath"
+
+LABEL="pretend_mpath"
+ENV{DM_MULTIPATH_DEVICE_PATH}="1"
+ENV{SYSTEMD_READY}="0"
+GOTO="end_mpath"
+
+LABEL="stop_wait"
+# If timeout hasn't expired but we're not in "maybe" state any more, stop timer
+# Do this only once, and only if the timer has been started before
+IMPORT{db}="FIND_MULTIPATHS_WAIT_CANCELLED"
+ENV{FIND_MULTIPATHS_WAIT_CANCELLED}!="?*", \
+	ENV{FIND_MULTIPATHS_WAIT_UNTIL}=="?*", \
+	ENV{FIND_MULTIPATHS_WAIT_UNTIL}!="0", \
+	ENV{FIND_MULTIPATHS_WAIT_CANCELLED}="1", \
+	RUN+="/usr/bin/systemctl stop cancel-multipath-wait-$kernel.timer"
 
 LABEL="end_mpath"
-- 
2.16.1

Re: [PATCH v3 17/20] multipath -u: test if path is busy

[Benjamin Marzinski]

On Mon, Apr 02, 2018 at 09:50:48PM +0200, Martin Wilck wrote:
> For "find_multipaths smart", check if a path is already in use
> before setting DM_MULTIPATH_DEVICE_PATH to 1 or 2 (and thus,
> SYSTEMD_READY=0). If we don't do this, a device which has already been
> mounted (e.g. during initrd processing) may be unmounted by systemd, causing
> havoc to the boot process.

I'm reviewing  v3 of this patch because I don't see patch 17/20 in your
emails from v4. Am I missing an email, or did it not get sent?


> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipath/main.c | 31 ++++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/multipath/main.c b/multipath/main.c
> index d09f117..392d5f0 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -629,16 +629,45 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  
>  
>  	if (cmd == CMD_VALID_PATH) {
> +		struct path *pp;
> +		int fd;
> +
>  		/* This only happens if find_multipaths and
>  		 * ignore_wwids is set.
>  		 * If there is currently a multipath device matching
>  		 * the refwwid, or there is more than one path matching
>  		 * the refwwid, then the path is valid */
> -		if (VECTOR_SIZE(curmp) != 0 || VECTOR_SIZE(pathvec) > 1)
> +		if (VECTOR_SIZE(curmp) != 0) {
> +			r = 0;
> +			goto print_valid;
> +		} else if (VECTOR_SIZE(pathvec) > 1)
>  			r = 0;
>  		else
>  			/* Use r=2 as an indication for "maybe" */
>  			r = 2;
> +
> +		/*
> +		 * If opening the path with O_EXCL fails, the path
> +		 * is in use (e.g. mounted during initramfs processing).
> +		 * We know that it's not used by dm-multipath.
> +		 * We may not set SYSTEMD_READY=0 on such devices, it
> +		 * might cause systemd to umount the device.
> +		 * Use O_RDONLY, because udevd would trigger another
> +		 * uevent for close-after-write.
> +		 *
> +		 * get_refwwid() above stores the path we examine in slot 0.
> +		 */
> +		pp = VECTOR_SLOT(pathvec, 0);
> +		fd = open(udev_device_get_devnode(pp->udev),
> +			  O_RDONLY|O_EXCL);

I'm worried about this.  Since we can't be sure that is_failed_wwid()
will really tell us that multipathd has tried to multipath the device
and failed, it is totally possible to get a maybe after multipath has
turned the path device over to the rest of the system. If this is true,
then the exclusive open might race with something else that is trying to
use the device, and cause that to fail.  Or worse, it might win but have
the other process mount the file system on it, only to have multipath go
and claim the device, unmounting it. I still think that the only safe
course is to only do this grab when we know that it is safe, such as on
add events, or if we have already labelled this device as a maybe
device, and we are still waiting on it.

Of course, this means I would exlcude the whole second "if (cmd ==
CMD_VALID_PATH)" section in configure() unless we know that it is safe
to grab the device.  Otherwise, there is nothing to stop us from
claiming a device that is in use. Clearly that exclusive grab check is
racy at any time except on add events or when the device already is set
to SYSTEMD_READY=0.  I'm pretty sure that the coldplug add event after
the switchroot is safe, since nothing will be racing to grab the device
then. 

You've already agreed that it should be fine to allow multipathd to try
to create a multipath device on top of a non-claimed path, since we can
just claim it later by issuing a uevent.  I feel like this is just
another instance of that.  If this isn't a new path, where we have
excluded everyone else from using it, we can't suddenly claim it just
because a second path appears. However, if multipathd manages to create
a multipath device on top of it, then it will add the wwid to the wwids
file, and be able to claim it.  But otherwise, I don't think that the
exclusive grab is safe or reliable enough to allow us to simply do this
on any uevent.

I would add a new option to multipath, that works with -u, to tell it
that maybes are allowed. If find_multipaths == FIND_MULTIPATHS_SMART,
then it should not claim the device if it doesn't get positively claimed
in the first "if (cmd == CMD_VALID_PATH)" section of configure(). That
will save us from claiming devices that are already in use, and speed
the multipath -u calls up.
 

> +		if (fd >= 0)
> +			close(fd);
> +		else {
> +			condlog(3, "%s: path %s is in use: %s",
> +				__func__, pp->dev,
> +				strerror(errno));
> +			r = 1;
> +		}
>  		goto print_valid;
>  	}
>  
> -- 
> 2.16.1

Re: [PATCH v3 17/20] multipath -u: test if path is busy

[Martin Wilck]

On Thu, 2018-04-12 at 13:41 -0500, Benjamin Marzinski wrote:
> On Mon, Apr 02, 2018 at 09:50:48PM +0200, Martin Wilck wrote:
> > For "find_multipaths smart", check if a path is already in use
> > before setting DM_MULTIPATH_DEVICE_PATH to 1 or 2 (and thus,
> > SYSTEMD_READY=0). If we don't do this, a device which has already
> > been
> > mounted (e.g. during initrd processing) may be unmounted by
> > systemd, causing
> > havoc to the boot process.
> 
> I'm reviewing  v3 of this patch because I don't see patch 17/20 in
> your
> emails from v4. Am I missing an email, or did it not get sent?

It seems so, it didn't reach the dm-devel archive either. Strange.
I got it on my suse.com address, so maybe something went wrong in our
outgoing server. Anyway, v3/17 and v4/17 are identical.

> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> > +
> > +		/*
> > +		 * If opening the path with O_EXCL fails, the path
> > +		 * is in use (e.g. mounted during initramfs
> > processing).
> > +		 * We know that it's not used by dm-multipath.
> > +		 * We may not set SYSTEMD_READY=0 on such devices,
> > it
> > +		 * might cause systemd to umount the device.
> > +		 * Use O_RDONLY, because udevd would trigger
> > another
> > +		 * uevent for close-after-write.
> > +		 *
> > +		 * get_refwwid() above stores the path we examine
> > in slot 0.
> > +		 */
> > +		pp = VECTOR_SLOT(pathvec, 0);
> > +		fd = open(udev_device_get_devnode(pp->udev),
> > +			  O_RDONLY|O_EXCL);
> 
> I'm worried about this.  Since we can't be sure that is_failed_wwid()
> will really tell us that multipathd has tried to multipath the device
> and failed, 

As I said already, I don't understand why you say that.

I can assert that if is_failed_wwid() returns true, multipathd has
definitely tried and failed since the last reboot, and no (other
instance of) multipathd or multipath has succeeded since then.

If is_failed_wwid() returns false, it's possible that the map already
exists (see patch 18), or that previous/current instances of multipathd
simply didn't try -  we have to check by other means.

> it is totally possible to get a maybe after multipath has
> turned the path device over to the rest of the system.

A transition from "no" to "maybe" is only possible if a single path,
which isn't in the WWIDs file and isn't part of a multipath map,
transitions A) from "failed" to  "not failed" or B) from "blacklisted"
to "not blacklisted". A) means that multipathd has successfully created
a map, thus the path is now part of a map, and we will transition to
"yes" and not to "maybe". B) is pathogical except for the coldplug
case.

However, transitioning from "no" to "yes" in multipath -u is just as
bad as "no" to "maybe", unless the device has already been multipathed.
This is a common case: a second path appears for a once-released
device. I agree that we shouldn't try open(O_EXCL) in that situation.

> 
> Of course, this means I would exlcude the whole second "if (cmd ==
> CMD_VALID_PATH)" section in configure() unless we know that it is
> safe
> to grab the device.  Otherwise, there is nothing to stop us from
> claiming a device that is in use. Clearly that exclusive grab check
> is
> racy at any time except on add events or when the device already is
> set
> to SYSTEMD_READY=0.  I'm pretty sure that the coldplug add event
> after
> the switchroot is safe, since nothing will be racing to grab the
> device
> then. 
> 
> You've already agreed that it should be fine to allow multipathd to
> try
> to create a multipath device on top of a non-claimed path, since we
> can
> just claim it later by issuing a uevent.  I feel like this is just
> another instance of that.  If this isn't a new path, where we have
> excluded everyone else from using it, we can't suddenly claim it just
> because a second path appears. However, if multipathd manages to
> create
> a multipath device on top of it, then it will add the wwid to the
> wwids
> file, and be able to claim it.  But otherwise, I don't think that the
> exclusive grab is safe or reliable enough to allow us to simply do
> this
> on any uevent.
> 
> I would add a new option to multipath, that works with -u, to tell it
> that maybes are allowed. If find_multipaths == FIND_MULTIPATHS_SMART,
> then it should not claim the device if it doesn't get positively
> claimed
> in the first "if (cmd == CMD_VALID_PATH)" section of configure().
> That
> will save us from claiming devices that are already in use, and speed
> the multipath -u calls up.

I don't think we need another option. We can use the uevent environment
in the -u case.

Regards,
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 v3 17/20] multipath -u: test if path is busy

[Benjamin Marzinski]

On Fri, Apr 13, 2018 at 12:17:54AM +0200, Martin Wilck wrote:
> On Thu, 2018-04-12 at 13:41 -0500, Benjamin Marzinski wrote:
> > On Mon, Apr 02, 2018 at 09:50:48PM +0200, Martin Wilck wrote:
> > > For "find_multipaths smart", check if a path is already in use
> > > before setting DM_MULTIPATH_DEVICE_PATH to 1 or 2 (and thus,
> > > SYSTEMD_READY=0). If we don't do this, a device which has already
> > > been
> > > mounted (e.g. during initrd processing) may be unmounted by
> > > systemd, causing
> > > havoc to the boot process.
> > 
> > I'm reviewing  v3 of this patch because I don't see patch 17/20 in
> > your
> > emails from v4. Am I missing an email, or did it not get sent?
> 
> It seems so, it didn't reach the dm-devel archive either. Strange.
> I got it on my suse.com address, so maybe something went wrong in our
> outgoing server. Anyway, v3/17 and v4/17 are identical.
> 
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > > +
> > > +		/*
> > > +		 * If opening the path with O_EXCL fails, the path
> > > +		 * is in use (e.g. mounted during initramfs
> > > processing).
> > > +		 * We know that it's not used by dm-multipath.
> > > +		 * We may not set SYSTEMD_READY=0 on such devices,
> > > it
> > > +		 * might cause systemd to umount the device.
> > > +		 * Use O_RDONLY, because udevd would trigger
> > > another
> > > +		 * uevent for close-after-write.
> > > +		 *
> > > +		 * get_refwwid() above stores the path we examine
> > > in slot 0.
> > > +		 */
> > > +		pp = VECTOR_SLOT(pathvec, 0);
> > > +		fd = open(udev_device_get_devnode(pp->udev),
> > > +			  O_RDONLY|O_EXCL);
> > 
> > I'm worried about this.  Since we can't be sure that is_failed_wwid()
> > will really tell us that multipathd has tried to multipath the device
> > and failed, 
> 
> As I said already, I don't understand why you say that.
> 
> I can assert that if is_failed_wwid() returns true, multipathd has
> definitely tried and failed since the last reboot, and no (other
> instance of) multipathd or multipath has succeeded since then.
> 
> If is_failed_wwid() returns false, it's possible that the map already
> exists (see patch 18), or that previous/current instances of multipathd
> simply didn't try -  we have to check by other means.

I probably shouldn't have brought up is_failed_wwid() up here at all.
It has really nothing to do with my main point.

But just to rehash this again, you do agree that multipathd can get a
uevent for for a path device, recognize that it should create a
multipath device on it, and then fail somewhere in ev_add_path before it
get around to calling domap, right? If this happens, multipathd won't
automatically try to create that device again until either it gets
another add event for a path in that device, or it is reconfigured.  In
this case the is_failed_wwid() result would make it seem like multipathd
might still be waiting to create this device, when in truth, that won't
happen.

But I already agreed that that code is fine without giving you that kind
of guarantee, so there's no point in bring it up here. Lets just ignore
that.
 
> > it is totally possible to get a maybe after multipath has
> > turned the path device over to the rest of the system.
> 
> A transition from "no" to "maybe" is only possible if a single path,
> which isn't in the WWIDs file and isn't part of a multipath map,
> transitions A) from "failed" to  "not failed" or B) from "blacklisted"
> to "not blacklisted". A) means that multipathd has successfully created
> a map, thus the path is now part of a map, and we will transition to
> "yes" and not to "maybe". B) is pathogical except for the coldplug
> case.

I agree that udev transitioning a device from "no" to "maybe" isn't
something that needs worrying about.  I do think it's valid to worry
about a device that previously was classified as "maybe", then timed out
to "no", and is now getting a new uevent, and having an exlusive open
run on it because at this point in configure(), it is classified as
"maybe". Obviously it has already timed out, so it will eventually be
classified as "no". But the exclusive open is still dangerous. Since you
agree that it is dangerous in the "no" to "yes" case, I assume you agree
it's dangerous here as well. This is why I said that the whole second
(cmd == CMD_VALID_PATH) section, where we check for devices that aren't
obviously multipath paths and haven't been multipathed yet, shouldn't
happen after we have classified a device as "no". 

> However, transitioning from "no" to "yes" in multipath -u is just as
> bad as "no" to "maybe", unless the device has already been multipathed.
> This is a common case: a second path appears for a once-released
> device. I agree that we shouldn't try open(O_EXCL) in that situation.
> 
> > 
> > Of course, this means I would exlcude the whole second "if (cmd ==
> > CMD_VALID_PATH)" section in configure() unless we know that it is
> > safe
> > to grab the device.  Otherwise, there is nothing to stop us from
> > claiming a device that is in use. Clearly that exclusive grab check
> > is
> > racy at any time except on add events or when the device already is
> > set
> > to SYSTEMD_READY=0.  I'm pretty sure that the coldplug add event
> > after
> > the switchroot is safe, since nothing will be racing to grab the
> > device
> > then. 
> > 
> > You've already agreed that it should be fine to allow multipathd to
> > try
> > to create a multipath device on top of a non-claimed path, since we
> > can
> > just claim it later by issuing a uevent.  I feel like this is just
> > another instance of that.  If this isn't a new path, where we have
> > excluded everyone else from using it, we can't suddenly claim it just
> > because a second path appears. However, if multipathd manages to
> > create
> > a multipath device on top of it, then it will add the wwid to the
> > wwids
> > file, and be able to claim it.  But otherwise, I don't think that the
> > exclusive grab is safe or reliable enough to allow us to simply do
> > this
> > on any uevent.
> > 
> > I would add a new option to multipath, that works with -u, to tell it
> > that maybes are allowed. If find_multipaths == FIND_MULTIPATHS_SMART,
> > then it should not claim the device if it doesn't get positively
> > claimed
> > in the first "if (cmd == CMD_VALID_PATH)" section of configure().
> > That
> > will save us from claiming devices that are already in use, and speed
> > the multipath -u calls up.
> 
> I don't think we need another option. We can use the uevent environment
> in the -u case.

Sure.
 
> Regards,
> 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 v3 17/20] multipath -u: test if path is busy

[Martin Wilck]

On Fri, 2018-04-13 at 10:53 -0500, Benjamin Marzinski wrote:
> On Fri, Apr 13, 2018 at 12:17:54AM +0200, Martin Wilck wrote:
> > 
> > As I said already, I don't understand why you say that.
> > 
> > I can assert that if is_failed_wwid() returns true, multipathd has
> > definitely tried and failed since the last reboot, and no (other
> > instance of) multipathd or multipath has succeeded since then.
> > 
> > If is_failed_wwid() returns false, it's possible that the map
> > already
> > exists (see patch 18), or that previous/current instances of
> > multipathd
> > simply didn't try -  we have to check by other means.
> 
> I probably shouldn't have brought up is_failed_wwid() up here at all.
> It has really nothing to do with my main point.
> 
> But just to rehash this again, you do agree that multipathd can get a
> uevent for for a path device, recognize that it should create a
> multipath device on it, and then fail somewhere in ev_add_path before
> it
> get around to calling domap, right? If this happens, multipathd won't
> automatically try to create that device again until either it gets
> another add event for a path in that device, or it is
> reconfigured.  In
> this case the is_failed_wwid() result would make it seem like
> multipathd
> might still be waiting to create this device, when in truth, that
> won't
> happen.

I agree. (is_failed_wwid(refwwid) != WWID_IS_FAILED) doesn't really
mean a lot, it happens in many different situations. Luckily we test
only for (is_failed_wwid(refwwid) == WWID_IS_FAILED), which is well-
defined, and make further checks otherwise.

Regards
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