[PATCH v5 00/22] multipath path classification

[PATCH v5 00/22] multipath path classification

[Martin Wilck]

Hello Christophe,

(this set is based on 1cb704b2 plus those recent patches that have been
positively reviewed, see
https://github.com/openSUSE/multipath-tools/commits/upstream-queue).

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.
Also, with v5 of the set, the exit status of "multipath -u" is 0 even if the
path is not a multipath member, in order to make udev happy.

Changes v4->v5:

 - rebased on top of latest reviewed patches, see above
 - numbering v4->v5: 17->20, 18->18, 19->21, 20->22
 - whitespace and other style fixes in 05/22, 06/22, 07/22, 09/22, 15/22,
   16/22, 18/22 (checkpatch.pl; I took the freedom to keep Ben's reviewed-by tags).
 - 07/22: fixed error I made in v4 while rebasing to latest upstream
 - 15/22: fixed a compilation warning regarding "const"
 - 16/22: symbolic return code in find_multipaths_check_timeout (Ben)
 - 16/22: add udev rule to remove timeout marker on remove uevent (Ben)
 - 17/22: NEW, make "multipath -u" logic a bit simpler to read
 - 19/22: NEW, avoid returning "maybe" or "yes" after "no" (Ben)
 - 20/22: by virtue of 19/22, the O_EXCL test is never run after a device
   has been been released to systemd. This was Ben's main concern for v4.

Only 16, 17, 19, 20 are lacking Reviewed-by: tags now.

This series is also available here:
https://github.com/mwilck/multipath-tools/commits/path-detection-v5

A broken-out version showing the FIX patches v4->v5 is here:
https://github.com/mwilck/multipath-tools/commits/path-detection-v4_v5

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 (21):
  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: cleanup logic
  multipath -u: quick check if path is multipathed
  multipath -u: don't grab devices already passed to system
  multipath -u: test if path is busy
  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        |  55 ++++++++-
 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       | 128 ++++++++++++++++++-
 libmultipath/wwids.h       |  13 +-
 multipath/main.c           | 297 ++++++++++++++++++++++++++++++++++++++++-----
 multipath/multipath.8      |   9 +-
 multipath/multipath.conf.5 |  79 ++++++++----
 multipath/multipath.rules  |  70 ++++++++++-
 multipathd/main.c          |  15 +--
 multipathd/multipathd.8    |   8 +-
 21 files changed, 802 insertions(+), 93 deletions(-)

-- 
2.16.1

[PATCH v5 01/22] 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 d08c6881..d26c7444 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -776,16 +776,6 @@ main (int argc, char *argv[])
 		}
 	}
 
-	/*
-	 * FIXME: new device detection with find_multipaths currently
-	 * doesn't work reliably.
-	 */
-	if (cmd ==  CMD_VALID_PATH &&
-	    conf->find_multipaths && conf->ignore_wwids) {
-		condlog(2, "ignoring -i flag because find_multipath is set in multipath.conf");
-		conf->ignore_wwids = 0;
-	}
-
 	if (getuid() != 0) {
 		fprintf(stderr, "need to be root\n");
 		exit(1);
-- 
2.16.1

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

[Hannes Reinecke]

On Fri, 13 Apr 2018 23:59:54 +0200
Martin Wilck <mwilck@suse.com> wrote:

> This reverts commit ffbb886a8a16cb063d669cd76a1e656fd3ec8c4b.
> 
> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipath/main.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 

Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes

[PATCH v5 02/22] 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 4d65f1d0..17175a85 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2403,10 +2403,6 @@ reconfigure (struct vectors * vecs)
 		conf->verbosity = verbosity;
 	if (bindings_read_only)
 		conf->bindings_read_only = bindings_read_only;
-	if (conf->find_multipaths) {
-		condlog(2, "find_multipaths is set: -n is implied");
-		ignore_new_devs = 1;
-	}
 	if (ignore_new_devs)
 		conf->ignore_new_devs = ignore_new_devs;
 	uxsock_timeout = conf->uxsock_timeout;
-- 
2.16.1

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

[Hannes Reinecke]

On Fri, 13 Apr 2018 23:59:55 +0200
Martin Wilck <mwilck@suse.com> wrote:

> This reverts commit 64e27ec066a001012f44550f095c93443e91d845.
> 
> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipathd/main.c | 4 ----
>  1 file changed, 4 deletions(-)
> 

Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes

[PATCH v5 03/22] 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 51a45d4f..1cd575e4 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 8c21b33f..d0256c2c 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 95270129..d9a78b38 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 17175a85..20b6fe02 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -940,7 +940,7 @@ rescan:
 		mpp->action = ACT_RELOAD;
 		extract_hwe_from_path(mpp);
 	} else {
-		if (!should_multipath(pp, vecs->pathvec)) {
+		if (!should_multipath(pp, vecs->pathvec, vecs->mpvec)) {
 			orphan_path(pp, "only one path");
 			return 0;
 		}
-- 
2.16.1

Re: [PATCH v5 03/22] libmultipath: should_multipath: keep existing maps

[Hannes Reinecke]

On Fri, 13 Apr 2018 23:59:56 +0200
Martin Wilck <mwilck@suse.com> wrote:

> with find_multipaths "yes" and without the "-n" option to multipathd,
> if a path is already multipathed, keep it. The same logic is applied
> by "multipath -u -i".
> 
> To do this, we need to add a "mpvec" parameter to should_multipath().
> 
> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/configure.c |  2 +-
>  libmultipath/wwids.c     | 12 +++++++++++-
>  libmultipath/wwids.h     |  2 +-
>  multipathd/main.c        |  2 +-
>  4 files changed, 14 insertions(+), 4 deletions(-)
> 

Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes

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

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

[Hannes Reinecke]

On Fri, 13 Apr 2018 23:59:57 +0200
Martin Wilck <mwilck@suse.com> wrote:

> Previously, if find_multipaths was set, devices listed in the WWIDs
> file weren't classified as multipath members by "multipath -u -i"
> unless they also met the "find_multipaths" criteria (at least two
> paths, or existing map with this WWID). Now we classify all paths in
> the WWIDs file as multipath members, too.
> 
> The rationale for this patch is to match the logic that multipathd
> applies by default (i.e. without "-n").
> 
> 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(-)
> 

Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes

[PATCH v5 05/22] 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>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c | 26 ++++++++++++++++++++++++--
 libmultipath/configure.h |  1 +
 libmultipath/wwids.c     |  2 +-
 multipath/main.c         |  2 +-
 multipathd/main.c        |  3 ++-
 5 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 1cd575e4..c56e972f 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 27a7e6f6..545cbc20 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 d0256c2c..28063429 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 ba837710..686b0372 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 20b6fe02..10a0ee76 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2323,7 +2323,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

Re: [PATCH v5 05/22] libmultipath: trigger change uevent on new device creation

[Hannes Reinecke]

On Fri, 13 Apr 2018 23:59:58 +0200
Martin Wilck <mwilck@suse.com> wrote:

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

Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes

[PATCH v5 06/22] 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 c56e972f..2cae9240 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

Re: [PATCH v5 06/22] libmultipath: trigger path uevent only when necessary

[Hannes Reinecke]

On Fri, 13 Apr 2018 23:59:59 +0200
Martin Wilck <mwilck@suse.com> wrote:

> Paths that are already classified as DM_MULTIPATH_DEVICE_PATH don't
> need to be retriggered.
> 
> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/configure.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 

Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes

[PATCH v5 07/22] 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       |  4 ++--
 multipath/main.c           |  9 +++++----
 multipath/multipath.8      |  9 ++++++++-
 multipath/multipath.conf.5 | 46 +++++++++++++++++++++++++--------------------
 multipathd/main.c          |  6 +-----
 multipathd/multipathd.8    |  8 +++++---
 10 files changed, 119 insertions(+), 40 deletions(-)

diff --git a/libmultipath/config.h b/libmultipath/config.h
index 329f3ed3..c71d972b 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 2b270ca4..690182c3 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 1a18337b..b03197b6 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -240,8 +240,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 * const 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 88a4b786..c43f2c3b 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 28063429..5e9596fe 100644
--- a/libmultipath/wwids.c
+++ b/libmultipath/wwids.c
@@ -282,8 +282,8 @@ 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);
 	put_multipath_config(conf);
 	if (!find_multipaths && !ignore_new_devs)
 		return 1;
diff --git a/multipath/main.c b/multipath/main.c
index 686b0372..2be9b9c2 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 56f87035..914a8cb2 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 c4d07894..6965dacb 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
+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
+.I no
+Multipath behaves like \fBstrict\fR. Multipathd behaves like \fBgreedy\fR.
+.TP
 .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.
-.RS
+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
-The default is: \fBno\fR
+.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 10a0ee76..80905848 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2404,8 +2404,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);
@@ -2635,8 +2633,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)) {
@@ -2991,7 +2987,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 5c96680c..e78ac9ed 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 v5 07/22] libmultipath: change find_multipaths option to multi-value

[Hannes Reinecke]

On Sat, 14 Apr 2018 00:00:00 +0200
Martin Wilck <mwilck@suse.com> wrote:

> Change the "find_multipaths" option from yes/no to multi-value. This
> option now covers the effects of "find_multipaths" as it used to be,
> plus the -i option to multipath (ignore_wwids) and the -n option to
> multipathd (ignore_new_devs), excluding such combinations of the
> former options that are dangerous or inconsistent.
> 
> The possible values for "find_multipaths" are now:
> 
>  - strict: strictly stick to the WWIDs file; never add new maps
> automatically (new default; upstream behaviour with with
> find_multipaths "yes" and commit 64e27ec "multipathd: imply -n if
> find_multipaths is set")
>  - off|no: multipath like "strict", multipathd like "greedy" (previous
>    upstream default)
>  - on|yes: set up multipath if >1 paths are seen (current Red
> Hat/Ubuntu behavior with find_multipaths "yes")
>  - greedy: set up multipath for all non-blacklisted devices (current
> SUSE default)
>  - smart: Like "yes", but try to avoid inconsistencies between udev
> processing and multipathd processing by waiting for path siblings to
>    appear (fully implemented in follow-up patches)
> 
> The default has been changed to "strict", because "no" may cause
> inconsistent behavior between "multipath -u" and multipathd. This is
> deliberately a conservative choice.
> 
> The patch also updates the related man pages.
> 
> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> 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       |  4 ++-- multipath/main.c           |  9
> +++++---- multipath/multipath.8      |  9 ++++++++-
>  multipath/multipath.conf.5 | 46
> +++++++++++++++++++++++++--------------------
> multipathd/main.c          |  6 +----- multipathd/multipathd.8    |
> 8 +++++--- 10 files changed, 119 insertions(+), 40 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes

[PATCH v5 08/22] 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 e4951c92..d5165ec6 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 4f96dbf5..70bffa53 100644
--- a/libmultipath/file.h
+++ b/libmultipath/file.h
@@ -6,6 +6,6 @@
 #define _FILE_H
 
 #define FILE_TIMEOUT 30
-int open_file(char *file, int *can_write, char *header);
+int open_file(const char *file, int *can_write, const char *header);
 
 #endif /* _FILE_H */
-- 
2.16.1

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

[Hannes Reinecke]

On Sat, 14 Apr 2018 00:00:01 +0200
Martin Wilck <mwilck@suse.com> wrote:

> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/file.c | 6 +++---
>  libmultipath/file.h | 2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes

[PATCH v5 09/22] 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. This has been chosen because it
"survives" during pivot-root between initrd and root file system.

The exact semantics of /dev/shm/multipath/failed_wwids/$WWID is:
"multipath or multipathd has tried to create this map from its
members with a DM_DEVICE_CREATE call, and failed on the latest,
or only, attempt to do so".

In particular, the existence of the file proves that here was at
least one unsuccessful attempt to create the map since the last
reboot. On the contrary, the non-existence of this file does not
indicate a successful attempt - perhaps multipathd never tried to
set up the map.

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.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 690182c3..19ad2bfc 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 5e9596fe..53e79511 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"
@@ -333,3 +334,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 d9a78b38..0c6ee54d 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 v5 09/22] libmultipath: functions to indicate mapping failure in /dev/shm

[Hannes Reinecke]

On Sat, 14 Apr 2018 00:00:02 +0200
Martin Wilck <mwilck@suse.com> 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. This has been chosen because it
> "survives" during pivot-root between initrd and root file system.
> 
> The exact semantics of /dev/shm/multipath/failed_wwids/$WWID is:
> "multipath or multipathd has tried to create this map from its
> members with a DM_DEVICE_CREATE call, and failed on the latest,
> or only, attempt to do so".
> 
> In particular, the existence of the file proves that here was at
> least one unsuccessful attempt to create the map since the last
> reboot. On the contrary, the non-existence of this file does not
> indicate a successful attempt - perhaps multipathd never tried to
> set up the map.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/defaults.h |   1 +
>  libmultipath/wwids.c    | 110
> ++++++++++++++++++++++++++++++++++++++++++++++++
> libmultipath/wwids.h    |  11 +++++ 3 files changed, 122 insertions(+)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes

[PATCH v5 10/22] 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>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/configure.c | 35 +++++++++++++++++++++++++++--------
 libmultipath/configure.h |  2 +-
 libmultipath/devmapper.c |  9 ++++++++-
 libmultipath/structs.h   |  1 +
 multipathd/main.c        |  2 +-
 5 files changed, 38 insertions(+), 11 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 2cae9240..f17f8ec0 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"))
+
+			if (is_mpath && env != NULL && !strcmp(env, "1"))
+				continue;
+			else if (!is_mpath &&
+				   (env == NULL || !strcmp(env, "0")))
 				continue;
 
-			condlog(4, "triggering change uevent for %s", pp->dev);
-			sysfs_attr_set_value(pp->udev, "uevent", "change",
-					     strlen("change"));
+			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 545cbc20..8b56d33a 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 2a921051..f2befad5 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 c43f2c3b..1d3a34f6 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 80905848..1ac7245f 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2324,7 +2324,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 v5 10/22] libmultipath: indicate wwid failure in dm_addmap_create()

[Hannes Reinecke]

On Sat, 14 Apr 2018 00:00:03 +0200
Martin Wilck <mwilck@suse.com> 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.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/configure.c | 35 +++++++++++++++++++++++++++--------
>  libmultipath/configure.h |  2 +-
>  libmultipath/devmapper.c |  9 ++++++++-
>  libmultipath/structs.h   |  1 +
>  multipathd/main.c        |  2 +-
>  5 files changed, 38 insertions(+), 11 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes

[PATCH v5 11/22] 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>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipath/main.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/multipath/main.c b/multipath/main.c
index 2be9b9c2..bf28735f 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 v5 11/22] multipath -u: common code path for result message

[Hannes Reinecke]

On Sat, 14 Apr 2018 00:00:04 +0200
Martin Wilck <mwilck@suse.com> wrote:

> 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>
> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  multipath/main.c | 32 +++++++++++++++++++-------------
>  1 file changed, 19 insertions(+), 13 deletions(-)
> 

Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes

[PATCH v5 12/22] 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>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.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 bf28735f..59a72ed0 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 6f8ee2be..aab64dc7 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 v5 12/22] multipath -u: change output to environment/key format

[Hannes Reinecke]

On Sat, 14 Apr 2018 00:00:05 +0200
Martin Wilck <mwilck@suse.com> 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.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  multipath/main.c          | 14 ++++++++------
>  multipath/multipath.rules |  6 +++---
>  2 files changed, 11 insertions(+), 9 deletions(-)
> 

Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes

[PATCH v5 13/22] 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>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipath/main.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/multipath/main.c b/multipath/main.c
index 59a72ed0..942caf4c 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 v5 13/22] multipath -u: treat failed wwids as invalid

[Hannes Reinecke]

On Sat, 14 Apr 2018 00:00:06 +0200
Martin Wilck <mwilck@suse.com> 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.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  multipath/main.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes

[PATCH v5 14/22] 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 942caf4c..f62e18a9 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

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

[Hannes Reinecke]

On Sat, 14 Apr 2018 00:00:07 +0200
Martin Wilck <mwilck@suse.com> wrote:

> Use DM_MULTIPATH_DEVICE_PATH="2" to indicate that this might be a
> valid path, but we aren't certain. This happens with find_multipaths
> "smart", when the first path to a device (or a single-path
> device) is encountered, and the device is neither blacklisted,
> nor marked failed, nor whitelisted in the wwids file.
> 
> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipath/main.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/multipath/main.c b/multipath/main.c
> index 942caf4c..f62e18a9 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;
>  	}
>  

A very unusual array mapping ... Maybe it should be cleaned up?

Other than that:

Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes

[PATCH v5 15/22] 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>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/config.h      |  1 +
 libmultipath/defaults.h    |  2 ++
 libmultipath/dict.c        |  7 +++++++
 libmultipath/propsel.c     | 25 +++++++++++++++++++++++++
 libmultipath/propsel.h     |  1 +
 libmultipath/structs.h     |  1 +
 multipath/multipath.conf.5 | 18 ++++++++++++++++++
 7 files changed, 55 insertions(+)

diff --git a/libmultipath/config.h b/libmultipath/config.h
index c71d972b..6e69a37e 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 19ad2bfc..d7b87b44 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 b03197b6..eda3f1f0 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -491,6 +491,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)
 {
@@ -1527,6 +1531,9 @@ 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 2d3b7adf..627d3665 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -953,3 +953,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)
+{
+	const 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 "unknown" 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 unknown 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 136f9060..a022beef 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 1d3a34f6..eb6a1788 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 6965dacb..3a34aef3 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
+unknown, 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 v5 15/22] libmultipath: implement find_multipaths_timeout

[Hannes Reinecke]

On Sat, 14 Apr 2018 00:00:08 +0200
Martin Wilck <mwilck@suse.com> 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.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/config.h      |  1 +
>  libmultipath/defaults.h    |  2 ++
>  libmultipath/dict.c        |  7 +++++++
>  libmultipath/propsel.c     | 25 +++++++++++++++++++++++++
>  libmultipath/propsel.h     |  1 +
>  libmultipath/structs.h     |  1 +
>  multipath/multipath.conf.5 | 18 ++++++++++++++++++
>  7 files changed, 55 insertions(+)
> 

Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes

[PATCH v5 16/22] 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.

Because we use major:minor to identify the devices in /dev/shm, and because
a removed device might be replaced by a different one with the same
major/minor number, add a rule to multipath.rules to remove the
find_multipaths marker on remove uevents.

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

diff --git a/libmultipath/file.c b/libmultipath/file.c
index d5165ec6..8727f160 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 70bffa53..29520c74 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 f62e18a9..5d847f08 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,142 @@ 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 occurred
+ * @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 FIND_MULTIPATHS_ERROR;
+	}
+
+	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;
+
+	*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;
 }
diff --git a/multipath/multipath.rules b/multipath/multipath.rules
index aab64dc7..37f4f6d8 100644
--- a/multipath/multipath.rules
+++ b/multipath/multipath.rules
@@ -1,7 +1,9 @@
 # Set DM_MULTIPATH_DEVICE_PATH if the device should be handled by multipath
 SUBSYSTEM!="block", GOTO="end_mpath"
-ACTION!="add|change", GOTO="end_mpath"
 KERNEL!="sd*|dasd*|nvme*", GOTO="end_mpath"
+ACTION=="remove", TEST=="/dev/shm/multipath/find_multipaths/$major:$minor", \
+	RUN+="/usr/bin/rm -f /dev/shm/multipath/find_multipaths/$major:$minor"
+ACTION!="add|change", GOTO="end_mpath"
 
 IMPORT{cmdline}="nompath"
 ENV{nompath}=="?*", GOTO="end_mpath"
-- 
2.16.1

Re: [PATCH v5 16/22] multipath -u : set FIND_MULTIPATHS_WAIT_UNTIL from /dev/shm

[Hannes Reinecke]

On Sat, 14 Apr 2018 00:00:09 +0200
Martin Wilck <mwilck@suse.com> 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.
> 
> Because we use major:minor to identify the devices in /dev/shm, and
> because a removed device might be replaced by a different one with
> the same major/minor number, add a rule to multipath.rules to remove
> the find_multipaths marker on remove uevents.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/file.c       |   2 +-
>  libmultipath/file.h       |   1 +
>  multipath/main.c          | 132
> ++++++++++++++++++++++++++++++++++++++++++++++
> multipath/multipath.rules |   4 +- 4 files changed, 137
> insertions(+), 2 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes

Re: [PATCH v5 16/22] multipath -u : set FIND_MULTIPATHS_WAIT_UNTIL from /dev/shm

[Benjamin Marzinski]

On Sat, Apr 14, 2018 at 12:00:09AM +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.
> 
> Because we use major:minor to identify the devices in /dev/shm, and because
> a removed device might be replaced by a different one with the same
> major/minor number, add a rule to multipath.rules to remove the
> find_multipaths marker on remove uevents.
> 

Reviewed-by: Benjamin Marzinsk <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/file.c       |   2 +-
>  libmultipath/file.h       |   1 +
>  multipath/main.c          | 132 ++++++++++++++++++++++++++++++++++++++++++++++
>  multipath/multipath.rules |   4 +-
>  4 files changed, 137 insertions(+), 2 deletions(-)
> 
> diff --git a/libmultipath/file.c b/libmultipath/file.c
> index d5165ec6..8727f160 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 70bffa53..29520c74 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 f62e18a9..5d847f08 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,142 @@ 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 occurred
> + * @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 FIND_MULTIPATHS_ERROR;
> +	}
> +
> +	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;
> +
> +	*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;
>  }
> diff --git a/multipath/multipath.rules b/multipath/multipath.rules
> index aab64dc7..37f4f6d8 100644
> --- a/multipath/multipath.rules
> +++ b/multipath/multipath.rules
> @@ -1,7 +1,9 @@
>  # Set DM_MULTIPATH_DEVICE_PATH if the device should be handled by multipath
>  SUBSYSTEM!="block", GOTO="end_mpath"
> -ACTION!="add|change", GOTO="end_mpath"
>  KERNEL!="sd*|dasd*|nvme*", GOTO="end_mpath"
> +ACTION=="remove", TEST=="/dev/shm/multipath/find_multipaths/$major:$minor", \
> +	RUN+="/usr/bin/rm -f /dev/shm/multipath/find_multipaths/$major:$minor"
> +ACTION!="add|change", GOTO="end_mpath"
>  
>  IMPORT{cmdline}="nompath"
>  ENV{nompath}=="?*", GOTO="end_mpath"
> -- 
> 2.16.1

[PATCH v5 17/22] multipath -u: cleanup logic

[Martin Wilck]

The CMD_VALID_PATH logic is complex and hard to read and understand.
This patch cleans it up a bit (preparing for folluw-up patches).
It doesn't change any logic.

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

diff --git a/multipath/main.c b/multipath/main.c
index 5d847f08..61ba90a6 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -585,15 +585,17 @@ configure (struct config *conf, enum mpath_cmds cmd,
 			if (is_failed_wwid(refwwid) == WWID_IS_FAILED) {
 				r = 1;
 				goto print_valid;
-			} else if ((!find_multipaths_on(conf) &&
+			}
+			if ((!find_multipaths_on(conf) &&
 				    ignore_wwids_on(conf)) ||
 				   check_wwids_file(refwwid, 0) == 0)
 				r = 0;
-			if (r == 0 ||
-			    !find_multipaths_on(conf) ||
-			    !ignore_wwids_on(conf)) {
+			if (!ignore_wwids_on(conf))
 				goto print_valid;
-			}
+			/* At this point, either r==0 or find_multipaths_on. */
+			if (r == 0)
+				goto print_valid;
+			/* find_multipaths_on: Fall through to path detection */
 		}
 	}
 
-- 
2.16.1

Re: [PATCH v5 17/22] multipath -u: cleanup logic

[Hannes Reinecke]

On Sat, 14 Apr 2018 00:00:10 +0200
Martin Wilck <mwilck@suse.com> wrote:

> The CMD_VALID_PATH logic is complex and hard to read and understand.
> This patch cleans it up a bit (preparing for folluw-up patches).
> It doesn't change any logic.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipath/main.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes

Re: [PATCH v5 17/22] multipath -u: cleanup logic

[Benjamin Marzinski]

On Sat, Apr 14, 2018 at 12:00:10AM +0200, Martin Wilck wrote:
> The CMD_VALID_PATH logic is complex and hard to read and understand.
> This patch cleans it up a bit (preparing for folluw-up patches).
> It doesn't change any logic.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipath/main.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/multipath/main.c b/multipath/main.c
> index 5d847f08..61ba90a6 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -585,15 +585,17 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  			if (is_failed_wwid(refwwid) == WWID_IS_FAILED) {
>  				r = 1;
>  				goto print_valid;
> -			} else if ((!find_multipaths_on(conf) &&
> +			}
> +			if ((!find_multipaths_on(conf) &&
>  				    ignore_wwids_on(conf)) ||
>  				   check_wwids_file(refwwid, 0) == 0)
>  				r = 0;
> -			if (r == 0 ||
> -			    !find_multipaths_on(conf) ||
> -			    !ignore_wwids_on(conf)) {
> +			if (!ignore_wwids_on(conf))
>  				goto print_valid;
> -			}
> +			/* At this point, either r==0 or find_multipaths_on. */
> +			if (r == 0)
> +				goto print_valid;
> +			/* find_multipaths_on: Fall through to path detection */
>  		}
>  	}
>  
> -- 
> 2.16.1

[PATCH v5 18/22] 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>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/sysfs.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 libmultipath/sysfs.h |  2 ++
 multipath/main.c     |  9 +++++++
 3 files changed, 77 insertions(+)

diff --git a/libmultipath/sysfs.c b/libmultipath/sysfs.c
index 97e09977..ee72e6a3 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 75c0f9c1..9ae30b39 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 61ba90a6..96e37a7a 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -593,6 +593,15 @@ configure (struct config *conf, enum mpath_cmds cmd,
 			if (!ignore_wwids_on(conf))
 				goto print_valid;
 			/* At this point, either r==0 or find_multipaths_on. */
+
+			/*
+			 * Shortcut for find_multipaths smart:
+			 * Quick check if path is already multipathed.
+			 */
+			if (sysfs_is_multipathed(VECTOR_SLOT(pathvec, 0))) {
+				r = 0;
+				goto print_valid;
+			}
 			if (r == 0)
 				goto print_valid;
 			/* find_multipaths_on: Fall through to path detection */
-- 
2.16.1

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

[Hannes Reinecke]

On Sat, 14 Apr 2018 00:00:11 +0200
Martin Wilck <mwilck@suse.com> 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.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/sysfs.c | 66
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
> libmultipath/sysfs.h |  2 ++ multipath/main.c     |  9 +++++++
>  3 files changed, 77 insertions(+)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes

[PATCH v5 19/22] multipath -u: don't grab devices already passed to system

[Martin Wilck]

Setting SYSTEMD_READY=0 on a device that has previously been passed to
systemd is dangerous - already mounted file systems might be unmounted by
systemd.

Avoid that by checking for previously set DM_MULTIPATH_DEVICE_PATH
environment variable.

This requires to change the exit status of multipath -u - it needs to exit
with status 0 even if the path is not a multipath device path, otherwise
udev doesn't import the printed key-value pairs. We do this only for
"multipath -u"; legacy "multipath -c", which is more likely to be run in user
scripts, still exits with 1 for non-multipath devices.

The condition ENV{DM_MULTIPATH_DEVICE_PATH}!="1" before the "multipath -u"
statement in multipath.rules needs to be removed. This condition was
pointless anyway, because until this patch, DM_MULTIPATH_DEVICE_PATH hadn't
been imported from the db and thus was never set, and "multipath -u" was
always invoked. We want to keep this behavior.

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

diff --git a/multipath/main.c b/multipath/main.c
index 96e37a7a..573d94f9 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -25,6 +25,7 @@
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <stdio.h>
+#include <stdlib.h>
 #include <unistd.h>
 #include <ctype.h>
 #include <libudev.h>
@@ -494,6 +495,25 @@ static int print_cmd_valid(int k, const vector pathvec,
 	return k == 1;
 }
 
+/*
+ * Returns true if this device has been handled before,
+ * and released to systemd.
+ *
+ * This must be called before get_refwwid(),
+ * otherwise udev_device_new_from_environment() will have
+ * destroyed environ(7).
+ */
+static bool released_to_systemd(void)
+{
+	static const char dmdp[] = "DM_MULTIPATH_DEVICE_PATH";
+	const char *dm_mp_dev_path = getenv(dmdp);
+	bool ret;
+
+	ret = dm_mp_dev_path != NULL && !strcmp(dm_mp_dev_path, "0");
+	condlog(4, "%s: %s=%s -> %d", __func__, dmdp, dm_mp_dev_path, ret);
+	return ret;
+}
+
 /*
  * Return value:
  *  -1: Retry
@@ -511,6 +531,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
 	int di_flag = 0;
 	char * refwwid = NULL;
 	char * dev = NULL;
+	bool released = released_to_systemd();
 
 	/*
 	 * allocate core vectors to store paths and multipaths
@@ -602,6 +623,20 @@ configure (struct config *conf, enum mpath_cmds cmd,
 				r = 0;
 				goto print_valid;
 			}
+
+			/*
+			 * DM_MULTIPATH_DEVICE_PATH=="0" means that we have
+			 * been called for this device already, and have
+			 * released it to systemd. Unless the device is now
+			 * already multipathed (see above), we can't try to
+			 * grab it, because setting SYSTEMD_READY=0 would
+			 * cause file systems to be unmounted.
+			 * Leave DM_MULTIPATH_DEVICE_PATH="0".
+			 */
+			if (released) {
+				r = 1;
+				goto print_valid;
+			}
 			if (r == 0)
 				goto print_valid;
 			/* find_multipaths_on: Fall through to path detection */
@@ -641,7 +676,9 @@ configure (struct config *conf, enum mpath_cmds cmd,
 
 	if (cmd == CMD_VALID_PATH) {
 		/* This only happens if find_multipaths and
-		 * ignore_wwids is set.
+		 * ignore_wwids is set, and the path is not in WWIDs
+		 * file, not currently multipathed, and has
+		 * never been released to systemd.
 		 * If there is currently a multipath device matching
 		 * the refwwid, or there is more than one path matching
 		 * the refwwid, then the path is valid */
@@ -1064,6 +1101,13 @@ out:
 	cleanup_prio();
 	cleanup_checkers();
 
+	/*
+	 * multipath -u must exit with status 0, otherwise udev won't
+	 * import its output.
+	 */
+	if (cmd == CMD_VALID_PATH && dev_type == DEV_UEVENT && r == 1)
+		r = 0;
+
 	if (dev_type == DEV_UEVENT)
 		closelog();
 
diff --git a/multipath/multipath.rules b/multipath/multipath.rules
index 37f4f6d8..ef857271 100644
--- a/multipath/multipath.rules
+++ b/multipath/multipath.rules
@@ -21,8 +21,11 @@ LABEL="test_dev"
 ENV{MPATH_SBIN_PATH}="/sbin"
 TEST!="$env{MPATH_SBIN_PATH}/multipath", ENV{MPATH_SBIN_PATH}="/usr/sbin"
 
+# multipath -u needs to know if this device has ever been exported
+IMPORT{db}="DM_MULTIPATH_DEVICE_PATH"
+
 # multipath -u sets DM_MULTIPATH_DEVICE_PATH
-ENV{DM_MULTIPATH_DEVICE_PATH}!="1", IMPORT{program}="$env{MPATH_SBIN_PATH}/multipath -u %k"
+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"
 
-- 
2.16.1

Re: [PATCH v5 19/22] multipath -u: don't grab devices already passed to system

[Hannes Reinecke]

On Sat, 14 Apr 2018 00:00:12 +0200
Martin Wilck <mwilck@suse.com> wrote:

> Setting SYSTEMD_READY=0 on a device that has previously been passed to
> systemd is dangerous - already mounted file systems might be
> unmounted by systemd.
> 
> Avoid that by checking for previously set DM_MULTIPATH_DEVICE_PATH
> environment variable.
> 
> This requires to change the exit status of multipath -u - it needs to
> exit with status 0 even if the path is not a multipath device path,
> otherwise udev doesn't import the printed key-value pairs. We do this
> only for "multipath -u"; legacy "multipath -c", which is more likely
> to be run in user scripts, still exits with 1 for non-multipath
> devices.
> 
> The condition ENV{DM_MULTIPATH_DEVICE_PATH}!="1" before the
> "multipath -u" statement in multipath.rules needs to be removed. This
> condition was pointless anyway, because until this patch,
> DM_MULTIPATH_DEVICE_PATH hadn't been imported from the db and thus
> was never set, and "multipath -u" was always invoked. We want to keep
> this behavior.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipath/main.c          | 46
> +++++++++++++++++++++++++++++++++++++++++++++-
> multipath/multipath.rules |  5 ++++- 2 files changed, 49
> insertions(+), 2 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes

Re: [PATCH v5 19/22] multipath -u: don't grab devices already passed to system

[Benjamin Marzinski]

On Sat, Apr 14, 2018 at 12:00:12AM +0200, Martin Wilck wrote:
> Setting SYSTEMD_READY=0 on a device that has previously been passed to
> systemd is dangerous - already mounted file systems might be unmounted by
> systemd.
> 
> Avoid that by checking for previously set DM_MULTIPATH_DEVICE_PATH
> environment variable.
> 
> This requires to change the exit status of multipath -u - it needs to exit
> with status 0 even if the path is not a multipath device path, otherwise
> udev doesn't import the printed key-value pairs. We do this only for
> "multipath -u"; legacy "multipath -c", which is more likely to be run in user
> scripts, still exits with 1 for non-multipath devices.
> 
> The condition ENV{DM_MULTIPATH_DEVICE_PATH}!="1" before the "multipath -u"
> statement in multipath.rules needs to be removed. This condition was
> pointless anyway, because until this patch, DM_MULTIPATH_DEVICE_PATH hadn't
> been imported from the db and thus was never set, and "multipath -u" was
> always invoked. We want to keep this behavior.
> 

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipath/main.c          | 46 +++++++++++++++++++++++++++++++++++++++++++++-
>  multipath/multipath.rules |  5 ++++-
>  2 files changed, 49 insertions(+), 2 deletions(-)
> 
> diff --git a/multipath/main.c b/multipath/main.c
> index 96e37a7a..573d94f9 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -25,6 +25,7 @@
>  #include <sys/types.h>
>  #include <sys/stat.h>
>  #include <stdio.h>
> +#include <stdlib.h>
>  #include <unistd.h>
>  #include <ctype.h>
>  #include <libudev.h>
> @@ -494,6 +495,25 @@ static int print_cmd_valid(int k, const vector pathvec,
>  	return k == 1;
>  }
>  
> +/*
> + * Returns true if this device has been handled before,
> + * and released to systemd.
> + *
> + * This must be called before get_refwwid(),
> + * otherwise udev_device_new_from_environment() will have
> + * destroyed environ(7).
> + */
> +static bool released_to_systemd(void)
> +{
> +	static const char dmdp[] = "DM_MULTIPATH_DEVICE_PATH";
> +	const char *dm_mp_dev_path = getenv(dmdp);
> +	bool ret;
> +
> +	ret = dm_mp_dev_path != NULL && !strcmp(dm_mp_dev_path, "0");
> +	condlog(4, "%s: %s=%s -> %d", __func__, dmdp, dm_mp_dev_path, ret);
> +	return ret;
> +}
> +
>  /*
>   * Return value:
>   *  -1: Retry
> @@ -511,6 +531,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  	int di_flag = 0;
>  	char * refwwid = NULL;
>  	char * dev = NULL;
> +	bool released = released_to_systemd();
>  
>  	/*
>  	 * allocate core vectors to store paths and multipaths
> @@ -602,6 +623,20 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  				r = 0;
>  				goto print_valid;
>  			}
> +
> +			/*
> +			 * DM_MULTIPATH_DEVICE_PATH=="0" means that we have
> +			 * been called for this device already, and have
> +			 * released it to systemd. Unless the device is now
> +			 * already multipathed (see above), we can't try to
> +			 * grab it, because setting SYSTEMD_READY=0 would
> +			 * cause file systems to be unmounted.
> +			 * Leave DM_MULTIPATH_DEVICE_PATH="0".
> +			 */
> +			if (released) {
> +				r = 1;
> +				goto print_valid;
> +			}
>  			if (r == 0)
>  				goto print_valid;
>  			/* find_multipaths_on: Fall through to path detection */
> @@ -641,7 +676,9 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  
>  	if (cmd == CMD_VALID_PATH) {
>  		/* This only happens if find_multipaths and
> -		 * ignore_wwids is set.
> +		 * ignore_wwids is set, and the path is not in WWIDs
> +		 * file, not currently multipathed, and has
> +		 * never been released to systemd.
>  		 * If there is currently a multipath device matching
>  		 * the refwwid, or there is more than one path matching
>  		 * the refwwid, then the path is valid */
> @@ -1064,6 +1101,13 @@ out:
>  	cleanup_prio();
>  	cleanup_checkers();
>  
> +	/*
> +	 * multipath -u must exit with status 0, otherwise udev won't
> +	 * import its output.
> +	 */
> +	if (cmd == CMD_VALID_PATH && dev_type == DEV_UEVENT && r == 1)
> +		r = 0;
> +
>  	if (dev_type == DEV_UEVENT)
>  		closelog();
>  
> diff --git a/multipath/multipath.rules b/multipath/multipath.rules
> index 37f4f6d8..ef857271 100644
> --- a/multipath/multipath.rules
> +++ b/multipath/multipath.rules
> @@ -21,8 +21,11 @@ LABEL="test_dev"
>  ENV{MPATH_SBIN_PATH}="/sbin"
>  TEST!="$env{MPATH_SBIN_PATH}/multipath", ENV{MPATH_SBIN_PATH}="/usr/sbin"
>  
> +# multipath -u needs to know if this device has ever been exported
> +IMPORT{db}="DM_MULTIPATH_DEVICE_PATH"
> +
>  # multipath -u sets DM_MULTIPATH_DEVICE_PATH
> -ENV{DM_MULTIPATH_DEVICE_PATH}!="1", IMPORT{program}="$env{MPATH_SBIN_PATH}/multipath -u %k"
> +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"
>  
> -- 
> 2.16.1

[PATCH v5 20/22] multipath -u: test if path is busy

[Martin Wilck]

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

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

diff --git a/multipath/main.c b/multipath/main.c
index 573d94f9..c69e996e 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -675,6 +675,9 @@ configure (struct config *conf, enum mpath_cmds cmd,
 
 
 	if (cmd == CMD_VALID_PATH) {
+		struct path *pp;
+		int fd;
+
 		/* This only happens if find_multipaths and
 		 * ignore_wwids is set, and the path is not in WWIDs
 		 * file, not currently multipathed, and has
@@ -682,11 +685,45 @@ configure (struct config *conf, enum mpath_cmds cmd,
 		 * If there is currently a multipath device matching
 		 * the refwwid, or there is more than one path matching
 		 * the refwwid, then the path is valid */
-		if (VECTOR_SIZE(curmp) != 0 || VECTOR_SIZE(pathvec) > 1)
+		if (VECTOR_SIZE(curmp) != 0) {
+			r = 0;
+			goto print_valid;
+		} else if (VECTOR_SIZE(pathvec) > 1)
 			r = 0;
 		else
 			/* Use r=2 as an indication for "maybe" */
 			r = 2;
+
+		/*
+		 * If opening the path with O_EXCL fails, the path
+		 * is in use (e.g. mounted during initramfs processing).
+		 * We know that it's not used by dm-multipath.
+		 * We may not set SYSTEMD_READY=0 on such devices, it
+		 * might cause systemd to umount the device.
+		 * Use O_RDONLY, because udevd would trigger another
+		 * uevent for close-after-write.
+		 *
+		 * The O_EXCL check is potentially dangerous, because it may
+		 * race with other tasks trying to access the device. Therefore
+		 * this code is only executed if the path hasn't been released
+		 * to systemd earlier (see above).
+		 *
+		 * get_refwwid() above stores the path we examine in slot 0.
+		 */
+		pp = VECTOR_SLOT(pathvec, 0);
+		fd = open(udev_device_get_devnode(pp->udev),
+			  O_RDONLY|O_EXCL);
+		if (fd >= 0)
+			close(fd);
+		else {
+			condlog(3, "%s: path %s is in use: %s",
+				__func__, pp->dev,
+				strerror(errno));
+			/*
+			 * Check if we raced with multipathd
+			 */
+			r = !sysfs_is_multipathed(VECTOR_SLOT(pathvec, 0));
+		}
 		goto print_valid;
 	}
 
-- 
2.16.1

Re: [PATCH v5 20/22] multipath -u: test if path is busy

[Hannes Reinecke]

On Sat, 14 Apr 2018 00:00:13 +0200
Martin Wilck <mwilck@suse.com> wrote:

> For "find_multipaths smart", check if a path is already in use
> before setting DM_MULTIPATH_DEVICE_PATH to 1 or 2 (and thus,
> SYSTEMD_READY=0). If we don't do this, a device which has already been
> mounted (e.g. during initrd processing) may be unmounted by systemd,
> causing havoc to the boot process.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipath/main.c | 39 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/multipath/main.c b/multipath/main.c
> index 573d94f9..c69e996e 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -675,6 +675,9 @@ configure (struct config *conf, enum mpath_cmds
> cmd, 
>  
>  	if (cmd == CMD_VALID_PATH) {
> +		struct path *pp;
> +		int fd;
> +
>  		/* This only happens if find_multipaths and
>  		 * ignore_wwids is set, and the path is not in WWIDs
>  		 * file, not currently multipathed, and has
> @@ -682,11 +685,45 @@ configure (struct config *conf, enum mpath_cmds
> cmd,
>  		 * If there is currently a multipath device matching
>  		 * the refwwid, or there is more than one path
> matching
>  		 * the refwwid, then the path is valid */
> -		if (VECTOR_SIZE(curmp) != 0 || VECTOR_SIZE(pathvec)
> > 1)
> +		if (VECTOR_SIZE(curmp) != 0) {
> +			r = 0;
> +			goto print_valid;
> +		} else if (VECTOR_SIZE(pathvec) > 1)
>  			r = 0;
>  		else
>  			/* Use r=2 as an indication for "maybe" */
>  			r = 2;
> +
> +		/*
> +		 * If opening the path with O_EXCL fails, the path
> +		 * is in use (e.g. mounted during initramfs
> processing).
> +		 * We know that it's not used by dm-multipath.
> +		 * We may not set SYSTEMD_READY=0 on such devices, it
> +		 * might cause systemd to umount the device.
> +		 * Use O_RDONLY, because udevd would trigger another
> +		 * uevent for close-after-write.
> +		 *
> +		 * The O_EXCL check is potentially dangerous,
> because it may
> +		 * race with other tasks trying to access the
> device. Therefore
> +		 * this code is only executed if the path hasn't
> been released
> +		 * to systemd earlier (see above).
> +		 *
> +		 * get_refwwid() above stores the path we examine in
> slot 0.
> +		 */
> +		pp = VECTOR_SLOT(pathvec, 0);
> +		fd = open(udev_device_get_devnode(pp->udev),
> +			  O_RDONLY|O_EXCL);
> +		if (fd >= 0)
> +			close(fd);
> +		else {
> +			condlog(3, "%s: path %s is in use: %s",
> +				__func__, pp->dev,
> +				strerror(errno));
> +			/*
> +			 * Check if we raced with multipathd
> +			 */
> +			r
> = !sysfs_is_multipathed(VECTOR_SLOT(pathvec, 0));
> +		}
>  		goto print_valid;
>  	}
>  

But doesn't this cause udev to generate another event upon release?
Or is that prevented by the O_RDONLY setting?


Cheers,

Hannes

Re: [PATCH v5 20/22] multipath -u: test if path is busy

[Martin Wilck]

On Mon, 2018-04-16 at 08:32 +0200, Hannes Reinecke wrote:
> 
> But doesn't this cause udev to generate another event upon release?
> Or is that prevented by the O_RDONLY setting?

Exactly, udev triggers only on "close after write" events.

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 v5 20/22] multipath -u: test if path is busy

[Benjamin Marzinski]

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

Reviewed-by: Benjamin Marzinsk <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipath/main.c | 39 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/multipath/main.c b/multipath/main.c
> index 573d94f9..c69e996e 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -675,6 +675,9 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  
>  
>  	if (cmd == CMD_VALID_PATH) {
> +		struct path *pp;
> +		int fd;
> +
>  		/* This only happens if find_multipaths and
>  		 * ignore_wwids is set, and the path is not in WWIDs
>  		 * file, not currently multipathed, and has
> @@ -682,11 +685,45 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  		 * If there is currently a multipath device matching
>  		 * the refwwid, or there is more than one path matching
>  		 * the refwwid, then the path is valid */
> -		if (VECTOR_SIZE(curmp) != 0 || VECTOR_SIZE(pathvec) > 1)
> +		if (VECTOR_SIZE(curmp) != 0) {
> +			r = 0;
> +			goto print_valid;
> +		} else if (VECTOR_SIZE(pathvec) > 1)
>  			r = 0;
>  		else
>  			/* Use r=2 as an indication for "maybe" */
>  			r = 2;
> +
> +		/*
> +		 * If opening the path with O_EXCL fails, the path
> +		 * is in use (e.g. mounted during initramfs processing).
> +		 * We know that it's not used by dm-multipath.
> +		 * We may not set SYSTEMD_READY=0 on such devices, it
> +		 * might cause systemd to umount the device.
> +		 * Use O_RDONLY, because udevd would trigger another
> +		 * uevent for close-after-write.
> +		 *
> +		 * The O_EXCL check is potentially dangerous, because it may
> +		 * race with other tasks trying to access the device. Therefore
> +		 * this code is only executed if the path hasn't been released
> +		 * to systemd earlier (see above).
> +		 *
> +		 * get_refwwid() above stores the path we examine in slot 0.
> +		 */
> +		pp = VECTOR_SLOT(pathvec, 0);
> +		fd = open(udev_device_get_devnode(pp->udev),
> +			  O_RDONLY|O_EXCL);
> +		if (fd >= 0)
> +			close(fd);
> +		else {
> +			condlog(3, "%s: path %s is in use: %s",
> +				__func__, pp->dev,
> +				strerror(errno));
> +			/*
> +			 * Check if we raced with multipathd
> +			 */
> +			r = !sysfs_is_multipathed(VECTOR_SLOT(pathvec, 0));
> +		}
>  		goto print_valid;
>  	}
>  
> -- 
> 2.16.1

[PATCH v5 21/22] 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.

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

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index f17f8ec0..5c54f9b4 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 eda3f1f0..40406116 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -245,6 +245,7 @@ static const char * const 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 3a34aef3..f6897954 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 v5 21/22] libmultipath: enable find_multipaths "smart"

[Hannes Reinecke]

On Sat, 14 Apr 2018 00:00:14 +0200
Martin Wilck <mwilck@suse.com> wrote:

> This activates "smart" path detection. This is similar to
> "find_multipaths yes", but doesn't generally ignore single paths
> that are not listed in the WWIDs file. Rather, such paths are
> temporarily treated like multipath members. If no additional paths
> are detected after a certain time, the paths are re-added to the
> system as non-multipath. This needs support by the udev rules, to
> be added in a follow-up patch.
> 
> If a multipath map is successfully successfully created, and paths are
> in waiting state, trigger path uevents to update their status.
> 
> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/configure.c   | 15 ++++++++++++---
>  libmultipath/dict.c        |  1 +
>  multipath/multipath.conf.5 | 13 +++++++++++++
>  3 files changed, 26 insertions(+), 3 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes

[PATCH v5 22/22] 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>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipath/multipath.rules | 61 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 58 insertions(+), 3 deletions(-)

diff --git a/multipath/multipath.rules b/multipath/multipath.rules
index ef857271..d658073f 100644
--- a/multipath/multipath.rules
+++ b/multipath/multipath.rules
@@ -21,12 +21,67 @@ LABEL="test_dev"
 ENV{MPATH_SBIN_PATH}="/sbin"
 TEST!="$env{MPATH_SBIN_PATH}/multipath", ENV{MPATH_SBIN_PATH}="/usr/sbin"
 
+# 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 needs to know if this device has ever been exported
 IMPORT{db}="DM_MULTIPATH_DEVICE_PATH"
 
-# multipath -u sets DM_MULTIPATH_DEVICE_PATH
+# multipath -u sets DM_MULTIPATH_DEVICE_PATH and,
+# if "find_multipaths smart", also FIND_MULTIPATHS_WAIT_UNTIL.
 IMPORT{program}="$env{MPATH_SBIN_PATH}/multipath -u %k"
-ENV{DM_MULTIPATH_DEVICE_PATH}=="1", ENV{ID_FS_TYPE}="mpath_member", \
-	ENV{SYSTEMD_READY}="0"
+
+# case 1: this is definitely multipath
+ENV{DM_MULTIPATH_DEVICE_PATH}=="1", \
+	ENV{ID_FS_TYPE}="mpath_member", ENV{SYSTEMD_READY}="0", \
+	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 v5 22/22] multipath.rules: find_multipaths "smart" logic

[Hannes Reinecke]

On Sat, 14 Apr 2018 00:00:15 +0200
Martin Wilck <mwilck@suse.com> 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.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  multipath/multipath.rules | 61
> ++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 58
> insertions(+), 3 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes