[PATCH 00/12] Misc fixes

[PATCH 00/12] Misc fixes

[Benjamin Marzinski]

The first patch here is just a rebased version of an earlier patch.  The last
patch is fixes for commit 95d594fd6f031e59bb73d04a631b6c592fe26214
"multipath-tools: intermittent IO error accounting to improve reliability".
The rest of the patches are various fixes and code cleanups that I've been
accumulating while working on removing the waiter threads. That work has run
into some hiccups with corner cases, so I thought I would send these cleanup
patches first, since they are generally useful, even without the waiter thread
changes.

Benjamin Marzinski (12):
  multipath: add "ghost_delay" parameter
  kpartx: don't delete partitions from partitions
  multipath: fix hwhandler check in select_action
  libmultipath: cleanup features handling code
  multipathd: move helper functions to libmultipath
  multipathd: fix device creation issues
  multipathd: remove select_* from setup_multipath
  libmultipath: __setup_multipath param cleanup
  multipathd: move recovery mode code to function
  multipathd: clean up set_no_path_retry
  multipath: check failed path dmstate in check_path
  multipathd: marginal path code fixes

 kpartx/del-part-nodes.rules |   1 +
 libmultipath/config.c       |   3 +
 libmultipath/config.h       |   3 +
 libmultipath/configure.c    |  38 +++----
 libmultipath/defaults.h     |   1 +
 libmultipath/devmapper.c    |   2 +-
 libmultipath/dict.c         |  12 +++
 libmultipath/dmparser.c     |  35 ++----
 libmultipath/hwtable.c      |   1 +
 libmultipath/io_err_stat.c  |  12 +--
 libmultipath/propsel.c      |  21 +++-
 libmultipath/propsel.h      |   1 +
 libmultipath/structs.c      |  17 ---
 libmultipath/structs.h      |  10 +-
 libmultipath/structs_vec.c  | 256 +++++++++++++++++++++++++-------------------
 libmultipath/structs_vec.h  |  12 ++-
 multipath/multipath.conf.5  |  21 +++-
 multipathd/cli_handlers.c   |  34 +++---
 multipathd/main.c           | 134 +++++++++--------------
 multipathd/main.h           |   1 -
 20 files changed, 322 insertions(+), 293 deletions(-)

-- 
2.7.4

[PATCH 01/12] multipath: add "ghost_delay" parameter

[Benjamin Marzinski]

If the lower-priority passive paths for a multipath device appear first,
IO can go to them and cause the hardware handler to activate them,
before the higher priority paths appear, causing the devices to
failback. Setting the "ghost_delay" parameter to a value greater than
0 can avoid this ping-ponging by causing udev to not mark the device as
Ready after its initial creation until either an active path appears,
or ghost_delay seconds have passed. Multipathd does this by setting
the MPATH_UDEV_NO_PATHS_FLAG.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/config.c      |  3 +++
 libmultipath/config.h      |  3 +++
 libmultipath/configure.c   | 11 +++++++++++
 libmultipath/defaults.h    |  1 +
 libmultipath/devmapper.c   |  2 +-
 libmultipath/dict.c        | 12 ++++++++++++
 libmultipath/hwtable.c     |  1 +
 libmultipath/propsel.c     | 15 +++++++++++++++
 libmultipath/propsel.h     |  1 +
 libmultipath/structs.h     |  7 +++++++
 multipath/multipath.conf.5 | 19 +++++++++++++++++++
 multipathd/main.c          | 28 +++++++++++++++++++++++++++-
 12 files changed, 101 insertions(+), 2 deletions(-)

diff --git a/libmultipath/config.c b/libmultipath/config.c
index eb03f0a..3d6a24c 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -351,6 +351,7 @@ merge_hwe (struct hwentry * dst, struct hwentry * src)
 	merge_num(delay_wait_checks);
 	merge_num(skip_kpartx);
 	merge_num(max_sectors_kb);
+	merge_num(ghost_delay);
 
 	snprintf(id, sizeof(id), "%s/%s", dst->vendor, dst->product);
 	reconcile_features_with_options(id, &dst->features,
@@ -419,6 +420,7 @@ store_hwe (vector hwtable, struct hwentry * dhwe)
 	hwe->retain_hwhandler = dhwe->retain_hwhandler;
 	hwe->detect_prio = dhwe->detect_prio;
 	hwe->detect_checker = dhwe->detect_checker;
+	hwe->ghost_delay = dhwe->ghost_delay;
 
 	if (dhwe->bl_product && !(hwe->bl_product = set_param_str(dhwe->bl_product)))
 		goto out;
@@ -619,6 +621,7 @@ load_config (char * file)
 	conf->uev_wait_timeout = DEFAULT_UEV_WAIT_TIMEOUT;
 	conf->disable_changed_wwids = DEFAULT_DISABLE_CHANGED_WWIDS;
 	conf->remove_retries = 0;
+	conf->ghost_delay = DEFAULT_GHOST_DELAY;
 
 	/*
 	 * preload default hwtable
diff --git a/libmultipath/config.h b/libmultipath/config.h
index 51fe27b..a20ac2a 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -81,6 +81,7 @@ struct hwentry {
 	int marginal_path_double_failed_time;
 	int skip_kpartx;
 	int max_sectors_kb;
+	int ghost_delay;
 	char * bl_product;
 };
 
@@ -114,6 +115,7 @@ struct mpentry {
 	int marginal_path_double_failed_time;
 	int skip_kpartx;
 	int max_sectors_kb;
+	int ghost_delay;
 	uid_t uid;
 	gid_t gid;
 	mode_t mode;
@@ -173,6 +175,7 @@ struct config {
 	int disable_changed_wwids;
 	int remove_retries;
 	int max_sectors_kb;
+	int ghost_delay;
 	unsigned int version[3];
 
 	char * multipath_dir;
diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 09821e8..09cbe3c 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -301,6 +301,7 @@ int setup_map(struct multipath *mpp, char *params, int params_size)
 	select_marginal_path_double_failed_time(conf, mpp);
 	select_skip_kpartx(conf, mpp);
 	select_max_sectors_kb(conf, mpp);
+	select_ghost_delay(conf, mpp);
 
 	sysfs_set_scsi_tmo(mpp, conf->checkint);
 	put_multipath_config(conf);
@@ -761,6 +762,9 @@ int domap(struct multipath *mpp, char *params, int is_daemon)
 		}
 
 		sysfs_set_max_sectors_kb(mpp, 0);
+		if (is_daemon && mpp->ghost_delay > 0 && mpp->nr_active &&
+		    pathcount(mpp, PATH_GHOST) == mpp->nr_active)
+			mpp->ghost_delay_tick = mpp->ghost_delay;
 		r = dm_addmap_create(mpp, params);
 
 		lock_multipath(mpp, 0);
@@ -768,11 +772,15 @@ int domap(struct multipath *mpp, char *params, int is_daemon)
 
 	case ACT_RELOAD:
 		sysfs_set_max_sectors_kb(mpp, 1);
+		if (mpp->ghost_delay_tick > 0 && pathcount(mpp, PATH_UP))
+			mpp->ghost_delay_tick = 0;
 		r = dm_addmap_reload(mpp, params, 0);
 		break;
 
 	case ACT_RESIZE:
 		sysfs_set_max_sectors_kb(mpp, 1);
+		if (mpp->ghost_delay_tick > 0 && pathcount(mpp, PATH_UP))
+			mpp->ghost_delay_tick = 0;
 		r = dm_addmap_reload(mpp, params, 1);
 		break;
 
@@ -790,6 +798,9 @@ int domap(struct multipath *mpp, char *params, int is_daemon)
 		put_multipath_config(conf);
 		if (r) {
 			sysfs_set_max_sectors_kb(mpp, 1);
+			if (mpp->ghost_delay_tick > 0 &&
+			    pathcount(mpp, PATH_UP))
+				mpp->ghost_delay_tick = 0;
 			r = dm_addmap_reload(mpp, params, 0);
 		}
 		break;
diff --git a/libmultipath/defaults.h b/libmultipath/defaults.h
index 740ccf4..c9e3411 100644
--- a/libmultipath/defaults.h
+++ b/libmultipath/defaults.h
@@ -40,6 +40,7 @@
 #define DEFAULT_SKIP_KPARTX SKIP_KPARTX_OFF
 #define DEFAULT_DISABLE_CHANGED_WWIDS 0
 #define DEFAULT_MAX_SECTORS_KB MAX_SECTORS_KB_UNDEF
+#define DEFAULT_GHOST_DELAY GHOST_DELAY_OFF
 
 #define DEFAULT_CHECKINT	5
 #define MAX_CHECKINT(a)		(a << 2)
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index fcac6bc..573fc75 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -378,7 +378,7 @@ static uint16_t build_udev_flags(const struct multipath *mpp, int reload)
 	/* DM_UDEV_DISABLE_LIBRARY_FALLBACK is added in dm_addmap */
 	return	(mpp->skip_kpartx == SKIP_KPARTX_ON ?
 		 MPATH_UDEV_NO_KPARTX_FLAG : 0) |
-		(mpp->nr_active == 0 ?
+		((mpp->nr_active == 0 || mpp->ghost_delay_tick > 0)?
 		 MPATH_UDEV_NO_PATHS_FLAG : 0) |
 		(reload && !mpp->force_udev_reload ?
 		 MPATH_UDEV_RELOAD_FLAG : 0);
diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index 3b36e1d..e52f1f7 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -1120,6 +1120,14 @@ declare_hw_snprint(marginal_path_double_failed_time, print_off_int_undef)
 declare_mp_handler(marginal_path_double_failed_time, set_off_int_undef)
 declare_mp_snprint(marginal_path_double_failed_time, print_off_int_undef)
 
+declare_def_handler(ghost_delay, set_off_int_undef)
+declare_def_snprint(ghost_delay, print_off_int_undef)
+declare_ovr_handler(ghost_delay, set_off_int_undef)
+declare_ovr_snprint(ghost_delay, print_off_int_undef)
+declare_hw_handler(ghost_delay, set_off_int_undef)
+declare_hw_snprint(ghost_delay, print_off_int_undef)
+declare_mp_handler(ghost_delay, set_off_int_undef)
+declare_mp_snprint(ghost_delay, print_off_int_undef)
 
 
 static int
@@ -1469,6 +1477,7 @@ init_keywords(vector keywords)
 	install_keyword("disable_changed_wwids", &def_disable_changed_wwids_handler, &snprint_def_disable_changed_wwids);
 	install_keyword("remove_retries", &def_remove_retries_handler, &snprint_def_remove_retries);
 	install_keyword("max_sectors_kb", &def_max_sectors_kb_handler, &snprint_def_max_sectors_kb);
+	install_keyword("ghost_delay", &def_ghost_delay_handler, &snprint_def_ghost_delay);
 	__deprecated install_keyword("default_selector", &def_selector_handler, NULL);
 	__deprecated install_keyword("default_path_grouping_policy", &def_pgpolicy_handler, NULL);
 	__deprecated install_keyword("default_uid_attribute", &def_uid_attribute_handler, NULL);
@@ -1549,6 +1558,7 @@ init_keywords(vector keywords)
 	install_keyword("marginal_path_double_failed_time", &hw_marginal_path_double_failed_time_handler, &snprint_hw_marginal_path_double_failed_time);
 	install_keyword("skip_kpartx", &hw_skip_kpartx_handler, &snprint_hw_skip_kpartx);
 	install_keyword("max_sectors_kb", &hw_max_sectors_kb_handler, &snprint_hw_max_sectors_kb);
+	install_keyword("ghost_delay", &hw_ghost_delay_handler, &snprint_hw_ghost_delay);
 	install_sublevel_end();
 
 	install_keyword_root("overrides", &overrides_handler);
@@ -1584,6 +1594,7 @@ init_keywords(vector keywords)
 
 	install_keyword("skip_kpartx", &ovr_skip_kpartx_handler, &snprint_ovr_skip_kpartx);
 	install_keyword("max_sectors_kb", &ovr_max_sectors_kb_handler, &snprint_ovr_max_sectors_kb);
+	install_keyword("ghost_delay", &ovr_ghost_delay_handler, &snprint_ovr_ghost_delay);
 
 	install_keyword_root("multipaths", &multipaths_handler);
 	install_keyword_multi("multipath", &multipath_handler, NULL);
@@ -1616,5 +1627,6 @@ init_keywords(vector keywords)
 	install_keyword("marginal_path_double_failed_time", &mp_marginal_path_double_failed_time_handler, &snprint_mp_marginal_path_double_failed_time);
 	install_keyword("skip_kpartx", &mp_skip_kpartx_handler, &snprint_mp_skip_kpartx);
 	install_keyword("max_sectors_kb", &mp_max_sectors_kb_handler, &snprint_mp_max_sectors_kb);
+	install_keyword("ghost_delay", &mp_ghost_delay_handler, &snprint_mp_ghost_delay);
 	install_sublevel_end();
 }
diff --git a/libmultipath/hwtable.c b/libmultipath/hwtable.c
index 40c4724..448effe 100644
--- a/libmultipath/hwtable.c
+++ b/libmultipath/hwtable.c
@@ -72,6 +72,7 @@
 		.delay_wait_checks = DELAY_CHECKS_OFF,
 		.skip_kpartx   = SKIP_KPARTX_OFF,
 		.max_sectors_kb = MAX_SECTORS_KB_UNDEF,
+		.ghost_delay = GHOST_DELAY_OFF
 	},
 #endif
 
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index 0d29ed2..4404e68 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -863,3 +863,18 @@ out:
 		origin);
 	return 0;
 }
+
+int select_ghost_delay (struct config *conf, struct multipath * mp)
+{
+	char *origin, buff[12];
+
+	mp_set_mpe(ghost_delay);
+	mp_set_ovr(ghost_delay);
+	mp_set_hwe(ghost_delay);
+	mp_set_conf(ghost_delay);
+	mp_set_default(ghost_delay, DEFAULT_GHOST_DELAY);
+out:
+	print_off_int_undef(buff, 12, &mp->ghost_delay);
+	condlog(3, "%s: ghost_delay = %s %s", mp->alias, buff, origin);
+	return 0;
+}
diff --git a/libmultipath/propsel.h b/libmultipath/propsel.h
index 347cb32..136f906 100644
--- a/libmultipath/propsel.h
+++ b/libmultipath/propsel.h
@@ -29,6 +29,7 @@ int select_marginal_path_err_sample_time(struct config *conf, struct multipath *
 int select_marginal_path_err_rate_threshold(struct config *conf, struct multipath *mp);
 int select_marginal_path_err_recheck_gap_time(struct config *conf, struct multipath *mp);
 int select_marginal_path_double_failed_time(struct config *conf, struct multipath *mp);
+int select_ghost_delay(struct config *conf, struct multipath * mp);
 void reconcile_features_with_options(const char *id, char **features,
 				     int* no_path_retry,
 				     int *retain_hwhandler);
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index c2cf3fb..4d738d2 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -167,6 +167,11 @@ enum no_undef_states {
 	NU_UNDEF = 0,
 };
 
+enum ghost_delay_states {
+	GHOST_DELAY_OFF = NU_NO,
+	GHOST_DELAY_UNDEF = NU_UNDEF,
+};
+
 enum initialized_states {
 	INIT_FAILED,
 	INIT_MISSING_UDEV,
@@ -283,6 +288,8 @@ struct multipath {
 	int max_sectors_kb;
 	int force_readonly;
 	int force_udev_reload;
+	int ghost_delay;
+	int ghost_delay_tick;
 	unsigned int dev_loss;
 	uid_t uid;
 	gid_t gid;
diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
index 36551b4..ba5d3bc 100644
--- a/multipath/multipath.conf.5
+++ b/multipath/multipath.conf.5
@@ -1046,6 +1046,19 @@ The default is: \fB<device dependent>\fR
 .RE
 .
 .
+.TP
+.B ghost_delay
+Sets the number of seconds that multipath will wait after creating a device
+with only ghost paths before marking it ready for use in systemd. This gives
+the active paths time to appear before the multipath runs the hardware handler
+to switch the ghost paths to active ones. Setting this to \fI0\fR or \fIon\fR
+makes multipath immediately mark a device with only ghost paths as ready.
+.RS
+.TP
+The default is \fBno\fR
+.RE
+.
+.
 .\" ----------------------------------------------------------------------------
 .SH "blacklist section"
 .\" ----------------------------------------------------------------------------
@@ -1188,6 +1201,8 @@ are taken from the \fIdefaults\fR or \fIdevices\fR section:
 .B skip_kpartx
 .TP
 .B max_sectors_kb
+.TP
+.B ghost_delay
 .RE
 .PD
 .LP
@@ -1317,6 +1332,8 @@ section:
 .B skip_kpartx
 .TP
 .B max_sectors_kb
+.TP
+.B ghost_delay
 .RE
 .PD
 .LP
@@ -1389,6 +1406,8 @@ the values are taken from the \fIdevices\fR or \fIdefaults\fR sections:
 .B delay_wait_checks
 .TP
 .B skip_kpartx
+.TP
+.B ghost_delay
 .RE
 .PD
 .LP
diff --git a/multipathd/main.c b/multipathd/main.c
index 31ce923..25f1f52 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -354,6 +354,8 @@ sync_map_state(struct multipath *mpp)
 			    pp->state == PATH_WILD ||
 			    pp->state == PATH_DELAYED)
 				continue;
+			if (mpp->ghost_delay_tick > 0)
+				continue;
 			if ((pp->dmstate == PSTATE_FAILED ||
 			     pp->dmstate == PSTATE_UNDEF) &&
 			    (pp->state == PATH_UP || pp->state == PATH_GHOST))
@@ -738,7 +740,8 @@ ev_add_path (struct path * pp, struct vectors * vecs, int need_do_map)
 	mpp = find_mp_by_wwid(vecs->mpvec, pp->wwid);
 	if (mpp && mpp->wait_for_udev &&
 	    (pathcount(mpp, PATH_UP) > 0 ||
-	     (pathcount(mpp, PATH_GHOST) > 0 && pp->tpgs != TPGS_IMPLICIT))) {
+	     (pathcount(mpp, PATH_GHOST) > 0 && pp->tpgs != TPGS_IMPLICIT &&
+	      mpp->ghost_delay_tick <= 0))) {
 		/* if wait_for_udev is set and valid paths exist */
 		condlog(2, "%s: delaying path addition until %s is fully initialized", pp->dev, mpp->alias);
 		mpp->wait_for_udev = 2;
@@ -1463,6 +1466,28 @@ missing_uev_wait_tick(struct vectors *vecs)
 }
 
 static void
+ghost_delay_tick(struct vectors *vecs)
+{
+	struct multipath * mpp;
+	unsigned int i;
+
+	vector_foreach_slot (vecs->mpvec, mpp, i) {
+		if (mpp->ghost_delay_tick <= 0)
+			continue;
+		if (--mpp->ghost_delay_tick <= 0) {
+			condlog(0, "%s: timed out waiting for active path",
+				mpp->alias);
+			mpp->force_udev_reload = 1;
+			if (update_map(mpp, vecs) != 0) {
+				/* update_map removed map */
+				i--;
+				continue;
+			}
+		}
+	}
+}
+
+static void
 defered_failback_tick (vector mpvec)
 {
 	struct multipath * mpp;
@@ -1935,6 +1960,7 @@ checkerloop (void *ap)
 		defered_failback_tick(vecs->mpvec);
 		retry_count_tick(vecs->mpvec);
 		missing_uev_wait_tick(vecs);
+		ghost_delay_tick(vecs);
 		lock_cleanup_pop(vecs->lock);
 
 		if (count)
-- 
2.7.4

[PATCH 02/12] kpartx: don't delete partitions from partitions

[Benjamin Marzinski]

The current del-part-nodes rules try to run partx on the partitions
themselves, which will ofen fail with an error in the log, because the
partitions will have been deleted before partx can run on them.

Cc: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 kpartx/del-part-nodes.rules | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kpartx/del-part-nodes.rules b/kpartx/del-part-nodes.rules
index cee945d..17bc505 100644
--- a/kpartx/del-part-nodes.rules
+++ b/kpartx/del-part-nodes.rules
@@ -12,6 +12,7 @@
 SUBSYSTEM!="block", GOTO="end_del_part_nodes"
 KERNEL!="sd*|dasd*|rbd*", GOTO="end_del_part_nodes"
 ACTION!="add|change", GOTO="end_del_part_nodes"
+ENV{DEVTYPE}=="partition", GOTO="end_del_part_nodes"
 
 IMPORT{cmdline}="dont_del_part_nodes"
 ENV{dont_del_part_nodes}=="1", GOTO="end_del_part_nodes"
-- 
2.7.4

[PATCH 03/12] multipath: fix hwhandler check in select_action

[Benjamin Marzinski]

If the existing multipath table does not have a hardware handler set,
then even if retain_hwhandler is enabled on the new table, it may still
be possible to set the hardware handler on reload. So, adding a
hardware handler to the table should trigger a reload in this case.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/configure.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 09cbe3c..0dfa250 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -563,7 +563,8 @@ select_action (struct multipath * mpp, vector curmp, int force_reload)
 			mpp->alias);
 		return;
 	}
-	if (mpp->retain_hwhandler != RETAIN_HWHANDLER_ON &&
+	if ((mpp->retain_hwhandler != RETAIN_HWHANDLER_ON ||
+	     strcmp(cmpp->hwhandler, "0") == 0) &&
 	    (strlen(cmpp->hwhandler) != strlen(mpp->hwhandler) ||
 	     strncmp(cmpp->hwhandler, mpp->hwhandler,
 		    strlen(mpp->hwhandler)))) {
-- 
2.7.4

[PATCH 04/12] libmultipath: cleanup features handling code

[Benjamin Marzinski]

Right now multipath does some extra work to set the values for
no_path_retry and retain_hwhandler on existing maps it reads in from the
kernel.  This is so that select_action() can use these values to see if
it needs to reload the devices. However, the way it works, the
queue_if_no_path feature isn't always set correctly, and multipath has
to go back afterwards and reset the value anyways.

It's simpler for select_action to just look at the values in the
features line it read in from the kernel and the features line it would
like the new map to have.  By comparing these, it also avoids the
problem where the no_path_retry values match, so it doesn't reload, but
the actual queue_if_no_path feature value is incorrect, so it has to go
back and reset it. It can also skip calling setup_feature() entirely.

To do this, assemble_map() needs to update mp->features. This would
otherwise partially happen when it had to reset the queue_if_no_path
value.  retain_attached_hw_handler was never getting updated before, so
the output when you created a map was incorrect.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/configure.c | 21 ++++-----------------
 libmultipath/dmparser.c  | 35 ++++++-----------------------------
 libmultipath/structs.c   | 17 -----------------
 libmultipath/structs.h   |  1 -
 4 files changed, 10 insertions(+), 64 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 0dfa250..7ca84b8 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -557,7 +557,8 @@ select_action (struct multipath * mpp, vector curmp, int force_reload)
 	}
 
 	if (mpp->no_path_retry != NO_PATH_RETRY_UNDEF &&
-	    mpp->no_path_retry != cmpp->no_path_retry) {
+	    !!strstr(mpp->features, "queue_if_no_path") !=
+	    !!strstr(cmpp->features, "queue_if_no_path")) {
 		mpp->action =  ACT_RELOAD;
 		condlog(3, "%s: set ACT_RELOAD (no_path_retry change)",
 			mpp->alias);
@@ -575,7 +576,8 @@ select_action (struct multipath * mpp, vector curmp, int force_reload)
 	}
 
 	if (mpp->retain_hwhandler != RETAIN_HWHANDLER_UNDEF &&
-	    mpp->retain_hwhandler != cmpp->retain_hwhandler &&
+	    !!strstr(mpp->features, "retain_attached_hw_handler") !=
+	    !!strstr(cmpp->features, "retain_attached_hw_handler") &&
 	    get_linux_version_code() < KERNEL_VERSION(4, 3, 0)) {
 		mpp->action = ACT_RELOAD;
 		condlog(3, "%s: set ACT_RELOAD (retain_hwhandler change)",
@@ -1060,21 +1062,6 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
 				remove_feature(&mpp->features,
 					       "queue_if_no_path");
 		}
-		else if (mpp->no_path_retry != NO_PATH_RETRY_UNDEF) {
-			if (mpp->no_path_retry == NO_PATH_RETRY_FAIL) {
-				condlog(3, "%s: unset queue_if_no_path feature",
-					mpp->alias);
-				if (!dm_queue_if_no_path(mpp->alias, 0))
-					remove_feature(&mpp->features,
-						       "queue_if_no_path");
-			} else {
-				condlog(3, "%s: set queue_if_no_path feature",
-					mpp->alias);
-				if (!dm_queue_if_no_path(mpp->alias, 1))
-					add_feature(&mpp->features,
-						    "queue_if_no_path");
-			}
-		}
 
 		if (!is_daemon && mpp->action != ACT_NOTHING) {
 			conf = get_multipath_config();
diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
index b647c25..642f44f 100644
--- a/libmultipath/dmparser.c
+++ b/libmultipath/dmparser.c
@@ -83,23 +83,15 @@ assemble_map (struct multipath * mp, char * params, int len)
 	nr_priority_groups = VECTOR_SIZE(mp->pg);
 	initial_pg_nr = (nr_priority_groups ? mp->bestpg : 0);
 
-	f = STRDUP(mp->features);
-
-	/*
-	 * We have to set 'queue_if_no_path' here even
-	 * to avoid path failures during map reload.
-	 */
-	if (mp->no_path_retry == NO_PATH_RETRY_FAIL) {
-		/* remove queue_if_no_path settings */
-		condlog(3, "%s: remove queue_if_no_path from '%s'",
-			mp->alias, mp->features);
-		remove_feature(&f, no_path_retry);
-	} else if (mp->no_path_retry != NO_PATH_RETRY_UNDEF) {
-		add_feature(&f, no_path_retry);
+	if (mp->no_path_retry != NO_PATH_RETRY_UNDEF  &&
+	    mp->no_path_retry != NO_PATH_RETRY_FAIL) {
+		add_feature(&mp->features, no_path_retry);
 	}
 	if (mp->retain_hwhandler == RETAIN_HWHANDLER_ON &&
 	    get_linux_version_code() < KERNEL_VERSION(4, 3, 0))
-		add_feature(&f, retain_hwhandler);
+		add_feature(&mp->features, retain_hwhandler);
+
+	f = STRDUP(mp->features);
 
 	APPEND(p, end, "%s %s %i %i", f, mp->hwhandler, nr_priority_groups,
 	       initial_pg_nr);
@@ -148,7 +140,6 @@ int disassemble_map(vector pathvec, char *params, struct multipath *mpp,
 	int num_paths = 0;
 	int num_paths_args = 0;
 	int def_minio = 0;
-	int no_path_retry = NO_PATH_RETRY_UNDEF;
 	struct path * pp;
 	struct pathgroup * pgp;
 
@@ -165,8 +156,6 @@ int disassemble_map(vector pathvec, char *params, struct multipath *mpp,
 		return 1;
 
 	num_features = atoi(mpp->features);
-	no_path_retry = mpp->no_path_retry;
-	mpp->no_path_retry = NO_PATH_RETRY_UNDEF;
 
 	for (i = 0; i < num_features; i++) {
 		p += get_word(p, &word);
@@ -178,23 +167,11 @@ int disassemble_map(vector pathvec, char *params, struct multipath *mpp,
 			FREE(word);
 			return 1;
 		}
-		setup_feature(mpp, word);
 
 		FREE(word);
 	}
 
 	/*
-	 * Reset no_path_retry.
-	 * - if not set from features
-	 * - if queue_if_no_path is set from features but
-	 *   no_path_retry > 0 is selected.
-	 */
-	if ((mpp->no_path_retry == NO_PATH_RETRY_UNDEF ||
-	     mpp->no_path_retry == NO_PATH_RETRY_QUEUE) &&
-	    mpp->no_path_retry != no_path_retry)
-		mpp->no_path_retry = no_path_retry;
-
-	/*
 	 * hwhandler
 	 */
 	p += get_word(p, &mpp->hwhandler);
diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index 3e057f5..c42d765 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -497,23 +497,6 @@ first_path (struct multipath * mpp)
 	return pgp?VECTOR_SLOT(pgp->paths, 0):NULL;
 }
 
-void setup_feature(struct multipath *mpp, char *feature)
-{
-	if (!strncmp(feature, "queue_if_no_path", 16)) {
-		if (mpp->no_path_retry <= NO_PATH_RETRY_UNDEF)
-			mpp->no_path_retry = NO_PATH_RETRY_QUEUE;
-		else
-			condlog(1, "%s: ignoring feature queue_if_no_path because no_path_retry = %d",
-				mpp->alias, mpp->no_path_retry);
-	} else if (!strcmp(feature, "retain_attached_hw_handler")) {
-		if (mpp->retain_hwhandler != RETAIN_HWHANDLER_OFF)
-			mpp->retain_hwhandler = RETAIN_HWHANDLER_ON;
-		else
-			condlog(1, "%s: ignoring feature 'retain_attached_hw_handler'",
-				mpp->alias);
-	}
-}
-
 int add_feature(char **f, const char *n)
 {
 	int c = 0, d, l;
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 4d738d2..effa52f 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -386,7 +386,6 @@ struct path * first_path (struct multipath * mpp);
 int pathcountgr (struct pathgroup *, int);
 int pathcount (struct multipath *, int);
 int pathcmp (struct pathgroup *, struct pathgroup *);
-void setup_feature(struct multipath *, char *);
 int add_feature (char **, const char *);
 int remove_feature (char **, const char *);
 
-- 
2.7.4

[PATCH 05/12] multipathd: move helper functions to libmultipath

[Benjamin Marzinski]

This commit simply moves sync_map_state() and update_map() from
multipathd/main.c to libmultipath/structs_vec.c, to enable future
changes.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/structs_vec.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++
 libmultipath/structs_vec.h |  2 ++
 multipathd/main.c          | 72 --------------------------------------------
 multipathd/main.h          |  1 -
 4 files changed, 76 insertions(+), 73 deletions(-)

diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 22be8e0..eddeeaf 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -16,6 +16,8 @@
 #include "propsel.h"
 #include "discovery.h"
 #include "prio.h"
+#include "configure.h"
+#include "libdevmapper.h"
 
 /*
  * creates or updates mpp->paths reading mpp->pg
@@ -415,6 +417,78 @@ out:
 	return 1;
 }
 
+void
+sync_map_state(struct multipath *mpp)
+{
+	struct pathgroup *pgp;
+	struct path *pp;
+	unsigned int i, j;
+
+	if (!mpp->pg)
+		return;
+
+	vector_foreach_slot (mpp->pg, pgp, i){
+		vector_foreach_slot (pgp->paths, pp, j){
+			if (pp->state == PATH_UNCHECKED ||
+			    pp->state == PATH_WILD ||
+			    pp->state == PATH_DELAYED)
+				continue;
+			if (mpp->ghost_delay_tick > 0)
+				continue;
+			if ((pp->dmstate == PSTATE_FAILED ||
+			     pp->dmstate == PSTATE_UNDEF) &&
+			    (pp->state == PATH_UP || pp->state == PATH_GHOST))
+				dm_reinstate_path(mpp->alias, pp->dev_t);
+			else if ((pp->dmstate == PSTATE_ACTIVE ||
+				  pp->dmstate == PSTATE_UNDEF) &&
+				 (pp->state == PATH_DOWN ||
+				  pp->state == PATH_SHAKY))
+				dm_fail_path(mpp->alias, pp->dev_t);
+		}
+	}
+}
+
+int
+update_map (struct multipath *mpp, struct vectors *vecs)
+{
+	int retries = 3;
+	char params[PARAMS_SIZE] = {0};
+
+retry:
+	condlog(4, "%s: updating new map", mpp->alias);
+	if (adopt_paths(vecs->pathvec, mpp)) {
+		condlog(0, "%s: failed to adopt paths for new map update",
+			mpp->alias);
+		retries = -1;
+		goto fail;
+	}
+	verify_paths(mpp, vecs);
+	mpp->flush_on_last_del = FLUSH_UNDEF;
+	mpp->action = ACT_RELOAD;
+
+	if (setup_map(mpp, params, PARAMS_SIZE)) {
+		condlog(0, "%s: failed to setup new map in update", mpp->alias);
+		retries = -1;
+		goto fail;
+	}
+	if (domap(mpp, params, 1) <= 0 && retries-- > 0) {
+		condlog(0, "%s: map_udate sleep", mpp->alias);
+		sleep(1);
+		goto retry;
+	}
+	dm_lib_release();
+
+fail:
+	if (setup_multipath(vecs, mpp))
+		return 1;
+
+	sync_map_state(mpp);
+
+	if (retries < 0)
+		condlog(0, "%s: failed reload in new map update", mpp->alias);
+	return 0;
+}
+
 struct multipath *add_map_without_path (struct vectors *vecs, char *alias)
 {
 	struct multipath * mpp = alloc_multipath();
diff --git a/libmultipath/structs_vec.h b/libmultipath/structs_vec.h
index 46f30af..54444e0 100644
--- a/libmultipath/structs_vec.h
+++ b/libmultipath/structs_vec.h
@@ -30,6 +30,8 @@ void remove_map_and_stop_waiter (struct multipath * mpp, struct vectors * vecs,
 void remove_maps (struct vectors * vecs);
 void remove_maps_and_stop_waiters (struct vectors * vecs);
 
+void sync_map_state (struct multipath *);
+int update_map (struct multipath *mpp, struct vectors *vecs);
 struct multipath * add_map_without_path (struct vectors * vecs, char * alias);
 struct multipath * add_map_with_path (struct vectors * vecs,
 				struct path * pp, int add_vec);
diff --git a/multipathd/main.c b/multipathd/main.c
index 25f1f52..93506ea 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -338,37 +338,6 @@ coalesce_maps(struct vectors *vecs, vector nmpv)
 	return 0;
 }
 
-void
-sync_map_state(struct multipath *mpp)
-{
-	struct pathgroup *pgp;
-	struct path *pp;
-	unsigned int i, j;
-
-	if (!mpp->pg)
-		return;
-
-	vector_foreach_slot (mpp->pg, pgp, i){
-		vector_foreach_slot (pgp->paths, pp, j){
-			if (pp->state == PATH_UNCHECKED ||
-			    pp->state == PATH_WILD ||
-			    pp->state == PATH_DELAYED)
-				continue;
-			if (mpp->ghost_delay_tick > 0)
-				continue;
-			if ((pp->dmstate == PSTATE_FAILED ||
-			     pp->dmstate == PSTATE_UNDEF) &&
-			    (pp->state == PATH_UP || pp->state == PATH_GHOST))
-				dm_reinstate_path(mpp->alias, pp->dev_t);
-			else if ((pp->dmstate == PSTATE_ACTIVE ||
-				  pp->dmstate == PSTATE_UNDEF) &&
-				 (pp->state == PATH_DOWN ||
-				  pp->state == PATH_SHAKY))
-				dm_fail_path(mpp->alias, pp->dev_t);
-		}
-	}
-}
-
 static void
 sync_maps_state(vector mpvec)
 {
@@ -416,47 +385,6 @@ flush_map(struct multipath * mpp, struct vectors * vecs, int nopaths)
 	return 0;
 }
 
-int
-update_map (struct multipath *mpp, struct vectors *vecs)
-{
-	int retries = 3;
-	char params[PARAMS_SIZE] = {0};
-
-retry:
-	condlog(4, "%s: updating new map", mpp->alias);
-	if (adopt_paths(vecs->pathvec, mpp)) {
-		condlog(0, "%s: failed to adopt paths for new map update",
-			mpp->alias);
-		retries = -1;
-		goto fail;
-	}
-	verify_paths(mpp, vecs);
-	mpp->flush_on_last_del = FLUSH_UNDEF;
-	mpp->action = ACT_RELOAD;
-
-	if (setup_map(mpp, params, PARAMS_SIZE)) {
-		condlog(0, "%s: failed to setup new map in update", mpp->alias);
-		retries = -1;
-		goto fail;
-	}
-	if (domap(mpp, params, 1) <= 0 && retries-- > 0) {
-		condlog(0, "%s: map_udate sleep", mpp->alias);
-		sleep(1);
-		goto retry;
-	}
-	dm_lib_release();
-
-fail:
-	if (setup_multipath(vecs, mpp))
-		return 1;
-
-	sync_map_state(mpp);
-
-	if (retries < 0)
-		condlog(0, "%s: failed reload in new map update", mpp->alias);
-	return 0;
-}
-
 static int
 uev_add_map (struct uevent * uev, struct vectors * vecs)
 {
diff --git a/multipathd/main.h b/multipathd/main.h
index 094b04f..ededdab 100644
--- a/multipathd/main.h
+++ b/multipathd/main.h
@@ -26,7 +26,6 @@ int ev_add_path (struct path *, struct vectors *, int);
 int ev_remove_path (struct path *, struct vectors *, int);
 int ev_add_map (char *, char *, struct vectors *);
 int ev_remove_map (char *, char *, int, struct vectors *);
-void sync_map_state (struct multipath *);
 int set_config_state(enum daemon_status);
 void * mpath_alloc_prin_response(int prin_sa);
 int prin_do_scsi_ioctl(char *, int rq_servact, struct prin_resp * resp,
-- 
2.7.4

[PATCH 06/12] multipathd: fix device creation issues

[Benjamin Marzinski]

Right now, devices created by multipath and added to multipthd via
ev_add_map() are setup up incorrectly until the first time they get
reloaded.  setup_map() is not run on these devices, which means that all
of the multipath variables set up there don't get initialized until a
later reload.  Also, adopt_paths is run to pull in any paths that the
device is missing, but the device is never reloaded afterwards, so these
paths aren't used.

Now, add_map_without_path() sets up the basic multipath device variables
and then calls update_map() to finish the setup and reload the device.
This patch also moves the code in __setup_multipath(), that only existed
to handle adding devices via ev_add_map(), to where it belongs.

However, it is possible to create a device with no paths, which means
the device cannot know which hwentry to use for its device
configuration.  __setup_multipath() used to help with this via
extract_hwe_from_path(), which grabbed the hwentry from a path if the
multipath device didn't already have it set. This is now done both when
paths are added or the map is updated, which means that
extract_hwe_from_path() runs before the device is configured in
setup_map() instead of after the table has already been loaded. This is
a good thing. But because of this, it can't check the dmstate or the
pathgroup state.  I don't believe it's necessary to care what state the
path is in, especially now that we use libudev. The vendor and product
information gets cached by libudev when the path device is first added,
and should remain the same regardless of whether or not the device is
currently up.  My version does try to take the hwe information from a
path in the PATH_UP state first, but this is mostly to satisfy the
paranoia of the old version.

Also, map_discovery(), which creates multipath devices during multipathd
startup and reconfiguration, that only exist to see if multipathd needs
to reload the device table, called __setup_multipath() as well. Even
after removing the unnecessary code from __setup_multpiath, that still
does pointless work for these devices, which will be removed before the
end of configure(). Now, map_discovery() just gets the necessary kernel
information to make the correct call in select_action().

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/structs_vec.c | 101 ++++++++++++++++-----------------------------
 libmultipath/structs_vec.h |   4 ++
 multipathd/main.c          |   6 ++-
 3 files changed, 45 insertions(+), 66 deletions(-)

diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index eddeeaf..32a4d9e 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -188,66 +188,36 @@ void remove_maps_and_stop_waiters(struct vectors *vecs)
 	_remove_maps(vecs, STOP_WAITER);
 }
 
-static struct hwentry *
+void
 extract_hwe_from_path(struct multipath * mpp)
 {
 	struct path * pp = NULL;
-	int pg_num = -1, p_num = -1, i;
-	struct pathgroup * pgp = NULL;
-
-	condlog(3, "%s: searching paths for valid hwe", mpp->alias);
+	int i;
 
-	if (mpp && mpp->pg) {
-		vector_foreach_slot(mpp->pg, pgp, i) {
-			if (pgp->status == PGSTATE_ACTIVE ||
-			    pgp->status == PGSTATE_ENABLED) {
-				pg_num = i;
-				break;
-			}
-		}
-		if (pg_num >= 0)
-			pgp = VECTOR_SLOT(mpp->pg, pg_num);
-	}
+	if (mpp->hwe || !mpp->paths)
+		return;
 
-	if (pgp && pgp->paths) {
-		vector_foreach_slot(pgp->paths, pp, i) {
-			if (pp->dmstate == PSTATE_FAILED)
-				continue;
-			if (strlen(pp->vendor_id) > 0 &&
-			    strlen(pp->product_id) > 0 &&
-			    strlen(pp->rev) > 0) {
-				p_num = i;
-				break;
-			}
+	condlog(3, "%s: searching paths for valid hwe", mpp->alias);
+	/* doing this in two passes seems like paranoia to me */
+	vector_foreach_slot(mpp->paths, pp, i) {
+		if (pp->state != PATH_UP)
+			continue;
+		if (pp->hwe) {
+			mpp->hwe = pp->hwe;
+			return;
 		}
-		if (p_num >= 0)
-			pp = VECTOR_SLOT(pgp->paths, i);
 	}
-
-	if (pp) {
-		if (!strlen(pp->vendor_id) ||
-		    !strlen(pp->product_id) ||
-		    !strlen(pp->rev)) {
-			condlog(3, "%s: no device details available", pp->dev);
-			return NULL;
-		}
-		condlog(3, "%s: vendor = %s", pp->dev, pp->vendor_id);
-		condlog(3, "%s: product = %s", pp->dev, pp->product_id);
-		condlog(3, "%s: rev = %s", pp->dev, pp->rev);
-		if (!pp->hwe) {
-			struct config *conf = get_multipath_config();
-
-			condlog(3, "searching hwtable");
-			pp->hwe = find_hwe(conf->hwtable, pp->vendor_id,
-					   pp->product_id, pp->rev);
-			put_multipath_config(conf);
+	vector_foreach_slot(mpp->paths, pp, i) {
+		if (pp->state == PATH_UP)
+			continue;
+		if (pp->hwe) {
+			mpp->hwe = pp->hwe;
+			return;
 		}
 	}
-
-	return pp?pp->hwe:NULL;
 }
 
-static int
+int
 update_multipath_table (struct multipath *mpp, vector pathvec, int is_daemon)
 {
 	char params[PARAMS_SIZE] = {0};
@@ -268,7 +238,7 @@ update_multipath_table (struct multipath *mpp, vector pathvec, int is_daemon)
 	return 0;
 }
 
-static int
+int
 update_multipath_status (struct multipath *mpp)
 {
 	char status[PARAMS_SIZE] = {0};
@@ -388,18 +358,6 @@ int __setup_multipath(struct vectors *vecs, struct multipath *mpp,
 		goto out;
 	}
 
-	set_multipath_wwid(mpp);
-	conf = get_multipath_config();
-	mpp->mpe = find_mpe(conf->mptable, mpp->wwid);
-	put_multipath_config(conf);
-	condlog(3, "%s: discover", mpp->alias);
-
-	if (!mpp->hwe)
-		mpp->hwe = extract_hwe_from_path(mpp);
-	if (!mpp->hwe) {
-		condlog(3, "%s: no hardware entry found, using defaults",
-			mpp->alias);
-	}
 	if (reset) {
 		conf = get_multipath_config();
 		select_rr_weight(conf, mpp);
@@ -466,6 +424,7 @@ retry:
 	mpp->flush_on_last_del = FLUSH_UNDEF;
 	mpp->action = ACT_RELOAD;
 
+	extract_hwe_from_path(mpp);
 	if (setup_map(mpp, params, PARAMS_SIZE)) {
 		condlog(0, "%s: failed to setup new map in update", mpp->alias);
 		retries = -1;
@@ -492,6 +451,7 @@ fail:
 struct multipath *add_map_without_path (struct vectors *vecs, char *alias)
 {
 	struct multipath * mpp = alloc_multipath();
+	struct config *conf;
 
 	if (!mpp)
 		return NULL;
@@ -502,10 +462,18 @@ struct multipath *add_map_without_path (struct vectors *vecs, char *alias)
 
 	mpp->alias = STRDUP(alias);
 
-	if (setup_multipath(vecs, mpp))
-		return NULL; /* mpp freed in setup_multipath */
+	if (dm_get_info(mpp->alias, &mpp->dmi)) {
+		condlog(3, "%s: cannot access table", mpp->alias);
+		goto out;
+	}
+	set_multipath_wwid(mpp);
+	conf = get_multipath_config();
+	mpp->mpe = find_mpe(conf->mptable, mpp->wwid);
+	put_multipath_config(conf);
 
-	if (adopt_paths(vecs->pathvec, mpp))
+	if (update_multipath_table(mpp, vecs->pathvec, 1))
+		goto out;
+	if (update_multipath_status(mpp))
 		goto out;
 
 	if (!vector_alloc_slot(vecs->mpvec))
@@ -513,6 +481,9 @@ struct multipath *add_map_without_path (struct vectors *vecs, char *alias)
 
 	vector_set_slot(vecs->mpvec, mpp);
 
+	if (update_map(mpp, vecs) != 0) /* map removed */
+		return NULL;
+
 	if (start_waiter_thread(mpp, vecs))
 		goto out;
 
diff --git a/libmultipath/structs_vec.h b/libmultipath/structs_vec.h
index 54444e0..54865d7 100644
--- a/libmultipath/structs_vec.h
+++ b/libmultipath/structs_vec.h
@@ -24,6 +24,7 @@ int __setup_multipath (struct vectors * vecs, struct multipath * mpp,
 #define setup_multipath(vecs, mpp) __setup_multipath(vecs, mpp, 1, 1)
 int update_multipath_strings (struct multipath *mpp, vector pathvec,
 			      int is_daemon);
+void extract_hwe_from_path(struct multipath * mpp);
 
 void remove_map (struct multipath * mpp, struct vectors * vecs, int purge_vec);
 void remove_map_and_stop_waiter (struct multipath * mpp, struct vectors * vecs, int purge_vec);
@@ -38,5 +39,8 @@ struct multipath * add_map_with_path (struct vectors * vecs,
 int update_multipath (struct vectors *vecs, char *mapname, int reset);
 void update_queue_mode_del_path(struct multipath *mpp);
 void update_queue_mode_add_path(struct multipath *mpp);
+int update_multipath_table (struct multipath *mpp, vector pathvec,
+			    int is_daemon);
+int update_multipath_status (struct multipath *mpp);
 
 #endif /* _STRUCTS_VEC_H */
diff --git a/multipathd/main.c b/multipathd/main.c
index 93506ea..39fedc4 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -699,6 +699,7 @@ rescan:
 		verify_paths(mpp, vecs);
 		mpp->flush_on_last_del = FLUSH_UNDEF;
 		mpp->action = ACT_RELOAD;
+		extract_hwe_from_path(mpp);
 	} else {
 		if (!should_multipath(pp, vecs->pathvec)) {
 			orphan_path(pp, "only one path");
@@ -1045,8 +1046,11 @@ map_discovery (struct vectors * vecs)
 		return 1;
 
 	vector_foreach_slot (vecs->mpvec, mpp, i)
-		if (setup_multipath(vecs, mpp))
+		if (update_multipath_table(mpp, vecs->pathvec, 1) ||
+		    update_multipath_status(mpp)) {
+			remove_map(mpp, vecs, 1);
 			i--;
+		}
 
 	return 0;
 }
-- 
2.7.4

[PATCH 07/12] multipathd: remove select_* from setup_multipath

[Benjamin Marzinski]

select_rr_weight() and select_pgfailback() don't need to be set each
time setup_multipath is called, since nothing ever changes the value
of the multipath variable that they set.

select_flush_on_last_del() and select_no_path retry() are a little more
involved.  In multipathd, it is possible to override no_path_retry by
either setting flush_on_last_del, or by manually running the
"disablequeueing" mutipathd command. Queueing gets restored when a path
gets added back to the multipath device. This was done by moving the
select_* functions into setup_multipath, where they frequently get
called unnecessarily.  select_flush_on_last_del() can get removed by
simply using another variable besides flush_on_last_del to track wether
or not we should be force queueing to be disabled.

Since it's only possible to change whether or not you have queueing
force disabled by reloading the device with path, or by manually
restoring it, there is no reason to call select_no_path_retry() on every
call to setup_multipath

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/configure.c   |  3 +++
 libmultipath/propsel.c     |  6 ++----
 libmultipath/structs.h     |  2 +-
 libmultipath/structs_vec.c |  5 -----
 multipathd/cli_handlers.c  | 22 ++++++++++++++++++----
 multipathd/main.c          |  3 +--
 6 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 7ca84b8..b94e7ea 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -272,6 +272,8 @@ int setup_map(struct multipath *mpp, char *params, int params_size)
 	 * free features, selector, and hwhandler properties if they are being reused
 	 */
 	free_multipath_attributes(mpp);
+	if (mpp->disable_queueing && VECTOR_SIZE(mpp->paths) != 0)
+		mpp->disable_queueing = 0;
 
 	/*
 	 * properties selectors
@@ -302,6 +304,7 @@ int setup_map(struct multipath *mpp, char *params, int params_size)
 	select_skip_kpartx(conf, mpp);
 	select_max_sectors_kb(conf, mpp);
 	select_ghost_delay(conf, mpp);
+	select_flush_on_last_del(conf, mpp);
 
 	sysfs_set_scsi_tmo(mpp, conf->checkint);
 	put_multipath_config(conf);
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index 4404e68..be371fc 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -520,8 +520,8 @@ int select_no_path_retry(struct config *conf, struct multipath *mp)
 	char *origin = NULL;
 	char buff[12];
 
-	if (mp->flush_on_last_del == FLUSH_IN_PROGRESS) {
-		condlog(0, "%s: flush_on_last_del in progress", mp->alias);
+	if (mp->disable_queueing) {
+		condlog(0, "%s: queueing disabled", mp->alias);
 		mp->no_path_retry = NO_PATH_RETRY_FAIL;
 		return 0;
 	}
@@ -613,8 +613,6 @@ int select_flush_on_last_del(struct config *conf, struct multipath *mp)
 {
 	char *origin;
 
-	if (mp->flush_on_last_del == FLUSH_IN_PROGRESS)
-		return 0;
 	mp_set_mpe(flush_on_last_del);
 	mp_set_ovr(flush_on_last_del);
 	mp_set_hwe(flush_on_last_del);
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index effa52f..bfa660a 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -99,7 +99,6 @@ enum flush_states {
 	FLUSH_UNDEF = YNU_UNDEF,
 	FLUSH_DISABLED = YNU_NO,
 	FLUSH_ENABLED = YNU_YES,
-	FLUSH_IN_PROGRESS,
 };
 
 enum log_checker_err_states {
@@ -272,6 +271,7 @@ struct multipath {
 	int nr_active;     /* current available(= not known as failed) paths */
 	int no_path_retry; /* number of retries after all paths are down */
 	int retry_tick;    /* remaining times for retries */
+	int disable_queueing;
 	int minio;
 	int flush_on_last_del;
 	int attribute_flags;
diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 32a4d9e..70fb005 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -311,7 +311,6 @@ void set_no_path_retry(struct config *conf, struct multipath *mpp)
 {
 	mpp->retry_tick = 0;
 	mpp->nr_active = pathcount(mpp, PATH_UP) + pathcount(mpp, PATH_GHOST);
-	select_no_path_retry(conf, mpp);
 
 	switch (mpp->no_path_retry) {
 	case NO_PATH_RETRY_UNDEF:
@@ -360,10 +359,7 @@ int __setup_multipath(struct vectors *vecs, struct multipath *mpp,
 
 	if (reset) {
 		conf = get_multipath_config();
-		select_rr_weight(conf, mpp);
-		select_pgfailback(conf, mpp);
 		set_no_path_retry(conf, mpp);
-		select_flush_on_last_del(conf, mpp);
 		put_multipath_config(conf);
 		if (VECTOR_SIZE(mpp->paths) != 0)
 			dm_cancel_deferred_remove(mpp);
@@ -421,7 +417,6 @@ retry:
 		goto fail;
 	}
 	verify_paths(mpp, vecs);
-	mpp->flush_on_last_del = FLUSH_UNDEF;
 	mpp->action = ACT_RELOAD;
 
 	extract_hwe_from_path(mpp);
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index e232fb2..bf0d9f6 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -22,7 +22,7 @@
 #include <libudev.h>
 #include "util.h"
 #include "prkey.h"
-
+#include "propsel.h"
 #include "main.h"
 #include "cli.h"
 #include "uevent.h"
@@ -973,6 +973,7 @@ cli_restore_queueing(void *v, char **reply, int *len, void *data)
 	char * mapname = get_keyparam(v, MAP);
 	struct multipath *mpp;
 	int minor;
+	struct config *conf;
 
 	mapname = convert_dev(mapname, 0);
 	condlog(2, "%s: restore map queueing (operator)", mapname);
@@ -986,6 +987,11 @@ cli_restore_queueing(void *v, char **reply, int *len, void *data)
 		return 1;
 	}
 
+	conf = get_multipath_config();
+	mpp->disable_queueing = 0;
+	select_no_path_retry(conf, mpp);
+	put_multipath_config(conf);
+
 	if (mpp->no_path_retry != NO_PATH_RETRY_UNDEF &&
 			mpp->no_path_retry != NO_PATH_RETRY_FAIL) {
 		dm_queue_if_no_path(mpp->alias, 1);
@@ -1009,13 +1015,17 @@ cli_restore_all_queueing(void *v, char **reply, int *len, void *data)
 
 	condlog(2, "restore queueing (operator)");
 	vector_foreach_slot(vecs->mpvec, mpp, i) {
+		struct config *conf = get_multipath_config();
+		mpp->disable_queueing = 0;
+		select_no_path_retry(conf, mpp);
+		put_multipath_config(conf);
 		if (mpp->no_path_retry != NO_PATH_RETRY_UNDEF &&
 		    mpp->no_path_retry != NO_PATH_RETRY_FAIL) {
 			dm_queue_if_no_path(mpp->alias, 1);
 			if (mpp->nr_active > 0)
 				mpp->retry_tick = 0;
 			else {
-				struct config *conf = get_multipath_config();
+				conf = get_multipath_config();
 				mpp->retry_tick = mpp->no_path_retry * conf->checkint;
 				put_multipath_config(conf);
 			}
@@ -1046,7 +1056,9 @@ cli_disable_queueing(void *v, char **reply, int *len, void *data)
 
 	if (mpp->nr_active == 0)
 		mpp->stat_map_failures++;
-	mpp->retry_tick = -1;
+	mpp->retry_tick = 0;
+	mpp->no_path_retry = NO_PATH_RETRY_FAIL;
+	mpp->disable_queueing = 1;
 	dm_queue_if_no_path(mpp->alias, 0);
 	return 0;
 }
@@ -1062,7 +1074,9 @@ cli_disable_all_queueing(void *v, char **reply, int *len, void *data)
 	vector_foreach_slot(vecs->mpvec, mpp, i) {
 		if (mpp->nr_active == 0)
 			mpp->stat_map_failures++;
-		mpp->retry_tick = -1;
+		mpp->retry_tick = 0;
+		mpp->no_path_retry = NO_PATH_RETRY_FAIL;
+		mpp->disable_queueing = 1;
 		dm_queue_if_no_path(mpp->alias, 0);
 	}
 	return 0;
diff --git a/multipathd/main.c b/multipathd/main.c
index 39fedc4..a420c83 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -697,7 +697,6 @@ rescan:
 			goto fail; /* leave path added to pathvec */
 
 		verify_paths(mpp, vecs);
-		mpp->flush_on_last_del = FLUSH_UNDEF;
 		mpp->action = ACT_RELOAD;
 		extract_hwe_from_path(mpp);
 	} else {
@@ -852,7 +851,7 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
 				condlog(2, "%s Last path deleted, disabling queueing", mpp->alias);
 				mpp->retry_tick = 0;
 				mpp->no_path_retry = NO_PATH_RETRY_FAIL;
-				mpp->flush_on_last_del = FLUSH_IN_PROGRESS;
+				mpp->disable_queueing = 1;
 				mpp->stat_map_failures++;
 				dm_queue_if_no_path(mpp->alias, 0);
 			}
-- 
2.7.4

[PATCH 08/12] libmultipath: __setup_multipath param cleanup

[Benjamin Marzinski]

setup multipath is only called by the daemon, so there is no point in
the is_daemon parameter.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/structs_vec.c | 6 +++---
 libmultipath/structs_vec.h | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 70fb005..814a4b9 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -336,7 +336,7 @@ void set_no_path_retry(struct config *conf, struct multipath *mpp)
 }
 
 int __setup_multipath(struct vectors *vecs, struct multipath *mpp,
-		      int reset, int is_daemon)
+		      int reset)
 {
 	struct config *conf;
 
@@ -352,7 +352,7 @@ int __setup_multipath(struct vectors *vecs, struct multipath *mpp,
 		goto out;
 	}
 
-	if (update_multipath_strings(mpp, vecs->pathvec, is_daemon)) {
+	if (update_multipath_strings(mpp, vecs->pathvec, 1)) {
 		condlog(0, "%s: failed to setup multipath", mpp->alias);
 		goto out;
 	}
@@ -594,7 +594,7 @@ int update_multipath (struct vectors *vecs, char *mapname, int reset)
 		return 2;
 	}
 
-	if (__setup_multipath(vecs, mpp, reset, 1))
+	if (__setup_multipath(vecs, mpp, reset))
 		return 1; /* mpp freed in setup_multipath */
 
 	/*
diff --git a/libmultipath/structs_vec.h b/libmultipath/structs_vec.h
index 54865d7..04fa0dd 100644
--- a/libmultipath/structs_vec.h
+++ b/libmultipath/structs_vec.h
@@ -20,8 +20,8 @@ void orphan_path (struct path * pp, const char *reason);
 int verify_paths(struct multipath * mpp, struct vectors * vecs);
 int update_mpp_paths(struct multipath * mpp, vector pathvec);
 int __setup_multipath (struct vectors * vecs, struct multipath * mpp,
-		       int reset, int is_daemon);
-#define setup_multipath(vecs, mpp) __setup_multipath(vecs, mpp, 1, 1)
+		       int reset);
+#define setup_multipath(vecs, mpp) __setup_multipath(vecs, mpp, 1)
 int update_multipath_strings (struct multipath *mpp, vector pathvec,
 			      int is_daemon);
 void extract_hwe_from_path(struct multipath * mpp);
-- 
2.7.4

[PATCH 09/12] multipathd: move recovery mode code to function

[Benjamin Marzinski]

Instead of having multiple functions all running slightly different
code to move a multipath device to recovery mode, there is now
an enter_recovery_mode() function that all of them call.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/structs_vec.c | 52 ++++++++++++++++++++--------------------------
 libmultipath/structs_vec.h |  2 +-
 multipathd/cli_handlers.c  | 14 ++++---------
 3 files changed, 28 insertions(+), 40 deletions(-)

diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 814a4b9..2f1f277 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -307,7 +307,23 @@ update_multipath_strings(struct multipath *mpp, vector pathvec, int is_daemon)
 	return 0;
 }
 
-void set_no_path_retry(struct config *conf, struct multipath *mpp)
+void enter_recovery_mode(struct multipath *mpp)
+{
+	struct config *conf = get_multipath_config();
+
+	/*
+	 * Enter retry mode.
+	 * meaning of +1: retry_tick may be decremented in checkerloop before
+	 * starting retry.
+	 */
+	mpp->stat_queueing_timeouts++;
+	mpp->retry_tick = mpp->no_path_retry * conf->checkint + 1;
+	condlog(1, "%s: Entering recovery mode: max_retries=%d",
+		mpp->alias, mpp->no_path_retry);
+	put_multipath_config(conf);
+}
+
+static void set_no_path_retry(struct multipath *mpp)
 {
 	mpp->retry_tick = 0;
 	mpp->nr_active = pathcount(mpp, PATH_UP) + pathcount(mpp, PATH_GHOST);
@@ -323,14 +339,8 @@ void set_no_path_retry(struct config *conf, struct multipath *mpp)
 		break;
 	default:
 		dm_queue_if_no_path(mpp->alias, 1);
-		if (mpp->nr_active == 0) {
-			struct config *conf = get_multipath_config();
-			/* Enter retry mode */
-			mpp->retry_tick = mpp->no_path_retry * conf->checkint;
-			condlog(1, "%s: Entering recovery mode: max_retries=%d",
-				mpp->alias, mpp->no_path_retry);
-			put_multipath_config(conf);
-		}
+		if (mpp->nr_active == 0)
+			enter_recovery_mode(mpp);
 		break;
 	}
 }
@@ -338,8 +348,6 @@ void set_no_path_retry(struct config *conf, struct multipath *mpp)
 int __setup_multipath(struct vectors *vecs, struct multipath *mpp,
 		      int reset)
 {
-	struct config *conf;
-
 	if (dm_get_info(mpp->alias, &mpp->dmi)) {
 		/* Error accessing table */
 		condlog(3, "%s: cannot access table", mpp->alias);
@@ -358,9 +366,7 @@ int __setup_multipath(struct vectors *vecs, struct multipath *mpp,
 	}
 
 	if (reset) {
-		conf = get_multipath_config();
-		set_no_path_retry(conf, mpp);
-		put_multipath_config(conf);
+		set_no_path_retry(mpp);
 		if (VECTOR_SIZE(mpp->paths) != 0)
 			dm_cancel_deferred_remove(mpp);
 	}
@@ -638,21 +644,9 @@ int update_multipath (struct vectors *vecs, char *mapname, int reset)
 void update_queue_mode_del_path(struct multipath *mpp)
 {
 	if (--mpp->nr_active == 0) {
-		if (mpp->no_path_retry > 0) {
-			struct config *conf = get_multipath_config();
-
-			/*
-			 * Enter retry mode.
-			 * meaning of +1: retry_tick may be decremented in
-			 *                checkerloop before starting retry.
-			 */
-			mpp->stat_queueing_timeouts++;
-			mpp->retry_tick = mpp->no_path_retry *
-					  conf->checkint + 1;
-			condlog(1, "%s: Entering recovery mode: max_retries=%d",
-				mpp->alias, mpp->no_path_retry);
-			put_multipath_config(conf);
-		} else if (mpp->no_path_retry != NO_PATH_RETRY_QUEUE)
+		if (mpp->no_path_retry > 0)
+			enter_recovery_mode(mpp);
+		else if (mpp->no_path_retry != NO_PATH_RETRY_QUEUE)
 			mpp->stat_map_failures++;
 	}
 	condlog(2, "%s: remaining active paths: %d", mpp->alias, mpp->nr_active);
diff --git a/libmultipath/structs_vec.h b/libmultipath/structs_vec.h
index 04fa0dd..b81413b 100644
--- a/libmultipath/structs_vec.h
+++ b/libmultipath/structs_vec.h
@@ -11,7 +11,7 @@ struct vectors {
 	vector mpvec;
 };
 
-void set_no_path_retry(struct config *conf, struct multipath *mpp);
+void enter_recovery_mode(struct multipath *mpp);
 
 int adopt_paths (vector pathvec, struct multipath * mpp);
 void orphan_paths (vector pathvec, struct multipath * mpp);
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index bf0d9f6..7f13bc9 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -997,11 +997,8 @@ cli_restore_queueing(void *v, char **reply, int *len, void *data)
 		dm_queue_if_no_path(mpp->alias, 1);
 		if (mpp->nr_active > 0)
 			mpp->retry_tick = 0;
-		else {
-			struct config *conf = get_multipath_config();
-			mpp->retry_tick = mpp->no_path_retry * conf->checkint;
-			put_multipath_config(conf);
-		}
+		else
+			enter_recovery_mode(mpp);
 	}
 	return 0;
 }
@@ -1024,11 +1021,8 @@ cli_restore_all_queueing(void *v, char **reply, int *len, void *data)
 			dm_queue_if_no_path(mpp->alias, 1);
 			if (mpp->nr_active > 0)
 				mpp->retry_tick = 0;
-			else {
-				conf = get_multipath_config();
-				mpp->retry_tick = mpp->no_path_retry * conf->checkint;
-				put_multipath_config(conf);
-			}
+			else
+				enter_recovery_mode(mpp);
 		}
 	}
 	return 0;
-- 
2.7.4

[PATCH 10/12] multipathd: clean up set_no_path_retry

[Benjamin Marzinski]

set_no_path_retry() was always updating the queue_if_no_path setting,
even if it was unnecessary.  This caused problems when no_path_retry was
set to a number, where any time __setup_multipath() was called with
reset, queuing automatically restarted, even if there were no usable
paths.

Instead, set_no_path_retry() should just fix the queueing if it is set
to an incorrect value.  This is simple to detect if no_path_retry is set
to "queue" or "fail".  When it is set to a number, multipathd needs to
see if there are usable paths. If so, queueing should be enabled.  If
not, queueing may be either enabled or disabled, but if it is enabled
and the device isn't already in recovery mode, it should be.

Also, calling dm_map_present() is pointless in __setup_multipath(),
since the existence of the map was just checked in dm_get_info().

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/structs_vec.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 2f1f277..fbab61f 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -325,21 +325,27 @@ void enter_recovery_mode(struct multipath *mpp)
 
 static void set_no_path_retry(struct multipath *mpp)
 {
-	mpp->retry_tick = 0;
+	char is_queueing = 0;
+
 	mpp->nr_active = pathcount(mpp, PATH_UP) + pathcount(mpp, PATH_GHOST);
+	if (mpp->features && strstr(mpp->features, "queue_if_no_path"))
+		is_queueing = 1;
 
 	switch (mpp->no_path_retry) {
 	case NO_PATH_RETRY_UNDEF:
 		break;
 	case NO_PATH_RETRY_FAIL:
-		dm_queue_if_no_path(mpp->alias, 0);
+		if (is_queueing)
+			dm_queue_if_no_path(mpp->alias, 0);
 		break;
 	case NO_PATH_RETRY_QUEUE:
-		dm_queue_if_no_path(mpp->alias, 1);
+		if (!is_queueing)
+			dm_queue_if_no_path(mpp->alias, 1);
 		break;
 	default:
-		dm_queue_if_no_path(mpp->alias, 1);
-		if (mpp->nr_active == 0)
+		if (mpp->nr_active > 0)
+			dm_queue_if_no_path(mpp->alias, 1);
+		else if (is_queueing && mpp->retry_tick == 0)
 			enter_recovery_mode(mpp);
 		break;
 	}
@@ -354,12 +360,6 @@ int __setup_multipath(struct vectors *vecs, struct multipath *mpp,
 		goto out;
 	}
 
-	if (!dm_map_present(mpp->alias)) {
-		/* Table has been removed */
-		condlog(3, "%s: table does not exist", mpp->alias);
-		goto out;
-	}
-
 	if (update_multipath_strings(mpp, vecs->pathvec, 1)) {
 		condlog(0, "%s: failed to setup multipath", mpp->alias);
 		goto out;
-- 
2.7.4

[PATCH 11/12] multipath: check failed path dmstate in check_path

[Benjamin Marzinski]

If a path's checker state is down before and after a path check, but the
path's dmstate is active, mutipath won't update the dmstate. It only
updates the dmstate when the path first fails.  This can cause the
kernel to try known faulty paths, if the multipath device was reloaded
outside of multipathd.  check_path() already checks for and deals with a
similar case where the path's checker state is up before and after a
path check, but the dmstate is failed.  It should do the same thing for
faulty paths.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index a420c83..16e4fdf 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1770,16 +1770,21 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
 			pp->tick = pp->checkint;
 		}
 	}
-	else if (newstate == PATH_DOWN) {
-		int log_checker_err;
+	else if (newstate != PATH_UP && newstate != PATH_GHOST) {
+		if (pp->dmstate == PSTATE_ACTIVE ||
+		    pp->dmstate == PSTATE_UNDEF)
+			fail_path(pp, 0);
+		if (newstate == PATH_DOWN) {
+			int log_checker_err;
 
-		conf = get_multipath_config();
-		log_checker_err = conf->log_checker_err;
-		put_multipath_config(conf);
-		if (log_checker_err == LOG_CHKR_ERR_ONCE)
-			LOG_MSG(3, checker_message(&pp->checker));
-		else
-			LOG_MSG(2, checker_message(&pp->checker));
+			conf = get_multipath_config();
+			log_checker_err = conf->log_checker_err;
+			put_multipath_config(conf);
+			if (log_checker_err == LOG_CHKR_ERR_ONCE)
+				LOG_MSG(3, checker_message(&pp->checker));
+			else
+				LOG_MSG(2, checker_message(&pp->checker));
+		}
 	}
 
 	pp->state = newstate;
-- 
2.7.4

[PATCH 12/12] multipathd: marginal path code fixes

[Benjamin Marzinski]

There are a couple of issues I noticed with the marginal paths code.

In hit_io_err_recheck_time() there are some problems with the initial
checks. We should always recover the path if there are no other usable
paths to the device, so this check should be first. Also, we just
checked that io_err_disable_reinstate isn't zero before calling this
function, so we don't need to check again here (and it doesn't make any
sense to continue disabling the path if io_err_disable_reinstate is set
to zero).  Finally, the only kind of errors we can get while calling
clock_gettime() are going to happen on every call. So, if we can't get
the time, assume that the timeout has passed.

The multipath.conf.5 description of marginal_path_err_sample_time,
states that sampling is stopped for marginal_path_err_rate_threshold
seconds, when it should be marginal_path_err_recheck_gap_time
seconds.

Lastly, there is a race that can cause multipathd to access freed memory
on shutdown. io_err_stat_thr is started as a detached thread. This means
that stop_io_err_stat_thread() can't know when it has actually stopped,
after pthread_cancel() and pthread_kill() are called. To be safe, it
should not start the thread in a deteched state, and call join to verify
that it has stopped before freeing the memory it uses.  But more
importantly, stop_io_err_stat_thread() was being called before the
checker and uevent threads were being canceled. Both of these threads
access data that is freed in stop_io_err_stat_thread(). To avoid the
chance of these threads accessing freed memory, child() should wait
until these threads are stopped before calling
stop_io_err_stat_thread().

Cc: Guan Junxiong <guanjunxiong@huawei.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/io_err_stat.c | 12 +++++-------
 multipath/multipath.conf.5 |  2 +-
 multipathd/main.c          |  6 +++---
 3 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/libmultipath/io_err_stat.c b/libmultipath/io_err_stat.c
index 75a6df6..5b10f03 100644
--- a/libmultipath/io_err_stat.c
+++ b/libmultipath/io_err_stat.c
@@ -379,17 +379,14 @@ int hit_io_err_recheck_time(struct path *pp)
 	struct timespec curr_time;
 	int r;
 
-	if (pp->io_err_disable_reinstate == 0)
-		return 1;
-	if (clock_gettime(CLOCK_MONOTONIC, &curr_time) != 0)
-		return 1;
-	if (pp->io_err_pathfail_cnt != PATH_IO_ERR_IN_POLLING_RECHECK)
-		return 1;
 	if (pp->mpp->nr_active <= 0) {
 		io_err_stat_log(2, "%s: recover path early", pp->dev);
 		goto recover;
 	}
-	if ((curr_time.tv_sec - pp->io_err_dis_reinstate_time) >
+	if (pp->io_err_pathfail_cnt != PATH_IO_ERR_IN_POLLING_RECHECK)
+		return 1;
+	if (clock_gettime(CLOCK_MONOTONIC, &curr_time) != 0 ||
+	    (curr_time.tv_sec - pp->io_err_dis_reinstate_time) >
 			pp->mpp->marginal_path_err_recheck_gap_time) {
 		io_err_stat_log(4, "%s: reschedule checking after %d seconds",
 				pp->dev,
@@ -738,6 +735,7 @@ void stop_io_err_stat_thread(void)
 {
 	pthread_cancel(io_err_stat_thr);
 	pthread_kill(io_err_stat_thr, SIGUSR2);
+	pthread_join(io_err_stat_thr, NULL);
 	free_io_err_pathvec(paths);
 	io_destroy(ioctx);
 }
diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
index ba5d3bc..ab151e7 100644
--- a/multipath/multipath.conf.5
+++ b/multipath/multipath.conf.5
@@ -867,7 +867,7 @@ accounting process for the path will last for
 \fImarginal_path_err_sample_time\fR.
 If the rate of IO error on a particular path is greater than the
 \fImarginal_path_err_rate_threshold\fR, then the path will not reinstate for
-\fImarginal_path_err_rate_threshold\fR seconds unless there is only one
+\fImarginal_path_err_recheck_gap_time\fR seconds unless there is only one
 active path. After \fImarginal_path_err_recheck_gap_time\fR expires, the path
 will be requeueed for rechecking. If checking result is good enough, the
 path will be reinstated.
diff --git a/multipathd/main.c b/multipathd/main.c
index 16e4fdf..0703ca0 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2332,7 +2332,7 @@ child (void * param)
 	setup_thread_attr(&misc_attr, 64 * 1024, 0);
 	setup_thread_attr(&uevent_attr, DEFAULT_UEVENT_STACKSIZE * 1024, 0);
 	setup_thread_attr(&waiter_attr, 32 * 1024, 1);
-	setup_thread_attr(&io_err_stat_attr, 32 * 1024, 1);
+	setup_thread_attr(&io_err_stat_attr, 32 * 1024, 0);
 
 	if (logsink == 1) {
 		setup_thread_attr(&log_attr, 64 * 1024, 0);
@@ -2508,8 +2508,6 @@ child (void * param)
 	remove_maps_and_stop_waiters(vecs);
 	unlock(&vecs->lock);
 
-	stop_io_err_stat_thread();
-
 	pthread_cancel(check_thr);
 	pthread_cancel(uevent_thr);
 	pthread_cancel(uxlsnr_thr);
@@ -2520,6 +2518,8 @@ child (void * param)
 	pthread_join(uxlsnr_thr, NULL);
 	pthread_join(uevq_thr, NULL);
 
+	stop_io_err_stat_thread();
+
 	lock(&vecs->lock);
 	free_pathvec(vecs->pathvec, FREE_PATHS);
 	vecs->pathvec = NULL;
-- 
2.7.4

Re: [PATCH 01/12] multipath: add "ghost_delay" parameter

[Martin Wilck]

On Thu, 2017-12-07 at 12:48 -0600, Benjamin Marzinski wrote:
> If the lower-priority passive paths for a multipath device appear
> first,
> IO can go to them and cause the hardware handler to activate them,
> before the higher priority paths appear, causing the devices to
> failback. Setting the "ghost_delay" parameter to a value greater than
> 0 can avoid this ping-ponging by causing udev to not mark the device
> as
> Ready after its initial creation until either an active path appears,
> or ghost_delay seconds have passed. Multipathd does this by setting
> the MPATH_UDEV_NO_PATHS_FLAG.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Martin Wilck <mwilck@suse.com>

-- 
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 02/12] kpartx: don't delete partitions from partitions

[Martin Wilck]

On Thu, 2017-12-07 at 12:48 -0600, Benjamin Marzinski wrote:
> The current del-part-nodes rules try to run partx on the partitions
> themselves, which will ofen fail with an error in the log, because
> the
> partitions will have been deleted before partx can run on them.
> 
> Cc: Martin Wilck <mwilck@suse.com>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Martin Wilck <mwilck@suse.com>

-- 
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 03/12] multipath: fix hwhandler check in select_action

[Martin Wilck]

On Thu, 2017-12-07 at 12:48 -0600, Benjamin Marzinski wrote:
> If the existing multipath table does not have a hardware handler set,
> then even if retain_hwhandler is enabled on the new table, it may
> still
> be possible to set the hardware handler on reload. So, adding a
> hardware handler to the table should trigger a reload in this case.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Martin Wilck <mwilck@suse.com>

-- 
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 04/12] libmultipath: cleanup features handling code

[Martin Wilck]

On Thu, 2017-12-07 at 12:48 -0600, Benjamin Marzinski wrote:
> Right now multipath does some extra work to set the values for
> no_path_retry and retain_hwhandler on existing maps it reads in from
> the
> kernel.  This is so that select_action() can use these values to see
> if
> it needs to reload the devices. However, the way it works, the
> queue_if_no_path feature isn't always set correctly, and multipath
> has
> to go back afterwards and reset the value anyways.
> 
> It's simpler for select_action to just look at the values in the
> features line it read in from the kernel and the features line it
> would
> like the new map to have.  By comparing these, it also avoids the
> problem where the no_path_retry values match, so it doesn't reload,
> but
> the actual queue_if_no_path feature value is incorrect, so it has to
> go
> back and reset it. It can also skip calling setup_feature() entirely.
> 
> To do this, assemble_map() needs to update mp->features. This would
> otherwise partially happen when it had to reset the queue_if_no_path
> value.  retain_attached_hw_handler was never getting updated before,
> so
> the output when you created a map was incorrect.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

I had something similar in mind, but this is actually cleaner.
Reviewed-by: Martin Wilck <mwilck@suse.com>
-- 
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 05/12] multipathd: move helper functions to libmultipath

[Martin Wilck]

On Thu, 2017-12-07 at 12:48 -0600, Benjamin Marzinski wrote:
> This commit simply moves sync_map_state() and update_map() from
> multipathd/main.c to libmultipath/structs_vec.c, to enable future
> changes.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> 

Reviewed-by: Martin Wilck <mwilck@suse.com>

-- 
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 09/12] multipathd: move recovery mode code to function

[Martin Wilck]

On Thu, 2017-12-07 at 12:49 -0600, Benjamin Marzinski wrote:
> Instead of having multiple functions all running slightly different
> code to move a multipath device to recovery mode, there is now
> an enter_recovery_mode() function that all of them call.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---

Reviewed-by: Martin Wilck <mwilck@suse.com>

-- 
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 10/12] multipathd: clean up set_no_path_retry

[Martin Wilck]

On Thu, 2017-12-07 at 12:49 -0600, Benjamin Marzinski wrote:
> set_no_path_retry() was always updating the queue_if_no_path setting,
> even if it was unnecessary.  This caused problems when no_path_retry
> was
> set to a number, where any time __setup_multipath() was called with
> reset, queuing automatically restarted, even if there were no usable
> paths.
> 
> Instead, set_no_path_retry() should just fix the queueing if it is
> set
> to an incorrect value.  This is simple to detect if no_path_retry is
> set
> to "queue" or "fail".  When it is set to a number, multipathd needs
> to
> see if there are usable paths. If so, queueing should be enabled.  If
> not, queueing may be either enabled or disabled, but if it is enabled
> and the device isn't already in recovery mode, it should be.
> 
> Also, calling dm_map_present() is pointless in __setup_multipath(),
> since the existence of the map was just checked in dm_get_info().
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---

Reviewed-by: Martin Wilck <mwilck@suse.com>

-- 
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 11/12] multipath: check failed path dmstate in check_path

[Martin Wilck]

On Thu, 2017-12-07 at 12:49 -0600, Benjamin Marzinski wrote:
> If a path's checker state is down before and after a path check, but
> the
> path's dmstate is active, mutipath won't update the dmstate. It only
> updates the dmstate when the path first fails.  This can cause the
> kernel to try known faulty paths, if the multipath device was
> reloaded
> outside of multipathd.  check_path() already checks for and deals
> with a
> similar case where the path's checker state is up before and after a
> path check, but the dmstate is failed.  It should do the same thing
> for
> faulty paths.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---

Reviewed-by: Martin Wilck <mwilck@suse.com>

-- 
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 12/12] multipathd: marginal path code fixes

[Martin Wilck]

On Thu, 2017-12-07 at 12:49 -0600, Benjamin Marzinski wrote:
> There are a couple of issues I noticed with the marginal paths code.
> 
> In hit_io_err_recheck_time() there are some problems with the initial
> checks. We should always recover the path if there are no other
> usable
> paths to the device, so this check should be first. Also, we just
> checked that io_err_disable_reinstate isn't zero before calling this
> function, so we don't need to check again here (and it doesn't make
> any
> sense to continue disabling the path if io_err_disable_reinstate is
> set
> to zero).  Finally, the only kind of errors we can get while calling
> clock_gettime() are going to happen on every call. So, if we can't
> get
> the time, assume that the timeout has passed.
> 
> The multipath.conf.5 description of marginal_path_err_sample_time,
> states that sampling is stopped for marginal_path_err_rate_threshold
> seconds, when it should be marginal_path_err_recheck_gap_time
> seconds.
> 
> Lastly, there is a race that can cause multipathd to access freed
> memory
> on shutdown. io_err_stat_thr is started as a detached thread. This
> means
> that stop_io_err_stat_thread() can't know when it has actually
> stopped,
> after pthread_cancel() and pthread_kill() are called. To be safe, it
> should not start the thread in a deteched state, and call join to
> verify
> that it has stopped before freeing the memory it uses.  But more
> importantly, stop_io_err_stat_thread() was being called before the
> checker and uevent threads were being canceled. Both of these threads
> access data that is freed in stop_io_err_stat_thread(). To avoid the
> chance of these threads accessing freed memory, child() should wait
> until these threads are stopped before calling
> stop_io_err_stat_thread().
> 
> Cc: Guan Junxiong <guanjunxiong@huawei.com>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---

Reviewed-by: Martin Wilck <mwilck@suse.com>

-- 
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 04/12] libmultipath: cleanup features handling code

[Martin Wilck]

On Thu, 2017-12-07 at 12:48 -0600, Benjamin Marzinski wrote:

> retain_attached_hw_handler was never getting updated before, so
> the output when you created a map was incorrect.

While I've already ACKed your patch, I don't understand what you mean
here. Before your patch, "retain_attached_hw_handler" was set from
config options and correctly copied to the features string in
assemble_map, no?

> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index 0dfa250..7ca84b8 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -1060,21 +1062,6 @@ int coalesce_paths (struct vectors * vecs,
> vector newmp, char * refwwid,
>  				remove_feature(&mpp->features,
>  					       "queue_if_no_path");
>  		}
> -		else if (mpp->no_path_retry != NO_PATH_RETRY_UNDEF)
> {
> -			if (mpp->no_path_retry ==
> NO_PATH_RETRY_FAIL) {
> -				condlog(3, "%s: unset
> queue_if_no_path feature",
> -					mpp->alias);
> -				if (!dm_queue_if_no_path(mpp->alias, 
> 0))
> -					remove_feature(&mpp-
> >features,
> -						       "queue_if_no_
> path");
> -			} else {
> -				condlog(3, "%s: set queue_if_no_path
> feature",
> -					mpp->alias);
> -				if (!dm_queue_if_no_path(mpp->alias, 
> 1))
> -					add_feature(&mpp->features,
> -						    "queue_if_no_pat
> h");
> -			}
> -		}

AFAICS we could also get rid of the calls to dm_queue_if_no_path() in 
reload_map(), right?

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 06/12] multipathd: fix device creation issues

[Martin Wilck]

On Thu, 2017-12-07 at 12:49 -0600, Benjamin Marzinski wrote:
> Right now, devices created by multipath and added to multipthd via
> ev_add_map() are setup up incorrectly until the first time they get
> reloaded.  setup_map() is not run on these devices, which means that
> all
> of the multipath variables set up there don't get initialized until a
> later reload.  Also, adopt_paths is run to pull in any paths that the
> device is missing, but the device is never reloaded afterwards, so
> these
> paths aren't used.
> 
> Now, add_map_without_path() sets up the basic multipath device
> variables
> and then calls update_map() to finish the setup and reload the
> device.
> This patch also moves the code in __setup_multipath(), that only
> existed
> to handle adding devices via ev_add_map(), to where it belongs.
> 
> However, it is possible to create a device with no paths, which means
> the device cannot know which hwentry to use for its device
> configuration.  __setup_multipath() used to help with this via
> extract_hwe_from_path(), which grabbed the hwentry from a path if the
> multipath device didn't already have it set. This is now done both
> when
> paths are added or the map is updated, which means that
> extract_hwe_from_path() runs before the device is configured in
> setup_map() instead of after the table has already been loaded. This
> is
> a good thing. But because of this, it can't check the dmstate or the
> pathgroup state.  I don't believe it's necessary to care what state
> the
> path is in, especially now that we use libudev. The vendor and
> product
> information gets cached by libudev when the path device is first
> added,
> and should remain the same regardless of whether or not the device is
> currently up.  My version does try to take the hwe information from a
> path in the PATH_UP state first, but this is mostly to satisfy the
> paranoia of the old version.
> 
> Also, map_discovery(), which creates multipath devices during
> multipathd
> startup and reconfiguration, that only exist to see if multipathd
> needs
> to reload the device table, called __setup_multipath() as well. Even
> after removing the unnecessary code from __setup_multpiath, that
> still
> does pointless work for these devices, which will be removed before
> the
> end of configure(). Now, map_discovery() just gets the necessary
> kernel
> information to make the correct call in select_action().
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

Reviewed-by: Martin Wilck <mwilck@suse.com>

-- 
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 07/12] multipathd: remove select_* from setup_multipath

[Martin Wilck]

On Thu, 2017-12-07 at 12:49 -0600, Benjamin Marzinski wrote:
> select_rr_weight() and select_pgfailback() don't need to be set each
> time setup_multipath is called, since nothing ever changes the value
> of the multipath variable that they set.
> 
> select_flush_on_last_del() and select_no_path retry() are a little
> more
> involved.  In multipathd, it is possible to override no_path_retry by
> either setting flush_on_last_del, or by manually running the
> "disablequeueing" mutipathd command. Queueing gets restored when a
> path
> gets added back to the multipath device. This was done by moving the
> select_* functions into setup_multipath, where they frequently get
> called unnecessarily.  select_flush_on_last_del() can get removed by
> simply using another variable besides flush_on_last_del to track
> wether
> or not we should be force queueing to be disabled.
> 
> Since it's only possible to change whether or not you have queueing
> force disabled by reloading the device with path, or by manually
> restoring it, there is no reason to call select_no_path_retry() on
> every
> call to setup_multipath
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---

Reviewed-by: Martin Wilck <mwilck@suse.com>

-- 
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 08/12] libmultipath: __setup_multipath param cleanup

[Martin Wilck]

On Thu, 2017-12-07 at 12:49 -0600, Benjamin Marzinski wrote:
> setup multipath is only called by the daemon, so there is no point in
> the is_daemon parameter.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---

Reviewed-by: Martin Wilck <mwilck@suse.com>

-- 
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 04/12] libmultipath: cleanup features handling code

[Benjamin Marzinski]

On Fri, Dec 08, 2017 at 04:24:52PM +0100, Martin Wilck wrote:
> On Thu, 2017-12-07 at 12:48 -0600, Benjamin Marzinski wrote:
> 
> > retain_attached_hw_handler was never getting updated before, so
> > the output when you created a map was incorrect.
> 
> While I've already ACKed your patch, I don't understand what you mean
> here. Before your patch, "retain_attached_hw_handler" was set from
> config options and correctly copied to the features string in
> assemble_map, no?
> 

It is correctly copied to the features string you send to the kernel. It
was not copied to the multipath object that the multipath program is
dealing with.  So if you run "multipath" and it creates the device, what
is prints out as the device it created will not include
"retain_attached_hw_handler", even if it did send that feature to the
kernel.  If you run "multipath -l" afterwards, you will see that the
device does have "retain_attached_hw_handler" set.  This doesn't happen
with queue_if_no_path because of the add_feature() call in the code
below.  That goes through and adds "queue_if_no_path" back to the
features string of the multipath object that multipath will using to
print its output on creation.  Of course, none of that matters on newer
kernels anyway, because we will remove "retain_attached_hw_handler" from
the features line, since that behaviour is automatic now.

> > diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> > index 0dfa250..7ca84b8 100644
> > --- a/libmultipath/configure.c
> > +++ b/libmultipath/configure.c
> > @@ -1060,21 +1062,6 @@ int coalesce_paths (struct vectors * vecs,
> > vector newmp, char * refwwid,
> >  				remove_feature(&mpp->features,
> >  					       "queue_if_no_path");
> >  		}
> > -		else if (mpp->no_path_retry != NO_PATH_RETRY_UNDEF)
> > {
> > -			if (mpp->no_path_retry ==
> > NO_PATH_RETRY_FAIL) {
> > -				condlog(3, "%s: unset
> > queue_if_no_path feature",
> > -					mpp->alias);
> > -				if (!dm_queue_if_no_path(mpp->alias, 
> > 0))
> > -					remove_feature(&mpp-
> > >features,
> > -						       "queue_if_no_
> > path");
> > -			} else {
> > -				condlog(3, "%s: set queue_if_no_path
> > feature",
> > -					mpp->alias);
> > -				if (!dm_queue_if_no_path(mpp->alias, 
> > 1))
> > -					add_feature(&mpp->features,
> > -						    "queue_if_no_pat
> > h");
> > -			}
> > -		}
> 
> AFAICS we could also get rid of the calls to dm_queue_if_no_path() in 
> reload_map(), right?

Oops. I meant to remove it there as well. Good catch. I'll send a
follow-up patch.

-Ben

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

Re: [PATCH 00/12] Misc fixes

[Christophe Varoqui]

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

Applied.
Thanks.

On Thu, Dec 7, 2017 at 7:48 PM, Benjamin Marzinski <bmarzins@redhat.com>
wrote:

> The first patch here is just a rebased version of an earlier patch.  The
> last
> patch is fixes for commit 95d594fd6f031e59bb73d04a631b6c592fe26214
> "multipath-tools: intermittent IO error accounting to improve reliability".
> The rest of the patches are various fixes and code cleanups that I've been
> accumulating while working on removing the waiter threads. That work has
> run
> into some hiccups with corner cases, so I thought I would send these
> cleanup
> patches first, since they are generally useful, even without the waiter
> thread
> changes.
>
> Benjamin Marzinski (12):
>   multipath: add "ghost_delay" parameter
>   kpartx: don't delete partitions from partitions
>   multipath: fix hwhandler check in select_action
>   libmultipath: cleanup features handling code
>   multipathd: move helper functions to libmultipath
>   multipathd: fix device creation issues
>   multipathd: remove select_* from setup_multipath
>   libmultipath: __setup_multipath param cleanup
>   multipathd: move recovery mode code to function
>   multipathd: clean up set_no_path_retry
>   multipath: check failed path dmstate in check_path
>   multipathd: marginal path code fixes
>
>  kpartx/del-part-nodes.rules |   1 +
>  libmultipath/config.c       |   3 +
>  libmultipath/config.h       |   3 +
>  libmultipath/configure.c    |  38 +++----
>  libmultipath/defaults.h     |   1 +
>  libmultipath/devmapper.c    |   2 +-
>  libmultipath/dict.c         |  12 +++
>  libmultipath/dmparser.c     |  35 ++----
>  libmultipath/hwtable.c      |   1 +
>  libmultipath/io_err_stat.c  |  12 +--
>  libmultipath/propsel.c      |  21 +++-
>  libmultipath/propsel.h      |   1 +
>  libmultipath/structs.c      |  17 ---
>  libmultipath/structs.h      |  10 +-
>  libmultipath/structs_vec.c  | 256 +++++++++++++++++++++++++-----
> --------------
>  libmultipath/structs_vec.h  |  12 ++-
>  multipath/multipath.conf.5  |  21 +++-
>  multipathd/cli_handlers.c   |  34 +++---
>  multipathd/main.c           | 134 +++++++++--------------
>  multipathd/main.h           |   1 -
>  20 files changed, 322 insertions(+), 293 deletions(-)
>
> --
> 2.7.4
>
>

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

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

Re: [PATCH 06/12] multipathd: fix device creation issues

[Martin Wilck]

On Thu, 2017-12-07 at 12:49 -0600, Benjamin Marzinski wrote:

> However, it is possible to create a device with no paths, which means
> the device cannot know which hwentry to use for its device
> configuration.  __setup_multipath() used to help with this via
> extract_hwe_from_path(), which grabbed the hwentry from a path if the
> multipath device didn't already have it set. This is now done both
> when
> paths are added or the map is updated, which means that
> extract_hwe_from_path() runs before the device is configured in
> setup_map() instead of after the table has already been loaded. This
> is
> a good thing. But because of this, it can't check the dmstate or the
> pathgroup state.  I don't believe it's necessary to care what state
> the
> path is in, especially now that we use libudev. The vendor and
> product
> information gets cached by libudev when the path device is first
> added,
> and should remain the same regardless of whether or not the device is
> currently up.  My version does try to take the hwe information from a
> path in the PATH_UP state first, but this is mostly to satisfy the
> paranoia of the old version.

It just occured to me while reviewing this patch once more that this
might cause problems if the path WWID changes, as recently
discussed.Perhaps you should at least skip paths with the
"wwid_changed" attribute?


Regards,
Martin

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

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

Re: [PATCH 06/12] multipathd: fix device creation issues

[Benjamin Marzinski]

On Tue, Jan 30, 2018 at 05:51:03PM +0100, Martin Wilck wrote:
> On Thu, 2017-12-07 at 12:49 -0600, Benjamin Marzinski wrote:
> 
> > However, it is possible to create a device with no paths, which means
> > the device cannot know which hwentry to use for its device
> > configuration.  __setup_multipath() used to help with this via
> > extract_hwe_from_path(), which grabbed the hwentry from a path if the
> > multipath device didn't already have it set. This is now done both
> > when
> > paths are added or the map is updated, which means that
> > extract_hwe_from_path() runs before the device is configured in
> > setup_map() instead of after the table has already been loaded. This
> > is
> > a good thing. But because of this, it can't check the dmstate or the
> > pathgroup state.  I don't believe it's necessary to care what state
> > the
> > path is in, especially now that we use libudev. The vendor and
> > product
> > information gets cached by libudev when the path device is first
> > added,
> > and should remain the same regardless of whether or not the device is
> > currently up.  My version does try to take the hwe information from a
> > path in the PATH_UP state first, but this is mostly to satisfy the
> > paranoia of the old version.
> 
> It just occured to me while reviewing this patch once more that this
> might cause problems if the path WWID changes, as recently
> discussed.Perhaps you should at least skip paths with the
> "wwid_changed" attribute?

I thought that we agreed that updating pp->udev was a bad idea if the
wwid changed.  I'm still not sure that we need to update the udev device
at all.  For things that might reasonably change, we should grab them
from sysfs or from the current uevent (without necessarily updating the
one attached to the path). For things that aren't supposed to change, we
can get them through libudev, and then they'll be cached for the future.

But at any rate, as long as we don't update pp->udev when the new udev
device has a different wwid, this shouldn't be a problem. right?

-Ben

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

Re: [PATCH 06/12] multipathd: fix device creation issues

[Martin Wilck]

On Tue, 2018-01-30 at 12:34 -0600, Benjamin Marzinski wrote:
> On Tue, Jan 30, 2018 at 05:51:03PM +0100, Martin Wilck wrote:
> > 
> > It just occured to me while reviewing this patch once more that
> > this
> > might cause problems if the path WWID changes, as recently
> > discussed.Perhaps you should at least skip paths with the
> > "wwid_changed" attribute?
> 
> I thought that we agreed that updating pp->udev was a bad idea if the
> wwid changed.  I'm still not sure that we need to update the udev
> device
> at all.  For things that might reasonably change, we should grab them
> from sysfs or from the current uevent (without necessarily updating
> the
> one attached to the path). For things that aren't supposed to change,
> we
> can get them through libudev, and then they'll be cached for the
> future.
> 
> But at any rate, as long as we don't update pp->udev when the new
> udev
> device has a different wwid, this shouldn't be a problem. right?

Allright, the only thing that matters here is pp->hwe. So yes, we may
be ok. I have a rather bad feeling about these "zombie" paths being
carried around in multipathd, and parts of the code relying on that
being the case, but I guess I can't offer a better solution.

Thanks,
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