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

Discussion:

Martin Wilck

2018-12-03 19:36:18 UTC

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

Martin Wilck

2018-12-03 19:36:21 UTC

Permalink

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 <***@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

Martin Wilck

2018-12-03 19:36:24 UTC

Permalink

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 <***@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

Benjamin Marzinski

2018-12-03 23:48:21 UTC

Permalink

Post by 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.

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

ACK.

-Ben

Post by Martin Wilck
This patch doesn't introduce a change in behavior.
---
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;
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;
}
-/*
- * -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;
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;
@@ -934,14 +944,14 @@ main (int argc, char *argv[])
conf->find_multipaths |= _FIND_MULTIPATHS_I;
break;
- r = dump_config(conf, NULL, NULL);
+ r = dump_config(conf, NULL, NULL) ? RTVL_FAIL : RTVL_OK;
goto out_free_config;
cmd = CMD_DUMP_CONFIG;
break;
usage(argv[0]);
- exit(0);
+ exit(RTVL_OK);
cmd = CMD_VALID_PATH;
dev_type = DEV_UEVENT;
@@ -965,20 +975,20 @@ main (int argc, char *argv[])
fprintf(stderr, "Missing option argument\n");
usage(argv[0]);
- exit(1);
+ exit(RTVL_FAIL);
fprintf(stderr, "Unknown switch: %s\n", optarg);
usage(argv[0]);
- exit(1);
+ exit(RTVL_FAIL);
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)) {
- exit(0);
+ exit(RTVL_OK);
- exit(1);
+ exit(RTVL_FAIL);
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");
* 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

Martin Wilck

2018-12-03 19:36:19 UTC

Permalink

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 <***@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

Martin Wilck

2018-12-03 19:36:20 UTC

Permalink

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

Signed-off-by: Martin Wilck <***@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

Martin Wilck

2018-12-03 19:36:22 UTC

Permalink

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 <***@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

Martin Wilck

2018-12-03 19:36:23 UTC

Permalink

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 <***@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

Benjamin Marzinski

2018-12-03 23:45:48 UTC

Permalink

Post by 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().
---
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
/*
* reload the map for the multipath mapped device
*/
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

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

Benjamin Marzinski

2018-12-03 23:50:36 UTC

Permalink

Post by 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
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

Post by Martin Wilck
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

Christophe Varoqui

2018-12-07 16:02:03 UTC

Permalink

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

Post by Martin Wilck

many

Post by Martin Wilck
failures. With this series, running multipathd with verbosity 3 becomes a
realistic option even in production environments. So far the amount of

output

Post by Martin Wilck
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

Post by Martin Wilck
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

Post by Martin Wilck
last "checker overhaul" patch series (proving that I forgot to run the
tests before submitting :-( ), and fixes a problem that I found while

testing

Post by Martin Wilck
handling of a bad configuration (paths with size mismatch).
Regards,
Martin
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