[PATCH v4 00/11] multipath-tools: no_path_retry/queue_if_no_path/hwhandler logic

[PATCH v4 00/11] multipath-tools: no_path_retry/queue_if_no_path/hwhandler logic

[Martin Wilck]

This patch set attempts to sanitize the logic used for consistently handling
options that can be set both via the "features" string and explicit multipath.conf
options. This is most prominently "no_path_retry" vs. "queue_if_no_path", but also
"retain_attached_hw_handler" vs. the feature of the same name.

The logic this patch set follows is:
 - If "no_path_retry" is set to any value, "queue_if_no_path" is ignored
   (this is the case nowadays for almost all "real" storage arrays, thanks to hwtable).
 - Otherwise, if "queue_if_no_path" is set, treat it like "no_path_retry queue".

... likewise for "retain_attached_hw_handler".

In the long run, we should get rid of the "features" settings duplicating
configuration options altogether; the patch set prepares this by printing
warning messages.

The logic is implemented in the new function reconcile_features_with_options,
which is called from both select_features() and merge_hwe(). In setup_map(),
we need to call select_features() after select_no_path_retry() to make this work.
The actual feature setting for device-mapper is made in assemble_map(), the
patch set also fixes the logic there.

The patch set documents the behavior in the man page, and adds some more
man page fixes.

By skipping superfluous default initializations in load_config(), the
log messages for the respective config settings become more appropriate.

The logic for setting hardware handler is also improved. Since kernel 4.3,
"retain_attached_hw_handler yes" is implicitly set by the kernel, and setting
the hardware handler from user space is only possible in special situations.
Acknowledge that in multipathd, and don't try to set or unset either this
feature or the hwhandler attribute itself if it's doomed to fail.

Review and comments are highly welcome.

Changes wrt v1:
  - Suggestions from Ben Marzinski:
    * Made sure "multipathd show config" still works (1/8).
    * Fixed logging for default setting of "max_sectors" (1/8)
    * Consistent internal treatment of mp->features (3/8, 4/8)
    * Fixed whitespace error (6/8)
    * Added deprecated warnings (8/8)
  - Made sure the same logic is used in propsel.c and config.c by
    calling the same function (3/8, 4/8)
  - Added deprecated warnings (8/8)

Changes wrt v2:
 - Added Acked-by:/Reviewed-by: tags
 - 1/11: clarify comment in select_max_sectors_kb (Hannes Reinecke)
 - 3/11: call select_retain_hwhandler before select_features
 - 8/11: don't suggest using retain_attached_hw_handler in log msg
 - 9/11, 10/11, 11/11: new

Changes wrt v3:
 - Added Reviewed-by: tags (kept Ben's Ack in 4/11 although patch slightly changed)
 - 4/11: use a buffer on stack rather than malloc (Hannes Reinecke)
 - 10/11: Simplify by checking dh_state only in select_handler (Hannes Reinecke)

Martin Wilck (11):
  libmultipath: load_config: skip setting unnecessary defaults
  libmultipath: add/remove_feature: use const char* for feature
  libmultipath: clarify option conflicts for "features"
  libmultipath: merge_hwe: fix queue_if_no_path logic
  libmultipath: assemble_map: fix queue_if_no_path logic
  multipath.conf.5: document no_path_retry vs. queue_if_no_path
  multipath.conf.5: Remove ??? and other minor fixes
  libmultipath: add deprecated warning for some features settings
  libmultipath: retain_attached_hw_handler obsolete with 4.3+
  libmultipath: don't try to set hwhandler if it is retained
  libmultipath: don't [un]set queue_if_no_path after domap

 libmultipath/config.c      |  31 +++---------
 libmultipath/configure.c   |  28 ++++-------
 libmultipath/dict.c        |  11 +++--
 libmultipath/dmparser.c    |   8 +--
 libmultipath/propsel.c     | 121 +++++++++++++++++++++++++++++++++++++++------
 libmultipath/propsel.h     |   3 ++
 libmultipath/structs.c     |  30 +++++------
 libmultipath/structs.h     |   4 +-
 libmultipath/util.c        |  36 ++++++++++++++
 libmultipath/util.h        |   2 +
 multipath/multipath.conf.5 |  67 +++++++++++++++----------
 11 files changed, 231 insertions(+), 110 deletions(-)

-- 
2.13.1

[PATCH v4 01/11] libmultipath: load_config: skip setting unnecessary defaults

[Martin Wilck]

We have the logic for setting defaults for paths and maps
in propsel.c. By pre-setting conf values with defaults in
load_config(), we generate irritating log messages like
'features = "0" (setting: multipath.conf defaults/devices section)'
if multipath.conf doesn't contain a features setting at all.

For some config settings, we need to use declare_def_snprint_defint()
now to make sure "multipathd show config" prints all defaults correctly.
To avoid confusion, we don't do this for "max_sectors", because
multipathd leaves this untouched by default.

Signed-off-by: Martin Wilck <mwilck@suse.com>
Acked-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Hannes Reinecke <hare@suse.com>
---
 libmultipath/config.c  | 16 ----------------
 libmultipath/dict.c    | 11 +++++++----
 libmultipath/propsel.c |  6 ++++++
 3 files changed, 13 insertions(+), 20 deletions(-)

diff --git a/libmultipath/config.c b/libmultipath/config.c
index 6b236019..60e345b3 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -606,40 +606,24 @@ load_config (char * file)
 	if (!conf->verbosity)
 		conf->verbosity = DEFAULT_VERBOSITY;
 
-	conf->minio = DEFAULT_MINIO;
-	conf->minio_rq = DEFAULT_MINIO_RQ;
 	get_sys_max_fds(&conf->max_fds);
 	conf->bindings_file = set_default(DEFAULT_BINDINGS_FILE);
 	conf->wwids_file = set_default(DEFAULT_WWIDS_FILE);
 	conf->multipath_dir = set_default(DEFAULT_MULTIPATHDIR);
-	conf->features = set_default(DEFAULT_FEATURES);
-	conf->flush_on_last_del = DEFAULT_FLUSH;
 	conf->attribute_flags = 0;
 	conf->reassign_maps = DEFAULT_REASSIGN_MAPS;
 	conf->checkint = DEFAULT_CHECKINT;
 	conf->max_checkint = 0;
-	conf->pgfailback = DEFAULT_FAILBACK;
-	conf->fast_io_fail = DEFAULT_FAST_IO_FAIL;
-	conf->retain_hwhandler = DEFAULT_RETAIN_HWHANDLER;
-	conf->detect_prio = DEFAULT_DETECT_PRIO;
-	conf->detect_checker = DEFAULT_DETECT_CHECKER;
 	conf->force_sync = DEFAULT_FORCE_SYNC;
 	conf->partition_delim = DEFAULT_PARTITION_DELIM;
 	conf->processed_main_config = 0;
 	conf->find_multipaths = DEFAULT_FIND_MULTIPATHS;
 	conf->uxsock_timeout = DEFAULT_REPLY_TIMEOUT;
-	conf->uid_attribute = set_default(DEFAULT_UID_ATTRIBUTE);
 	conf->retrigger_tries = DEFAULT_RETRIGGER_TRIES;
 	conf->retrigger_delay = DEFAULT_RETRIGGER_DELAY;
 	conf->uev_wait_timeout = DEFAULT_UEV_WAIT_TIMEOUT;
-	conf->deferred_remove = DEFAULT_DEFERRED_REMOVE;
-	conf->skip_kpartx = DEFAULT_SKIP_KPARTX;
 	conf->disable_changed_wwids = DEFAULT_DISABLE_CHANGED_WWIDS;
 	conf->remove_retries = 0;
-	conf->max_sectors_kb = DEFAULT_MAX_SECTORS_KB;
-	conf->san_path_err_threshold = DEFAULT_ERR_CHECKS;
-	conf->san_path_err_forget_rate = DEFAULT_ERR_CHECKS;
-	conf->san_path_err_recovery_time = DEFAULT_ERR_CHECKS;
 
 	/*
 	 * preload default hwtable
diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index 82066f67..9dc10904 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -630,7 +630,7 @@ print_fast_io_fail(char * buff, int len, void *ptr)
 }
 
 declare_def_handler(fast_io_fail, set_fast_io_fail)
-declare_def_snprint(fast_io_fail, print_fast_io_fail)
+declare_def_snprint_defint(fast_io_fail, print_fast_io_fail, DEFAULT_FAST_IO_FAIL)
 declare_ovr_handler(fast_io_fail, set_fast_io_fail)
 declare_ovr_snprint(fast_io_fail, print_fast_io_fail)
 declare_hw_handler(fast_io_fail, set_fast_io_fail)
@@ -1082,7 +1082,8 @@ declare_hw_snprint(delay_wait_checks, print_off_int_undef)
 declare_mp_handler(delay_wait_checks, set_off_int_undef)
 declare_mp_snprint(delay_wait_checks, print_off_int_undef)
 declare_def_handler(san_path_err_threshold, set_off_int_undef)
-declare_def_snprint(san_path_err_threshold, print_off_int_undef)
+declare_def_snprint_defint(san_path_err_threshold, print_off_int_undef,
+			   DEFAULT_ERR_CHECKS)
 declare_ovr_handler(san_path_err_threshold, set_off_int_undef)
 declare_ovr_snprint(san_path_err_threshold, print_off_int_undef)
 declare_hw_handler(san_path_err_threshold, set_off_int_undef)
@@ -1090,7 +1091,8 @@ declare_hw_snprint(san_path_err_threshold, print_off_int_undef)
 declare_mp_handler(san_path_err_threshold, set_off_int_undef)
 declare_mp_snprint(san_path_err_threshold, print_off_int_undef)
 declare_def_handler(san_path_err_forget_rate, set_off_int_undef)
-declare_def_snprint(san_path_err_forget_rate, print_off_int_undef)
+declare_def_snprint_defint(san_path_err_forget_rate, print_off_int_undef,
+			   DEFAULT_ERR_CHECKS)
 declare_ovr_handler(san_path_err_forget_rate, set_off_int_undef)
 declare_ovr_snprint(san_path_err_forget_rate, print_off_int_undef)
 declare_hw_handler(san_path_err_forget_rate, set_off_int_undef)
@@ -1098,7 +1100,8 @@ declare_hw_snprint(san_path_err_forget_rate, print_off_int_undef)
 declare_mp_handler(san_path_err_forget_rate, set_off_int_undef)
 declare_mp_snprint(san_path_err_forget_rate, print_off_int_undef)
 declare_def_handler(san_path_err_recovery_time, set_off_int_undef)
-declare_def_snprint(san_path_err_recovery_time, print_off_int_undef)
+declare_def_snprint_defint(san_path_err_recovery_time, print_off_int_undef,
+			   DEFAULT_ERR_CHECKS)
 declare_ovr_handler(san_path_err_recovery_time, set_off_int_undef)
 declare_ovr_snprint(san_path_err_recovery_time, print_off_int_undef)
 declare_hw_handler(san_path_err_recovery_time, set_off_int_undef)
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index 27f39517..c8ccede5 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -752,6 +752,12 @@ int select_max_sectors_kb(struct config *conf, struct multipath * mp)
 	mp_set_ovr(max_sectors_kb);
 	mp_set_hwe(max_sectors_kb);
 	mp_set_conf(max_sectors_kb);
+	mp_set_default(max_sectors_kb, DEFAULT_MAX_SECTORS_KB);
+	/*
+	 * In the default case, we will not modify max_sectors_kb in sysfs
+	 * (see sysfs_set_max_sectors_kb()).
+	 * Don't print a log message here to avoid user confusion.
+	 */
 	return 0;
 out:
 	condlog(3, "%s: max_sectors_kb = %i %s", mp->alias, mp->max_sectors_kb,
-- 
2.13.1

[PATCH v4 02/11] libmultipath: add/remove_feature: use const char* for feature

[Martin Wilck]

Change the argument type for the feature to add or remove to
const char*, making it possible to pass const strings without
warnings.

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

diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index e225f8b4..28704676 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -513,10 +513,11 @@ void setup_feature(struct multipath *mpp, char *feature)
 	}
 }
 
-int add_feature(char **f, char *n)
+int add_feature(char **f, const char *n)
 {
 	int c = 0, d, l = 0;
 	char *e, *p, *t;
+	const char *q;
 
 	if (!f)
 		return 1;
@@ -554,14 +555,14 @@ int add_feature(char **f, char *n)
 	if ((c % 10) == 9)
 		l++;
 	c++;
-	p = n;
-	while (*p != '\0') {
-		if (*p == ' ' && p[1] != '\0' && p[1] != ' ') {
+	q = n;
+	while (*q != '\0') {
+		if (*q == ' ' && q[1] != '\0' && q[1] != ' ') {
 			if ((c % 10) == 9)
 				l++;
 			c++;
 		}
-		p++;
+		q++;
 	}
 
 	t = MALLOC(l + 1);
@@ -601,10 +602,11 @@ int add_feature(char **f, char *n)
 	return 0;
 }
 
-int remove_feature(char **f, char *o)
+int remove_feature(char **f, const char *o)
 {
 	int c = 0, d, l;
 	char *e, *p, *n;
+	const char *q;
 
 	if (!f || !*f)
 		return 1;
@@ -630,18 +632,18 @@ int remove_feature(char **f, char *o)
 	/* Just spaces, return */
 	if (*o == '\0')
 		return 0;
-	e = o + strlen(o);
-	while (*e == ' ')
-		e--;
-	d = (int)(e - o);
+	q = o + strlen(o);
+	while (*q == ' ')
+		q--;
+	d = (int)(q - o);
 
 	/* Update feature count */
 	c--;
-	p = o;
-	while (p[0] != '\0') {
-		if (p[0] == ' ' && p[1] != ' ' && p[1] != '\0')
+	q = o;
+	while (q[0] != '\0') {
+		if (q[0] == ' ' && q[1] != ' ' && q[1] != '\0')
 			c--;
-		p++;
+		q++;
 	}
 
 	/* Quick exit if all features have been removed */
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 01e031ad..8ea984d9 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -369,8 +369,8 @@ 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 **, char *);
-int remove_feature (char **, char *);
+int add_feature (char **, const char *);
+int remove_feature (char **, const char *);
 
 extern char sysfs_path[PATH_SIZE];
 
-- 
2.13.1

[PATCH v4 03/11] libmultipath: clarify option conflicts for "features"

[Martin Wilck]

The "features" option in multipath.conf can possibly conflict
with "no_path_retry" and "retain_attached_hw_handler".

Currently, "no_path_retry" takes precedence, unless it is set to
"fail", in which case it's overridden by "queue_if_no_path".
This is odd, either "features" or "no_path_retry" should take
precedence.
No precedence rules are defined for "retain_attached_hw_handler".

Make this behavior more consistent by always giving precedence
to the explicit config file options, and improve logging.

Put this logic into a separate function, which can be used
from other places in the code.

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Hannes Reinecke <hare@suse.com>
---
 libmultipath/configure.c |  6 ++---
 libmultipath/propsel.c   | 68 +++++++++++++++++++++++++++++++++++++-----------
 libmultipath/propsel.h   |  3 +++
 3 files changed, 59 insertions(+), 18 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index bd090d9a..a7f2b443 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -280,18 +280,18 @@ int setup_map(struct multipath *mpp, char *params, int params_size)
 	select_pgfailback(conf, mpp);
 	select_pgpolicy(conf, mpp);
 	select_selector(conf, mpp);
-	select_features(conf, mpp);
 	select_hwhandler(conf, mpp);
+	select_no_path_retry(conf, mpp);
+	select_retain_hwhandler(conf, mpp);
+	select_features(conf, mpp);
 	select_rr_weight(conf, mpp);
 	select_minio(conf, mpp);
-	select_no_path_retry(conf, mpp);
 	select_mode(conf, mpp);
 	select_uid(conf, mpp);
 	select_gid(conf, mpp);
 	select_fast_io_fail(conf, mpp);
 	select_dev_loss(conf, mpp);
 	select_reservation_key(conf, mpp);
-	select_retain_hwhandler(conf, mpp);
 	select_deferred_remove(conf, mpp);
 	select_delay_watch_checks(conf, mpp);
 	select_delay_wait_checks(conf, mpp);
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index c8ccede5..bc9e130b 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -269,6 +269,55 @@ out:
 	return mp->alias ? 0 : 1;
 }
 
+void reconcile_features_with_options(const char *id, char **features, int* no_path_retry,
+		  int *retain_hwhandler)
+{
+	static const char q_i_n_p[] = "queue_if_no_path";
+	static const char r_a_h_h[] = "retain_attached_hw_handler";
+	char buff[12];
+
+	if (*features == NULL)
+		return;
+	if (id == NULL)
+		id = "UNKNOWN";
+
+	/*
+	 * We only use no_path_retry internally. The "queue_if_no_path"
+	 * device-mapper feature is derived from it when the map is loaded.
+	 * For consistency, "queue_if_no_path" is removed from the
+	 * internal libmultipath features string.
+	 * For backward compatibility we allow 'features "1 queue_if_no_path"';
+	 * it's translated into "no_path_retry queue" here.
+	 */
+	if (strstr(*features, q_i_n_p)) {
+		if (*no_path_retry == NO_PATH_RETRY_UNDEF) {
+			*no_path_retry = NO_PATH_RETRY_QUEUE;
+			print_no_path_retry(buff, sizeof(buff),
+					    no_path_retry);
+			condlog(3, "%s: no_path_retry = %s (inherited setting from feature '%s')",
+				id, buff, q_i_n_p);
+		};
+		/* Warn only if features string is overridden */
+		if (*no_path_retry != NO_PATH_RETRY_QUEUE) {
+			print_no_path_retry(buff, sizeof(buff),
+					    no_path_retry);
+			condlog(2, "%s: ignoring feature '%s' because no_path_retry is set to '%s'",
+				id, q_i_n_p, buff);
+		}
+		remove_feature(features, q_i_n_p);
+	}
+	if (strstr(*features, r_a_h_h)) {
+		if (*retain_hwhandler == RETAIN_HWHANDLER_UNDEF) {
+			condlog(3, "%s: %s = on (inherited setting from feature '%s')",
+				id, r_a_h_h, r_a_h_h);
+			*retain_hwhandler = RETAIN_HWHANDLER_ON;
+		} else if (*retain_hwhandler == RETAIN_HWHANDLER_OFF)
+			condlog(2, "%s: ignoring feature '%s' because %s is set to 'off'",
+				id, r_a_h_h, r_a_h_h);
+		remove_feature(features, r_a_h_h);
+	}
+}
+
 int select_features(struct config *conf, struct multipath *mp)
 {
 	char *origin;
@@ -280,19 +329,11 @@ int select_features(struct config *conf, struct multipath *mp)
 	mp_set_default(features, DEFAULT_FEATURES);
 out:
 	mp->features = STRDUP(mp->features);
-	condlog(3, "%s: features = \"%s\" %s", mp->alias, mp->features, origin);
 
-	if (strstr(mp->features, "queue_if_no_path")) {
-		if (mp->no_path_retry == NO_PATH_RETRY_UNDEF)
-			mp->no_path_retry = NO_PATH_RETRY_QUEUE;
-		else if (mp->no_path_retry == NO_PATH_RETRY_FAIL) {
-			condlog(1, "%s: config ERROR (setting: overriding 'no_path_retry' value)",
-				mp->alias);
-			mp->no_path_retry = NO_PATH_RETRY_QUEUE;
-		} else if (mp->no_path_retry != NO_PATH_RETRY_QUEUE)
-			condlog(1, "%s: config ERROR (setting: ignoring 'queue_if_no_path' because no_path_retry = %d)",
-				mp->alias, mp->no_path_retry);
-	}
+	reconcile_features_with_options(mp->alias, &mp->features,
+					&mp->no_path_retry,
+					&mp->retain_hwhandler);
+	condlog(3, "%s: features = \"%s\" %s", mp->alias, mp->features, origin);
 	return 0;
 }
 
@@ -469,9 +510,6 @@ out:
 	if (origin)
 		condlog(3, "%s: no_path_retry = %s %s", mp->alias, buff,
 			origin);
-	else if (mp->no_path_retry != NO_PATH_RETRY_UNDEF)
-		condlog(3, "%s: no_path_retry = %s (setting: inherited value)",
-			mp->alias, buff);
 	else
 		condlog(3, "%s: no_path_retry = undef (setting: multipath internal)",
 			mp->alias);
diff --git a/libmultipath/propsel.h b/libmultipath/propsel.h
index 58a32f3c..f8e96d85 100644
--- a/libmultipath/propsel.h
+++ b/libmultipath/propsel.h
@@ -28,3 +28,6 @@ int select_max_sectors_kb (struct config *conf, struct multipath * mp);
 int select_san_path_err_forget_rate(struct config *conf, struct multipath *mp);
 int select_san_path_err_threshold(struct config *conf, struct multipath *mp);
 int select_san_path_err_recovery_time(struct config *conf, struct multipath *mp);
+void reconcile_features_with_options(const char *id, char **features,
+				     int* no_path_retry,
+				     int *retain_hwhandler);
-- 
2.13.1

[PATCH v4 04/11] libmultipath: merge_hwe: fix queue_if_no_path logic

[Martin Wilck]

The logic applied here should match the logic in select_features().
This is achieved by calling reconcile_features_with_options().

Signed-off-by: Martin Wilck <mwilck@suse.com>
Acked-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/config.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/libmultipath/config.c b/libmultipath/config.c
index 60e345b3..b21a3aa1 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -25,6 +25,7 @@
 #include "prio.h"
 #include "devmapper.h"
 #include "mpath_cmd.h"
+#include "propsel.h"
 
 static int
 hwe_strmatch (struct hwentry *hwe1, struct hwentry *hwe2)
@@ -318,6 +319,7 @@ set_param_str(char * str)
 static int
 merge_hwe (struct hwentry * dst, struct hwentry * src)
 {
+	char id[SCSI_VENDOR_SIZE+SCSI_PRODUCT_SIZE];
 	merge_str(vendor);
 	merge_str(product);
 	merge_str(revision);
@@ -353,15 +355,10 @@ merge_hwe (struct hwentry * dst, struct hwentry * src)
 	merge_num(san_path_err_forget_rate);
 	merge_num(san_path_err_recovery_time);
 
-	/*
-	 * Make sure features is consistent with
-	 * no_path_retry
-	 */
-	if (dst->no_path_retry == NO_PATH_RETRY_FAIL)
-		remove_feature(&dst->features, "queue_if_no_path");
-	else if (dst->no_path_retry != NO_PATH_RETRY_UNDEF)
-		add_feature(&dst->features, "queue_if_no_path");
-
+	snprintf(id, sizeof(id), "%s/%s", dst->vendor, dst->product);
+	reconcile_features_with_options(id, &dst->features,
+					&dst->no_path_retry,
+					&dst->retain_hwhandler);
 	return 0;
 }
 
-- 
2.13.1

[PATCH v4 05/11] libmultipath: assemble_map: fix queue_if_no_path logic

[Martin Wilck]

It is wrong to remove the queue_if_no_path feature if no_path_retry
is unset. Rather, in this case the feature should neither be added
nor removed.

Signed-off-by: Martin Wilck <mwilck@suse.com>
Acked-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Hannes Reinecke <hare@suse.com>
---
 libmultipath/dmparser.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
index ba09dc72..1121c715 100644
--- a/libmultipath/dmparser.c
+++ b/libmultipath/dmparser.c
@@ -89,13 +89,12 @@ assemble_map (struct multipath * mp, char * params, int len)
 	 * 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_UNDEF ||
-	    mp->no_path_retry == NO_PATH_RETRY_FAIL) {
+	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 {
+	} else if (mp->no_path_retry != NO_PATH_RETRY_UNDEF) {
 		add_feature(&f, no_path_retry);
 	}
 	if (mp->retain_hwhandler == RETAIN_HWHANDLER_ON)
-- 
2.13.1

[PATCH v4 06/11] multipath.conf.5: document no_path_retry vs. queue_if_no_path

[Martin Wilck]

Clarify the documentation about option precedence.

Signed-off-by: Martin Wilck <mwilck@suse.com>
Acked-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Hannes Reinecke <hare@suse.com>
---
 multipath/multipath.conf.5 | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
index 0049cba7..d5d9438a 100644
--- a/multipath/multipath.conf.5
+++ b/multipath/multipath.conf.5
@@ -385,7 +385,9 @@ Possible values for the feature list are:
 .\" XXX
 .I queue_if_no_path
 (Superseded by \fIno_path_retry\fR) (Since ??? kernel) Queue I/O if no path is active.
-Identical to the \fIno_path_retry\fR with \fIqueue\fR value. See KNOWN ISSUES.
+Identical to the \fIno_path_retry\fR with \fIqueue\fR value. If both this
+feature and \fIno_path_retry\fR are set, the latter value takes
+precedence. See KNOWN ISSUES.
 .TP
 .I no_partitions
 Disable automatic partitions generation via kpartx.
-- 
2.13.1

[PATCH v4 07/11] multipath.conf.5: Remove ??? and other minor fixes

[Martin Wilck]

Remove the FIXME markers by filling in missing content,
and make some other minor fixes.

Signed-off-by: Martin Wilck <mwilck@suse.com>
Acked-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Hannes Reinecke <hare@suse.com>
---
 multipath/multipath.conf.5 | 48 +++++++++++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 22 deletions(-)

diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
index d5d9438a..000a42ec 100644
--- a/multipath/multipath.conf.5
+++ b/multipath/multipath.conf.5
@@ -258,7 +258,7 @@ Return a constant priority of \fI1\fR.
 .I sysfs
 Use the sysfs attributes \fIaccess_state\fR and \fIpreferred_path\fR to
 generate the path priority. This prioritizer accepts the optional prio_arg
-\fIexclusive_pref_bit\fR
+\fIexclusive_pref_bit\fR.
 .TP
 .I emc
 (Hardware-dependent)
@@ -300,14 +300,19 @@ Generate the path priority based on a latency algorithm.
 Requires prio_args keyword.
 .TP
 .I datacore
-.\" XXX
-???. Requires prio_args keyword.
+(Hardware-dependent)
+Generate the path priority for some Datacore storage arrays. Requires prio_args
+keyword.
 .TP
 .I iet
-.\" XXX
-???. Requires prio_args keyword.
-.TP
-The default is: \fBconst\fR
+(iSCSI only)
+Generate path priority for iSCSI targets based on IP address. Requires
+prio_args keyword.
+.PP
+The default depends on the \fBdetect_prio\fR setting: If \fBdetect_prio\fR is
+\fByes\fR (default), the default priority algorithm is \fBsysfs\fR (except for
+NetAPP E-Series, where it is \fBalua\fR). If \fBdetect_prio\fR is
+\fBno\fR, the default priority algorithm is \fBconst\fR.
 .RE
 .
 .
@@ -364,12 +369,12 @@ If \fIexclusive_pref_bit\fR is set, paths with the \fIpreferred path\fR bit
 set will always be in their own path group.
 .TP
 .I datacore
-.\" XXX
-\fIpreferredsds\fR ???.
+\fIpreferredsds\fR (required) denotes the preferred "SDS name" for datacore
+arrays. \fItimeout\fR (optional) is the timeout for the INQUIRY, in ms.
 .TP
 .I iet
-.\" XXX
-\fIpreferredip\fR ???.
+\fIpreferredip=...\fR (required) denotes the preferred IP address (in dotted decimal
+notation) for iSCSI targets.
 .TP
 The default is: \fB<unset>\fR
 .RE
@@ -384,29 +389,28 @@ Possible values for the feature list are:
 .TP 12
 .\" XXX
 .I queue_if_no_path
-(Superseded by \fIno_path_retry\fR) (Since ??? kernel) Queue I/O if no path is active.
+(Superseded by \fIno_path_retry\fR) Queue I/O if no path is active.
 Identical to the \fIno_path_retry\fR with \fIqueue\fR value. If both this
 feature and \fIno_path_retry\fR are set, the latter value takes
 precedence. See KNOWN ISSUES.
 .TP
-.I no_partitions
-Disable automatic partitions generation via kpartx.
-.TP
 .\" XXX
 .I pg_init_retries <times>
-(Since ??? kernel) Number of times to retry pg_init, it must be between 1 and 50.
+(Since kernel 2.6.24) Number of times to retry pg_init, it must be between 1 and 50.
 .TP
 .\" XXX
 .I pg_init_delay_msecs <msecs>
-(Since ??? kernel) Number of msecs before pg_init retry, it must be between 0 and 60000.
+(Since kernel 2.6.38) Number of msecs before pg_init retry, it must be between 0 and 60000.
 .TP
 .\" XXX
 .I queue_mode <mode>
-(Since ??? kernel) Select the the queue_mode per multipath device.
-Where <mode> can be \fIbio\fR, \fIrq\fR or \fImq\fR. Which corresponds to
-bio-based, request_fn rq-based, and blk-mq rq-based respectively.
-.TP
-The default is: \fB0\fR
+(Since kernel 4.8) Select the the queueing mode per multipath device.
+<mode> can be \fIbio\fR, \fIrq\fR or \fImq\f, which corresponds to
+bio-based, request-based, and block-multiqueue (blk-mq) request-based,
+respectively.
+
+The default depends on the kernel parameter \fBdm_mod.use_blk_mq\fR. It is
+\fImq\fR if the latter is set, and \fIrq\fR otherwise.
 .RE
 .
 .
-- 
2.13.1

[PATCH v4 08/11] libmultipath: add deprecated warning for some features settings

[Martin Wilck]

The device-mapper features "queue_if_no_path" and
"retain_attached_hw_handler" should be set via the configuration
keywords "no_path_retry" and "retain_attached_hw_handler",
respectively, not via "features".
Print a warning if these "features" settings are encountered.

So far these "features" settings will only be ignored if the
respective other keyword is set, so in particular
'features "1 queue_if_no_path"' will still work as expected, but
cause the warning to be printed.

We should consider ignoring these completely in a future version.

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Hannes Reinecke <hare@suse.com>
---
 libmultipath/propsel.c     | 5 +++++
 multipath/multipath.conf.5 | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index bc9e130b..e885a845 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -290,6 +290,9 @@ void reconcile_features_with_options(const char *id, char **features, int* no_pa
 	 * it's translated into "no_path_retry queue" here.
 	 */
 	if (strstr(*features, q_i_n_p)) {
+		condlog(0, "%s: option 'features \"1 %s\"' is deprecated, "
+			"please use 'no_path_retry queue' instead",
+			id, q_i_n_p);
 		if (*no_path_retry == NO_PATH_RETRY_UNDEF) {
 			*no_path_retry = NO_PATH_RETRY_QUEUE;
 			print_no_path_retry(buff, sizeof(buff),
@@ -307,6 +310,8 @@ void reconcile_features_with_options(const char *id, char **features, int* no_pa
 		remove_feature(features, q_i_n_p);
 	}
 	if (strstr(*features, r_a_h_h)) {
+		condlog(0, "%s: option 'features \"1 %s\"' is deprecated",
+			id, r_a_h_h);
 		if (*retain_hwhandler == RETAIN_HWHANDLER_UNDEF) {
 			condlog(3, "%s: %s = on (inherited setting from feature '%s')",
 				id, r_a_h_h, r_a_h_h);
diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
index 000a42ec..b32d0383 100644
--- a/multipath/multipath.conf.5
+++ b/multipath/multipath.conf.5
@@ -389,7 +389,7 @@ Possible values for the feature list are:
 .TP 12
 .\" XXX
 .I queue_if_no_path
-(Superseded by \fIno_path_retry\fR) Queue I/O if no path is active.
+(Deprecated, superseded by \fIno_path_retry\fR) Queue I/O if no path is active.
 Identical to the \fIno_path_retry\fR with \fIqueue\fR value. If both this
 feature and \fIno_path_retry\fR are set, the latter value takes
 precedence. See KNOWN ISSUES.
-- 
2.13.1

[PATCH v4 09/11] libmultipath: retain_attached_hw_handler obsolete with 4.3+

[Martin Wilck]

Kernels 4.3 and newer (commit 1bab0de0 "dm-mpath, scsi_dh: don't
let dm detach device handlers") imply "retain_attached_hw_handler yes".

Clarify this in the propsel code, log messages, and documentation.

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Hannes Reinecke <hare@suse.com>
---
 libmultipath/configure.c   |  3 ++-
 libmultipath/dmparser.c    |  3 ++-
 libmultipath/propsel.c     |  7 ++++++-
 libmultipath/util.c        | 36 ++++++++++++++++++++++++++++++++++++
 libmultipath/util.h        |  2 ++
 multipath/multipath.conf.5 | 15 +++++++++++----
 6 files changed, 59 insertions(+), 7 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index a7f2b443..74b6f52a 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -572,7 +572,8 @@ select_action (struct multipath * mpp, vector curmp, int force_reload)
 	}
 
 	if (mpp->retain_hwhandler != RETAIN_HWHANDLER_UNDEF &&
-	    mpp->retain_hwhandler != cmpp->retain_hwhandler) {
+	    mpp->retain_hwhandler != cmpp->retain_hwhandler &&
+	    get_linux_version_code() < KERNEL_VERSION(4, 3, 0)) {
 		mpp->action = ACT_RELOAD;
 		condlog(3, "%s: set ACT_RELOAD (retain_hwhandler change)",
 			mpp->alias);
diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
index 1121c715..b647c256 100644
--- a/libmultipath/dmparser.c
+++ b/libmultipath/dmparser.c
@@ -97,7 +97,8 @@ assemble_map (struct multipath * mp, char * params, int len)
 	} else if (mp->no_path_retry != NO_PATH_RETRY_UNDEF) {
 		add_feature(&f, no_path_retry);
 	}
-	if (mp->retain_hwhandler == RETAIN_HWHANDLER_ON)
+	if (mp->retain_hwhandler == RETAIN_HWHANDLER_ON &&
+	    get_linux_version_code() < KERNEL_VERSION(4, 3, 0))
 		add_feature(&f, retain_hwhandler);
 
 	APPEND(p, end, "%s %s %i %i", f, mp->hwhandler, nr_priority_groups,
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index e885a845..d609394e 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -628,7 +628,12 @@ int select_retain_hwhandler(struct config *conf, struct multipath *mp)
 
 	if (!VERSION_GE(conf->version, minv_dm_retain)) {
 		mp->retain_hwhandler = RETAIN_HWHANDLER_OFF;
-		origin = "(setting: WARNING, requires kernel version >= 1.5.0)";
+		origin = "(setting: WARNING, requires kernel dm-mpath version >= 1.5.0)";
+		goto out;
+	}
+	if (get_linux_version_code() >= KERNEL_VERSION(4, 3, 0)) {
+		mp->retain_hwhandler = RETAIN_HWHANDLER_ON;
+		origin = "(setting: implied in kernel >= 4.3.0)";
 		goto out;
 	}
 	mp_set_ovr(retain_hwhandler);
diff --git a/libmultipath/util.c b/libmultipath/util.c
index b90cd8b0..dff2ed3c 100644
--- a/libmultipath/util.c
+++ b/libmultipath/util.c
@@ -6,6 +6,7 @@
 #include <sys/stat.h>
 #include <sys/sysmacros.h>
 #include <sys/types.h>
+#include <sys/utsname.h>
 #include <dirent.h>
 #include <unistd.h>
 #include <errno.h>
@@ -380,3 +381,38 @@ int systemd_service_enabled(const char *dev)
 		found = systemd_service_enabled_in(dev, "/run");
 	return found;
 }
+
+static int _linux_version_code;
+static pthread_once_t _lvc_initialized = PTHREAD_ONCE_INIT;
+
+/* Returns current kernel version encoded as major*65536 + minor*256 + patch,
+ * so, for example,  to check if the kernel is greater than 2.2.11:
+ *
+ *     if (get_linux_version_code() > KERNEL_VERSION(2,2,11)) { <stuff> }
+ *
+ * Copyright (C) 1999-2004 by Erik Andersen <andersen@codepoet.org>
+ * Code copied from busybox (GPLv2 or later)
+ */
+static void
+_set_linux_version_code(void)
+{
+	struct utsname name;
+	char *t;
+	int i, r;
+
+	uname(&name); /* never fails */
+	t = name.release;
+	r = 0;
+	for (i = 0; i < 3; i++) {
+		t = strtok(t, ".");
+		r = r * 256 + (t ? atoi(t) : 0);
+		t = NULL;
+	}
+	_linux_version_code = r;
+}
+
+int get_linux_version_code(void)
+{
+	pthread_once(&_lvc_initialized, _set_linux_version_code);
+	return _linux_version_code;
+}
diff --git a/libmultipath/util.h b/libmultipath/util.h
index b087e32e..45291be8 100644
--- a/libmultipath/util.h
+++ b/libmultipath/util.h
@@ -15,6 +15,8 @@ char *convert_dev(char *dev, int is_path_device);
 char *parse_uid_attribute_by_attrs(char *uid_attrs, char *path_dev);
 void setup_thread_attr(pthread_attr_t *attr, size_t stacksize, int detached);
 int systemd_service_enabled(const char *dev);
+int get_linux_version_code(void);
+#define KERNEL_VERSION(maj, min, ptc) ((((maj) * 256) + (min)) * 256 + (ptc))
 
 #define safe_sprintf(var, format, args...)	\
 	snprintf(var, sizeof(var), format, ##args) >= sizeof(var)
diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
index b32d0383..3b4e5187 100644
--- a/multipath/multipath.conf.5
+++ b/multipath/multipath.conf.5
@@ -698,15 +698,16 @@ The default is: \fB<unset>\fR
 .
 .TP
 .B retain_attached_hw_handler
-If set to
+(Obsolete for kernels >= 4.3) If set to
 .I yes
 and the SCSI layer has already attached a hardware_handler to the device,
 multipath will not force the device to use the hardware_handler specified by
 mutipath.conf. If the SCSI layer has not attached a hardware handler,
 multipath will continue to use its configured hardware handler.
 .RS
-.TP
-The default is: \fByes\fR
+.PP
+The default is: \fByes\fR. Linux kernel 4.3 or newer always behaves as if
+\fB"retain_attached_hw_handler yes"\fR was set.
 .RE
 .
 .
@@ -1182,8 +1183,14 @@ Active/Standby mode exclusively.
 .I 1 alua
 (Hardware-dependent)
 Hardware handler for SCSI-3 ALUA compatible arrays.
-.TP
+.PP
 The default is: \fB<unset>\fR
+.PP
+\fBImportant Note:\fR Linux kernels 4.3 and newer automatically attach a device
+handler to known devices (which includes all devices supporting SCSI-3 ALUA)
+and disallow changing the handler
+afterwards. Setting \fBhardware_handler\fR for such devices on these kernels
+has no effect.
 .RE
 .
 .
-- 
2.13.1

Re: [PATCH v4 09/11] libmultipath: retain_attached_hw_handler obsolete with 4.3+

[Benjamin Marzinski]

On Thu, Jun 22, 2017 at 04:59:11PM +0200, Martin Wilck wrote:
> Kernels 4.3 and newer (commit 1bab0de0 "dm-mpath, scsi_dh: don't
> let dm detach device handlers") imply "retain_attached_hw_handler yes".
> 
> Clarify this in the propsel code, log messages, and documentation.

ACK

-Ben

> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> Reviewed-by: Hannes Reinecke <hare@suse.com>
> ---
>  libmultipath/configure.c   |  3 ++-
>  libmultipath/dmparser.c    |  3 ++-
>  libmultipath/propsel.c     |  7 ++++++-
>  libmultipath/util.c        | 36 ++++++++++++++++++++++++++++++++++++
>  libmultipath/util.h        |  2 ++
>  multipath/multipath.conf.5 | 15 +++++++++++----
>  6 files changed, 59 insertions(+), 7 deletions(-)
> 
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index a7f2b443..74b6f52a 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -572,7 +572,8 @@ select_action (struct multipath * mpp, vector curmp, int force_reload)
>  	}
>  
>  	if (mpp->retain_hwhandler != RETAIN_HWHANDLER_UNDEF &&
> -	    mpp->retain_hwhandler != cmpp->retain_hwhandler) {
> +	    mpp->retain_hwhandler != cmpp->retain_hwhandler &&
> +	    get_linux_version_code() < KERNEL_VERSION(4, 3, 0)) {
>  		mpp->action = ACT_RELOAD;
>  		condlog(3, "%s: set ACT_RELOAD (retain_hwhandler change)",
>  			mpp->alias);
> diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
> index 1121c715..b647c256 100644
> --- a/libmultipath/dmparser.c
> +++ b/libmultipath/dmparser.c
> @@ -97,7 +97,8 @@ assemble_map (struct multipath * mp, char * params, int len)
>  	} else if (mp->no_path_retry != NO_PATH_RETRY_UNDEF) {
>  		add_feature(&f, no_path_retry);
>  	}
> -	if (mp->retain_hwhandler == RETAIN_HWHANDLER_ON)
> +	if (mp->retain_hwhandler == RETAIN_HWHANDLER_ON &&
> +	    get_linux_version_code() < KERNEL_VERSION(4, 3, 0))
>  		add_feature(&f, retain_hwhandler);
>  
>  	APPEND(p, end, "%s %s %i %i", f, mp->hwhandler, nr_priority_groups,
> diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
> index e885a845..d609394e 100644
> --- a/libmultipath/propsel.c
> +++ b/libmultipath/propsel.c
> @@ -628,7 +628,12 @@ int select_retain_hwhandler(struct config *conf, struct multipath *mp)
>  
>  	if (!VERSION_GE(conf->version, minv_dm_retain)) {
>  		mp->retain_hwhandler = RETAIN_HWHANDLER_OFF;
> -		origin = "(setting: WARNING, requires kernel version >= 1.5.0)";
> +		origin = "(setting: WARNING, requires kernel dm-mpath version >= 1.5.0)";
> +		goto out;
> +	}
> +	if (get_linux_version_code() >= KERNEL_VERSION(4, 3, 0)) {
> +		mp->retain_hwhandler = RETAIN_HWHANDLER_ON;
> +		origin = "(setting: implied in kernel >= 4.3.0)";
>  		goto out;
>  	}
>  	mp_set_ovr(retain_hwhandler);
> diff --git a/libmultipath/util.c b/libmultipath/util.c
> index b90cd8b0..dff2ed3c 100644
> --- a/libmultipath/util.c
> +++ b/libmultipath/util.c
> @@ -6,6 +6,7 @@
>  #include <sys/stat.h>
>  #include <sys/sysmacros.h>
>  #include <sys/types.h>
> +#include <sys/utsname.h>
>  #include <dirent.h>
>  #include <unistd.h>
>  #include <errno.h>
> @@ -380,3 +381,38 @@ int systemd_service_enabled(const char *dev)
>  		found = systemd_service_enabled_in(dev, "/run");
>  	return found;
>  }
> +
> +static int _linux_version_code;
> +static pthread_once_t _lvc_initialized = PTHREAD_ONCE_INIT;
> +
> +/* Returns current kernel version encoded as major*65536 + minor*256 + patch,
> + * so, for example,  to check if the kernel is greater than 2.2.11:
> + *
> + *     if (get_linux_version_code() > KERNEL_VERSION(2,2,11)) { <stuff> }
> + *
> + * Copyright (C) 1999-2004 by Erik Andersen <andersen@codepoet.org>
> + * Code copied from busybox (GPLv2 or later)
> + */
> +static void
> +_set_linux_version_code(void)
> +{
> +	struct utsname name;
> +	char *t;
> +	int i, r;
> +
> +	uname(&name); /* never fails */
> +	t = name.release;
> +	r = 0;
> +	for (i = 0; i < 3; i++) {
> +		t = strtok(t, ".");
> +		r = r * 256 + (t ? atoi(t) : 0);
> +		t = NULL;
> +	}
> +	_linux_version_code = r;
> +}
> +
> +int get_linux_version_code(void)
> +{
> +	pthread_once(&_lvc_initialized, _set_linux_version_code);
> +	return _linux_version_code;
> +}
> diff --git a/libmultipath/util.h b/libmultipath/util.h
> index b087e32e..45291be8 100644
> --- a/libmultipath/util.h
> +++ b/libmultipath/util.h
> @@ -15,6 +15,8 @@ char *convert_dev(char *dev, int is_path_device);
>  char *parse_uid_attribute_by_attrs(char *uid_attrs, char *path_dev);
>  void setup_thread_attr(pthread_attr_t *attr, size_t stacksize, int detached);
>  int systemd_service_enabled(const char *dev);
> +int get_linux_version_code(void);
> +#define KERNEL_VERSION(maj, min, ptc) ((((maj) * 256) + (min)) * 256 + (ptc))
>  
>  #define safe_sprintf(var, format, args...)	\
>  	snprintf(var, sizeof(var), format, ##args) >= sizeof(var)
> diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
> index b32d0383..3b4e5187 100644
> --- a/multipath/multipath.conf.5
> +++ b/multipath/multipath.conf.5
> @@ -698,15 +698,16 @@ The default is: \fB<unset>\fR
>  .
>  .TP
>  .B retain_attached_hw_handler
> -If set to
> +(Obsolete for kernels >= 4.3) If set to
>  .I yes
>  and the SCSI layer has already attached a hardware_handler to the device,
>  multipath will not force the device to use the hardware_handler specified by
>  mutipath.conf. If the SCSI layer has not attached a hardware handler,
>  multipath will continue to use its configured hardware handler.
>  .RS
> -.TP
> -The default is: \fByes\fR
> +.PP
> +The default is: \fByes\fR. Linux kernel 4.3 or newer always behaves as if
> +\fB"retain_attached_hw_handler yes"\fR was set.
>  .RE
>  .
>  .
> @@ -1182,8 +1183,14 @@ Active/Standby mode exclusively.
>  .I 1 alua
>  (Hardware-dependent)
>  Hardware handler for SCSI-3 ALUA compatible arrays.
> -.TP
> +.PP
>  The default is: \fB<unset>\fR
> +.PP
> +\fBImportant Note:\fR Linux kernels 4.3 and newer automatically attach a device
> +handler to known devices (which includes all devices supporting SCSI-3 ALUA)
> +and disallow changing the handler
> +afterwards. Setting \fBhardware_handler\fR for such devices on these kernels
> +has no effect.
>  .RE
>  .
>  .
> -- 
> 2.13.1

Re: [PATCH v4 09/11] libmultipath: retain_attached_hw_handler obsolete with 4.3+

[Xose Vazquez Perez]

On 06/22/2017 04:59 PM, Martin Wilck wrote:

> Kernels 4.3 and newer (commit 1bab0de0 "dm-mpath, scsi_dh: don't
> let dm detach device handlers") imply "retain_attached_hw_handler yes".
> 
> Clarify this in the propsel code, log messages, and documentation.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> Reviewed-by: Hannes Reinecke <hare@suse.com>
> ---
>  libmultipath/configure.c   |  3 ++-
>  libmultipath/dmparser.c    |  3 ++-
>  libmultipath/propsel.c     |  7 ++++++-
>  libmultipath/util.c        | 36 ++++++++++++++++++++++++++++++++++++
>  libmultipath/util.h        |  2 ++
>  multipath/multipath.conf.5 | 15 +++++++++++----
>  6 files changed, 59 insertions(+), 7 deletions(-)
[...]
> --- a/libmultipath/propsel.c
> +++ b/libmultipath/propsel.c
> @@ -628,7 +628,12 @@ int select_retain_hwhandler(struct config *conf, struct multipath *mp)
>  
>  	if (!VERSION_GE(conf->version, minv_dm_retain)) {
>  		mp->retain_hwhandler = RETAIN_HWHANDLER_OFF;
> -		origin = "(setting: WARNING, requires kernel version >= 1.5.0)";
> +		origin = "(setting: WARNING, requires kernel dm-mpath version >= 1.5.0)";
It would be more informative replace the dm-mpath version with the
kernel version. No one cares about subsystems versions.

Re: [PATCH v4 09/11] libmultipath: retain_attached_hw_handler obsolete with 4.3+

[Martin Wilck]

On Fri, 2017-06-23 at 19:25 +0200, Xose Vazquez Perez wrote:
> On 06/22/2017 04:59 PM, Martin Wilck wrote:
> 
> > Kernels 4.3 and newer (commit 1bab0de0 "dm-mpath, scsi_dh: don't
> > let dm detach device handlers") imply "retain_attached_hw_handler
> > yes".
> > 
> > Clarify this in the propsel code, log messages, and documentation.
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > Reviewed-by: Hannes Reinecke <hare@suse.com>
> > ---
> >  libmultipath/configure.c   |  3 ++-
> >  libmultipath/dmparser.c    |  3 ++-
> >  libmultipath/propsel.c     |  7 ++++++-
> >  libmultipath/util.c        | 36
> > ++++++++++++++++++++++++++++++++++++
> >  libmultipath/util.h        |  2 ++
> >  multipath/multipath.conf.5 | 15 +++++++++++----
> >  6 files changed, 59 insertions(+), 7 deletions(-)
> 
> [...]
> > --- a/libmultipath/propsel.c
> > +++ b/libmultipath/propsel.c
> > @@ -628,7 +628,12 @@ int select_retain_hwhandler(struct config
> > *conf, struct multipath *mp)
> >  
> >  	if (!VERSION_GE(conf->version, minv_dm_retain)) {
> >  		mp->retain_hwhandler = RETAIN_HWHANDLER_OFF;
> > -		origin = "(setting: WARNING, requires kernel
> > version >= 1.5.0)";
> > +		origin = "(setting: WARNING, requires kernel dm-
> > mpath version >= 1.5.0)";
> 
> It would be more informative replace the dm-mpath version with the
> kernel version. No one cares about subsystems versions.

I disagree. This code should also work for vendor kernels which may
e.g. contain patches to update dm-mpath without updating the main
kernel (utsname) version.

The reason I used get_linux_version_code() for the new check my patch
introduced was that unfortunately, the dm-mpath version has not been
changed when the "retain_attached_hwhandler" feature was removed in
4.3. The next dm-mpath version change (to 1.10) happened in 4.4.
Thus I couldn't use the dm-mpath version and had to fallback to
utsname.

Thinking about it, the new check should probably be (dm_mpath version
>= 1.10 OR kernel verson >= 4.3). IMO that can be handled in an
incremental patch.

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

[PATCH v4 10/11] libmultipath: don't try to set hwhandler if it is retained

[Martin Wilck]

Setting a device handler only works if retain_attached_hw_handler
is 'no', or if the kernel didn't auto-assign a handler. If this
is not the case, don't even attempt to set a different handler.

This requires reading the sysfs "dh_state" path attribute.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c |  8 +++++++-
 libmultipath/propsel.c   | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 74b6f52a..03874f47 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -275,15 +275,21 @@ int setup_map(struct multipath *mpp, char *params, int params_size)
 
 	/*
 	 * properties selectors
+	 *
+	 * Ordering matters for some properties:
+	 * - features after no_path_retry and retain_hwhandler
+	 * - hwhandler after retain_hwhandler
+	 * No guarantee that this list is complete, check code in
+	 * propsel.c if in doubt.
 	 */
 	conf = get_multipath_config();
 	select_pgfailback(conf, mpp);
 	select_pgpolicy(conf, mpp);
 	select_selector(conf, mpp);
-	select_hwhandler(conf, mpp);
 	select_no_path_retry(conf, mpp);
 	select_retain_hwhandler(conf, mpp);
 	select_features(conf, mpp);
+	select_hwhandler(conf, mpp);
 	select_rr_weight(conf, mpp);
 	select_minio(conf, mpp);
 	select_mode(conf, mpp);
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index d609394e..a86207a0 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -19,8 +19,10 @@
 #include "discovery.h"
 #include "dict.h"
 #include "util.h"
+#include "sysfs.h"
 #include "prioritizers/alua_rtpg.h"
 #include <inttypes.h>
+#include <libudev.h>
 
 pgpolicyfn *pgpolicies[] = {
 	NULL,
@@ -342,9 +344,42 @@ out:
 	return 0;
 }
 
+static int get_dh_state(struct path *pp, char *value, size_t value_len)
+{
+	struct udev_device *ud;
+
+	if (pp->udev == NULL)
+		return -1;
+
+	ud = udev_device_get_parent_with_subsystem_devtype(
+		pp->udev, "scsi", "scsi_device");
+	if (ud == NULL)
+		return -1;
+
+	return sysfs_attr_get_value(ud, "dh_state", value, value_len);
+}
+
 int select_hwhandler(struct config *conf, struct multipath *mp)
 {
 	char *origin;
+	struct path *pp;
+	/* dh_state is no longer than "detached" */
+	char handler[12];
+	char *dh_state;
+	int i;
+
+	dh_state = &handler[2];
+	if (mp->retain_hwhandler != RETAIN_HWHANDLER_OFF) {
+		vector_foreach_slot(mp->paths, pp, i) {
+			if (get_dh_state(pp, dh_state, sizeof(handler) - 2) > 0
+			    && strcmp(dh_state, "detached")) {
+				memcpy(handler, "1 ", 2);
+				mp->hwhandler = handler;
+				origin = "(setting: retained by kernel driver)";
+				goto out;
+			}
+		}
+	}
 
 	mp_set_hwe(hwhandler);
 	mp_set_conf(hwhandler);
-- 
2.13.1

Re: [PATCH v4 10/11] libmultipath: don't try to set hwhandler if it is retained

[Benjamin Marzinski]

On Thu, Jun 22, 2017 at 04:59:12PM +0200, Martin Wilck wrote:
> Setting a device handler only works if retain_attached_hw_handler
> is 'no', or if the kernel didn't auto-assign a handler. If this
> is not the case, don't even attempt to set a different handler.
> 
> This requires reading the sysfs "dh_state" path attribute.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>

ACK

-Ben

> ---
>  libmultipath/configure.c |  8 +++++++-
>  libmultipath/propsel.c   | 35 +++++++++++++++++++++++++++++++++++
>  2 files changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index 74b6f52a..03874f47 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -275,15 +275,21 @@ int setup_map(struct multipath *mpp, char *params, int params_size)
>  
>  	/*
>  	 * properties selectors
> +	 *
> +	 * Ordering matters for some properties:
> +	 * - features after no_path_retry and retain_hwhandler
> +	 * - hwhandler after retain_hwhandler
> +	 * No guarantee that this list is complete, check code in
> +	 * propsel.c if in doubt.
>  	 */
>  	conf = get_multipath_config();
>  	select_pgfailback(conf, mpp);
>  	select_pgpolicy(conf, mpp);
>  	select_selector(conf, mpp);
> -	select_hwhandler(conf, mpp);
>  	select_no_path_retry(conf, mpp);
>  	select_retain_hwhandler(conf, mpp);
>  	select_features(conf, mpp);
> +	select_hwhandler(conf, mpp);
>  	select_rr_weight(conf, mpp);
>  	select_minio(conf, mpp);
>  	select_mode(conf, mpp);
> diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
> index d609394e..a86207a0 100644
> --- a/libmultipath/propsel.c
> +++ b/libmultipath/propsel.c
> @@ -19,8 +19,10 @@
>  #include "discovery.h"
>  #include "dict.h"
>  #include "util.h"
> +#include "sysfs.h"
>  #include "prioritizers/alua_rtpg.h"
>  #include <inttypes.h>
> +#include <libudev.h>
>  
>  pgpolicyfn *pgpolicies[] = {
>  	NULL,
> @@ -342,9 +344,42 @@ out:
>  	return 0;
>  }
>  
> +static int get_dh_state(struct path *pp, char *value, size_t value_len)
> +{
> +	struct udev_device *ud;
> +
> +	if (pp->udev == NULL)
> +		return -1;
> +
> +	ud = udev_device_get_parent_with_subsystem_devtype(
> +		pp->udev, "scsi", "scsi_device");
> +	if (ud == NULL)
> +		return -1;
> +
> +	return sysfs_attr_get_value(ud, "dh_state", value, value_len);
> +}
> +
>  int select_hwhandler(struct config *conf, struct multipath *mp)
>  {
>  	char *origin;
> +	struct path *pp;
> +	/* dh_state is no longer than "detached" */
> +	char handler[12];
> +	char *dh_state;
> +	int i;
> +
> +	dh_state = &handler[2];
> +	if (mp->retain_hwhandler != RETAIN_HWHANDLER_OFF) {
> +		vector_foreach_slot(mp->paths, pp, i) {
> +			if (get_dh_state(pp, dh_state, sizeof(handler) - 2) > 0
> +			    && strcmp(dh_state, "detached")) {
> +				memcpy(handler, "1 ", 2);
> +				mp->hwhandler = handler;
> +				origin = "(setting: retained by kernel driver)";
> +				goto out;
> +			}
> +		}
> +	}
>  
>  	mp_set_hwe(hwhandler);
>  	mp_set_conf(hwhandler);
> -- 
> 2.13.1

[PATCH v4 11/11] libmultipath: don't [un]set queue_if_no_path after domap

[Martin Wilck]

We set the queue_if_no_path feature in assemble_map() already,
no need to set it here again.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 03874f47..98589150 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -1053,21 +1053,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();
-- 
2.13.1

Re: [PATCH v4 00/11] multipath-tools: no_path_retry/queue_if_no_path/hwhandler logic

[Christophe Varoqui]

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

Patch 1 to 9 are now merged.


On Thu, Jun 22, 2017 at 4:59 PM, Martin Wilck <mwilck@suse.com> wrote:

> This patch set attempts to sanitize the logic used for consistently
> handling
> options that can be set both via the "features" string and explicit
> multipath.conf
> options. This is most prominently "no_path_retry" vs. "queue_if_no_path",
> but also
> "retain_attached_hw_handler" vs. the feature of the same name.
>
> The logic this patch set follows is:
>  - If "no_path_retry" is set to any value, "queue_if_no_path" is ignored
>    (this is the case nowadays for almost all "real" storage arrays, thanks
> to hwtable).
>  - Otherwise, if "queue_if_no_path" is set, treat it like "no_path_retry
> queue".
>
> ... likewise for "retain_attached_hw_handler".
>
> In the long run, we should get rid of the "features" settings duplicating
> configuration options altogether; the patch set prepares this by printing
> warning messages.
>
> The logic is implemented in the new function reconcile_features_with_
> options,
> which is called from both select_features() and merge_hwe(). In
> setup_map(),
> we need to call select_features() after select_no_path_retry() to make
> this work.
> The actual feature setting for device-mapper is made in assemble_map(), the
> patch set also fixes the logic there.
>
> The patch set documents the behavior in the man page, and adds some more
> man page fixes.
>
> By skipping superfluous default initializations in load_config(), the
> log messages for the respective config settings become more appropriate.
>
> The logic for setting hardware handler is also improved. Since kernel 4.3,
> "retain_attached_hw_handler yes" is implicitly set by the kernel, and
> setting
> the hardware handler from user space is only possible in special
> situations.
> Acknowledge that in multipathd, and don't try to set or unset either this
> feature or the hwhandler attribute itself if it's doomed to fail.
>
> Review and comments are highly welcome.
>
> Changes wrt v1:
>   - Suggestions from Ben Marzinski:
>     * Made sure "multipathd show config" still works (1/8).
>     * Fixed logging for default setting of "max_sectors" (1/8)
>     * Consistent internal treatment of mp->features (3/8, 4/8)
>     * Fixed whitespace error (6/8)
>     * Added deprecated warnings (8/8)
>   - Made sure the same logic is used in propsel.c and config.c by
>     calling the same function (3/8, 4/8)
>   - Added deprecated warnings (8/8)
>
> Changes wrt v2:
>  - Added Acked-by:/Reviewed-by: tags
>  - 1/11: clarify comment in select_max_sectors_kb (Hannes Reinecke)
>  - 3/11: call select_retain_hwhandler before select_features
>  - 8/11: don't suggest using retain_attached_hw_handler in log msg
>  - 9/11, 10/11, 11/11: new
>
> Changes wrt v3:
>  - Added Reviewed-by: tags (kept Ben's Ack in 4/11 although patch slightly
> changed)
>  - 4/11: use a buffer on stack rather than malloc (Hannes Reinecke)
>  - 10/11: Simplify by checking dh_state only in select_handler (Hannes
> Reinecke)
>
> Martin Wilck (11):
>   libmultipath: load_config: skip setting unnecessary defaults
>   libmultipath: add/remove_feature: use const char* for feature
>   libmultipath: clarify option conflicts for "features"
>   libmultipath: merge_hwe: fix queue_if_no_path logic
>   libmultipath: assemble_map: fix queue_if_no_path logic
>   multipath.conf.5: document no_path_retry vs. queue_if_no_path
>   multipath.conf.5: Remove ??? and other minor fixes
>   libmultipath: add deprecated warning for some features settings
>   libmultipath: retain_attached_hw_handler obsolete with 4.3+
>   libmultipath: don't try to set hwhandler if it is retained
>   libmultipath: don't [un]set queue_if_no_path after domap
>
>  libmultipath/config.c      |  31 +++---------
>  libmultipath/configure.c   |  28 ++++-------
>  libmultipath/dict.c        |  11 +++--
>  libmultipath/dmparser.c    |   8 +--
>  libmultipath/propsel.c     | 121 ++++++++++++++++++++++++++++++
> +++++++++------
>  libmultipath/propsel.h     |   3 ++
>  libmultipath/structs.c     |  30 +++++------
>  libmultipath/structs.h     |   4 +-
>  libmultipath/util.c        |  36 ++++++++++++++
>  libmultipath/util.h        |   2 +
>  multipath/multipath.conf.5 |  67 +++++++++++++++----------
>  11 files changed, 231 insertions(+), 110 deletions(-)
>
> --
> 2.13.1
>
>

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

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

Re: [PATCH v4 00/11] multipath-tools: no_path_retry/queue_if_no_path/hwhandler logic

[Martin Wilck]

Hi Christophe,

On Thu, 2017-08-03 at 08:36 +0200, Christophe Varoqui wrote:
> Patch 1 to 9 are now merged.

So you have issues with patch 10 and 11? Could you elaborate, or advise
what you'd like to have changed?

> >  (10/11) libmultipath: don't try to set hwhandler if it is retained
> >  (11/11) libmultipath: don't [un]set queue_if_no_path after domap

11/11 can be viewed as an optimization only (though a reasonable one, I
think), but 10/11 is important to preserve consistent state between
kernel and multipathd.

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