[PATCH v4 00/20] multipath path classification

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

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 v3->v4:

 - Rebased onto latest upstream, commit 1cb704b
 - 07/20: changed macro names to ignore_wwids_on, ignore_new_devs_on to
   avoid ambiguity with variable names

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           | 247 +++++++++++++++++++++++++++++++++++++++------
 multipath/multipath.8      |   9 +-
 multipath/multipath.conf.5 |  75 ++++++++++----
 multipath/multipath.rules  |  62 +++++++++++-
 multipathd/main.c          |  15 +--
 multipathd/multipathd.8    |   8 +-
 21 files changed, 747 insertions(+), 89 deletions(-)

-- 
2.16.1

[PATCH v4 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 d08c688..d26c744 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 v4 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 9a4f671..08878b1 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2404,10 +2404,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 v4 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 61f68f8..6df8359 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -987,7 +987,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 0ec9f25..b7392bf 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
@@ -274,7 +275,7 @@ out:
 }
 
 int
-should_multipath(struct path *pp1, vector pathvec)
+should_multipath(struct path *pp1, vector pathvec, vector mpvec)
 {
 	int i, ignore_new_devs, find_multipaths;
 	struct path *pp2;
@@ -289,6 +290,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 08878b1..7d3d5b1 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -941,7 +941,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 v4 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 d26c744..ba83771 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 v4 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 6df8359..088d69f 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -444,6 +444,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)
 {
@@ -842,8 +864,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 b7392bf..11afe09 100644
--- a/libmultipath/wwids.c
+++ b/libmultipath/wwids.c
@@ -331,5 +331,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 ba83771..686b037 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 7d3d5b1..644de72 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2324,7 +2324,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 v4 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 088d69f..c1a50e4 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -458,8 +458,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 v4 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           |  9 +++++----
 multipath/multipath.8      |  9 ++++++++-
 multipath/multipath.conf.5 | 46 +++++++++++++++++++++++++--------------------
 multipathd/main.c          |  6 +-----
 multipathd/multipathd.8    |  8 +++++---
 10 files changed, 123 insertions(+), 40 deletions(-)

diff --git a/libmultipath/config.h b/libmultipath/config.h
index 329f3ed..c71d972 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 ac9216c..e695fdc 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -234,8 +234,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..c43f2c3 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_on(conf) \
+	(!!((conf)->find_multipaths & _FIND_MULTIPATHS_I))
+#define ignore_new_devs_on(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 11afe09..da685be 100644
--- a/libmultipath/wwids.c
+++ b/libmultipath/wwids.c
@@ -282,8 +282,12 @@ should_multipath(struct path *pp1, vector pathvec, vector mpvec)
 	struct config *conf;
 
 	conf = get_multipath_config();
-	ignore_new_devs = conf->ignore_new_devs;
-	find_multipaths = conf->find_multipaths;
+	ignore_new_devs = ignore_new_devs_on(conf);
+	find_multipaths = find_multipaths_on(conf);
+	if (!find_multipaths && !ignore_new_devs) {
+		put_multipath_config(conf);
+		return 1;
+	}
 	put_multipath_config(conf);
 	if (find_multipaths && !ignore_new_devs)
 		return 1;
diff --git a/multipath/main.c b/multipath/main.c
index 686b037..2be9b9c 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -441,11 +441,12 @@ configure (struct config *conf, enum mpath_cmds cmd,
 		 * Paths listed in the wwids file are always considered valid.
 		 */
 		if (cmd == CMD_VALID_PATH) {
-			if ((!conf->find_multipaths && conf->ignore_wwids) ||
-			    check_wwids_file(refwwid, 0) == 0)
+			if ((!find_multipaths_on(conf) && ignore_wwids_on(conf))
+			    || check_wwids_file(refwwid, 0) == 0)
 				r = 0;
 			if (r == 0 ||
-			    !conf->find_multipaths || !conf->ignore_wwids) {
+			    !find_multipaths_on(conf) ||
+			    !ignore_wwids_on(conf)) {
 				printf("%s %s a valid multipath device path\n",
 				       devpath, r == 0 ? "is" : "is not");
 				goto out;
@@ -737,7 +738,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 644de72..e6f4e77 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2405,8 +2405,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);
@@ -2636,8 +2634,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)) {
@@ -2992,7 +2988,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 v4 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 v4 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 da685be..c9f5743 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"
@@ -337,3 +338,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 v4 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 c1a50e4..9aa3d21 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -445,11 +445,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;
@@ -468,14 +475,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
@@ -876,8 +890,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);
@@ -902,7 +918,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 2a92105..f2befad 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>
@@ -415,8 +416,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.
@@ -432,6 +437,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 c43f2c3..1d3a34f 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 e6f4e77..7eeb743 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2325,7 +2325,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 v4 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 2be9b9c..bf28735 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;
@@ -447,9 +454,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
 			if (r == 0 ||
 			    !find_multipaths_on(conf) ||
 			    !ignore_wwids_on(conf)) {
-				printf("%s %s a valid multipath device path\n",
-				       devpath, r == 0 ? "is" : "is not");
-				goto out;
+				goto print_valid;
 			}
 		}
 	}
@@ -493,9 +498,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) {
@@ -509,6 +512,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);
@@ -844,8 +851,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 v4 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 bf28735..59a72ed 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;
 }
 
@@ -514,7 +516,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)
@@ -851,7 +853,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 v4 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 59a72ed..942caf4 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_on(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_on(conf)) ||
+				   check_wwids_file(refwwid, 0) == 0)
 				r = 0;
 			if (r == 0 ||
 			    !find_multipaths_on(conf) ||
-- 
2.16.1

[PATCH v4 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 942caf4..f62e18a 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;
@@ -504,6 +504,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 v4 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 c71d972..6e69a37 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 e695fdc..4be808d 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -485,6 +485,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)
 {
@@ -1519,6 +1523,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 93974a4..70fb997 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -912,3 +912,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 1d3a34f..eb6a178 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 v4 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 f62e18a..126f90f 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 v4 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 a60a5f3..ae46e5a 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -595,6 +595,15 @@ configure (struct config *conf, enum mpath_cmds cmd,
 			    !ignore_wwids_on(conf)) {
 				goto print_valid;
 			}
+			/*
+			 * Shortcut for find_multipaths smart:
+			 * Quick check if path is already multipathed.
+			 */
+			if (ignore_wwids_on(conf) &&
+			    sysfs_is_multipathed(VECTOR_SLOT(pathvec, 0))) {
+				r = 0;
+				goto print_valid;
+			}
 		}
 	}
 
@@ -667,7 +676,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 v4 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 9aa3d21..17c2fa1 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"
@@ -476,9 +477,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 4be808d..8a18b1f 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -239,6 +239,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 v4 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 v4 07/20] libmultipath: change find_multipaths option to multi-value

[Benjamin Marzinski]

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

This patch adds trailing whitespace. Otherwise, it's still fine.

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

> 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           |  9 +++++----
>  multipath/multipath.8      |  9 ++++++++-
>  multipath/multipath.conf.5 | 46 +++++++++++++++++++++++++--------------------
>  multipathd/main.c          |  6 +-----
>  multipathd/multipathd.8    |  8 +++++---
>  10 files changed, 123 insertions(+), 40 deletions(-)
> 
> diff --git a/libmultipath/config.h b/libmultipath/config.h
> index 329f3ed..c71d972 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 ac9216c..e695fdc 100644
> --- a/libmultipath/dict.c
> +++ b/libmultipath/dict.c
> @@ -234,8 +234,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..c43f2c3 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_on(conf) \
> +	(!!((conf)->find_multipaths & _FIND_MULTIPATHS_I))
> +#define ignore_new_devs_on(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 11afe09..da685be 100644
> --- a/libmultipath/wwids.c
> +++ b/libmultipath/wwids.c
> @@ -282,8 +282,12 @@ should_multipath(struct path *pp1, vector pathvec, vector mpvec)
>  	struct config *conf;
>  
>  	conf = get_multipath_config();
> -	ignore_new_devs = conf->ignore_new_devs;
> -	find_multipaths = conf->find_multipaths;
> +	ignore_new_devs = ignore_new_devs_on(conf);
> +	find_multipaths = find_multipaths_on(conf);
> +	if (!find_multipaths && !ignore_new_devs) {
> +		put_multipath_config(conf);
> +		return 1;
> +	}
>  	put_multipath_config(conf);
>  	if (find_multipaths && !ignore_new_devs)
>  		return 1;
> diff --git a/multipath/main.c b/multipath/main.c
> index 686b037..2be9b9c 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -441,11 +441,12 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  		 * Paths listed in the wwids file are always considered valid.
>  		 */
>  		if (cmd == CMD_VALID_PATH) {
> -			if ((!conf->find_multipaths && conf->ignore_wwids) ||
> -			    check_wwids_file(refwwid, 0) == 0)
> +			if ((!find_multipaths_on(conf) && ignore_wwids_on(conf))
> +			    || check_wwids_file(refwwid, 0) == 0)
>  				r = 0;
>  			if (r == 0 ||
> -			    !conf->find_multipaths || !conf->ignore_wwids) {
> +			    !find_multipaths_on(conf) ||
> +			    !ignore_wwids_on(conf)) {
>  				printf("%s %s a valid multipath device path\n",
>  				       devpath, r == 0 ? "is" : "is not");
>  				goto out;
> @@ -737,7 +738,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 644de72..e6f4e77 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2405,8 +2405,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);
> @@ -2636,8 +2634,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)) {
> @@ -2992,7 +2988,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

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

[Benjamin Marzinski]

On Wed, Apr 04, 2018 at 06:16:16PM +0200, Martin Wilck wrote:
> 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.

I'm a little confused about the necessity of a lock file here.  What it
the race that you are worried about? If two processes try to create a
file at the same time, surely one of them will succeed. If two processes
try to delete a file at the same time, it will get deleted.  If one
process is trying to create a file and one is trying to remove it, the
outcome depends on the who wins the race. But this is true whether you
add a lock file to make those actions atomic or not. The same goes with
stating a file that's being created or removed. As far a I can tell,
this should work if you simply create an empty file without any locking.
Are you worried about some odd errno due to a race?

The enums do make things less confusing, though.

-Ben

> 
> 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 da685be..c9f5743 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"
> @@ -337,3 +338,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

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

[Benjamin Marzinski]

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

By only looking at domap, we will miss instances of multipathd failing
to create maps earlier in the process. This isn't necessarily wrong. It
just means that we can't rely on checking
/dev/shm/multipath/failed_wwids to definitively tell us whether
multipathd has tried and failed to create the device.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> 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 c1a50e4..9aa3d21 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -445,11 +445,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;
> @@ -468,14 +475,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
> @@ -876,8 +890,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);
> @@ -902,7 +918,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 2a92105..f2befad 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>
> @@ -415,8 +416,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.
> @@ -432,6 +437,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 c43f2c3..1d3a34f 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 e6f4e77..7eeb743 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2325,7 +2325,7 @@ configure (struct vectors * vecs)
>  	sync_maps_state(mpvec);
>  	vector_foreach_slot(mpvec, mpp, i){
>  		if (remember_wwid(mpp->wwid) == 1)
> -			trigger_paths_udev_change(mpp);
> +			trigger_paths_udev_change(mpp, true);
>  		update_map_pr(mpp);
>  	}
>  
> -- 
> 2.16.1

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

[Benjamin Marzinski]

On Wed, Apr 04, 2018 at 06:16:18PM +0200, Martin Wilck wrote:
> Print the result message in one place only. This simplifies
> future changes. multipath -c is also affected.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> 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 2be9b9c..bf28735 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;
> @@ -447,9 +454,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  			if (r == 0 ||
>  			    !find_multipaths_on(conf) ||
>  			    !ignore_wwids_on(conf)) {
> -				printf("%s %s a valid multipath device path\n",
> -				       devpath, r == 0 ? "is" : "is not");
> -				goto out;
> +				goto print_valid;
>  			}
>  		}
>  	}
> @@ -493,9 +498,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) {
> @@ -509,6 +512,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);
> @@ -844,8 +851,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

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

[Benjamin Marzinski]

On Wed, Apr 04, 2018 at 06:16:19PM +0200, Martin Wilck wrote:
> ... instead of free format. This provides more flexibility
> for udev rule processing for the future. Adapt code in multipath.rules.
> The exit status remains as usual. This affects "multipath -c", too.
> 
> The parameters "pathvec" and "conf" for print_cmd_valid are currently
> unused, but will be in follow-up patches.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> 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 bf28735..59a72ed 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;
>  }
>  
> @@ -514,7 +516,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)
> @@ -851,7 +853,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

Re: [PATCH v4 13/20] multipath -u: treat failed wwids as invalid

[Benjamin Marzinski]

On Wed, Apr 04, 2018 at 06:16:20PM +0200, Martin Wilck wrote:
> 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.
> 

Like I said, this isn't a definitive check for multipathd failure, but
it will help, so

Reviewed-by: Benjamin Marzinski <benmarz@gmail.com>
> 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 59a72ed..942caf4 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_on(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_on(conf)) ||
> +				   check_wwids_file(refwwid, 0) == 0)
>  				r = 0;
>  			if (r == 0 ||
>  			    !find_multipaths_on(conf) ||
> -- 
> 2.16.1

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

[Benjamin Marzinski]

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

This patch adds trailing whitespace, but otherwise,

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/config.h      |  1 +
>  libmultipath/defaults.h    |  2 ++
>  libmultipath/dict.c        |  6 ++++++
>  libmultipath/propsel.c     | 25 +++++++++++++++++++++++++
>  libmultipath/propsel.h     |  1 +
>  libmultipath/structs.h     |  1 +
>  multipath/multipath.conf.5 | 18 ++++++++++++++++++
>  7 files changed, 54 insertions(+)
> 
> diff --git a/libmultipath/config.h b/libmultipath/config.h
> index c71d972..6e69a37 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 e695fdc..4be808d 100644
> --- a/libmultipath/dict.c
> +++ b/libmultipath/dict.c
> @@ -485,6 +485,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)
>  {
> @@ -1519,6 +1523,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 93974a4..70fb997 100644
> --- a/libmultipath/propsel.c
> +++ b/libmultipath/propsel.c
> @@ -912,3 +912,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 1d3a34f..eb6a178 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

Re: [PATCH v4 16/20] multipath -u : set FIND_MULTIPATHS_WAIT_UNTIL from /dev/shm

[Benjamin Marzinski]

On Wed, Apr 04, 2018 at 06:16:23PM +0200, Martin Wilck wrote:
> 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 f62e18a..126f90f 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);
> +

I'm worried about using pp->dev_t here with no method of removing these
files.  What happens when a path device, say 8:32 is removed and a
completely new device comes in reusing the same dev_t?

> +	if (snprintf(path, sizeof(path), "%s/%s", shm_find_mp_dir, pp->dev_t)
> +	    >= sizeof(path)) {
> +		condlog(1, "%s: path name overflow", __func__);

shouldn't this be:
		return FIND_MULTIPATHS_ERROR;

> +		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");

If we get an error trying to check the timeout, should we just keep
FIND_MULTIPATHS_WAIT_UNTIL the same? Or would it be better to set it to
0, and fail the smart claiming?

>  	printf("DM_MULTIPATH_DEVICE_PATH=\"%d\"\n", vals[k]);
>  	return k == 1;
>  }
> -- 
> 2.16.1

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

[Benjamin Marzinski]

On Wed, Apr 04, 2018 at 06:16:25PM +0200, Martin Wilck wrote:
> 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.

I feel like this supports my idea from 17/20. I really think that in
smart mode, the only time we should be claiming a device as multipath is
on an add event, if it has always been claimed (or pretend claimed) as a
multipath path, or if it is currently a multipath path. Once we have let
a uevent go by for a path without setting SYSTEMD_READY=0, anything else
is free to use it, and we simply can't safely set SYSTEMD_READY=0,
unless we know that multipathd has already grabbed the device. I feel
like checking the wwids file should give you confidence that a device
either currently is a multipath path, or has always been claimed as one.
However, this patch can fix any loop holes where a device isn't in the
wwids file, but it multipathed (I don't know of any offhand).

-Ben


> 
> 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 a60a5f3..ae46e5a 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -595,6 +595,15 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  			    !ignore_wwids_on(conf)) {
>  				goto print_valid;
>  			}
> +			/*
> +			 * Shortcut for find_multipaths smart:
> +			 * Quick check if path is already multipathed.
> +			 */
> +			if (ignore_wwids_on(conf) &&
> +			    sysfs_is_multipathed(VECTOR_SLOT(pathvec, 0))) {
> +				r = 0;
> +				goto print_valid;
> +			}
>  		}
>  	}
>  
> @@ -667,7 +676,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

Re: [PATCH v4 19/20] libmultipath: enable find_multipaths "smart"

[Benjamin Marzinski]

On Wed, Apr 04, 2018 at 06:16:26PM +0200, Martin Wilck wrote:
> This activates "smart" path detection. This is similar to
> "find_multipaths yes", but doesn't generally ignore single paths
> that are not listed in the WWIDs file. Rather, such paths are
> temporarily treated like multipath members. If no additional paths
> are detected after a certain time, the paths are re-added to the
> system as non-multipath. This needs support by the udev rules, to
> be added in a follow-up patch.
> 
> If a multipath map is successfully successfully created, and paths are
> in waiting state, trigger path uevents to update their status.
> 
> 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 9aa3d21..17c2fa1 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"
> @@ -476,9 +477,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 DM_MULTIPATH_DEVICE_PATH=1 then there has already been a uevent where
udev recognized that the device should be claimed.  Wouldn't udev have
stopped the timer then?  I don't see why this is necessary.

> +			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 4be808d..8a18b1f 100644
> --- a/libmultipath/dict.c
> +++ b/libmultipath/dict.c
> @@ -239,6 +239,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

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

[Benjamin Marzinski]

On Wed, Apr 04, 2018 at 06:16:27PM +0200, Martin Wilck wrote:
> When the first path to a device appears, we don't know if more paths are going
> to follow. find_multipath "smart" logic attempts to solve this dilemma by
> waiting for additional paths for a configurable time before giving up
> and releasing single paths to upper layers.
> 
> These rules apply only if both find_multipaths is set to "smart" in
> multipath.conf. In this mode, multipath -u sets DM_MULTIPATH_DEVICE_PATH=2 if
> there's no clear evidence wheteher a given device should be a multipath member
> (not blacklisted, not listed as "failed", not in WWIDs file, not member of an
> 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.
> 


Like I've mentioned, I think multipath -u should be called with a
special option on add events or if FIND_MULTIPATHS_WAIT_UNTIL exists and
is non-zero (or perhaps we should IMPORT DM_MULTIPATH_DEVICE_PATH, and
check if it's 2 or import SYSTEMD_READY and check if it's 0), that smart
mode could use to allow maybes and new claims on existing paths because
another path appeared.

> 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 v4 09/20] libmultipath: functions to indicate mapping failure in /dev/shm

[Martin Wilck]

On Thu, 2018-04-12 at 13:33 -0500, Benjamin Marzinski wrote:
> On Wed, Apr 04, 2018 at 06:16:16PM +0200, Martin Wilck wrote:
> > 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.
> 
> I'm a little confused about the necessity of a lock file here.  What
> it
> the race that you are worried about? If two processes try to create a
> file at the same time, surely one of them will succeed. If two
> processes
> try to delete a file at the same time, it will get deleted.  If one
> process is trying to create a file and one is trying to remove it,
> the
> outcome depends on the who wins the race. But this is true whether
> you
> add a lock file to make those actions atomic or not. The same goes
> with
> stating a file that's being created or removed. As far a I can tell,
> this should work if you simply create an empty file without any
> locking.
> Are you worried about some odd errno due to a race?

My thinking was that it's generally a good thing to make file system
operations like this atomic, and the well-tested and mature open_file()
API was there ready to be used, thus I did.

You're probably right that the locking not strictly necessary. I don't
think it hurts, performance-wise the impact is quite low.
Do you require me to change this, or can we change it later?

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 v4 10/20] libmultipath: indicate wwid failure in dm_addmap_create()

[Martin Wilck]

On Thu, 2018-04-12 at 13:33 -0500, Benjamin Marzinski wrote:
> On Wed, Apr 04, 2018 at 06:16:17PM +0200, Martin Wilck wrote:
> > dm_addmap_create() is where we actually try to set up a new
> > multipath map. Depending on the result, mark the wwid as
> > failed (or not), and re-trigger an uevent if necessary.
> > If a path changes from multipath to non-multipath, use an "add"
> > event to make sure LVM2 rules pick it up. Increase log level
> > of this event to 3.
> > 
> 
> By only looking at domap, we will miss instances of multipathd
> failing
> to create maps earlier in the process. This isn't necessarily wrong.
> It
> just means that we can't rely on checking
> /dev/shm/multipath/failed_wwids to definitively tell us whether
> multipathd has tried and failed to create the device.

Sorry, I can't follow you. Where else except in the 
domap->dm_addmap_create->dm_addmap() code path do we create maps?
I'm feeling stupid, I really can't see it.

Martin

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

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

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

[Martin Wilck]

On Thu, 2018-04-12 at 22:16 +0200, Martin Wilck wrote:
> On Thu, 2018-04-12 at 13:33 -0500, Benjamin Marzinski wrote:
> > On Wed, Apr 04, 2018 at 06:16:17PM +0200, Martin Wilck wrote:
> > > dm_addmap_create() is where we actually try to set up a new
> > > multipath map. Depending on the result, mark the wwid as
> > > failed (or not), and re-trigger an uevent if necessary.
> > > If a path changes from multipath to non-multipath, use an "add"
> > > event to make sure LVM2 rules pick it up. Increase log level
> > > of this event to 3.
> > > 
> > 
> > By only looking at domap, we will miss instances of multipathd
> > failing
> > to create maps earlier in the process. This isn't necessarily
> > wrong.
> > It
> > just means that we can't rely on checking
> > /dev/shm/multipath/failed_wwids to definitively tell us whether
> > multipathd has tried and failed to create the device.
> 
> Sorry, I can't follow you. Where else except in the 
> domap->dm_addmap_create->dm_addmap() code path do we create maps?
> I'm feeling stupid, I really can't see it.

If you were referring to other instances of multipathd which have
already terminated (e.g. multipathd which ran in initramfs): these
leave the failed markers under /dev/shm when they quit. That's the
whole point of this patch. A failed marker will only be removed if a)
the system is rebooted, or b) another attempt to create the map
succeeds, or c) a user removes the marker manually.

But I suppose I'm still missing your point.

Cheers,
Martin

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

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

Re: [PATCH v4 16/20] multipath -u : set FIND_MULTIPATHS_WAIT_UNTIL from /dev/shm

[Martin Wilck]

On Thu, 2018-04-12 at 13:36 -0500, Benjamin Marzinski wrote:
> On Wed, Apr 04, 2018 at 06:16:23PM +0200, Martin Wilck wrote:
> > 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(-)
> > 
> > +
> > +/**
> > + * 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);
> > +
> 
> I'm worried about using pp->dev_t here with no method of removing
> these
> files.  What happens when a path device, say 8:32 is removed and a
> completely new device comes in reusing the same dev_t?

Hm, what else should we use? devnode name is even worse, and most other
options are a can of worms...  In the worst case, the new device
wouldn't be waited for (or not long enough), because the marker existed
before it was detected.

I could simply add a rule that removes the marker in case of a "remove"
uevent, ok?

> 
> > +	if (snprintf(path, sizeof(path), "%s/%s", shm_find_mp_dir,
> > pp->dev_t)
> > +	    >= sizeof(path)) {
> > +		condlog(1, "%s: path name overflow", __func__);
> 
> shouldn't this be:
> 		return FIND_MULTIPATHS_ERROR;

Bah, thank for catching it.

> >  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");
> 
> If we get an error trying to check the timeout, should we just keep
> FIND_MULTIPATHS_WAIT_UNTIL the same? Or would it be better to set it
> to
> 0, and fail the smart claiming?

The latter is what we do. We set k=1 if find_multipaths_check_timeout()
returns anything but FIND_MULTIPATHS_WAITING, resulting in
DM_MULTIPATH_DEVICE_PATH="0".

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 v4 09/20] libmultipath: functions to indicate mapping failure in /dev/shm

[Benjamin Marzinski]

On Thu, Apr 12, 2018 at 10:07:13PM +0200, Martin Wilck wrote:
> On Thu, 2018-04-12 at 13:33 -0500, Benjamin Marzinski wrote:
> > On Wed, Apr 04, 2018 at 06:16:16PM +0200, Martin Wilck wrote:
> > > 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.
> > 
> > I'm a little confused about the necessity of a lock file here.  What
> > it
> > the race that you are worried about? If two processes try to create a
> > file at the same time, surely one of them will succeed. If two
> > processes
> > try to delete a file at the same time, it will get deleted.  If one
> > process is trying to create a file and one is trying to remove it,
> > the
> > outcome depends on the who wins the race. But this is true whether
> > you
> > add a lock file to make those actions atomic or not. The same goes
> > with
> > stating a file that's being created or removed. As far a I can tell,
> > this should work if you simply create an empty file without any
> > locking.
> > Are you worried about some odd errno due to a race?
> 
> My thinking was that it's generally a good thing to make file system
> operations like this atomic, and the well-tested and mature open_file()
> API was there ready to be used, thus I did.
> 
> You're probably right that the locking not strictly necessary. I don't
> think it hurts, performance-wise the impact is quite low.
> Do you require me to change this, or can we change it later?

That's fine. I don't see how it can hurt.

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

> 
> 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 v4 10/20] libmultipath: indicate wwid failure in dm_addmap_create()

[Benjamin Marzinski]

On Thu, Apr 12, 2018 at 10:22:19PM +0200, Martin Wilck wrote:
> On Thu, 2018-04-12 at 22:16 +0200, Martin Wilck wrote:
> > On Thu, 2018-04-12 at 13:33 -0500, Benjamin Marzinski wrote:
> > > On Wed, Apr 04, 2018 at 06:16:17PM +0200, Martin Wilck wrote:
> > > > dm_addmap_create() is where we actually try to set up a new
> > > > multipath map. Depending on the result, mark the wwid as
> > > > failed (or not), and re-trigger an uevent if necessary.
> > > > If a path changes from multipath to non-multipath, use an "add"
> > > > event to make sure LVM2 rules pick it up. Increase log level
> > > > of this event to 3.
> > > > 
> > > 
> > > By only looking at domap, we will miss instances of multipathd
> > > failing
> > > to create maps earlier in the process. This isn't necessarily
> > > wrong.
> > > It
> > > just means that we can't rely on checking
> > > /dev/shm/multipath/failed_wwids to definitively tell us whether
> > > multipathd has tried and failed to create the device.
> > 
> > Sorry, I can't follow you. Where else except in the 
> > domap->dm_addmap_create->dm_addmap() code path do we create maps?
> > I'm feeling stupid, I really can't see it.
> 
> If you were referring to other instances of multipathd which have
> already terminated (e.g. multipathd which ran in initramfs): these
> leave the failed markers under /dev/shm when they quit. That's the
> whole point of this patch. A failed marker will only be removed if a)
> the system is rebooted, or b) another attempt to create the map
> succeeds, or c) a user removes the marker manually.
> 
> But I suppose I'm still missing your point.

Any failure in ev_add_path() before the call to domap() could cause
multipath device creation to fail, without triggering this. For instance
add_map_with_path() or setup_map() can fail for a host of reasons.

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

Re: [PATCH v4 16/20] multipath -u : set FIND_MULTIPATHS_WAIT_UNTIL from /dev/shm

[Benjamin Marzinski]

On Thu, Apr 12, 2018 at 10:35:02PM +0200, Martin Wilck wrote:
> On Thu, 2018-04-12 at 13:36 -0500, Benjamin Marzinski wrote:
> > On Wed, Apr 04, 2018 at 06:16:23PM +0200, Martin Wilck wrote:
> > > 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(-)
> > > 
> > > +
> > > +/**
> > > + * 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);
> > > +
> > 
> > I'm worried about using pp->dev_t here with no method of removing
> > these
> > files.  What happens when a path device, say 8:32 is removed and a
> > completely new device comes in reusing the same dev_t?
> 
> Hm, what else should we use? devnode name is even worse, and most other
> options are a can of worms...  In the worst case, the new device
> wouldn't be waited for (or not long enough), because the marker existed
> before it was detected.
> 
> I could simply add a rule that removes the marker in case of a "remove"
> uevent, ok?

Fair enough. Nothing really bad happens, and removing the markers on
remove uevents would solve it.
 
> > 
> > > +	if (snprintf(path, sizeof(path), "%s/%s", shm_find_mp_dir,
> > > pp->dev_t)
> > > +	    >= sizeof(path)) {
> > > +		condlog(1, "%s: path name overflow", __func__);
> > 
> > shouldn't this be:
> > 		return FIND_MULTIPATHS_ERROR;
> 
> Bah, thank for catching it.
> 
> > >  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");
> > 
> > If we get an error trying to check the timeout, should we just keep
> > FIND_MULTIPATHS_WAIT_UNTIL the same? Or would it be better to set it
> > to
> > 0, and fail the smart claiming?
> 
> The latter is what we do. We set k=1 if find_multipaths_check_timeout()
> returns anything but FIND_MULTIPATHS_WAITING, resulting in
> DM_MULTIPATH_DEVICE_PATH="0".

Oops.  I got confused here.

-Ben

> 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 v4 18/20] multipath -u: quick check if path is multipathed

[Martin Wilck]

On Thu, 2018-04-12 at 13:46 -0500, Benjamin Marzinski wrote:
> On Wed, Apr 04, 2018 at 06:16:25PM +0200, Martin Wilck wrote:
> > 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.
> 
> I feel like this supports my idea from 17/20. I really think that in
> smart mode, the only time we should be claiming a device as multipath
> is
> on an add event, if it has always been claimed (or pretend claimed)
> as a
> multipath path, or if it is currently a multipath path. Once we have
> let
> a uevent go by for a path without setting SYSTEMD_READY=0, anything
> else
> is free to use it, and we simply can't safely set SYSTEMD_READY=0,
> unless we know that multipathd has already grabbed the device.

We have to remember that in the udev db then. That doesn't work for
coldplug aside, but we agree that's not an issue. By checking a
suitable udev variable, we can make sure that we never set
SYSTEMD_READY=0 after having released the device to the system.

>  I feel
> like checking the wwids file should give you confidence that a device
> either currently is a multipath path, or has always been claimed as
> one.
> However, this patch can fix any loop holes where a device isn't in
> the
> wwids file, but it multipathed (I don't know of any offhand).

multipath -u can't assume that multipathd is running. The map may have
been set up in the initrd, and thus would not necessarily be in the
WWIDs file in the root FS. 

Currently multipath -u scans all paths and maps in this case, just to
figure out if the given path is maybe already part of a map. My patch
was just meant as a shortcut for this case. If we could rely on the
WWIDs file that would be even easier, but I don't think we can.

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 v4 19/20] libmultipath: enable find_multipaths "smart"

[Martin Wilck]

On Thu, 2018-04-12 at 13:47 -0500, Benjamin Marzinski wrote:
> On Wed, Apr 04, 2018 at 06:16:26PM +0200, Martin Wilck wrote:
> > This activates "smart" path detection. This is similar to
> > "find_multipaths yes", but doesn't generally ignore single paths
> > that are not listed in the WWIDs file. Rather, such paths are
> > temporarily treated like multipath members. If no additional paths
> > are detected after a certain time, the paths are re-added to the
> > system as non-multipath. This needs support by the udev rules, to
> > be added in a follow-up patch.
> > 
> > If a multipath map is successfully successfully created, and paths
> > are
> > in waiting state, trigger path uevents to update their status.
> > 
> > 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 9aa3d21..17c2fa1 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"
> > @@ -476,9 +477,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 DM_MULTIPATH_DEVICE_PATH=1 then there has already been a uevent
> where
> udev recognized that the device should be claimed.  Wouldn't udev
> have
> stopped the timer then?  I don't see why this is necessary.

I set DM_MULTIPATH_DEVICE_PATH=1 in the "pretend_mpath" section
in multipath.rules in the "maybe" case. The value
DM_MULTIPATH_DEVICE_PATH=2 is never passed on to other udev rules,
and never seen by multipathd.

The reason I did that was that rules may have checks like
'DM_MULTIPATH_DEVICE_PATH!="1", ...' and I wanted to avoid these to be
run in the "maybe" case, which, for all actors above multipath, 
should be temporarily treated like "yes".

I hope this explains it.

Martin

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

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

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

[Martin Wilck]

On Thu, 2018-04-12 at 16:32 -0500, Benjamin Marzinski wrote:
> On Thu, Apr 12, 2018 at 10:22:19PM +0200, Martin Wilck wrote:
> > 
> > But I suppose I'm still missing your point.
> 
> Any failure in ev_add_path() before the call to domap() could cause
> multipath device creation to fail, without triggering this. For
> instance
> add_map_with_path() or setup_map() can fail for a host of reasons.

OK, got it, finally. The semantics of my failed marker were intended to
be exactly what they are now: we tried DM_DEVICE_CREATE for this WWID,
and failed. Other semantics are certainly possible, but much harder to
define cleanly.

I believe that catches a large portion of the real-world failures. And
in many cases, this failure means that one or more devices are busy.
(Oh, it could also mean that we tried to set 'hwhandler "1 alua"' for a
device that doesn't support ALUA, but I digress ;-)

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 v4 19/20] libmultipath: enable find_multipaths "smart"

[Benjamin Marzinski]

On Fri, Apr 13, 2018 at 12:27:29AM +0200, Martin Wilck wrote:
> On Thu, 2018-04-12 at 13:47 -0500, Benjamin Marzinski wrote:
> > On Wed, Apr 04, 2018 at 06:16:26PM +0200, Martin Wilck wrote:
> > > This activates "smart" path detection. This is similar to
> > > "find_multipaths yes", but doesn't generally ignore single paths
> > > that are not listed in the WWIDs file. Rather, such paths are
> > > temporarily treated like multipath members. If no additional paths
> > > are detected after a certain time, the paths are re-added to the
> > > system as non-multipath. This needs support by the udev rules, to
> > > be added in a follow-up patch.
> > > 
> > > If a multipath map is successfully successfully created, and paths
> > > are
> > > in waiting state, trigger path uevents to update their status.
> > > 
> > > 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 9aa3d21..17c2fa1 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"
> > > @@ -476,9 +477,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 DM_MULTIPATH_DEVICE_PATH=1 then there has already been a uevent
> > where
> > udev recognized that the device should be claimed.  Wouldn't udev
> > have
> > stopped the timer then?  I don't see why this is necessary.
> 
> I set DM_MULTIPATH_DEVICE_PATH=1 in the "pretend_mpath" section
> in multipath.rules in the "maybe" case. The value
> DM_MULTIPATH_DEVICE_PATH=2 is never passed on to other udev rules,
> and never seen by multipathd.
> 
> The reason I did that was that rules may have checks like
> 'DM_MULTIPATH_DEVICE_PATH!="1", ...' and I wanted to avoid these to be
> run in the "maybe" case, which, for all actors above multipath, 
> should be temporarily treated like "yes".
> 
> I hope this explains it.

Yeah, I probably should have looked back over the code I was confused
about after I read the udev rules patch.  I apparently figured that out
last time I reviewed the patch, and just forgot it.

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

> 
> 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 v4 20/20] multipath.rules: find_multipaths "smart" logic

[Benjamin Marzinski]

On Thu, Apr 12, 2018 at 01:48:51PM -0500, Benjamin Marzinski wrote:
> On Wed, Apr 04, 2018 at 06:16:27PM +0200, Martin Wilck wrote:
> > When the first path to a device appears, we don't know if more paths are going
> > to follow. find_multipath "smart" logic attempts to solve this dilemma by
> > waiting for additional paths for a configurable time before giving up
> > and releasing single paths to upper layers.
> > 
> > These rules apply only if both find_multipaths is set to "smart" in
> > multipath.conf. In this mode, multipath -u sets DM_MULTIPATH_DEVICE_PATH=2 if
> > there's no clear evidence wheteher a given device should be a multipath member
> > (not blacklisted, not listed as "failed", not in WWIDs file, not member of an
> > 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.
> > 
> 
> 
> Like I've mentioned, I think multipath -u should be called with a
> special option on add events or if FIND_MULTIPATHS_WAIT_UNTIL exists and
> is non-zero (or perhaps we should IMPORT DM_MULTIPATH_DEVICE_PATH, and
> check if it's 2 or import SYSTEMD_READY and check if it's 0), that smart
> mode could use to allow maybes and new claims on existing paths because
> another path appeared.

Since you have a different way of solving the issue, that doesn't need this
change to the udev rules

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
 
> > 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 v4 18/20] multipath -u: quick check if path is multipathed

[Benjamin Marzinski]

On Fri, Apr 13, 2018 at 12:19:12AM +0200, Martin Wilck wrote:
> On Thu, 2018-04-12 at 13:46 -0500, Benjamin Marzinski wrote:
> > On Wed, Apr 04, 2018 at 06:16:25PM +0200, Martin Wilck wrote:
> > > 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.
> > 
> > I feel like this supports my idea from 17/20. I really think that in
> > smart mode, the only time we should be claiming a device as multipath
> > is
> > on an add event, if it has always been claimed (or pretend claimed)
> > as a
> > multipath path, or if it is currently a multipath path. Once we have
> > let
> > a uevent go by for a path without setting SYSTEMD_READY=0, anything
> > else
> > is free to use it, and we simply can't safely set SYSTEMD_READY=0,
> > unless we know that multipathd has already grabbed the device.
> 
> We have to remember that in the udev db then. That doesn't work for
> coldplug aside, but we agree that's not an issue. By checking a
> suitable udev variable, we can make sure that we never set
> SYSTEMD_READY=0 after having released the device to the system.

We can safely set SYSTEMD_READY=0 if we know that the device is now part
of an existing multipath device, but yeah.
 
> >  I feel
> > like checking the wwids file should give you confidence that a device
> > either currently is a multipath path, or has always been claimed as
> > one.
> > However, this patch can fix any loop holes where a device isn't in
> > the
> > wwids file, but it multipathed (I don't know of any offhand).
> 
> multipath -u can't assume that multipathd is running. The map may have
> been set up in the initrd, and thus would not necessarily be in the
> WWIDs file in the root FS. 

That's a loophole. And because I accidentally forgot to add it before

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

> Currently multipath -u scans all paths and maps in this case, just to
> figure out if the given path is maybe already part of a map. My patch
> was just meant as a shortcut for this case. If we could rely on the
> WWIDs file that would be even easier, but I don't think we can.
> 
> 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 v4 18/20] multipath -u: quick check if path is multipathed

[Martin Wilck]

On Fri, 2018-04-13 at 11:12 -0500, Benjamin Marzinski wrote:
> On Fri, Apr 13, 2018 at 12:19:12AM +0200, Martin Wilck wrote:
> > On Thu, 2018-04-12 at 13:46 -0500, Benjamin Marzinski wrote:
> > > On Wed, Apr 04, 2018 at 06:16:25PM +0200, Martin Wilck wrote:
> > > > 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.
> > > 
> > > I feel like this supports my idea from 17/20. I really think that
> > > in
> > > smart mode, the only time we should be claiming a device as
> > > multipath
> > > is
> > > on an add event, if it has always been claimed (or pretend
> > > claimed)
> > > as a
> > > multipath path, or if it is currently a multipath path. Once we
> > > have
> > > let
> > > a uevent go by for a path without setting SYSTEMD_READY=0,
> > > anything
> > > else
> > > is free to use it, and we simply can't safely set
> > > SYSTEMD_READY=0,
> > > unless we know that multipathd has already grabbed the device.
> > 
> > We have to remember that in the udev db then. That doesn't work for
> > coldplug aside, but we agree that's not an issue. By checking a
> > suitable udev variable, we can make sure that we never set
> > SYSTEMD_READY=0 after having released the device to the system.
> 
> We can safely set SYSTEMD_READY=0 if we know that the device is now
> part
> of an existing multipath device, but yeah.

Patch set v5 will implement this logic. I'm currently testing it. I
simply look at DM_MULTIPATH_DEVICE_PATH as saved in the udev db.

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 v4 07/20] libmultipath: change find_multipaths option to multi-value

[Martin Wilck]

On Wed, 2018-04-04 at 18:16 +0200, Martin Wilck wrote:

> diff --git a/libmultipath/wwids.c b/libmultipath/wwids.c
> index 11afe09..da685be 100644
> --- a/libmultipath/wwids.c
> +++ b/libmultipath/wwids.c
> @@ -282,8 +282,12 @@ should_multipath(struct path *pp1, vector
> pathvec, vector mpvec)
>  	struct config *conf;
>  
>  	conf = get_multipath_config();
> -	ignore_new_devs = conf->ignore_new_devs;
> -	find_multipaths = conf->find_multipaths;
> +	ignore_new_devs = ignore_new_devs_on(conf);
> +	find_multipaths = find_multipaths_on(conf);
> +	if (!find_multipaths && !ignore_new_devs) {
> +		put_multipath_config(conf);
> +		return 1;
> +	}
>  	put_multipath_config(conf);
>  	if (find_multipaths && !ignore_new_devs)
>  		return 1;
> 

I made a mistake during rebase here. Will be fixed in v5.

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