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

[PATCH v3 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

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      |  36 +++++-----------
 libmultipath/configure.c   |  72 ++++++++++++++++++++++++--------
 libmultipath/dict.c        |  11 +++--
 libmultipath/discovery.c   |   4 ++
 libmultipath/dmparser.c    |   8 ++--
 libmultipath/propsel.c     | 101 ++++++++++++++++++++++++++++++++++++++-------
 libmultipath/propsel.h     |   3 ++
 libmultipath/structs.c     |  30 +++++++-------
 libmultipath/structs.h     |   6 ++-
 libmultipath/util.c        |  36 ++++++++++++++++
 libmultipath/util.h        |   2 +
 multipath/multipath.conf.5 |  67 ++++++++++++++++++------------
 12 files changed, 266 insertions(+), 110 deletions(-)

-- 
2.13.1

[PATCH v3 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>
---
 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

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

[Hannes Reinecke]

On 06/21/2017 05:06 PM, Martin Wilck wrote:
> 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>
> ---
>  libmultipath/config.c  | 16 ----------------
>  libmultipath/dict.c    | 11 +++++++----
>  libmultipath/propsel.c |  6 ++++++
>  3 files changed, 13 insertions(+), 20 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

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

[PATCH v3 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>
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 v3 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>
---
 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

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

[Hannes Reinecke]

On 06/21/2017 05:06 PM, Martin Wilck wrote:
> 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>
> ---
>  libmultipath/configure.c |  6 ++---
>  libmultipath/propsel.c   | 68 +++++++++++++++++++++++++++++++++++++-----------
>  libmultipath/propsel.h   |  3 +++
>  3 files changed, 59 insertions(+), 18 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

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

[PATCH v3 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 | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/libmultipath/config.c b/libmultipath/config.c
index 60e345b3..a9b3eda2 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,8 @@ set_param_str(char * str)
 static int
 merge_hwe (struct hwentry * dst, struct hwentry * src)
 {
+	int id_len;
+	char *id;
 	merge_str(vendor);
 	merge_str(product);
 	merge_str(revision);
@@ -353,15 +356,14 @@ 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");
-
+	id_len = strlen(dst->vendor) + strlen(dst->product) + 2;
+	id = MALLOC(id_len);
+	if (id != NULL)
+		snprintf(id, id_len, "%s/%s", dst->vendor, dst->product);
+	reconcile_features_with_options(id, &dst->features,
+					&dst->no_path_retry,
+					&dst->retain_hwhandler);
+	FREE(id);
 	return 0;
 }
 
-- 
2.13.1

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

[Hannes Reinecke]

On 06/21/2017 05:06 PM, Martin Wilck wrote:
> 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 | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/libmultipath/config.c b/libmultipath/config.c
> index 60e345b3..a9b3eda2 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,8 @@ set_param_str(char * str)
>  static int
>  merge_hwe (struct hwentry * dst, struct hwentry * src)
>  {
> +	int id_len;
> +	char *id;
>  	merge_str(vendor);
>  	merge_str(product);
>  	merge_str(revision);
> @@ -353,15 +356,14 @@ 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");
> -
> +	id_len = strlen(dst->vendor) + strlen(dst->product) + 2;
> +	id = MALLOC(id_len);
> +	if (id != NULL)
> +		snprintf(id, id_len, "%s/%s", dst->vendor, dst->product);
> +	reconcile_features_with_options(id, &dst->features,
> +					&dst->no_path_retry,
> +					&dst->retain_hwhandler);
> +	FREE(id);
>  	return 0;
>  }
>  
> 
Ugh.

Allocating a string just for having 'nice' debugging messages in
reconcile_features_with_options?
Please, don't.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

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

[PATCH v3 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>
---
 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

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

[Hannes Reinecke]

On 06/21/2017 05:06 PM, Martin Wilck wrote:
> 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>
> ---
>  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)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

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

[PATCH v3 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>
---
 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

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

[Hannes Reinecke]

On 06/21/2017 05:06 PM, Martin Wilck wrote:
> Clarify the documentation about option precedence.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> Acked-by: Benjamin Marzinski <bmarzins@redhat.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.
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

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

[PATCH v3 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>
---
 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

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

[Hannes Reinecke]

On 06/21/2017 05:06 PM, Martin Wilck wrote:
> 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>
> ---
>  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
>  .
>  .
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

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

[PATCH v3 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>
---
 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

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

[Hannes Reinecke]

On 06/21/2017 05:06 PM, Martin Wilck wrote:
> 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>
> ---
>  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.
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

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

[PATCH v3 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>
---
 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 v3 09/11] libmultipath: retain_attached_hw_handler obsolete with 4.3+

[Hannes Reinecke]

On 06/21/2017 05:06 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>
> ---
>  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(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

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

[PATCH v3 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.
For internal consistency, this attribute must be updated after domap().

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++-
 libmultipath/discovery.c |  4 ++++
 libmultipath/propsel.c   | 15 ++++++++++++++
 libmultipath/structs.h   |  2 ++
 4 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 74b6f52a..55dbb261 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);
@@ -706,6 +712,48 @@ fail:
 	return 1;
 }
 
+static void
+check_dh_state_changed(struct multipath *mp)
+{
+	struct config *conf;
+	struct path newp, *pp;
+	struct pathgroup *pg;
+	int i, j;
+
+	conf = get_multipath_config();
+
+	vector_foreach_slot (mp->pg, pg, j) {
+		vector_foreach_slot (pg->paths, pp, i) {
+			if (!pp->udev || !strlen(pp->dh_state) ||
+			    (conf->retain_hwhandler == RETAIN_HWHANDLER_ON &&
+			     strcmp(pp->dh_state, "detached")))
+				continue;
+
+			memset(&newp, 0, sizeof(newp));
+			memcpy(newp.dev, pp->dev, sizeof(newp.dev));
+			newp.udev = udev_device_ref(pp->udev);
+
+			if (pathinfo(&newp, conf, DI_SYSFS) == PATHINFO_OK) {
+				if (strncmp(newp.dh_state, pp->dh_state,
+					    SCSI_DH_SIZE)) {
+					condlog(3, "%s: dh_state changed from %s to %s",
+						pp->dev,
+						pp->dh_state,
+						newp.dh_state);
+					memcpy(pp->dh_state, newp.dh_state,
+					       SCSI_DH_SIZE);
+				}
+			} else
+				condlog(1, "%s: failed to update dh_state",
+					pp->dev);
+
+			udev_device_unref(newp.udev);
+		}
+	}
+
+	put_multipath_config(conf);
+}
+
 /*
  * Return value:
  */
@@ -828,6 +876,8 @@ int domap(struct multipath *mpp, char *params, int is_daemon)
 			}
 		}
 		dm_setgeometry(mpp);
+
+		check_dh_state_changed(mpp);
 		return DOMAP_OK;
 	}
 	return DOMAP_FAIL;
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 663c8eaa..8c0c6a9f 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1188,6 +1188,10 @@ scsi_sysfs_pathinfo (struct path * pp, vector hwtable)
 	condlog(3, "%s: tgt_node_name = %s",
 		pp->dev, pp->tgt_node_name);
 
+	if (sysfs_attr_get_value(parent, "dh_state",
+				 pp->dh_state, sizeof(pp->dh_state)) >= 0)
+		condlog(3, "%s: dh_state = %s", pp->dev, pp->dh_state);
+
 	return 0;
 }
 
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index d609394e..d1b3d416 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -345,7 +345,22 @@ out:
 int select_hwhandler(struct config *conf, struct multipath *mp)
 {
 	char *origin;
+	struct path *pp;
+	char handler[SCSI_DH_SIZE+2];
+	int i;
 
+	if (mp->retain_hwhandler != RETAIN_HWHANDLER_OFF) {
+		vector_foreach_slot(mp->paths, pp, i) {
+			if (strlen(pp->dh_state) &&
+			    strcmp(pp->dh_state, "detached")) {
+				snprintf(handler, sizeof(handler),
+					 "1 %s", pp->dh_state);
+				mp->hwhandler = handler;
+				origin = "(setting: retained by kernel driver)";
+				goto out;
+			}
+		}
+	}
 	mp_set_hwe(hwhandler);
 	mp_set_conf(hwhandler);
 	mp_set_default(hwhandler, DEFAULT_HWHANDLER);
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 8ea984d9..4203e2b0 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -22,6 +22,7 @@
 #define SCSI_PRODUCT_SIZE	17
 #define SCSI_REV_SIZE		5
 #define SCSI_STATE_SIZE		19
+#define SCSI_DH_SIZE		9 /* must hold "detached" */
 
 #define NO_PATH_RETRY_UNDEF	0
 #define NO_PATH_RETRY_FAIL	-1
@@ -205,6 +206,7 @@ struct path {
 	char rev[SCSI_REV_SIZE];
 	char serial[SERIAL_SIZE];
 	char tgt_node_name[NODE_NAME_SIZE];
+	char dh_state[SCSI_DH_SIZE];
 	unsigned long long size;
 	unsigned int checkint;
 	unsigned int tick;
-- 
2.13.1

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

[Hannes Reinecke]

On 06/21/2017 05:06 PM, 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.
> For internal consistency, this attribute must be updated after domap().
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/configure.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++-
>  libmultipath/discovery.c |  4 ++++
>  libmultipath/propsel.c   | 15 ++++++++++++++
>  libmultipath/structs.h   |  2 ++
>  4 files changed, 72 insertions(+), 1 deletion(-)
> 
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index 74b6f52a..55dbb261 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);
> @@ -706,6 +712,48 @@ fail:
>  	return 1;
>  }
>  
> +static void
> +check_dh_state_changed(struct multipath *mp)
> +{
> +	struct config *conf;
> +	struct path newp, *pp;
> +	struct pathgroup *pg;
> +	int i, j;
> +
> +	conf = get_multipath_config();
> +
> +	vector_foreach_slot (mp->pg, pg, j) {
> +		vector_foreach_slot (pg->paths, pp, i) {
> +			if (!pp->udev || !strlen(pp->dh_state) ||
> +			    (conf->retain_hwhandler == RETAIN_HWHANDLER_ON &&
> +			     strcmp(pp->dh_state, "detached")))
> +				continue;
> +
> +			memset(&newp, 0, sizeof(newp));
> +			memcpy(newp.dev, pp->dev, sizeof(newp.dev));
> +			newp.udev = udev_device_ref(pp->udev);
> +
> +			if (pathinfo(&newp, conf, DI_SYSFS) == PATHINFO_OK) {
> +				if (strncmp(newp.dh_state, pp->dh_state,
> +					    SCSI_DH_SIZE)) {
> +					condlog(3, "%s: dh_state changed from %s to %s",
> +						pp->dev,
> +						pp->dh_state,
> +						newp.dh_state);
> +					memcpy(pp->dh_state, newp.dh_state,
> +					       SCSI_DH_SIZE);
> +				}
> +			} else
> +				condlog(1, "%s: failed to update dh_state",
> +					pp->dev);
> +
> +			udev_device_unref(newp.udev);
> +		}
> +	}
> +
> +	put_multipath_config(conf);
> +}
> +
>  /*
>   * Return value:
>   */
I would avoid using a temporary udev device here; either save the
dh_state value and call pathinfo() on the _actual_ device or read
dh_state directly from sysfs without an intermediate udev device.

> @@ -828,6 +876,8 @@ int domap(struct multipath *mpp, char *params, int is_daemon)
>  			}
>  		}
>  		dm_setgeometry(mpp);
> +
> +		check_dh_state_changed(mpp);
>  		return DOMAP_OK;
>  	}
>  	return DOMAP_FAIL;
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 663c8eaa..8c0c6a9f 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -1188,6 +1188,10 @@ scsi_sysfs_pathinfo (struct path * pp, vector hwtable)
>  	condlog(3, "%s: tgt_node_name = %s",
>  		pp->dev, pp->tgt_node_name);
>  
> +	if (sysfs_attr_get_value(parent, "dh_state",
> +				 pp->dh_state, sizeof(pp->dh_state)) >= 0)
> +		condlog(3, "%s: dh_state = %s", pp->dev, pp->dh_state);
> +
>  	return 0;
>  }
>  
> diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
> index d609394e..d1b3d416 100644
> --- a/libmultipath/propsel.c
> +++ b/libmultipath/propsel.c
> @@ -345,7 +345,22 @@ out:
>  int select_hwhandler(struct config *conf, struct multipath *mp)
>  {
>  	char *origin;
> +	struct path *pp;
> +	char handler[SCSI_DH_SIZE+2];
> +	int i;
>  
> +	if (mp->retain_hwhandler != RETAIN_HWHANDLER_OFF) {
> +		vector_foreach_slot(mp->paths, pp, i) {
> +			if (strlen(pp->dh_state) &&
> +			    strcmp(pp->dh_state, "detached")) {
> +				snprintf(handler, sizeof(handler),
> +					 "1 %s", pp->dh_state);
> +				mp->hwhandler = handler;
> +				origin = "(setting: retained by kernel driver)";
> +				goto out;
> +			}
> +		}
> +	}
>  	mp_set_hwe(hwhandler);
>  	mp_set_conf(hwhandler);
>  	mp_set_default(hwhandler, DEFAULT_HWHANDLER);
> diff --git a/libmultipath/structs.h b/libmultipath/structs.h
> index 8ea984d9..4203e2b0 100644
> --- a/libmultipath/structs.h
> +++ b/libmultipath/structs.h
> @@ -22,6 +22,7 @@
>  #define SCSI_PRODUCT_SIZE	17
>  #define SCSI_REV_SIZE		5
>  #define SCSI_STATE_SIZE		19
> +#define SCSI_DH_SIZE		9 /* must hold "detached" */
>  
>  #define NO_PATH_RETRY_UNDEF	0
>  #define NO_PATH_RETRY_FAIL	-1
> @@ -205,6 +206,7 @@ struct path {
>  	char rev[SCSI_REV_SIZE];
>  	char serial[SERIAL_SIZE];
>  	char tgt_node_name[NODE_NAME_SIZE];
> +	char dh_state[SCSI_DH_SIZE];
>  	unsigned long long size;
>  	unsigned int checkint;
>  	unsigned int tick;
> 
Hmm.

Not sure if I fully agree with this.
I do see the need to read 'dh_state' from pathinfo(), just to figure out
if an hardware handler is already loaded.

But once select_hwhandler is done it's quite pointless to update the
dh_state; what exactly _would_ be the error action in this case?
Plus the code detects the failure, but then doesn't do anything with it...

So, please, if you insist on checking dh_state please implement correct
error action here, like updating the 'hwhandler' value to that found in
dh_state or disabling the hardware handler if it's found to be detached.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

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

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

[Martin Wilck]

On Thu, 2017-06-22 at 08:21 +0200, Hannes Reinecke wrote:
> On 06/21/2017 05:06 PM, 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.
> > For internal consistency, this attribute must be updated after
> > domap().
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  libmultipath/configure.c | 52
> > +++++++++++++++++++++++++++++++++++++++++++++++-
> >  libmultipath/discovery.c |  4 ++++
> >  libmultipath/propsel.c   | 15 ++++++++++++++
> >  libmultipath/structs.h   |  2 ++
> >  4 files changed, 72 insertions(+), 1 deletion(-)
> > 
> > [...]
>
> Hmm.
> 
> Not sure if I fully agree with this.
> I do see the need to read 'dh_state' from pathinfo(), just to figure
> out
> if an hardware handler is already loaded.
> 
> But once select_hwhandler is done it's quite pointless to update the
> dh_state; what exactly _would_ be the error action in this case?
> Plus the code detects the failure, but then doesn't do anything with
> it...

My concern was that multipathd might carry along wrong state and
possibly print it in log messages, irritating users. It's true, there
is no reasonable error action, and it's quite a lot of code just for
this minor purpose.

> So, please, if you insist on checking dh_state please implement
> correct
> error action here, like updating the 'hwhandler' value to that found
> in
> dh_state or disabling the hardware handler if it's found to be
> detached.

If it's fine with you and other reviewers, I'll happily remove that
part of the patch, and just keep the part in select_hwhandler().

If we want proper error handling, we'd need to check that the handler
of the loaded map as well as the handlers of all paths are set to the
handler configured in multipathd. Unless I'm mistaken, it isn't
guaranteed that all paths will be using the same handler after a map is
set up.

Besides re-reading the dh_state of all paths, this check would also
require re-reading and disassembling the map, and no reasonable error
action is to be seen other then updating the internal state, lots of
code for almost nothing.

Cheers,
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 v3 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 55dbb261..39912f05 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -1097,21 +1097,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 v3 11/11] libmultipath: don't [un]set queue_if_no_path after domap

[Hannes Reinecke]

On 06/21/2017 05:06 PM, Martin Wilck wrote:
> 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 55dbb261..39912f05 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -1097,21 +1097,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();
> 
Watch out.
'queue_if_no_path' is set _temporarily_ while 'no_path_retry' is active,
and removed afterwards.
So there might be valid reasons why it's set here.
Have you checked?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

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

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

[Martin Wilck]

On Thu, 2017-06-22 at 08:23 +0200, Hannes Reinecke wrote:
> On 06/21/2017 05:06 PM, Martin Wilck wrote:
> > 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(-)
> > 
> > [...]
> Watch out.
> 'queue_if_no_path' is set _temporarily_ while 'no_path_retry' is
> active,
> and removed afterwards.
> So there might be valid reasons why it's set here.
> Have you checked?

Yes, I'm pretty certain that this is correct. We're in coalesce_paths()
here, while we are setting up or reconfiguring maps. The call sequence
is 

   setup_map()
      assemble_map()
   domap()
   ... and then comes the code I'm removing.

We set the feature string in assemble_map() correctly. Thus the removed
code just repeated the same setting that had already been applied in
domap(). This code has been in that place for a very long time, AFAICS
it originates from times where the features string was not correctly
set up before creating or reloading the map.

The logic for disabling queue_if_no_path if the retry count is reached
is implemented elsewhere, mainly in set_no_path_retry() (called e.g.
from ev_remove_path()) and retry_count_tick() (called from checker
loop).

Do you see a case that I have overlooked?

Best,
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 v3 11/11] libmultipath: don't [un]set queue_if_no_path after domap

[Benjamin Marzinski]

On Thu, Jun 22, 2017 at 08:23:44AM +0200, Hannes Reinecke wrote:
> On 06/21/2017 05:06 PM, Martin Wilck wrote:
> > 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 55dbb261..39912f05 100644
> > --- a/libmultipath/configure.c
> > +++ b/libmultipath/configure.c
> > @@ -1097,21 +1097,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();
> > 
> Watch out.
> 'queue_if_no_path' is set _temporarily_ while 'no_path_retry' is active,
> and removed afterwards.
> So there might be valid reasons why it's set here.
> Have you checked?

IIRC, we used to always set queue_if_no_path before reloading the map,
and then call setup_multipath() afterwards, which would call
set_no_path_retry() to set it to the actual correct value. If we are
correctly setting the feature line when we reload the map, that's a
better solution. Obviously, we can't strip set_no_path_retry() out of
__setup_multipath() since we rely on that to deal with deal with going
into recovery mode. However, without some more thought and code reading,
I'm not sure if we do still need the calls to dm_queue_if_no_path()
there for some other reason anymore.  At any rate, they won't hurt
anything, except for causing a redundant call to device-mapper.

Also, if we're yanking these dm_queue_if_no_path() calls from
coalesce_paths, I don't see why we need them in reload_map(), where they
also seem redundant if we just correctly set the features argument when
we reloaded the table.

ACK, but I wouldn't mind seeing the calls pulled from reload_map as
well.

-Ben

> 
> Cheers,
> 
> Hannes
> -- 
> Dr. Hannes Reinecke		   Teamlead Storage & Networking
> hare@suse.de			               +49 911 74053 688
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
> HRB 21284 (AG Nürnberg)

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

[Martin Wilck]

On Thu, 2017-06-22 at 14:21 -0500, Benjamin Marzinski wrote:
> On Thu, Jun 22, 2017 at 08:23:44AM +0200, Hannes Reinecke wrote:
> > On 06/21/2017 05:06 PM, Martin Wilck wrote:
> > > 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(-)
> > > 
> > > 
> > 
> > Watch out.
> > 'queue_if_no_path' is set _temporarily_ while 'no_path_retry' is
> > active,
> > and removed afterwards.
> > So there might be valid reasons why it's set here.
> > Have you checked?
> 
> IIRC, we used to always set queue_if_no_path before reloading the
> map,

Why? I've searched for comments explaining that but found nothing.
Anyway, we've ceased to do so at least since 7331cf5 "Correctly update
feature strings", merged in May 2011.

> and then call setup_multipath() afterwards, which would call
> set_no_path_retry() to set it to the actual correct value. 

setup_multipath() is called too, a bit later in the code flow, from
configure() after coalesce_paths() and coalesce_maps(). So we're
currently setting queue_if_no_path 3 times when creating maps: in
domap(), after domap(), and in setup_multipath->set_no_path_retry().
With my patch, we do it only twice :-)

The call to dm_queue_if_no_path directly after domap(), which my patch
removes, is ancient, it came with 0e6e3113 "[multipath] Extension of
the "no_path_retry" scope to the multipath" in 2005.

> If we are
> correctly setting the feature line when we reload the map, that's a
> better solution. Obviously, we can't strip set_no_path_retry() out of
> __setup_multipath() since we rely on that to deal with deal with
> going
> into recovery mode. However, without some more thought and code
> reading,
> I'm not sure if we do still need the calls to dm_queue_if_no_path()
> there for some other reason anymore.  At any rate, they won't hurt
> anything, except for causing a redundant call to device-mapper.
> 
> Also, if we're yanking these dm_queue_if_no_path() calls from
> coalesce_paths, I don't see why we need them in reload_map(), where
> they
> also seem redundant if we just correctly set the features argument
> when
> we reloaded the table.
> 
> ACK, but I wouldn't mind seeing the calls pulled from reload_map as
> well.

Agreed, but I propose to do that in a separate patch. No need to repost
the whole series for that.

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 v3 00/11] multipath-tools: no_path_retry/queue_if_no_path/hwhandler logic

[Martin Wilck]

On Wed, 2017-06-21 at 17:06 +0200, Martin Wilck wrote:
> 
> 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

Forgot to mention: rebased to Christophe's latest code (847cc43c).

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