[PATCH v2 0/8] multipath-tools: no_path_retry/queue_if_no_path logic

[PATCH v2 0/8] multipath-tools: no_path_retry/queue_if_no_path 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.

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.

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)

Martin Wilck (8):
  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/config.c      | 36 +++++++--------------
 libmultipath/configure.c   |  4 +--
 libmultipath/dict.c        | 11 ++++---
 libmultipath/dmparser.c    |  5 ++-
 libmultipath/propsel.c     | 79 +++++++++++++++++++++++++++++++++++++---------
 libmultipath/propsel.h     |  3 ++
 libmultipath/structs.c     | 30 ++++++++++--------
 libmultipath/structs.h     |  4 +--
 multipath/multipath.conf.5 | 52 ++++++++++++++++--------------
 9 files changed, 136 insertions(+), 88 deletions(-)

-- 
2.13.1

[PATCH v2 1/8] 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>
---
 libmultipath/config.c  | 16 ----------------
 libmultipath/dict.c    | 11 +++++++----
 libmultipath/propsel.c |  5 +++++
 3 files changed, 12 insertions(+), 20 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/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 385063aa..68436433 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -752,6 +752,11 @@ 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.
+	 * Don't print a log message to avoid user confusion.
+	 */
 	return 0;
 out:
 	condlog(3, "%s: max_sectors_kb = %i %s", mp->alias, mp->max_sectors_kb,
-- 
2.13.1

[PATCH v2 2/8] 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.1

[PATCH v2 3/8] 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 |  4 +--
 libmultipath/propsel.c   | 68 +++++++++++++++++++++++++++++++++++++-----------
 libmultipath/propsel.h   |  3 +++
 3 files changed, 58 insertions(+), 17 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 68436433..ccd067f5 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, 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);
-	}
+	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 (inherited setting)",
-			mp->alias, buff);
 	else
 		condlog(3, "%s: no_path_retry = undef (setting: multipath internal)",
 			mp->alias);
diff --git a/libmultipath/propsel.h b/libmultipath/propsel.h
index 58a32f3c..f8e96d85 100644
--- a/libmultipath/propsel.h
+++ b/libmultipath/propsel.h
@@ -28,3 +28,6 @@ int select_max_sectors_kb (struct config *conf, struct multipath * mp);
 int select_san_path_err_forget_rate(struct config *conf, struct multipath *mp);
 int select_san_path_err_threshold(struct config *conf, struct multipath *mp);
 int select_san_path_err_recovery_time(struct config *conf, struct multipath *mp);
+void reconcile_features_with_options(const char *id, char **features,
+				     int* no_path_retry,
+				     int *retain_hwhandler);
-- 
2.13.1

[PATCH v2 4/8] 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>
---
 libmultipath/config.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/libmultipath/config.c b/libmultipath/config.c
index 61bbba91..4f1ecee3 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

[PATCH v2 5/8] 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.1

[PATCH v2 6/8] 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..6959ba5c 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.1

[PATCH v2 7/8] 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 6959ba5c..d8856577 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.1

[PATCH v2 8/8] 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     | 6 ++++++
 multipath/multipath.conf.5 | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index ccd067f5..f11052f2 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,9 @@ 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, "
+			"please use '%s yes' instead",
+			id, r_a_h_h, 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 d8856577..bacd5e30 100644
--- a/multipath/multipath.conf.5
+++ b/multipath/multipath.conf.5
@@ -369,7 +369,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 v2 1/8] libmultipath: load_config: skip setting unnecessary defaults

[Hannes Reinecke]

On 06/20/2017 01:46 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>
> ---
>  libmultipath/config.c  | 16 ----------------
>  libmultipath/dict.c    | 11 +++++++----
>  libmultipath/propsel.c |  5 +++++
>  3 files changed, 12 insertions(+), 20 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/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 385063aa..68436433 100644
> --- a/libmultipath/propsel.c
> +++ b/libmultipath/propsel.c
> @@ -752,6 +752,11 @@ 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.
> +	 * Don't print a log message to avoid user confusion.
> +	 */
>  	return 0;
>  out:
>  	condlog(3, "%s: max_sectors_kb = %i %s", mp->alias, mp->max_sectors_kb,
> 
Errm. You just set the default value, no?
Care to clarify this comment?

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 v2 2/8] libmultipath: add/remove_feature: use const char* for feature

[Hannes Reinecke]

On 06/20/2017 01:46 PM, Martin Wilck wrote:
> 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];
>  
> 
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

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

[Benjamin Marzinski]

On Tue, Jun 20, 2017 at 04:36:47PM +0200, Hannes Reinecke wrote:
> On 06/20/2017 01:46 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>
> > ---
> >  libmultipath/config.c  | 16 ----------------
> >  libmultipath/dict.c    | 11 +++++++----
> >  libmultipath/propsel.c |  5 +++++
> >  3 files changed, 12 insertions(+), 20 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/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 385063aa..68436433 100644
> > --- a/libmultipath/propsel.c
> > +++ b/libmultipath/propsel.c
> > @@ -752,6 +752,11 @@ 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.
> > +	 * Don't print a log message to avoid user confusion.
> > +	 */
> >  	return 0;
> >  out:
> >  	condlog(3, "%s: max_sectors_kb = %i %s", mp->alias, mp->max_sectors_kb,
> > 
> Errm. You just set the default value, no?
> Care to clarify this comment?

The default value is "undefined". This isn't strictly necessary if all
the other code is correct, but in case mpp->max_sectors_kb was
previously set to something (or never zeroed out initially), this will
correctly set it back to undefined.

-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)

[PATCH] Clarify commit message in select_max_sectors_kb()

[Martin Wilck]

Do you like it better this way?
---
 libmultipath/propsel.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index f11052f2..6d628052 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -798,8 +798,9 @@ int select_max_sectors_kb(struct config *conf, struct multipath * mp)
 	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.
-	 * Don't print a log message to avoid user confusion.
+	 * 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:
-- 
2.13.1

Re: [PATCH v2 0/8] multipath-tools: no_path_retry/queue_if_no_path logic

[Benjamin Marzinski]

On Tue, Jun 20, 2017 at 01:46:06PM +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".
> 
> 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.
> 
> 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.

ACK for the set

-Ben

> 
> 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)
> 
> Martin Wilck (8):
>   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/config.c      | 36 +++++++--------------
>  libmultipath/configure.c   |  4 +--
>  libmultipath/dict.c        | 11 ++++---
>  libmultipath/dmparser.c    |  5 ++-
>  libmultipath/propsel.c     | 79 +++++++++++++++++++++++++++++++++++++---------
>  libmultipath/propsel.h     |  3 ++
>  libmultipath/structs.c     | 30 ++++++++++--------
>  libmultipath/structs.h     |  4 +--
>  multipath/multipath.conf.5 | 52 ++++++++++++++++--------------
>  9 files changed, 136 insertions(+), 88 deletions(-)
> 
> -- 
> 2.13.1

Re: [PATCH v2 0/8] multipath-tools: no_path_retry/queue_if_no_path logic

[Martin Wilck]

Hello Ben,

On Tue, 2017-06-20 at 13:44 -0500, Benjamin Marzinski wrote:
> 
> > Review and comments are highly welcome.
> 
> ACK for the set

Thanks a lot. I'd be grateful if you could also take a look at my
previous submissions which have seen no review yet:

"libmpathpersist: use extern struct udev from main program"
"libmultipath: lazy device-mapper initialization"
"mpathpersist.8: add missing documentation for -K, -C, -l"

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 v2 0/8] multipath-tools: no_path_retry/queue_if_no_path logic

[Martin Wilck]

On Tue, 2017-06-20 at 13:44 -0500, Benjamin Marzinski wrote:
> Review and comments are highly welcome.
> 
> ACK for the set

I found another issue with the patch set, will send v3 shortly.

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