[PATCH v2 00/24] multipath-tools: improve logging at -v3

[PATCH v2 00/24] multipath-tools: improve logging at -v3

[Martin Wilck]

Hi Christophe,

most of the patches in this series reduce log levels of frequently
printed messages at verbosity level 3. My goal was to limit the
output of multipathd to one line per path per checker invocation,
which is sufficient to track multipathd's view of path health in
the logs.

The standard setting of -v2 is not enough for post-mortem analysis of many
failures. With this series, running multipathd with verbosity 3 becomes a
realistic option even in production environments. So far the amount of output
from multipathd with -v3 pretty much made this impossible, at least over
longer time periods, and also made reading these logs very cumbersome due to
the amount of redundant partly superfluos verbosity. I've taken care not
to loose important information in the logs.

Apart from that, the series fixes errors in the unit tests introduced by my
last "checker overhaul" patch series (proving that I forgot to run the
tests before submitting :-( ), and fixes a problem that I found while testing
handling of a bad configuration (paths with size mismatch).

Regards,
Martin

Changes in v2:

The first 19 patches are identical to v1 as ACK'd by Ben, except 16/24
"libmultipath: coalesce_paths: fix size mismatch handling".
No. 8/24 "libmultipath: decrease log level of word splitting"
(not yet ACKd by Ben) also stays the same; the issue Ben raised
in his review is addressed in a separate patch, 20/24.
21/24 addresses implements Ben's suggestion to use named constants
as return values in coalesce_paths(). 22, 23, 24 do the same for
other important, related functions, as I found it strange to make
this change just for coalesce_paths() alone.  

Martin Wilck (24):
  tests/hwtable: set multipath_dir in local configuration
  tests/hwtable: adjust to new checker API
  multipath-tools: decrease verbosity of state messages
  libmultipath: decrease verbosity of pathinfo messages
  libmultipath: decrease verbosity of TUR checker messages
  libmultipath: avoid frequent messages from filter_property()
  libmultipath: decrease log level of "disassembled" messages
  libmultipath: decrease log level of word splitting
  libmultipath: increase log level of map removal
  multipathd: decrease log level of checker timing
  libmultipath: decrease log level of "prioritizer refcount" message
  libmpathpersist/update_map_pr: decrease log level for nop
  libmultipath: simplify devt2devname()
  libmultipath: decrease log level for failed VPD c9
  libmultipath: adopt_paths: check for size match
  libmultipath: coalesce_paths: fix size mismatch handling
  tests: add unit tests for bitmask functions
  multipathd: uev_remove_path: remove redundant orphan_paths call
  libmultipath: improve logging from orphan_paths
  libmultipath: avoid syslog loglevel > LOG_DEBUG
  coalesce_paths(): use symbolic return value
  domap(): use symbolic return value
  domap(): never return DOMAP_RETRY in daemon mode
  multipath: use symbolic return value and exit code

 libmpathpersist/mpath_persist.c |   3 +-
 libmultipath/blacklist.c        |  54 +++++++-------
 libmultipath/blacklist.h        |   2 +-
 libmultipath/checkers/tur.c     |   6 +-
 libmultipath/configure.c        |  68 +++++++++---------
 libmultipath/configure.h        |  23 ++++++
 libmultipath/discovery.c        |  20 +++---
 libmultipath/dmparser.c         |   6 +-
 libmultipath/log_pthread.c      |   3 +
 libmultipath/prio.c             |   2 +-
 libmultipath/structs_vec.c      |  18 +++--
 libmultipath/structs_vec.h      |   3 +-
 libmultipath/util.c             |   7 +-
 libmultipath/util.h             |  16 +++++
 multipath/main.c                | 121 ++++++++++++++++++--------------
 multipathd/cli_handlers.c       |   5 +-
 multipathd/main.c               |  39 +++++-----
 tests/Makefile                  |   7 +-
 tests/blacklist.c               |   7 +-
 tests/hwtable.c                 |  89 ++++++++++++-----------
 tests/util.c                    |  98 ++++++++++++++++++++++++++
 21 files changed, 386 insertions(+), 211 deletions(-)

-- 
2.19.1

[PATCH v2 16/24] libmultipath: coalesce_paths: fix size mismatch handling

[Martin Wilck]

When there are paths with the same WWID but different sizes, and
coalesce_paths() walks the pathvec, it checks paths _after_
the current one for size mismatch and sets ACT_REJECT. However,
these paths will be reached in the main loop later, and this time
the already handled paths will not be checked for size mismatch;
thus a map could be created, possibly even with mismatching
devices.

Fix that by tracking which paths were already discarded, and
skipping them in the main loop later.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c | 32 ++++++++++++++++++++++++--------
 libmultipath/util.h      | 16 ++++++++++++++++
 2 files changed, 40 insertions(+), 8 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 406cd4c9..f48664a0 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -1009,6 +1009,7 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
 	vector pathvec = vecs->pathvec;
 	struct config *conf;
 	int allow_queueing;
+	uint64_t *size_mismatch_seen;
 
 	/* ignore refwwid if it's empty */
 	if (refwwid && !strlen(refwwid))
@@ -1019,6 +1020,14 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
 			pp1->mpp = NULL;
 		}
 	}
+
+	if (VECTOR_SIZE(pathvec) == 0)
+		return 0;
+	size_mismatch_seen = calloc((VECTOR_SIZE(pathvec) - 1) / 64 + 1,
+				    sizeof(uint64_t));
+	if (size_mismatch_seen == NULL)
+		return 1;
+
 	vector_foreach_slot (pathvec, pp1, k) {
 		int invalid;
 		/* skip this path for some reason */
@@ -1038,8 +1047,8 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
 			continue;
 		}
 
-		/* 2. if path already coalesced */
-		if (pp1->mpp)
+		/* 2. if path already coalesced, or seen and discarded */
+		if (pp1->mpp || is_bit_set_in_array(k, size_mismatch_seen))
 			continue;
 
 		/* 3. if path has disappeared */
@@ -1088,9 +1097,10 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
 				 * ouch, avoid feeding that to the DM
 				 */
 				condlog(0, "%s: size %llu, expected %llu. "
-					"Discard", pp2->dev_t, pp2->size,
+					"Discard", pp2->dev, pp2->size,
 					mpp->size);
 				mpp->action = ACT_REJECT;
+				set_bit_in_array(i, size_mismatch_seen);
 			}
 		}
 		verify_paths(mpp, vecs);
@@ -1119,8 +1129,9 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
 					"ignoring" : "removing");
 				remove_map(mpp, vecs, 0);
 				continue;
-			} else /* if (r == DOMAP_RETRY) */
-				return r;
+			} else /* if (r == DOMAP_RETRY && !is_daemon) */ {
+				goto out;
+			}
 		}
 		if (r == DOMAP_DRY)
 			continue;
@@ -1161,8 +1172,10 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
 
 		if (newmp) {
 			if (mpp->action != ACT_REJECT) {
-				if (!vector_alloc_slot(newmp))
-					return 1;
+				if (!vector_alloc_slot(newmp)) {
+					r = 1;
+					goto out;
+				}
 				vector_set_slot(newmp, mpp);
 			}
 			else
@@ -1193,7 +1206,10 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
 				condlog(2, "%s: remove (dead)", alias);
 		}
 	}
-	return 0;
+	r = 0;
+out:
+	free(size_mismatch_seen);
+	return r;
 }
 
 struct udev_device *get_udev_device(const char *dev, enum devtypes dev_type)
diff --git a/libmultipath/util.h b/libmultipath/util.h
index a818e29a..1f13c913 100644
--- a/libmultipath/util.h
+++ b/libmultipath/util.h
@@ -3,6 +3,7 @@
 
 #include <sys/types.h>
 #include <inttypes.h>
+#include <stdbool.h>
 
 size_t strchop(char *);
 int basenamecpy (const char *src, char *dst, size_t size);
@@ -39,4 +40,19 @@ struct scandir_result {
 };
 void free_scandir_result(struct scandir_result *);
 
+static inline bool is_bit_set_in_array(unsigned int bit, const uint64_t *arr)
+{
+	return arr[bit / 64] & (1ULL << (bit % 64)) ? 1 : 0;
+}
+
+static inline void set_bit_in_array(unsigned int bit, uint64_t *arr)
+{
+	arr[bit / 64] |= (1ULL << (bit % 64));
+}
+
+static inline void clear_bit_in_array(unsigned int bit, uint64_t *arr)
+{
+	arr[bit / 64] &= ~(1ULL << (bit % 64));
+}
+
 #endif /* _UTIL_H */
-- 
2.19.1

[PATCH v2 20/24] libmultipath: avoid syslog loglevel > LOG_DEBUG

[Martin Wilck]

We use condlog(5, ...) in some places, which doesn't work well
with syslog.

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

diff --git a/libmultipath/log_pthread.c b/libmultipath/log_pthread.c
index bb35dfc7..be57bb1a 100644
--- a/libmultipath/log_pthread.c
+++ b/libmultipath/log_pthread.c
@@ -25,6 +25,9 @@ static int log_messages_pending;
 
 void log_safe (int prio, const char * fmt, va_list ap)
 {
+	if (prio > LOG_DEBUG)
+		prio = LOG_DEBUG;
+
 	if (log_thr == (pthread_t)0) {
 		vsyslog(prio, fmt, ap);
 		return;
-- 
2.19.1

[PATCH v2 21/24] coalesce_paths(): use symbolic return value

[Martin Wilck]

Use an enum to represent the return value of coalesce_paths() to
improve readability of the code.
This patch doesn't introduce changes in behavior.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c  | 17 ++++++++---------
 libmultipath/configure.h  | 10 ++++++++++
 multipath/main.c          |  5 +++--
 multipathd/cli_handlers.c |  3 ++-
 multipathd/main.c         |  2 +-
 5 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index f48664a0..5daf0c13 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -998,8 +998,8 @@ out:
 int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
 		    int force_reload, enum mpath_cmds cmd)
 {
-	int r = 1;
-	int k, i;
+	int ret = CP_FAIL;
+	int k, i, r;
 	int is_daemon = (cmd == CMD_NONE) ? 1 : 0;
 	char params[PARAMS_SIZE];
 	struct multipath * mpp;
@@ -1022,11 +1022,11 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
 	}
 
 	if (VECTOR_SIZE(pathvec) == 0)
-		return 0;
+		return CP_OK;
 	size_mismatch_seen = calloc((VECTOR_SIZE(pathvec) - 1) / 64 + 1,
 				    sizeof(uint64_t));
 	if (size_mismatch_seen == NULL)
-		return 1;
+		return CP_FAIL;
 
 	vector_foreach_slot (pathvec, pp1, k) {
 		int invalid;
@@ -1130,6 +1130,7 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
 				remove_map(mpp, vecs, 0);
 				continue;
 			} else /* if (r == DOMAP_RETRY && !is_daemon) */ {
+				ret = CP_RETRY;
 				goto out;
 			}
 		}
@@ -1172,10 +1173,8 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
 
 		if (newmp) {
 			if (mpp->action != ACT_REJECT) {
-				if (!vector_alloc_slot(newmp)) {
-					r = 1;
+				if (!vector_alloc_slot(newmp))
 					goto out;
-				}
 				vector_set_slot(newmp, mpp);
 			}
 			else
@@ -1206,10 +1205,10 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
 				condlog(2, "%s: remove (dead)", alias);
 		}
 	}
-	r = 0;
+	ret = CP_OK;
 out:
 	free(size_mismatch_seen);
-	return r;
+	return ret;
 }
 
 struct udev_device *get_udev_device(const char *dev, enum devtypes dev_type)
diff --git a/libmultipath/configure.h b/libmultipath/configure.h
index 8b56d33a..64520c57 100644
--- a/libmultipath/configure.h
+++ b/libmultipath/configure.h
@@ -23,6 +23,16 @@ enum actions {
 	ACT_IMPOSSIBLE,
 };
 
+/*
+ * Return value of coalesce_paths()
+ * CP_RETRY is only used in non-daemon case (multipath).
+ */
+enum {
+	CP_OK = 0,
+	CP_FAIL,
+	CP_RETRY,
+};
+
 #define FLUSH_ONE 1
 #define FLUSH_ALL 2
 
diff --git a/multipath/main.c b/multipath/main.c
index 05b7bf0c..eb087482 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -537,7 +537,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
 	vector curmp = NULL;
 	vector pathvec = NULL;
 	struct vectors vecs;
-	int r = 1;
+	int r = 1, rc;
 	int di_flag = 0;
 	char * refwwid = NULL;
 	char * dev = NULL;
@@ -752,8 +752,9 @@ configure (struct config *conf, enum mpath_cmds cmd,
 	/*
 	 * core logic entry point
 	 */
-	r = coalesce_paths(&vecs, NULL, refwwid,
+	rc = coalesce_paths(&vecs, NULL, refwwid,
 			   conf->force_reload, cmd);
+	r = rc == CP_RETRY ? -1 : rc == CP_OK ? 0 : 1;
 
 print_valid:
 	if (cmd == CMD_VALID_PATH)
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index a0d57a53..4fbd8841 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -803,7 +803,8 @@ cli_add_map (void * v, char ** reply, int * len, void * data)
 				    vecs->pathvec, &refwwid);
 			if (refwwid) {
 				if (coalesce_paths(vecs, NULL, refwwid,
-						   FORCE_RELOAD_NONE, CMD_NONE))
+						   FORCE_RELOAD_NONE, CMD_NONE)
+				    != CP_OK)
 					condlog(2, "%s: coalesce_paths failed",
 									param);
 				dm_lib_release();
diff --git a/multipathd/main.c b/multipathd/main.c
index 77126f9e..2ea954ae 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2371,7 +2371,7 @@ configure (struct vectors * vecs)
 	ret = coalesce_paths(vecs, mpvec, NULL, force_reload, CMD_NONE);
 	if (force_reload == FORCE_RELOAD_WEAK)
 		force_reload = FORCE_RELOAD_YES;
-	if (ret) {
+	if (ret != CP_OK) {
 		condlog(0, "configure failed while coalescing paths");
 		goto fail;
 	}
-- 
2.19.1

[PATCH v2 22/24] domap(): use symbolic return value

[Martin Wilck]

Use an enum for the already-symbolic return value of domap(), and
avoid the use of "<= 0" for the return value, which is against the
spirit of symbolic values. A return value less or equal than 0 means
DOMAP_FAIL or DOMAP_RETRY. Use the fact that DOMAP_RETRY is only
returned in the ACT_CREATE case for simplification of the logic
in those cases where ACT_CREATE is not used.

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

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 5daf0c13..84ae5f56 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -788,15 +788,6 @@ fail:
 	return 1;
 }
 
-/*
- * Return value:
- */
-#define DOMAP_RETRY	-1
-#define DOMAP_FAIL	0
-#define DOMAP_OK	1
-#define DOMAP_EXIST	2
-#define DOMAP_DRY	3
-
 int domap(struct multipath *mpp, char *params, int is_daemon)
 {
 	int r = DOMAP_FAIL;
diff --git a/libmultipath/configure.h b/libmultipath/configure.h
index 64520c57..1b73bf42 100644
--- a/libmultipath/configure.h
+++ b/libmultipath/configure.h
@@ -23,6 +23,19 @@ enum actions {
 	ACT_IMPOSSIBLE,
 };
 
+/*
+ * Return value of domap()
+ * DAEMON_RETRY is only used in non-daemon case (multipath),
+ * and only for ACT_CREATE (see domap()).
+ */
+enum {
+	DOMAP_RETRY	= -1,
+	DOMAP_FAIL	= 0,
+	DOMAP_OK	= 1,
+	DOMAP_EXIST	= 2,
+	DOMAP_DRY	= 3
+};
+
 /*
  * Return value of coalesce_paths()
  * CP_RETRY is only used in non-daemon case (multipath).
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 4fbd8841..14aec17b 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -893,7 +893,7 @@ int resize_map(struct multipath *mpp, unsigned long long size,
 	}
 	mpp->action = ACT_RESIZE;
 	mpp->force_udev_reload = 1;
-	if (domap(mpp, params, 1) <= 0) {
+	if (domap(mpp, params, 1) == DOMAP_FAIL) {
 		condlog(0, "%s: failed to resize map : %s", mpp->alias,
 			strerror(errno));
 		mpp->size = orig_size;
diff --git a/multipathd/main.c b/multipathd/main.c
index 2ea954ae..d0e7107c 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -498,7 +498,7 @@ retry:
 		retries = -1;
 		goto fail;
 	}
-	if (domap(mpp, params, 1) <= 0 && retries-- > 0) {
+	if (domap(mpp, params, 1) == DOMAP_FAIL && retries-- > 0) {
 		condlog(0, "%s: map_udate sleep", mpp->alias);
 		sleep(1);
 		goto retry;
@@ -1000,7 +1000,7 @@ rescan:
 	 */
 retry:
 	ret = domap(mpp, params, 1);
-	if (ret <= 0) {
+	if (ret == DOMAP_FAIL || ret == DOMAP_RETRY) {
 		if (ret < 0 && retries-- > 0) {
 			condlog(0, "%s: retry domap for addition of new "
 				"path %s", mpp->alias, pp->dev);
@@ -1157,7 +1157,7 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
 		 * reload the map
 		 */
 		mpp->action = ACT_RELOAD;
-		if (domap(mpp, params, 1) <= 0) {
+		if (domap(mpp, params, 1) == DOMAP_FAIL) {
 			condlog(0, "%s: failed in domap for "
 				"removal of path %s",
 				mpp->alias, pp->dev);
-- 
2.19.1

[PATCH v2 23/24] domap(): never return DOMAP_RETRY in daemon mode

[Martin Wilck]

DOMAP_RETRY is only used by the multipath tool. multipathd always treats
it exactly like DOMAP_FAIL. Simplify logic by only returning
DOMAP_RETRY in non-daemon mode from domap().

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

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 84ae5f56..1c549817 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -831,7 +831,7 @@ int domap(struct multipath *mpp, char *params, int is_daemon)
 		if (lock_multipath(mpp, 1)) {
 			condlog(3, "%s: failed to create map (in use)",
 				mpp->alias);
-			return DOMAP_RETRY;
+			return is_daemon ? DOMAP_FAIL : DOMAP_RETRY;
 		}
 
 		sysfs_set_max_sectors_kb(mpp, 0);
@@ -1110,20 +1110,18 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
 
 		r = domap(mpp, params, is_daemon);
 
-		if (r == DOMAP_FAIL || r == DOMAP_RETRY) {
-			condlog(3, "%s: domap (%u) failure "
-				   "for create/reload map",
-				mpp->alias, r);
-			if (r == DOMAP_FAIL || is_daemon) {
-				condlog(2, "%s: %s map",
-					mpp->alias, (mpp->action == ACT_CREATE)?
-					"ignoring" : "removing");
-				remove_map(mpp, vecs, 0);
-				continue;
-			} else /* if (r == DOMAP_RETRY && !is_daemon) */ {
-				ret = CP_RETRY;
-				goto out;
-			}
+		if (r == DOMAP_RETRY) {
+			/* This happens in non-daemon case only */
+			ret = CP_RETRY;
+			goto out;
+		}
+
+		if (r == DOMAP_FAIL) {
+			condlog(2, "%s: domap failure, %s map",
+				mpp->alias, (mpp->action == ACT_CREATE) ?
+				"ignoring" : "removing");
+			remove_map(mpp, vecs, 0);
+			continue;
 		}
 		if (r == DOMAP_DRY)
 			continue;
diff --git a/multipathd/main.c b/multipathd/main.c
index d0e7107c..1bf3c481 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -998,15 +998,8 @@ rescan:
 	/*
 	 * reload the map for the multipath mapped device
 	 */
-retry:
 	ret = domap(mpp, params, 1);
-	if (ret == DOMAP_FAIL || ret == DOMAP_RETRY) {
-		if (ret < 0 && retries-- > 0) {
-			condlog(0, "%s: retry domap for addition of new "
-				"path %s", mpp->alias, pp->dev);
-			sleep(1);
-			goto retry;
-		}
+	if (ret == DOMAP_FAIL) {
 		condlog(0, "%s: failed in domap for addition of new "
 			"path %s", mpp->alias, pp->dev);
 		/*
-- 
2.19.1

[PATCH v2 24/24] multipath: use symbolic return value and exit code

[Martin Wilck]

Use an enum to represent the return code and exit status of
multipath and its most important subroutine, configure().
This clarifies the confusing use of booleans and status
codes a bit, hopefully.

This patch doesn't introduce a change in behavior.

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

diff --git a/multipath/main.c b/multipath/main.c
index eb087482..e437746d 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -68,6 +68,19 @@ int logsink;
 struct udev *udev;
 struct config *multipath_conf;
 
+/*
+ * Return values of configure(), print_cmd_valid(), and main().
+ * RTVL_{YES,NO} are synonyms for RTVL_{OK,FAIL} for the CMD_VALID_PATH case.
+ */
+enum {
+	RTVL_OK = 0,
+	RTVL_YES = RTVL_OK,
+	RTVL_FAIL = 1,
+	RTVL_NO = RTVL_FAIL,
+	RTVL_MAYBE, /* only used internally, never returned */
+	RTVL_RETRY, /* returned by configure(), not by main() */
+};
+
 struct config *get_multipath_config(void)
 {
 	return multipath_conf;
@@ -475,15 +488,14 @@ retry:
 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) / sizeof(int)))
-		return 1;
+	if (k != RTVL_YES && k != RTVL_NO && k != RTVL_MAYBE)
+		return RTVL_NO;
 
-	if (k == 2) {
+	if (k == RTVL_MAYBE) {
 		/*
 		 * Caller ensures that pathvec[0] is the path to
 		 * examine.
@@ -493,7 +505,7 @@ static int print_cmd_valid(int k, const vector pathvec,
 		wait = find_multipaths_check_timeout(
 			pp, pp->find_multipaths_timeout, &until);
 		if (wait != FIND_MULTIPATHS_WAITING)
-			k = 1;
+			k = RTVL_NO;
 	} else if (pathvec != NULL && (pp = VECTOR_SLOT(pathvec, 0)))
 		wait = find_multipaths_check_timeout(pp, 0, &until);
 	if (wait == FIND_MULTIPATHS_WAITING)
@@ -501,8 +513,10 @@ static int print_cmd_valid(int k, const vector pathvec,
 			       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;
+	printf("DM_MULTIPATH_DEVICE_PATH=\"%d\"\n",
+	       k == RTVL_MAYBE ? 2 : k == RTVL_YES ? 1 : 0);
+	/* Never return RTVL_MAYBE */
+	return k == RTVL_NO ? RTVL_NO : RTVL_YES;
 }
 
 /*
@@ -524,12 +538,6 @@ static bool released_to_systemd(void)
 	return ret;
 }
 
-/*
- * Return value:
- *  -1: Retry
- *   0: Success
- *   1: Failure
- */
 static int
 configure (struct config *conf, enum mpath_cmds cmd,
 	   enum devtypes dev_type, char *devpath)
@@ -537,7 +545,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
 	vector curmp = NULL;
 	vector pathvec = NULL;
 	struct vectors vecs;
-	int r = 1, rc;
+	int r = RTVL_FAIL, rc;
 	int di_flag = 0;
 	char * refwwid = NULL;
 	char * dev = NULL;
@@ -585,21 +593,23 @@ configure (struct config *conf, enum mpath_cmds cmd,
 			goto out;
 		}
 		if (cmd == CMD_REMOVE_WWID) {
-			r = remove_wwid(refwwid);
-			if (r == 0)
+			rc = remove_wwid(refwwid);
+			if (rc == 0) {
 				printf("wwid '%s' removed\n", refwwid);
-			else if (r == 1) {
+				r = RTVL_OK;
+			} else if (rc == 1) {
 				printf("wwid '%s' not in wwids file\n",
 					refwwid);
-				r = 0;
+				r = RTVL_OK;
 			}
 			goto out;
 		}
 		if (cmd == CMD_ADD_WWID) {
-			r = remember_wwid(refwwid);
-			if (r >= 0)
+			rc = remember_wwid(refwwid);
+			if (rc >= 0) {
 				printf("wwid '%s' added\n", refwwid);
-			else
+				r = RTVL_OK;
+			} else
 				printf("failed adding '%s' to wwids file\n",
 				       refwwid);
 			goto out;
@@ -614,13 +624,13 @@ configure (struct config *conf, enum mpath_cmds cmd,
 		 */
 		if (cmd == CMD_VALID_PATH) {
 			if (is_failed_wwid(refwwid) == WWID_IS_FAILED) {
-				r = 1;
+				r = RTVL_NO;
 				goto print_valid;
 			}
 			if ((!find_multipaths_on(conf) &&
 				    ignore_wwids_on(conf)) ||
 				   check_wwids_file(refwwid, 0) == 0)
-				r = 0;
+				r = RTVL_YES;
 			if (!ignore_wwids_on(conf))
 				goto print_valid;
 			/* At this point, either r==0 or find_multipaths_on. */
@@ -630,7 +640,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
 			 * Quick check if path is already multipathed.
 			 */
 			if (sysfs_is_multipathed(VECTOR_SLOT(pathvec, 0))) {
-				r = 0;
+				r = RTVL_YES;
 				goto print_valid;
 			}
 
@@ -644,10 +654,10 @@ configure (struct config *conf, enum mpath_cmds cmd,
 			 * Leave DM_MULTIPATH_DEVICE_PATH="0".
 			 */
 			if (released) {
-				r = 1;
+				r = RTVL_NO;
 				goto print_valid;
 			}
-			if (r == 0)
+			if (r == RTVL_YES)
 				goto print_valid;
 			/* find_multipaths_on: Fall through to path detection */
 		}
@@ -703,13 +713,12 @@ configure (struct config *conf, enum mpath_cmds cmd,
 		 * the refwwid, or there is more than one path matching
 		 * the refwwid, then the path is valid */
 		if (VECTOR_SIZE(curmp) != 0) {
-			r = 0;
+			r = RTVL_YES;
 			goto print_valid;
 		} else if (VECTOR_SIZE(pathvec) > 1)
-			r = 0;
+			r = RTVL_YES;
 		else
-			/* Use r=2 as an indication for "maybe" */
-			r = 2;
+			r = RTVL_MAYBE;
 
 		/*
 		 * If opening the path with O_EXCL fails, the path
@@ -739,13 +748,14 @@ configure (struct config *conf, enum mpath_cmds cmd,
 			/*
 			 * Check if we raced with multipathd
 			 */
-			r = !sysfs_is_multipathed(VECTOR_SLOT(pathvec, 0));
+			r = sysfs_is_multipathed(VECTOR_SLOT(pathvec, 0)) ?
+				RTVL_YES : RTVL_NO;
 		}
 		goto print_valid;
 	}
 
 	if (cmd != CMD_CREATE && cmd != CMD_DRY_RUN) {
-		r = 0;
+		r = RTVL_OK;
 		goto out;
 	}
 
@@ -754,7 +764,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
 	 */
 	rc = coalesce_paths(&vecs, NULL, refwwid,
 			   conf->force_reload, cmd);
-	r = rc == CP_RETRY ? -1 : rc == CP_OK ? 0 : 1;
+	r = rc == CP_RETRY ? RTVL_RETRY : rc == CP_OK ? RTVL_OK : RTVL_FAIL;
 
 print_valid:
 	if (cmd == CMD_VALID_PATH)
@@ -855,7 +865,7 @@ main (int argc, char *argv[])
 	int arg;
 	extern char *optarg;
 	extern int optind;
-	int r = 1;
+	int r = RTVL_FAIL;
 	enum mpath_cmds cmd = CMD_CREATE;
 	enum devtypes dev_type = DEV_NONE;
 	char *dev = NULL;
@@ -866,7 +876,7 @@ main (int argc, char *argv[])
 	logsink = 0;
 	conf = load_config(DEFAULT_CONFIGFILE);
 	if (!conf)
-		exit(1);
+		exit(RTVL_FAIL);
 	multipath_conf = conf;
 	conf->retrigger_tries = 0;
 	while ((arg = getopt(argc, argv, ":adcChl::FfM:v:p:b:BrR:itTquUwW")) != EOF ) {
@@ -877,7 +887,7 @@ main (int argc, char *argv[])
 			if (sizeof(optarg) > sizeof(char *) ||
 			    !isdigit(optarg[0])) {
 				usage (argv[0]);
-				exit(1);
+				exit(RTVL_FAIL);
 			}
 
 			conf->verbosity = atoi(optarg);
@@ -924,7 +934,7 @@ main (int argc, char *argv[])
 			if (conf->pgpolicy_flag == IOPOLICY_UNDEF) {
 				printf("'%s' is not a valid policy\n", optarg);
 				usage(argv[0]);
-				exit(1);
+				exit(RTVL_FAIL);
 			}
 			break;
 		case 'r':
@@ -934,14 +944,14 @@ main (int argc, char *argv[])
 			conf->find_multipaths |= _FIND_MULTIPATHS_I;
 			break;
 		case 't':
-			r = dump_config(conf, NULL, NULL);
+			r = dump_config(conf, NULL, NULL) ? RTVL_FAIL : RTVL_OK;
 			goto out_free_config;
 		case 'T':
 			cmd = CMD_DUMP_CONFIG;
 			break;
 		case 'h':
 			usage(argv[0]);
-			exit(0);
+			exit(RTVL_OK);
 		case 'u':
 			cmd = CMD_VALID_PATH;
 			dev_type = DEV_UEVENT;
@@ -965,20 +975,20 @@ main (int argc, char *argv[])
 		case ':':
 			fprintf(stderr, "Missing option argument\n");
 			usage(argv[0]);
-			exit(1);
+			exit(RTVL_FAIL);
 		case '?':
 			fprintf(stderr, "Unknown switch: %s\n", optarg);
 			usage(argv[0]);
-			exit(1);
+			exit(RTVL_FAIL);
 		default:
 			usage(argv[0]);
-			exit(1);
+			exit(RTVL_FAIL);
 		}
 	}
 
 	if (getuid() != 0) {
 		fprintf(stderr, "need to be root\n");
-		exit(1);
+		exit(RTVL_FAIL);
 	}
 
 	if (optind < argc) {
@@ -1016,7 +1026,8 @@ main (int argc, char *argv[])
 	/* Failing here is non-fatal */
 	init_foreign(conf->multipath_dir);
 	if (cmd == CMD_USABLE_PATHS) {
-		r = check_usable_paths(conf, dev, dev_type);
+		r = check_usable_paths(conf, dev, dev_type) ?
+			RTVL_FAIL : RTVL_OK;
 		goto out;
 	}
 	if (cmd == CMD_VALID_PATH &&
@@ -1032,7 +1043,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(1, NULL, conf);
+				r = print_cmd_valid(RTVL_NO, NULL, conf);
 				goto out;
 			}
 		} else
@@ -1046,9 +1057,9 @@ main (int argc, char *argv[])
 
 	switch(delegate_to_multipathd(cmd, dev, dev_type, conf)) {
 	case DELEGATE_OK:
-		exit(0);
+		exit(RTVL_OK);
 	case DELEGATE_ERROR:
-		exit(1);
+		exit(RTVL_FAIL);
 	case NOT_DELEGATED:
 		break;
 	}
@@ -1064,8 +1075,8 @@ main (int argc, char *argv[])
 			goto out;
 		}
 		if (dm_get_maps(curmp) == 0)
-			r = replace_wwids(curmp);
-		if (r == 0)
+			r = replace_wwids(curmp) ? RTVL_FAIL : RTVL_OK;
+		if (r == RTVL_OK)
 			printf("successfully reset wwids\n");
 		vector_foreach_slot_backwards(curmp, mpp, i) {
 			vector_del_slot(curmp, i);
@@ -1078,17 +1089,18 @@ main (int argc, char *argv[])
 		retries = conf->remove_retries;
 	if (conf->remove == FLUSH_ONE) {
 		if (dev_type == DEV_DEVMAP) {
-			r = dm_suspend_and_flush_map(dev, retries);
+			r = dm_suspend_and_flush_map(dev, retries) ?
+				RTVL_FAIL : RTVL_OK;
 		} else
 			condlog(0, "must provide a map name to remove");
 
 		goto out;
 	}
 	else if (conf->remove == FLUSH_ALL) {
-		r = dm_flush_maps(retries);
+		r = dm_flush_maps(retries) ? RTVL_FAIL : RTVL_OK;
 		goto out;
 	}
-	while ((r = configure(conf, cmd, dev_type, dev)) < 0)
+	while ((r = configure(conf, cmd, dev_type, dev)) == RTVL_RETRY)
 		condlog(3, "restart multipath configuration process");
 
 out:
@@ -1103,8 +1115,8 @@ out:
 	 * 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 (cmd == CMD_VALID_PATH && dev_type == DEV_UEVENT && r == RTVL_NO)
+		r = RTVL_OK;
 
 	if (dev_type == DEV_UEVENT)
 		closelog();
-- 
2.19.1

Re: [PATCH v2 23/24] domap(): never return DOMAP_RETRY in daemon mode

[Benjamin Marzinski]

On Mon, Dec 03, 2018 at 08:36:23PM +0100, Martin Wilck wrote:
> DOMAP_RETRY is only used by the multipath tool. multipathd always treats
> it exactly like DOMAP_FAIL. Simplify logic by only returning
> DOMAP_RETRY in non-daemon mode from domap().
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/configure.c | 28 +++++++++++++---------------
>  multipathd/main.c        |  9 +--------
>  2 files changed, 14 insertions(+), 23 deletions(-)
> 
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index 84ae5f56..1c549817 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -831,7 +831,7 @@ int domap(struct multipath *mpp, char *params, int is_daemon)
>  		if (lock_multipath(mpp, 1)) {
>  			condlog(3, "%s: failed to create map (in use)",
>  				mpp->alias);
> -			return DOMAP_RETRY;
> +			return is_daemon ? DOMAP_FAIL : DOMAP_RETRY;
>  		}
>  
>  		sysfs_set_max_sectors_kb(mpp, 0);
> @@ -1110,20 +1110,18 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
>  
>  		r = domap(mpp, params, is_daemon);
>  
> -		if (r == DOMAP_FAIL || r == DOMAP_RETRY) {
> -			condlog(3, "%s: domap (%u) failure "
> -				   "for create/reload map",
> -				mpp->alias, r);
> -			if (r == DOMAP_FAIL || is_daemon) {
> -				condlog(2, "%s: %s map",
> -					mpp->alias, (mpp->action == ACT_CREATE)?
> -					"ignoring" : "removing");
> -				remove_map(mpp, vecs, 0);
> -				continue;
> -			} else /* if (r == DOMAP_RETRY && !is_daemon) */ {
> -				ret = CP_RETRY;
> -				goto out;
> -			}
> +		if (r == DOMAP_RETRY) {
> +			/* This happens in non-daemon case only */
> +			ret = CP_RETRY;
> +			goto out;
> +		}
> +
> +		if (r == DOMAP_FAIL) {
> +			condlog(2, "%s: domap failure, %s map",
> +				mpp->alias, (mpp->action == ACT_CREATE) ?
> +				"ignoring" : "removing");
> +			remove_map(mpp, vecs, 0);
> +			continue;
>  		}
>  		if (r == DOMAP_DRY)
>  			continue;
> diff --git a/multipathd/main.c b/multipathd/main.c
> index d0e7107c..1bf3c481 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -998,15 +998,8 @@ rescan:
>  	/*
>  	 * reload the map for the multipath mapped device
>  	 */
> -retry:
>  	ret = domap(mpp, params, 1);
> -	if (ret == DOMAP_FAIL || ret == DOMAP_RETRY) {
> -		if (ret < 0 && retries-- > 0) {
> -			condlog(0, "%s: retry domap for addition of new "
> -				"path %s", mpp->alias, pp->dev);
> -			sleep(1);
> -			goto retry;
> -		}
> +	if (ret == DOMAP_FAIL) {

I'm kind of conflicted about this change.  According to the commit
message (commit 1c50cd32), Hannes put this code in to deal with lock
contention on the paths (back when we used an exclusive lock in
lock_multipath(), and actually saw contention). I don't know of any
purpose for lock_multipath(), so removing this code probably won't hurt
anything. But if we believe that we need lock_multipath(), then we
should probably retry here (unless you have some understanding of
lock_multipath()'s purpose where retrying won't help).

If we do keep lock_multipath(), I definitely don't see a reason to retry
here if we got DOMAP_FAIL instead of DOMAP_RETRY, which means that
multipathd needs to continue to distingush between them.  On the other
hand, if we decide we don't need lock_multipath(), and thus DOMAP_RETRY,
then there's no point in coalesce_paths returning symbolic return
values, since we only have one non-failure return value.

Probably the most important question I have is "Does anyone know why we
lock the paths?" I believe that the code originally existed to keep
multipath and multipathd from trying to load a device at the same time.
It ended up causing problems with udev, because that tries to grab a
shared lock on the path while working on it.  When multipath switched to
using a shared lock (commit 5ec07b3a), I agreed based on the rational
that udev must be protecting against something, and we could need
protecting against the same thing. The discussion is at:

https://www.redhat.com/archives/dm-devel/2016-May/msg00009.html

But we've always seemed to be one step away from just removing this
code. I would be nice to either determine what we do need it for, or that we
don't need it at all.

-Ben

>  		condlog(0, "%s: failed in domap for addition of new "
>  			"path %s", mpp->alias, pp->dev);
>  		/*
> -- 
> 2.19.1

Re: [PATCH v2 24/24] multipath: use symbolic return value and exit code

[Benjamin Marzinski]

On Mon, Dec 03, 2018 at 08:36:24PM +0100, Martin Wilck wrote:
> Use an enum to represent the return code and exit status of
> multipath and its most important subroutine, configure().
> This clarifies the confusing use of booleans and status
> codes a bit, hopefully.

Thanks for this. print_cmd_valid() especially is much more readable.

ACK.

-Ben

> 
> This patch doesn't introduce a change in behavior.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipath/main.c | 120 ++++++++++++++++++++++++++---------------------
>  1 file changed, 66 insertions(+), 54 deletions(-)
> 
> diff --git a/multipath/main.c b/multipath/main.c
> index eb087482..e437746d 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -68,6 +68,19 @@ int logsink;
>  struct udev *udev;
>  struct config *multipath_conf;
>  
> +/*
> + * Return values of configure(), print_cmd_valid(), and main().
> + * RTVL_{YES,NO} are synonyms for RTVL_{OK,FAIL} for the CMD_VALID_PATH case.
> + */
> +enum {
> +	RTVL_OK = 0,
> +	RTVL_YES = RTVL_OK,
> +	RTVL_FAIL = 1,
> +	RTVL_NO = RTVL_FAIL,
> +	RTVL_MAYBE, /* only used internally, never returned */
> +	RTVL_RETRY, /* returned by configure(), not by main() */
> +};
> +
>  struct config *get_multipath_config(void)
>  {
>  	return multipath_conf;
> @@ -475,15 +488,14 @@ retry:
>  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) / sizeof(int)))
> -		return 1;
> +	if (k != RTVL_YES && k != RTVL_NO && k != RTVL_MAYBE)
> +		return RTVL_NO;
>  
> -	if (k == 2) {
> +	if (k == RTVL_MAYBE) {
>  		/*
>  		 * Caller ensures that pathvec[0] is the path to
>  		 * examine.
> @@ -493,7 +505,7 @@ static int print_cmd_valid(int k, const vector pathvec,
>  		wait = find_multipaths_check_timeout(
>  			pp, pp->find_multipaths_timeout, &until);
>  		if (wait != FIND_MULTIPATHS_WAITING)
> -			k = 1;
> +			k = RTVL_NO;
>  	} else if (pathvec != NULL && (pp = VECTOR_SLOT(pathvec, 0)))
>  		wait = find_multipaths_check_timeout(pp, 0, &until);
>  	if (wait == FIND_MULTIPATHS_WAITING)
> @@ -501,8 +513,10 @@ static int print_cmd_valid(int k, const vector pathvec,
>  			       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;
> +	printf("DM_MULTIPATH_DEVICE_PATH=\"%d\"\n",
> +	       k == RTVL_MAYBE ? 2 : k == RTVL_YES ? 1 : 0);
> +	/* Never return RTVL_MAYBE */
> +	return k == RTVL_NO ? RTVL_NO : RTVL_YES;
>  }
>  
>  /*
> @@ -524,12 +538,6 @@ static bool released_to_systemd(void)
>  	return ret;
>  }
>  
> -/*
> - * Return value:
> - *  -1: Retry
> - *   0: Success
> - *   1: Failure
> - */
>  static int
>  configure (struct config *conf, enum mpath_cmds cmd,
>  	   enum devtypes dev_type, char *devpath)
> @@ -537,7 +545,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  	vector curmp = NULL;
>  	vector pathvec = NULL;
>  	struct vectors vecs;
> -	int r = 1, rc;
> +	int r = RTVL_FAIL, rc;
>  	int di_flag = 0;
>  	char * refwwid = NULL;
>  	char * dev = NULL;
> @@ -585,21 +593,23 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  			goto out;
>  		}
>  		if (cmd == CMD_REMOVE_WWID) {
> -			r = remove_wwid(refwwid);
> -			if (r == 0)
> +			rc = remove_wwid(refwwid);
> +			if (rc == 0) {
>  				printf("wwid '%s' removed\n", refwwid);
> -			else if (r == 1) {
> +				r = RTVL_OK;
> +			} else if (rc == 1) {
>  				printf("wwid '%s' not in wwids file\n",
>  					refwwid);
> -				r = 0;
> +				r = RTVL_OK;
>  			}
>  			goto out;
>  		}
>  		if (cmd == CMD_ADD_WWID) {
> -			r = remember_wwid(refwwid);
> -			if (r >= 0)
> +			rc = remember_wwid(refwwid);
> +			if (rc >= 0) {
>  				printf("wwid '%s' added\n", refwwid);
> -			else
> +				r = RTVL_OK;
> +			} else
>  				printf("failed adding '%s' to wwids file\n",
>  				       refwwid);
>  			goto out;
> @@ -614,13 +624,13 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  		 */
>  		if (cmd == CMD_VALID_PATH) {
>  			if (is_failed_wwid(refwwid) == WWID_IS_FAILED) {
> -				r = 1;
> +				r = RTVL_NO;
>  				goto print_valid;
>  			}
>  			if ((!find_multipaths_on(conf) &&
>  				    ignore_wwids_on(conf)) ||
>  				   check_wwids_file(refwwid, 0) == 0)
> -				r = 0;
> +				r = RTVL_YES;
>  			if (!ignore_wwids_on(conf))
>  				goto print_valid;
>  			/* At this point, either r==0 or find_multipaths_on. */
> @@ -630,7 +640,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  			 * Quick check if path is already multipathed.
>  			 */
>  			if (sysfs_is_multipathed(VECTOR_SLOT(pathvec, 0))) {
> -				r = 0;
> +				r = RTVL_YES;
>  				goto print_valid;
>  			}
>  
> @@ -644,10 +654,10 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  			 * Leave DM_MULTIPATH_DEVICE_PATH="0".
>  			 */
>  			if (released) {
> -				r = 1;
> +				r = RTVL_NO;
>  				goto print_valid;
>  			}
> -			if (r == 0)
> +			if (r == RTVL_YES)
>  				goto print_valid;
>  			/* find_multipaths_on: Fall through to path detection */
>  		}
> @@ -703,13 +713,12 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  		 * the refwwid, or there is more than one path matching
>  		 * the refwwid, then the path is valid */
>  		if (VECTOR_SIZE(curmp) != 0) {
> -			r = 0;
> +			r = RTVL_YES;
>  			goto print_valid;
>  		} else if (VECTOR_SIZE(pathvec) > 1)
> -			r = 0;
> +			r = RTVL_YES;
>  		else
> -			/* Use r=2 as an indication for "maybe" */
> -			r = 2;
> +			r = RTVL_MAYBE;
>  
>  		/*
>  		 * If opening the path with O_EXCL fails, the path
> @@ -739,13 +748,14 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  			/*
>  			 * Check if we raced with multipathd
>  			 */
> -			r = !sysfs_is_multipathed(VECTOR_SLOT(pathvec, 0));
> +			r = sysfs_is_multipathed(VECTOR_SLOT(pathvec, 0)) ?
> +				RTVL_YES : RTVL_NO;
>  		}
>  		goto print_valid;
>  	}
>  
>  	if (cmd != CMD_CREATE && cmd != CMD_DRY_RUN) {
> -		r = 0;
> +		r = RTVL_OK;
>  		goto out;
>  	}
>  
> @@ -754,7 +764,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  	 */
>  	rc = coalesce_paths(&vecs, NULL, refwwid,
>  			   conf->force_reload, cmd);
> -	r = rc == CP_RETRY ? -1 : rc == CP_OK ? 0 : 1;
> +	r = rc == CP_RETRY ? RTVL_RETRY : rc == CP_OK ? RTVL_OK : RTVL_FAIL;
>  
>  print_valid:
>  	if (cmd == CMD_VALID_PATH)
> @@ -855,7 +865,7 @@ main (int argc, char *argv[])
>  	int arg;
>  	extern char *optarg;
>  	extern int optind;
> -	int r = 1;
> +	int r = RTVL_FAIL;
>  	enum mpath_cmds cmd = CMD_CREATE;
>  	enum devtypes dev_type = DEV_NONE;
>  	char *dev = NULL;
> @@ -866,7 +876,7 @@ main (int argc, char *argv[])
>  	logsink = 0;
>  	conf = load_config(DEFAULT_CONFIGFILE);
>  	if (!conf)
> -		exit(1);
> +		exit(RTVL_FAIL);
>  	multipath_conf = conf;
>  	conf->retrigger_tries = 0;
>  	while ((arg = getopt(argc, argv, ":adcChl::FfM:v:p:b:BrR:itTquUwW")) != EOF ) {
> @@ -877,7 +887,7 @@ main (int argc, char *argv[])
>  			if (sizeof(optarg) > sizeof(char *) ||
>  			    !isdigit(optarg[0])) {
>  				usage (argv[0]);
> -				exit(1);
> +				exit(RTVL_FAIL);
>  			}
>  
>  			conf->verbosity = atoi(optarg);
> @@ -924,7 +934,7 @@ main (int argc, char *argv[])
>  			if (conf->pgpolicy_flag == IOPOLICY_UNDEF) {
>  				printf("'%s' is not a valid policy\n", optarg);
>  				usage(argv[0]);
> -				exit(1);
> +				exit(RTVL_FAIL);
>  			}
>  			break;
>  		case 'r':
> @@ -934,14 +944,14 @@ main (int argc, char *argv[])
>  			conf->find_multipaths |= _FIND_MULTIPATHS_I;
>  			break;
>  		case 't':
> -			r = dump_config(conf, NULL, NULL);
> +			r = dump_config(conf, NULL, NULL) ? RTVL_FAIL : RTVL_OK;
>  			goto out_free_config;
>  		case 'T':
>  			cmd = CMD_DUMP_CONFIG;
>  			break;
>  		case 'h':
>  			usage(argv[0]);
> -			exit(0);
> +			exit(RTVL_OK);
>  		case 'u':
>  			cmd = CMD_VALID_PATH;
>  			dev_type = DEV_UEVENT;
> @@ -965,20 +975,20 @@ main (int argc, char *argv[])
>  		case ':':
>  			fprintf(stderr, "Missing option argument\n");
>  			usage(argv[0]);
> -			exit(1);
> +			exit(RTVL_FAIL);
>  		case '?':
>  			fprintf(stderr, "Unknown switch: %s\n", optarg);
>  			usage(argv[0]);
> -			exit(1);
> +			exit(RTVL_FAIL);
>  		default:
>  			usage(argv[0]);
> -			exit(1);
> +			exit(RTVL_FAIL);
>  		}
>  	}
>  
>  	if (getuid() != 0) {
>  		fprintf(stderr, "need to be root\n");
> -		exit(1);
> +		exit(RTVL_FAIL);
>  	}
>  
>  	if (optind < argc) {
> @@ -1016,7 +1026,8 @@ main (int argc, char *argv[])
>  	/* Failing here is non-fatal */
>  	init_foreign(conf->multipath_dir);
>  	if (cmd == CMD_USABLE_PATHS) {
> -		r = check_usable_paths(conf, dev, dev_type);
> +		r = check_usable_paths(conf, dev, dev_type) ?
> +			RTVL_FAIL : RTVL_OK;
>  		goto out;
>  	}
>  	if (cmd == CMD_VALID_PATH &&
> @@ -1032,7 +1043,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(1, NULL, conf);
> +				r = print_cmd_valid(RTVL_NO, NULL, conf);
>  				goto out;
>  			}
>  		} else
> @@ -1046,9 +1057,9 @@ main (int argc, char *argv[])
>  
>  	switch(delegate_to_multipathd(cmd, dev, dev_type, conf)) {
>  	case DELEGATE_OK:
> -		exit(0);
> +		exit(RTVL_OK);
>  	case DELEGATE_ERROR:
> -		exit(1);
> +		exit(RTVL_FAIL);
>  	case NOT_DELEGATED:
>  		break;
>  	}
> @@ -1064,8 +1075,8 @@ main (int argc, char *argv[])
>  			goto out;
>  		}
>  		if (dm_get_maps(curmp) == 0)
> -			r = replace_wwids(curmp);
> -		if (r == 0)
> +			r = replace_wwids(curmp) ? RTVL_FAIL : RTVL_OK;
> +		if (r == RTVL_OK)
>  			printf("successfully reset wwids\n");
>  		vector_foreach_slot_backwards(curmp, mpp, i) {
>  			vector_del_slot(curmp, i);
> @@ -1078,17 +1089,18 @@ main (int argc, char *argv[])
>  		retries = conf->remove_retries;
>  	if (conf->remove == FLUSH_ONE) {
>  		if (dev_type == DEV_DEVMAP) {
> -			r = dm_suspend_and_flush_map(dev, retries);
> +			r = dm_suspend_and_flush_map(dev, retries) ?
> +				RTVL_FAIL : RTVL_OK;
>  		} else
>  			condlog(0, "must provide a map name to remove");
>  
>  		goto out;
>  	}
>  	else if (conf->remove == FLUSH_ALL) {
> -		r = dm_flush_maps(retries);
> +		r = dm_flush_maps(retries) ? RTVL_FAIL : RTVL_OK;
>  		goto out;
>  	}
> -	while ((r = configure(conf, cmd, dev_type, dev)) < 0)
> +	while ((r = configure(conf, cmd, dev_type, dev)) == RTVL_RETRY)
>  		condlog(3, "restart multipath configuration process");
>  
>  out:
> @@ -1103,8 +1115,8 @@ out:
>  	 * 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 (cmd == CMD_VALID_PATH && dev_type == DEV_UEVENT && r == RTVL_NO)
> +		r = RTVL_OK;
>  
>  	if (dev_type == DEV_UEVENT)
>  		closelog();
> -- 
> 2.19.1

Re: [PATCH v2 00/24] multipath-tools: improve logging at -v3

[Benjamin Marzinski]

On Mon, Dec 03, 2018 at 08:36:18PM +0100, Martin Wilck wrote:
> Hi Christophe,
> 
> most of the patches in this series reduce log levels of frequently
> printed messages at verbosity level 3. My goal was to limit the
> output of multipathd to one line per path per checker invocation,
> which is sufficient to track multipathd's view of path health in
> the logs.
> 
> The standard setting of -v2 is not enough for post-mortem analysis of many
> failures. With this series, running multipathd with verbosity 3 becomes a
> realistic option even in production environments. So far the amount of output
> from multipathd with -v3 pretty much made this impossible, at least over
> longer time periods, and also made reading these logs very cumbersome due to
> the amount of redundant partly superfluos verbosity. I've taken care not
> to loose important information in the logs.
> 
> Apart from that, the series fixes errors in the unit tests introduced by my
> last "checker overhaul" patch series (proving that I forgot to run the
> tests before submitting :-( ), and fixes a problem that I found while testing
> handling of a bad configuration (paths with size mismatch).
> 
> Regards,
> Martin
> 
> Changes in v2:
> 
> The first 19 patches are identical to v1 as ACK'd by Ben, except 16/24
> "libmultipath: coalesce_paths: fix size mismatch handling".
> No. 8/24 "libmultipath: decrease log level of word splitting"
> (not yet ACKd by Ben) also stays the same; the issue Ben raised
> in his review is addressed in a separate patch, 20/24.
> 21/24 addresses implements Ben's suggestion to use named constants
> as return values in coalesce_paths(). 22, 23, 24 do the same for
> other important, related functions, as I found it strange to make
> this change just for coalesce_paths() alone.  

Thanks

ACK for everything except 23/24

-Ben

> 
> Martin Wilck (24):
>   tests/hwtable: set multipath_dir in local configuration
>   tests/hwtable: adjust to new checker API
>   multipath-tools: decrease verbosity of state messages
>   libmultipath: decrease verbosity of pathinfo messages
>   libmultipath: decrease verbosity of TUR checker messages
>   libmultipath: avoid frequent messages from filter_property()
>   libmultipath: decrease log level of "disassembled" messages
>   libmultipath: decrease log level of word splitting
>   libmultipath: increase log level of map removal
>   multipathd: decrease log level of checker timing
>   libmultipath: decrease log level of "prioritizer refcount" message
>   libmpathpersist/update_map_pr: decrease log level for nop
>   libmultipath: simplify devt2devname()
>   libmultipath: decrease log level for failed VPD c9
>   libmultipath: adopt_paths: check for size match
>   libmultipath: coalesce_paths: fix size mismatch handling
>   tests: add unit tests for bitmask functions
>   multipathd: uev_remove_path: remove redundant orphan_paths call
>   libmultipath: improve logging from orphan_paths
>   libmultipath: avoid syslog loglevel > LOG_DEBUG
>   coalesce_paths(): use symbolic return value
>   domap(): use symbolic return value
>   domap(): never return DOMAP_RETRY in daemon mode
>   multipath: use symbolic return value and exit code
> 
>  libmpathpersist/mpath_persist.c |   3 +-
>  libmultipath/blacklist.c        |  54 +++++++-------
>  libmultipath/blacklist.h        |   2 +-
>  libmultipath/checkers/tur.c     |   6 +-
>  libmultipath/configure.c        |  68 +++++++++---------
>  libmultipath/configure.h        |  23 ++++++
>  libmultipath/discovery.c        |  20 +++---
>  libmultipath/dmparser.c         |   6 +-
>  libmultipath/log_pthread.c      |   3 +
>  libmultipath/prio.c             |   2 +-
>  libmultipath/structs_vec.c      |  18 +++--
>  libmultipath/structs_vec.h      |   3 +-
>  libmultipath/util.c             |   7 +-
>  libmultipath/util.h             |  16 +++++
>  multipath/main.c                | 121 ++++++++++++++++++--------------
>  multipathd/cli_handlers.c       |   5 +-
>  multipathd/main.c               |  39 +++++-----
>  tests/Makefile                  |   7 +-
>  tests/blacklist.c               |   7 +-
>  tests/hwtable.c                 |  89 ++++++++++++-----------
>  tests/util.c                    |  98 ++++++++++++++++++++++++++
>  21 files changed, 386 insertions(+), 211 deletions(-)
> 
> -- 
> 2.19.1

Re: [PATCH v2 00/24] multipath-tools: improve logging at -v3

[Christophe Varoqui]

[-- Attachment #1.1: Type: text/plain, Size: 4829 bytes --]

Hi Martin,

pending patches, except the logging refactoring patchset, are merged.
When you'll be ready to send the v3 of this patchset, please send the full
set : some of the reviewed v1 patches are missing in my mailbox.

Thanks,
Christophe

On Tue, Dec 4, 2018 at 12:50 AM Benjamin Marzinski <bmarzins@redhat.com>
wrote:

> On Mon, Dec 03, 2018 at 08:36:18PM +0100, Martin Wilck wrote:
> > Hi Christophe,
> >
> > most of the patches in this series reduce log levels of frequently
> > printed messages at verbosity level 3. My goal was to limit the
> > output of multipathd to one line per path per checker invocation,
> > which is sufficient to track multipathd's view of path health in
> > the logs.
> >
> > The standard setting of -v2 is not enough for post-mortem analysis of
> many
> > failures. With this series, running multipathd with verbosity 3 becomes a
> > realistic option even in production environments. So far the amount of
> output
> > from multipathd with -v3 pretty much made this impossible, at least over
> > longer time periods, and also made reading these logs very cumbersome
> due to
> > the amount of redundant partly superfluos verbosity. I've taken care not
> > to loose important information in the logs.
> >
> > Apart from that, the series fixes errors in the unit tests introduced by
> my
> > last "checker overhaul" patch series (proving that I forgot to run the
> > tests before submitting :-( ), and fixes a problem that I found while
> testing
> > handling of a bad configuration (paths with size mismatch).
> >
> > Regards,
> > Martin
> >
> > Changes in v2:
> >
> > The first 19 patches are identical to v1 as ACK'd by Ben, except 16/24
> > "libmultipath: coalesce_paths: fix size mismatch handling".
> > No. 8/24 "libmultipath: decrease log level of word splitting"
> > (not yet ACKd by Ben) also stays the same; the issue Ben raised
> > in his review is addressed in a separate patch, 20/24.
> > 21/24 addresses implements Ben's suggestion to use named constants
> > as return values in coalesce_paths(). 22, 23, 24 do the same for
> > other important, related functions, as I found it strange to make
> > this change just for coalesce_paths() alone.
>
> Thanks
>
> ACK for everything except 23/24
>
> -Ben
>
> >
> > Martin Wilck (24):
> >   tests/hwtable: set multipath_dir in local configuration
> >   tests/hwtable: adjust to new checker API
> >   multipath-tools: decrease verbosity of state messages
> >   libmultipath: decrease verbosity of pathinfo messages
> >   libmultipath: decrease verbosity of TUR checker messages
> >   libmultipath: avoid frequent messages from filter_property()
> >   libmultipath: decrease log level of "disassembled" messages
> >   libmultipath: decrease log level of word splitting
> >   libmultipath: increase log level of map removal
> >   multipathd: decrease log level of checker timing
> >   libmultipath: decrease log level of "prioritizer refcount" message
> >   libmpathpersist/update_map_pr: decrease log level for nop
> >   libmultipath: simplify devt2devname()
> >   libmultipath: decrease log level for failed VPD c9
> >   libmultipath: adopt_paths: check for size match
> >   libmultipath: coalesce_paths: fix size mismatch handling
> >   tests: add unit tests for bitmask functions
> >   multipathd: uev_remove_path: remove redundant orphan_paths call
> >   libmultipath: improve logging from orphan_paths
> >   libmultipath: avoid syslog loglevel > LOG_DEBUG
> >   coalesce_paths(): use symbolic return value
> >   domap(): use symbolic return value
> >   domap(): never return DOMAP_RETRY in daemon mode
> >   multipath: use symbolic return value and exit code
> >
> >  libmpathpersist/mpath_persist.c |   3 +-
> >  libmultipath/blacklist.c        |  54 +++++++-------
> >  libmultipath/blacklist.h        |   2 +-
> >  libmultipath/checkers/tur.c     |   6 +-
> >  libmultipath/configure.c        |  68 +++++++++---------
> >  libmultipath/configure.h        |  23 ++++++
> >  libmultipath/discovery.c        |  20 +++---
> >  libmultipath/dmparser.c         |   6 +-
> >  libmultipath/log_pthread.c      |   3 +
> >  libmultipath/prio.c             |   2 +-
> >  libmultipath/structs_vec.c      |  18 +++--
> >  libmultipath/structs_vec.h      |   3 +-
> >  libmultipath/util.c             |   7 +-
> >  libmultipath/util.h             |  16 +++++
> >  multipath/main.c                | 121 ++++++++++++++++++--------------
> >  multipathd/cli_handlers.c       |   5 +-
> >  multipathd/main.c               |  39 +++++-----
> >  tests/Makefile                  |   7 +-
> >  tests/blacklist.c               |   7 +-
> >  tests/hwtable.c                 |  89 ++++++++++++-----------
> >  tests/util.c                    |  98 ++++++++++++++++++++++++++
> >  21 files changed, 386 insertions(+), 211 deletions(-)
> >
> > --
> > 2.19.1
>

[-- Attachment #1.2: Type: text/html, Size: 5977 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 23/24] domap(): never return DOMAP_RETRY in daemon mode

[Martin Wilck]

On Mon, 2018-12-03 at 17:45 -0600,  Benjamin Marzinski  wrote:
> On Mon, Dec 03, 2018 at 08:36:23PM +0100, Martin Wilck wrote:
> > DOMAP_RETRY is only used by the multipath tool. multipathd always
> > treats
> > it exactly like DOMAP_FAIL. Simplify logic by only returning
> > DOMAP_RETRY in non-daemon mode from domap().
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  libmultipath/configure.c | 28 +++++++++++++---------------
> >  multipathd/main.c        |  9 +--------
> >  2 files changed, 14 insertions(+), 23 deletions(-)
> > 
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index d0e7107c..1bf3c481 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -998,15 +998,8 @@ rescan:
> >  	/*
> >  	 * reload the map for the multipath mapped device
> >  	 */
> > -retry:
> >  	ret = domap(mpp, params, 1);
> > -	if (ret == DOMAP_FAIL || ret == DOMAP_RETRY) {
> > -		if (ret < 0 && retries-- > 0) {
> > -			condlog(0, "%s: retry domap for addition of new
> > "
> > -				"path %s", mpp->alias, pp->dev);
> > -			sleep(1);
> > -			goto retry;
> > -		}
> > +	if (ret == DOMAP_FAIL) {
> 
> I'm kind of conflicted about this change.

Right, I shot over the top here. I was irritated by the fact that 
"ret < 0" translates to "ret == DOMAP_RETRY" and that therefore
DOMAP_RETRY is tested twice.

>  According to the commit
> message (commit 1c50cd32), Hannes put this code in to deal with lock
> contention on the paths (back when we used an exclusive lock in
> lock_multipath(), and actually saw contention). I don't know of any
> purpose for lock_multipath(), so removing this code probably won't
> hurt
> anything. But if we believe that we need lock_multipath(), then we
> should probably retry here (unless you have some understanding of
> lock_multipath()'s purpose where retrying won't help).
>
> [...]
> Probably the most important question I have is "Does anyone know why
> we > Probably the most important question I have is "Does anyone know
> why we lock the paths?

TL;DR: I'm 99.7% sure we don't need lock_multipath() any more.

The historic reason is 4d7a270:

    'Multiple multipath(8) execs can race with udev storm.
    
    We can simulate this with the following :
    "multipath -F; /sbin/multipath 8:16 & /sbin/multipath 8:32"
    
    Problem arise when two runs are about to create the same map.
    One will fail, leaving us with a choice : abord or retry.'

This commit was made at a time (October 2005) when multipath was called
directly from udev rules to set up maps. Earlier versions of multipath
had a general locking that would not allow multiple multipath commands
to run in parallel, but that has been removed later. This was an
attempt to lock only (would-be) members of one specific map.

Obviously, the goal of this patch wouldn't be achieved any more since
the lock has been change to non-exclusive (1c50cd32). Multiple
multipath instances run happily on members of the same set now. I
haven't tested it, but I believe the historic race "/sbin/multipath
8:16 & /sbin/multipath 8:32" still exists; just we don't run multipath
this way from udev rules any more. 

lock_multipath() doesn't help us void this race, as we can't go back to
exclusive locking. If we want to avoid it, we could create a lock file
from the WWID before calling domap(), /dev/shm/multipath/$WWID.lock or
so. Or we could use a SYSV semaphore set.

> I believe that the code originally existed to keep
> multipath and multipathd from trying to load a device at the same
> time.
> It ended up causing problems with udev, because that tries to grab a
> shared lock on the path while working on it.  When multipath switched
> to
> using a shared lock (commit 5ec07b3a), I agreed based on the rational
> that udev must be protecting against something, and we could need
> protecting against the same thing. The discussion is at:
> 
> https://www.redhat.com/archives/dm-devel/2016-May/msg00009.html
> 

The message of udev commit 3ebdb81, which introduced "device ownership"
in udev, says just "udev: serialize/synchronize block device event
handling with file locks". A comment says "this establishes a concept
of device "ownership" to serialize device access. External processes
holding a "write lock" will cause udev to skip the event handling; in
the case udev acquired the lock, the external process will block until
udev has finished its event handling".

Because multipathd is an "external process" from udev's point of view,
this would mean that our old way of using a write lock was correct (at
least, it was what the udev author intended). But udev gets this really
wrong, it should itself block or retry if the device is locked, rather
than aborting the event processing.

Note that both dm and md devices are excluded from udev's locking
anyway, because the udev people encountered problems with this scheme.

Martin


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


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

Re: [PATCH v2 23/24] domap(): never return DOMAP_RETRY in daemon mode

[Benjamin Marzinski]

On Sun, Dec 09, 2018 at 10:06:05PM +0100, Martin Wilck wrote:
> On Mon, 2018-12-03 at 17:45 -0600,  Benjamin Marzinski  wrote:
> 
> TL;DR: I'm 99.7% sure we don't need lock_multipath() any more.
> 
> The historic reason is 4d7a270:
> 
>     'Multiple multipath(8) execs can race with udev storm.
>     
>     We can simulate this with the following :
>     "multipath -F; /sbin/multipath 8:16 & /sbin/multipath 8:32"
>     
>     Problem arise when two runs are about to create the same map.
>     One will fail, leaving us with a choice : abord or retry.'
> 
> This commit was made at a time (October 2005) when multipath was called
> directly from udev rules to set up maps. Earlier versions of multipath
> had a general locking that would not allow multiple multipath commands
> to run in parallel, but that has been removed later. This was an
> attempt to lock only (would-be) members of one specific map.
> 
> Obviously, the goal of this patch wouldn't be achieved any more since
> the lock has been change to non-exclusive (1c50cd32). Multiple
> multipath instances run happily on members of the same set now. I
> haven't tested it, but I believe the historic race "/sbin/multipath
> 8:16 & /sbin/multipath 8:32" still exists; just we don't run multipath
> this way from udev rules any more. 
> 
> lock_multipath() doesn't help us void this race, as we can't go back to
> exclusive locking. If we want to avoid it, we could create a lock file
> from the WWID before calling domap(), /dev/shm/multipath/$WWID.lock or
> so. Or we could use a SYSV semaphore set.

I vote for removing lock_multipath(). Personally, I've never seen anyone
report the anything that looks like a multiple creation race since we've
changed the locking to shared, so I'm fine with leaving it out, but I
certainly wouldn't NAK a patch that added useful locking back in.

-Ben