[PATCH 0/7] no_path_retry/queue_if_no_path logic & logging/man page fixes

[PATCH 0/7] no_path_retry/queue_if_no_path logic & logging/man page fixes

[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".

The logic is implemented in select_features() (which needs to be called after
select_no_path_retry()) for multipath groups, and similarly in merge_hwe() for
hwtable entries. The actual feature setting is made in assemble_map(), I also
fix a problem there.

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

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

Review and comments are highly welcome.

Regards,
Martin

Martin Wilck (7):
  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/config.c      | 36 ++++++++++++++------------------
 libmultipath/configure.c   |  4 ++--
 libmultipath/dmparser.c    |  5 ++---
 libmultipath/propsel.c     | 39 +++++++++++++++++++++-------------
 libmultipath/structs.c     | 30 +++++++++++++-------------
 libmultipath/structs.h     |  4 ++--
 multipath/multipath.conf.5 | 52 ++++++++++++++++++++++++++--------------------
 7 files changed, 92 insertions(+), 78 deletions(-)

-- 
2.13.0

[PATCH 1/7] 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)'
even if multipath.conf doesn't contain a 'features' setting at all.
With this patch, these defaults are correctly logged as "multipath internal".

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/config.c  | 16 ----------------
 libmultipath/propsel.c |  2 +-
 2 files changed, 1 insertion(+), 17 deletions(-)

diff --git a/libmultipath/config.c b/libmultipath/config.c
index bb6619b3..61bbba91 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -599,40 +599,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/propsel.c b/libmultipath/propsel.c
index 385063aa..99d17e65 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -752,7 +752,7 @@ 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);
-	return 0;
+	mp_set_default(max_sectors_kb, DEFAULT_MAX_SECTORS_KB);
 out:
 	condlog(3, "%s: max_sectors_kb = %i %s", mp->alias, mp->max_sectors_kb,
 		origin);
-- 
2.13.0

Re: [PATCH 1/7] libmultipath: load_config: skip setting unnecessary defaults

[Benjamin Marzinski]

On Wed, Jun 14, 2017 at 12:55:48AM +0200, 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)'
> even if multipath.conf doesn't contain a 'features' setting at all.
> With this patch, these defaults are correctly logged as "multipath internal".
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/config.c  | 16 ----------------
>  libmultipath/propsel.c |  2 +-
>  2 files changed, 1 insertion(+), 17 deletions(-)
> 
> diff --git a/libmultipath/config.c b/libmultipath/config.c
> index bb6619b3..61bbba91 100644
> --- a/libmultipath/config.c
> +++ b/libmultipath/config.c
> @@ -599,40 +599,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

If you look at the output of "multipathd show config", with this change
multipath is not longer printing the built-in defaults for some options
that do have defined defaults. Fixing this probably just requires using
declare_def_snprint_defint() instead of declare_def_snprint() for these
options it dict.c.

> diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
> index 385063aa..99d17e65 100644
> --- a/libmultipath/propsel.c
> +++ b/libmultipath/propsel.c
> @@ -752,7 +752,7 @@ 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);
> -	return 0;
> +	mp_set_default(max_sectors_kb, DEFAULT_MAX_SECTORS_KB);
>  out:
>  	condlog(3, "%s: max_sectors_kb = %i %s", mp->alias, mp->max_sectors_kb,
>  		origin);

I think this will be confusing.  The default for max_sectors_kb is
undefined (i.e. if users don't set it, multipath doesn't doesn't change
the existing sysfs value for max_sectors_kb for the path devices).
Instead of just printing nothing, we will now print that max_sectors_kb
has been set to 0, which isn't actually true. I don't disagree that we
should explicitly set the value for the multipath device. I just think
that we should either print nothing, or define a print function that
prints "undefined" or something for 0 and the number otherwise, and use
that to generate the string to print, much like how
select_san_path_err_recovery_time() and others use print_off_int_undef()

-Ben

> -- 
> 2.13.0

[PATCH 2/7] 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>
---
 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.0

[PATCH 3/7] libmultipath: clarify option conflicts for "features"

[Martin Wilck]

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

Currently, "no_path_retry" takes precedence, unless it is set to
"fail", in which case it's overridden. 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.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c |  4 ++--
 libmultipath/propsel.c   | 37 ++++++++++++++++++++++++-------------
 2 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index bd090d9a..fd4721dd 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -280,11 +280,11 @@ 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_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);
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index 99d17e65..4267aa04 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -272,6 +272,9 @@ out:
 int select_features(struct config *conf, struct multipath *mp)
 {
 	char *origin;
+	char buff[12];
+	static const char q_i_n_p[] = "queue_if_no_path";
+	static const char r_a_h_h[] = "retain_attached_hw_handler";
 
 	mp_set_mpe(features);
 	mp_set_ovr(features);
@@ -280,19 +283,30 @@ 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)
+	if (strstr(mp->features, q_i_n_p)) {
+		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, 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, ignoring 'queue_if_no_path' because no_path_retry=%d",
-				mp->alias, mp->no_path_retry);
+			print_no_path_retry(buff, sizeof(buff),
+					    &mp->no_path_retry);
+			condlog(3, "%s: no_path_retry = %s (inherited setting from feature '%s')",
+				mp->alias, buff, q_i_n_p);
+		} else {
+			print_no_path_retry(buff, sizeof(buff),
+					    &mp->no_path_retry);
+			condlog(2, "%s: ignoring feature '%s' because no_path_retry is set to '%s'",
+				mp->alias, q_i_n_p, buff);
+			remove_feature(&mp->features, q_i_n_p);
+		};
 	}
+	if (strstr(mp->features, r_a_h_h) &&
+	    mp->retain_hwhandler == RETAIN_HWHANDLER_OFF) {
+		condlog(2, "%s: ignoring feature '%s' because %s is set to 'off'",
+			mp->alias, r_a_h_h, r_a_h_h);
+		remove_feature(&mp->features, r_a_h_h);
+	}
+
+	condlog(3, "%s: features = \"%s\" %s", mp->alias, mp->features, origin);
 	return 0;
 }
 
@@ -469,9 +483,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 (inherited setting)",
-			mp->alias, buff);
 	else
 		condlog(3, "%s: no_path_retry = undef (setting: multipath internal)",
 			mp->alias);
-- 
2.13.0

Re: [PATCH 3/7] libmultipath: clarify option conflicts for "features"

[Benjamin Marzinski]

On Wed, Jun 14, 2017 at 12:55:50AM +0200, Martin Wilck wrote:
> The "features" option in multipath.conf can possibly conflict
> with the "no_path_retry" and "retain_attached_hw_handler" options.
> 
> Currently, "no_path_retry" takes precedence, unless it is set to
> "fail", in which case it's overridden. 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.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/configure.c |  4 ++--
>  libmultipath/propsel.c   | 37 ++++++++++++++++++++++++-------------
>  2 files changed, 26 insertions(+), 15 deletions(-)
> 
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index bd090d9a..fd4721dd 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -280,11 +280,11 @@ 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_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);
> diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
> index 99d17e65..4267aa04 100644
> --- a/libmultipath/propsel.c
> +++ b/libmultipath/propsel.c
> @@ -272,6 +272,9 @@ out:
>  int select_features(struct config *conf, struct multipath *mp)
>  {
>  	char *origin;
> +	char buff[12];
> +	static const char q_i_n_p[] = "queue_if_no_path";
> +	static const char r_a_h_h[] = "retain_attached_hw_handler";
>  
>  	mp_set_mpe(features);
>  	mp_set_ovr(features);
> @@ -280,19 +283,30 @@ 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)
> +	if (strstr(mp->features, q_i_n_p)) {
> +		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, 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, ignoring 'queue_if_no_path' because no_path_retry=%d",
> -				mp->alias, mp->no_path_retry);
> +			print_no_path_retry(buff, sizeof(buff),
> +					    &mp->no_path_retry);
> +			condlog(3, "%s: no_path_retry = %s (inherited setting from feature '%s')",
> +				mp->alias, buff, q_i_n_p);
> +		} else {
> +			print_no_path_retry(buff, sizeof(buff),
> +					    &mp->no_path_retry);
> +			condlog(2, "%s: ignoring feature '%s' because no_path_retry is set to '%s'",
> +				mp->alias, q_i_n_p, buff);
> +			remove_feature(&mp->features, q_i_n_p);
> +		};

This is just a nit, and it won't hurt anything by remaining unfixed, but
it is odd that if you go into select_features() with no_path_retry set
to queue (for any length of time) and features includes
"queue_if_no_path", you will exit with no_path_retry still set to queue
and "queue_if_no_path" removed from features. However if you go into
select_features with no_path_retry set to undef and features includes
"queue_if_no_path", you will exit with no_path_retry set to queue and
"queue_if_no_path" still included in features. It seems odd that in the
second case, you explicitly set these options to the values that you
started with in the first case (which you later changed). A perhaps more
sensible option would be to only remove "queue_if_no_path" from features
if no_path_retry is set to something other than NO_PATH_RETRY_QUEUE.

-Ben

>  	}
> +	if (strstr(mp->features, r_a_h_h) &&
> +	    mp->retain_hwhandler == RETAIN_HWHANDLER_OFF) {
> +		condlog(2, "%s: ignoring feature '%s' because %s is set to 'off'",
> +			mp->alias, r_a_h_h, r_a_h_h);
> +		remove_feature(&mp->features, r_a_h_h);
> +	}
> +
> +	condlog(3, "%s: features = \"%s\" %s", mp->alias, mp->features, origin);
>  	return 0;
>  }
>  
> @@ -469,9 +483,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 (inherited setting)",
> -			mp->alias, buff);
>  	else
>  		condlog(3, "%s: no_path_retry = undef (setting: multipath internal)",
>  			mp->alias);
> -- 
> 2.13.0

Re: [PATCH 3/7] libmultipath: clarify option conflicts for "features"

[Martin Wilck]

Hi Ben,

On Thu, 2017-06-15 at 14:54 -0500, Benjamin Marzinski wrote:
> On Wed, Jun 14, 2017 at 12:55:50AM +0200, Martin Wilck wrote:
> > The "features" option in multipath.conf can possibly conflict
> > with the "no_path_retry" and "retain_attached_hw_handler" options.
> > 
> > Currently, "no_path_retry" takes precedence, unless it is set to
> > "fail", in which case it's overridden. 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.
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  libmultipath/configure.c |  4 ++--
> >  libmultipath/propsel.c   | 37 ++++++++++++++++++++++++----------
> > ---
> >  2 files changed, 26 insertions(+), 15 deletions(-)
> > 
> > diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> > index bd090d9a..fd4721dd 100644
> > --- a/libmultipath/configure.c
> > +++ b/libmultipath/configure.c
> > @@ -280,11 +280,11 @@ 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_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);
> > diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
> > index 99d17e65..4267aa04 100644
> > --- a/libmultipath/propsel.c
> > +++ b/libmultipath/propsel.c
> > @@ -272,6 +272,9 @@ out:
> >  int select_features(struct config *conf, struct multipath *mp)
> >  {
> >  	char *origin;
> > +	char buff[12];
> > +	static const char q_i_n_p[] = "queue_if_no_path";
> > +	static const char r_a_h_h[] =
> > "retain_attached_hw_handler";
> >  
> >  	mp_set_mpe(features);
> >  	mp_set_ovr(features);
> > @@ -280,19 +283,30 @@ 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)
> > +	if (strstr(mp->features, q_i_n_p)) {
> > +		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, 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, ignoring
> > 'queue_if_no_path' because no_path_retry=%d",
> > -				mp->alias, mp->no_path_retry);
> > +			print_no_path_retry(buff, sizeof(buff),
> > +					    &mp->no_path_retry);
> > +			condlog(3, "%s: no_path_retry = %s
> > (inherited setting from feature '%s')",
> > +				mp->alias, buff, q_i_n_p);
> > +		} else {
> > +			print_no_path_retry(buff, sizeof(buff),
> > +					    &mp->no_path_retry);
> > +			condlog(2, "%s: ignoring feature '%s'
> > because no_path_retry is set to '%s'",
> > +				mp->alias, q_i_n_p, buff);
> > +			remove_feature(&mp->features, q_i_n_p);
> > +		};
> 
> This is just a nit, and it won't hurt anything by remaining unfixed,
> but
> it is odd that if you go into select_features() with no_path_retry
> set
> to queue (for any length of time) and features includes
> "queue_if_no_path", you will exit with no_path_retry still set to
> queue
> and "queue_if_no_path" removed from features. However if you go into
> select_features with no_path_retry set to undef and features includes
> "queue_if_no_path", you will exit with no_path_retry set to queue and
> "queue_if_no_path" still included in features. It seems odd that in
> the
> second case, you explicitly set these options to the values that you
> started with in the first case (which you later changed). A perhaps
> more
> sensible option would be to only remove "queue_if_no_path" from
> features
> if no_path_retry is set to something other than NO_PATH_RETRY_QUEUE.


You're right. For internal consistency, I prefer to remove
"queue_if_no_path" from the internal feature string always. Modified
patch is in the works.

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 4/7] libmultipath: merge_hwe: fix queue_if_no_path logic

[Martin Wilck]

The logic applied here should match the logic in select_features().
If no_path_retry is anything but "undef", queue_if_no_path can be
removed from the feature string, assemble_map() will infer it
correctly.
The case where no_path_retry is undefined and "queue_if_no_path"
is set is treated as if "no_path_retry queue" had been set.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/config.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/libmultipath/config.c b/libmultipath/config.c
index 61bbba91..b928fbe7 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -355,12 +355,24 @@ merge_hwe (struct hwentry * dst, struct hwentry * src)
 
 	/*
 	 * Make sure features is consistent with
-	 * no_path_retry
+	 * no_path_retry.
+	 * The logic should be consistent with select_features().
+	 * The actual queue_if_no_path feature is set in assemble_map().
 	 */
-	if (dst->no_path_retry == NO_PATH_RETRY_FAIL)
+	if (dst->no_path_retry == NO_PATH_RETRY_UNDEF &&
+	    strstr(dst->features, "queue_if_no_path")) {
+		condlog(3, "%s/%s: 'queue_if_no_path' is set, assuming no_path_retry='queue'",
+			dst->vendor, dst->product);
+		dst->no_path_retry = NO_PATH_RETRY_QUEUE;
+	}
+	else if (dst->no_path_retry != NO_PATH_RETRY_UNDEF) {
+		condlog(3, "%s/%s: 'no_path_retry' is set, ignoring 'queue_if_no_path'",
+			dst->vendor, dst->product);
 		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");
+	}
+
+	if (dst->retain_hwhandler != RETAIN_HWHANDLER_UNDEF)
+		remove_feature(&dst->features, "retain_attached_hw_handler");
 
 	return 0;
 }
-- 
2.13.0

Re: [PATCH 4/7] libmultipath: merge_hwe: fix queue_if_no_path logic

[Benjamin Marzinski]

On Wed, Jun 14, 2017 at 12:55:51AM +0200, Martin Wilck wrote:
> The logic applied here should match the logic in select_features().
> If no_path_retry is anything but "undef", queue_if_no_path can be
> removed from the feature string, assemble_map() will infer it
> correctly.
> The case where no_path_retry is undefined and "queue_if_no_path"
> is set is treated as if "no_path_retry queue" had been set.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/config.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/libmultipath/config.c b/libmultipath/config.c
> index 61bbba91..b928fbe7 100644
> --- a/libmultipath/config.c
> +++ b/libmultipath/config.c
> @@ -355,12 +355,24 @@ merge_hwe (struct hwentry * dst, struct hwentry * src)
>  
>  	/*
>  	 * Make sure features is consistent with
> -	 * no_path_retry
> +	 * no_path_retry.
> +	 * The logic should be consistent with select_features().
> +	 * The actual queue_if_no_path feature is set in assemble_map().
>  	 */
> -	if (dst->no_path_retry == NO_PATH_RETRY_FAIL)
> +	if (dst->no_path_retry == NO_PATH_RETRY_UNDEF &&
> +	    strstr(dst->features, "queue_if_no_path")) {
> +		condlog(3, "%s/%s: 'queue_if_no_path' is set, assuming no_path_retry='queue'",
> +			dst->vendor, dst->product);
> +		dst->no_path_retry = NO_PATH_RETRY_QUEUE;
> +	}
> +	else if (dst->no_path_retry != NO_PATH_RETRY_UNDEF) {
> +		condlog(3, "%s/%s: 'no_path_retry' is set, ignoring 'queue_if_no_path'",
> +			dst->vendor, dst->product);
>  		remove_feature(&dst->features, "queue_if_no_path");

This has the same nit as [PATCH 3/7], and it's just as unimportant to
fix here as there.

-Ben

> -	else if (dst->no_path_retry != NO_PATH_RETRY_UNDEF)
> -		add_feature(&dst->features, "queue_if_no_path");
> +	}
> +
> +	if (dst->retain_hwhandler != RETAIN_HWHANDLER_UNDEF)
> +		remove_feature(&dst->features, "retain_attached_hw_handler");
>  
>  	return 0;
>  }
> -- 
> 2.13.0

[PATCH 5/7] 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>
---
 libmultipath/dmparser.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
index 469e60d2..8d0c7af1 100644
--- a/libmultipath/dmparser.c
+++ b/libmultipath/dmparser.c
@@ -74,13 +74,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.0

[PATCH 6/7] 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>
---
 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 f04ff194..e6944faf 100644
--- a/multipath/multipath.conf.5
+++ b/multipath/multipath.conf.5
@@ -365,7 +365,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.0

Re: [PATCH 6/7] multipath.conf.5: document no_path_retry vs. queue_if_no_path

[Benjamin Marzinski]

On Wed, Jun 14, 2017 at 12:55:53AM +0200, Martin Wilck wrote:
> Clarify the documentation about option precedence.
> 
> Signed-off-by: Martin Wilck <mwilck@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 f04ff194..e6944faf 100644
> --- a/multipath/multipath.conf.5
> +++ b/multipath/multipath.conf.5
> @@ -365,7 +365,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. 

Speaking of nits, the above line adds trailing whitespace.

-Ben

>  .TP
>  .I no_partitions
>  Disable automatic partitions generation via kpartx.
> -- 
> 2.13.0

[PATCH 7/7] 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>
---
 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 e6944faf..033e2052 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)
@@ -296,14 +296,19 @@ Generate the path priority based on the regular expression and the
 priority provided as argument. 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
 .
 .
@@ -344,12 +349,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
@@ -364,29 +369,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.0

Re: [PATCH 0/7] no_path_retry/queue_if_no_path logic & logging/man page fixes

[Benjamin Marzinski]

On Wed, Jun 14, 2017 at 12:55:47AM +0200, Martin Wilck 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".

I wholly approve of this change. I do have some minor issue with some of the
individual patches.
 
> The logic is implemented in select_features() (which needs to be called after
> select_no_path_retry()) for multipath groups, and similarly in merge_hwe() for
> hwtable entries. The actual feature setting is made in assemble_map(), I also
> fix a problem there.
> 
> The patch set documents the behavior in the man page, and adds some more
> man page fixes.
> 
> Finally, by skipping superfluous default initializations in load_config(), the
> log messages for the respective config settings become more appropriate.
> 
> Review and comments are highly welcome.
> 
> Regards,
> Martin
> 
> Martin Wilck (7):
>   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/config.c      | 36 ++++++++++++++------------------
>  libmultipath/configure.c   |  4 ++--
>  libmultipath/dmparser.c    |  5 ++---
>  libmultipath/propsel.c     | 39 +++++++++++++++++++++-------------
>  libmultipath/structs.c     | 30 +++++++++++++-------------
>  libmultipath/structs.h     |  4 ++--
>  multipath/multipath.conf.5 | 52 ++++++++++++++++++++++++++--------------------
>  7 files changed, 92 insertions(+), 78 deletions(-)
> 
> -- 
> 2.13.0