[dm-devel] [PATCH V2 00/11] multipath: Add a group_by_tgp pgpolicy

[dm-devel] [PATCH V2 00/11] multipath: Add a group_by_tgp pgpolicy

[Benjamin Marzinski]

This patchset is a combination of my previous two patchsets

[PATCH 0/5] multipath: Add a group_by_tgp path grouping policy
[PATCH 0/5] priority and pathgroup switching changes

The first part adds a new path grouping policy that can be used with
ALUA devices. The goal is to avoid the temporary incorrect path
groupings that can happen when paths change priorities.

The second part is changes that mostly effect how multipathd handles
switching or reordering pathgroups for devices where group_by_prio isn't
set.

Differences from V1 (from changes suggested by Martin Wilck):
[05/11]: make detect_pgpolicy set group_by_prio instead of group_by_tpg
[06/11]: New patch. Add detect_pgpolicy_use_tpg to pick group_by_tpg
         instead of group_by_prio
[09/11]: Large rewrite of my "multipathd: refresh all priorities if one
         has changed" patch based on discussions with Martin
[10/11]: This just moves path_groups_in_order() to multiapthd, and adds
         a comment explaining why it works different from
         select_path_group().

Benjamin Marzinski (11):
  libmultipath: add group_by_tpg path_grouping_policy
  libmultipath: don't copy pgpolicy string in get_pgpolicy_name
  libmultipath: add ALUA tpg path wildcard
  multipath-tools tests: add tests for group_by_tpg policy
  libmultipath: add "detect_pgpolicy" config option
  libmultipath: add "detect_pgpolicy_use_tpg" config option
  libmultipath: don't count PRIO_UNDEF paths for pathgroup priority
  multipath-tools tests: add tests to verify PRIO_UDEF changes
  multipathd: only refresh priorities in update_prio()
  multipathd: reload map if the path groups are out of order
  multipathd: don't assume mpp->paths will exist in
    need_switch_pathgroup

 libmultipath/config.c             |   4 +
 libmultipath/config.h             |   4 +
 libmultipath/configure.c          |   2 +
 libmultipath/defaults.h           |   2 +
 libmultipath/dict.c               |  28 +++-
 libmultipath/discovery.c          |   1 +
 libmultipath/hwtable.c            |   2 +
 libmultipath/libmultipath.version |  10 +-
 libmultipath/pgpolicies.c         |  42 +++---
 libmultipath/pgpolicies.h         |   6 +-
 libmultipath/print.c              |   9 ++
 libmultipath/prioritizers/alua.c  |   1 +
 libmultipath/propsel.c            |  69 ++++++++-
 libmultipath/propsel.h            |   2 +
 libmultipath/structs.c            |   1 +
 libmultipath/structs.h            |  17 +++
 libmultipath/switchgroup.c        |  12 +-
 multipath/main.c                  |   1 +
 multipath/multipath.conf.5        |  29 ++++
 multipathd/cli_handlers.c         |   8 +-
 multipathd/fpin_handlers.c        |   4 +-
 multipathd/main.c                 | 174 +++++++++++----------
 multipathd/main.h                 |   3 +-
 tests/pgpolicy.c                  | 243 ++++++++++++++++++++++++++++++
 24 files changed, 547 insertions(+), 127 deletions(-)

-- 
2.17.2

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

[dm-devel] [PATCH V2 01/11] libmultipath: add group_by_tpg path_grouping_policy

[Benjamin Marzinski]

When we group paths by prio and the priority changes, paths can end up
temporarily in the wrong path groups.  This usually happens when some
paths are down, so their priority can't be updated. To avoid this for
ALUA paths, group them by their target port group instead. The path
groups chosen by this policy won't always match with those chosen by
group_by_prio, since it is possible for multiple ALUA target port
groups to have the same priority.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/discovery.c         |  1 +
 libmultipath/pgpolicies.c        | 19 +++++++++++++++++++
 libmultipath/pgpolicies.h        |  4 +++-
 libmultipath/prioritizers/alua.c |  1 +
 libmultipath/propsel.c           | 27 +++++++++++++++++++++++++--
 libmultipath/structs.c           |  1 +
 libmultipath/structs.h           |  3 +++
 multipath/main.c                 |  1 +
 multipath/multipath.conf.5       |  4 ++++
 9 files changed, 58 insertions(+), 3 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 6865cd92..2dcafe5d 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1051,6 +1051,7 @@ detect_alua(struct path * pp)
 		return;
 	}
 	pp->tpgs = tpgs;
+	pp->tpg_id = ret;
 }
 
 int path_get_tpgs(struct path *pp)
diff --git a/libmultipath/pgpolicies.c b/libmultipath/pgpolicies.c
index 10b44d37..e14da8cc 100644
--- a/libmultipath/pgpolicies.c
+++ b/libmultipath/pgpolicies.c
@@ -25,6 +25,8 @@ int get_pgpolicy_id(char * str)
 		return GROUP_BY_PRIO;
 	if (0 == strncmp(str, "group_by_node_name", 18))
 		return GROUP_BY_NODE_NAME;
+	if (0 == strncmp(str, "group_by_tpg", 12))
+		return GROUP_BY_TPG;
 
 	return IOPOLICY_UNDEF;
 }
@@ -49,6 +51,9 @@ int get_pgpolicy_name(char * buff, int len, int id)
 	case GROUP_BY_NODE_NAME:
 		s = "group_by_node_name";
 		break;
+	case GROUP_BY_TPG:
+		s = "group_by_tpg";
+		break;
 	default:
 		s = "undefined";
 		break;
@@ -191,6 +196,12 @@ prios_match(struct path *pp1, struct path *pp2)
 	return (pp1->priority == pp2->priority);
 }
 
+bool
+tpg_match(struct path *pp1, struct path *pp2)
+{
+	return (pp1->tpg_id == pp2->tpg_id);
+}
+
 int group_by_match(struct multipath * mp, vector paths,
 		   bool (*path_match_fn)(struct path *, struct path *))
 {
@@ -279,6 +290,14 @@ int group_by_prio(struct multipath *mp, vector paths)
 	return group_by_match(mp, paths, prios_match);
 }
 
+/*
+ * One path group per alua target port group present in the path vector
+ */
+int group_by_tpg(struct multipath *mp, vector paths)
+{
+	return group_by_match(mp, paths, tpg_match);
+}
+
 int one_path_per_group(struct multipath *mp, vector paths)
 {
 	int i;
diff --git a/libmultipath/pgpolicies.h b/libmultipath/pgpolicies.h
index 15927610..d3ab2f35 100644
--- a/libmultipath/pgpolicies.h
+++ b/libmultipath/pgpolicies.h
@@ -16,7 +16,8 @@ enum iopolicies {
 	MULTIBUS,
 	GROUP_BY_SERIAL,
 	GROUP_BY_PRIO,
-	GROUP_BY_NODE_NAME
+	GROUP_BY_NODE_NAME,
+	GROUP_BY_TPG,
 };
 
 int get_pgpolicy_id(char *);
@@ -30,5 +31,6 @@ int one_group(struct multipath *, vector);
 int group_by_serial(struct multipath *, vector);
 int group_by_prio(struct multipath *, vector);
 int group_by_node_name(struct multipath *, vector);
+int group_by_tpg(struct multipath *, vector);
 
 #endif
diff --git a/libmultipath/prioritizers/alua.c b/libmultipath/prioritizers/alua.c
index 0ab06e2b..4888a974 100644
--- a/libmultipath/prioritizers/alua.c
+++ b/libmultipath/prioritizers/alua.c
@@ -65,6 +65,7 @@ get_alua_info(struct path * pp, unsigned int timeout)
 			return -ALUA_PRIO_NOT_SUPPORTED;
 		return -ALUA_PRIO_RTPG_FAILED;
 	}
+	pp->tpg_id = tpg;
 	condlog(3, "%s: reported target port group is %i", pp->dev, tpg);
 	rc = get_asymmetric_access_state(pp, tpg, timeout);
 	if (rc < 0) {
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index a25cc921..841fa247 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -35,7 +35,8 @@ pgpolicyfn *pgpolicies[] = {
 	one_group,
 	group_by_serial,
 	group_by_prio,
-	group_by_node_name
+	group_by_node_name,
+	group_by_tpg,
 };
 
 #define do_set(var, src, dest, msg)					\
@@ -249,10 +250,26 @@ out:
 	return 0;
 }
 
+static bool
+verify_alua_prio(struct multipath *mp)
+{
+	int i;
+	struct path *pp;
+
+	vector_foreach_slot(mp->paths, pp, i) {
+		const char *name = prio_name(&pp->prio);
+		if (strncmp(name, PRIO_ALUA, PRIO_NAME_LEN) &&
+		    strncmp(name, PRIO_SYSFS, PRIO_NAME_LEN))
+			 return false;
+	}
+	return true;
+}
+
 int select_pgpolicy(struct config *conf, struct multipath * mp)
 {
 	const char *origin;
 	char buff[POLICY_NAME_SIZE];
+	int log_prio = 3;
 
 	if (conf->pgpolicy_flag > 0) {
 		mp->pgpolicy = conf->pgpolicy_flag;
@@ -265,9 +282,15 @@ int select_pgpolicy(struct config *conf, struct multipath * mp)
 	mp_set_conf(pgpolicy);
 	mp_set_default(pgpolicy, DEFAULT_PGPOLICY);
 out:
+	if (mp->pgpolicy == GROUP_BY_TPG && !verify_alua_prio(mp)) {
+		mp->pgpolicy = DEFAULT_PGPOLICY;
+		origin = "(setting: emergency fallback - not all paths use alua prio)";
+		log_prio = 1;
+	}
 	mp->pgpolicyfn = pgpolicies[mp->pgpolicy];
 	get_pgpolicy_name(buff, POLICY_NAME_SIZE, mp->pgpolicy);
-	condlog(3, "%s: path_grouping_policy = %s %s", mp->alias, buff, origin);
+	condlog(log_prio, "%s: path_grouping_policy = %s %s", mp->alias, buff,
+		origin);
 	return 0;
 }
 
diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index 87e84d5d..39504dca 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -125,6 +125,7 @@ alloc_path (void)
 		pp->sg_id.proto_id = PROTOCOL_UNSET;
 		pp->fd = -1;
 		pp->tpgs = TPGS_UNDEF;
+		pp->tpg_id = GROUP_ID_UNDEF;
 		pp->priority = PRIO_UNDEF;
 		pp->checkint = CHECKINT_UNDEF;
 		checker_clear(&pp->checker);
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index a1208751..0eea19b4 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -317,6 +317,8 @@ struct hd_geometry {
 };
 #endif
 
+#define GROUP_ID_UNDEF -1
+
 struct path {
 	char dev[FILE_NAME_SIZE];
 	char dev_t[BLK_DEV_SIZE];
@@ -372,6 +374,7 @@ struct path {
 	/* configlet pointers */
 	vector hwe;
 	struct gen_path generic_path;
+	int tpg_id;
 };
 
 typedef int (pgpolicyfn) (struct multipath *, vector);
diff --git a/multipath/main.c b/multipath/main.c
index 90f940f1..b78f3162 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -157,6 +157,7 @@ usage (char * progname)
 		"          . group_by_serial     one priority group per serial\n"
 		"          . group_by_prio       one priority group per priority lvl\n"
 		"          . group_by_node_name  one priority group per target node\n"
+		"          . group_by_tpg        one priority group per ALUA target port group\n"
 		"  -v lvl  verbosity level:\n"
 		"          . 0 no output\n"
 		"          . 1 print created devmap names only\n"
diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
index b4dccd1b..b65a543d 100644
--- a/multipath/multipath.conf.5
+++ b/multipath/multipath.conf.5
@@ -233,6 +233,10 @@ per-multipath option in the configuration file.
 One priority group per target node name. Target node names are fetched
 in \fI/sys/class/fc_transport/target*/node_name\fR.
 .TP
+.I group_by_tpg
+One priority group per ALUA target port group. In order to use this policy,
+all paths in the multipath device must have \fIprio\fR set to \fBalua\fR.
+.TP
 The default is: \fBfailover\fR
 .RE
 .
-- 
2.17.2

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

[dm-devel] [PATCH V2 02/11] libmultipath: don't copy pgpolicy string in get_pgpolicy_name

[Benjamin Marzinski]

copying the value into a passed in buffer doesn't help any of the
callers of this function. It's just wasted work.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/dict.c       |  6 +-----
 libmultipath/pgpolicies.c | 27 ++++++++-------------------
 libmultipath/pgpolicies.h |  2 +-
 libmultipath/propsel.c    |  6 ++----
 4 files changed, 12 insertions(+), 29 deletions(-)

diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index 2e9b45f9..dddd3cd6 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -1209,14 +1209,10 @@ set_pgpolicy(vector strvec, void *ptr, const char *file, int line_nr)
 int
 print_pgpolicy(struct strbuf *buff, long pgpolicy)
 {
-	char str[POLICY_NAME_SIZE];
-
 	if (!pgpolicy)
 		return 0;
 
-	get_pgpolicy_name(str, POLICY_NAME_SIZE, pgpolicy);
-
-	return append_strbuf_quoted(buff, str);
+	return append_strbuf_quoted(buff, get_pgpolicy_name(pgpolicy));
 }
 
 declare_def_handler(pgpolicy, set_pgpolicy)
diff --git a/libmultipath/pgpolicies.c b/libmultipath/pgpolicies.c
index e14da8cc..edc3c611 100644
--- a/libmultipath/pgpolicies.c
+++ b/libmultipath/pgpolicies.c
@@ -31,34 +31,23 @@ int get_pgpolicy_id(char * str)
 	return IOPOLICY_UNDEF;
 }
 
-int get_pgpolicy_name(char * buff, int len, int id)
+const char *get_pgpolicy_name(int id)
 {
-	char * s;
-
 	switch (id) {
 	case FAILOVER:
-		s = "failover";
-		break;
+		return "failover";
 	case MULTIBUS:
-		s = "multibus";
-		break;
+		return "multibus";
 	case GROUP_BY_SERIAL:
-		s = "group_by_serial";
-		break;
+		return "group_by_serial";
 	case GROUP_BY_PRIO:
-		s = "group_by_prio";
-		break;
+		return "group_by_prio";
 	case GROUP_BY_NODE_NAME:
-		s = "group_by_node_name";
-		break;
+		return "group_by_node_name";
 	case GROUP_BY_TPG:
-		s = "group_by_tpg";
-		break;
-	default:
-		s = "undefined";
-		break;
+		return "group_by_tpg";
 	}
-	return snprintf(buff, len, "%s", s);
+	return "undefined"; /* IOPOLICY_UNDEF */
 }
 
 
diff --git a/libmultipath/pgpolicies.h b/libmultipath/pgpolicies.h
index d3ab2f35..9e4ddda2 100644
--- a/libmultipath/pgpolicies.h
+++ b/libmultipath/pgpolicies.h
@@ -21,7 +21,7 @@ enum iopolicies {
 };
 
 int get_pgpolicy_id(char *);
-int get_pgpolicy_name (char *, int, int);
+const char *get_pgpolicy_name (int);
 int group_paths(struct multipath *, int);
 /*
  * policies
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index 841fa247..d214281b 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -268,7 +268,6 @@ verify_alua_prio(struct multipath *mp)
 int select_pgpolicy(struct config *conf, struct multipath * mp)
 {
 	const char *origin;
-	char buff[POLICY_NAME_SIZE];
 	int log_prio = 3;
 
 	if (conf->pgpolicy_flag > 0) {
@@ -288,9 +287,8 @@ out:
 		log_prio = 1;
 	}
 	mp->pgpolicyfn = pgpolicies[mp->pgpolicy];
-	get_pgpolicy_name(buff, POLICY_NAME_SIZE, mp->pgpolicy);
-	condlog(log_prio, "%s: path_grouping_policy = %s %s", mp->alias, buff,
-		origin);
+	condlog(log_prio, "%s: path_grouping_policy = %s %s", mp->alias,
+		get_pgpolicy_name(mp->pgpolicy), origin);
 	return 0;
 }
 
-- 
2.17.2

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

[dm-devel] [PATCH V2 03/11] libmultipath: add ALUA tpg path wildcard

[Benjamin Marzinski]

Make it possible to easily check a path's target port group.

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

diff --git a/libmultipath/print.c b/libmultipath/print.c
index 3193dbe0..360308d2 100644
--- a/libmultipath/print.c
+++ b/libmultipath/print.c
@@ -782,6 +782,14 @@ snprint_path_vpd_data(struct strbuf *buff, const struct path * pp)
 	return append_strbuf_str(buff, "[undef]");
 }
 
+static int
+snprint_alua_tpg(struct strbuf *buff, const struct path * pp)
+{
+	if (pp->tpg_id < 0)
+		return append_strbuf_str(buff, "[undef]");
+	return print_strbuf(buff, "0x%04x", pp->tpg_id);
+}
+
 static const struct multipath_data mpd[] = {
 	{'n', "name",          snprint_name},
 	{'w', "uuid",          snprint_multipath_uuid},
@@ -836,6 +844,7 @@ static const struct path_data pd[] = {
 	{'P', "protocol",      snprint_path_protocol},
 	{'I', "init_st",       snprint_initialized},
 	{'L', "LUN hex",       snprint_path_lunhex},
+	{'A', "TPG",           snprint_alua_tpg},
 };
 
 static const struct pathgroup_data pgd[] = {
-- 
2.17.2

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

[dm-devel] [PATCH V2 04/11] multipath-tools tests: add tests for group_by_tpg policy

[Benjamin Marzinski]

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 tests/pgpolicy.c | 201 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 201 insertions(+)

diff --git a/tests/pgpolicy.c b/tests/pgpolicy.c
index 43be831f..85fa30ce 100644
--- a/tests/pgpolicy.c
+++ b/tests/pgpolicy.c
@@ -32,6 +32,15 @@ struct multipath mp8, mp4, mp1, mp0, mp_null;
 struct path p8[8], p4[4], p1[1];
 
 
+static void set_tpg(struct path *pp, int *tpg, int size)
+{
+	int i;
+
+	for (i = 0; i < size; i++) {
+		pp[i].tpg_id = tpg[i];
+	}
+}
+
 static void set_priority(struct path *pp, int *prio, int size)
 {
 	int i;
@@ -639,6 +648,187 @@ static void test_group_by_prio_mixed_one_marginal8(void **state)
 	verify_pathgroups(&mp8, p8, groups, group_size, group_marginal, 7);
 }
 
+static void test_group_by_tpg_same8(void **state)
+{
+	int paths[] = {0,1,2,3,4,5,6,7};
+	int *groups[] = {paths};
+	int group_size[] = {8};
+
+	mp8.pgpolicyfn = group_by_tpg;
+	assert_int_equal(group_paths(&mp8, 0), 0);
+	verify_pathgroups(&mp8, p8, groups, group_size, NULL, 1);
+}
+
+static void test_group_by_tpg_different8(void **state)
+{
+	int prio[] = {1,2,3,4,5,6,7,8};
+	int tpg[] = {3,5,4,1,8,6,7,2};
+	int paths[] = {7,6,5,4,3,2,1,0};
+	int *groups[] = {&paths[0], &paths[1], &paths[2], &paths[3],
+			  &paths[4], &paths[5], &paths[6], &paths[7]};
+	int group_size[] = {1,1,1,1,1,1,1,1};
+
+	set_priority(p8, prio, 8);
+	set_tpg(p8, tpg, 8);
+	mp8.pgpolicyfn = group_by_tpg;
+	assert_int_equal(group_paths(&mp8, 0), 0);
+	verify_pathgroups(&mp8, p8, groups, group_size, NULL, 8);
+}
+
+static void test_group_by_tpg_mixed8(void **state)
+{
+	int prio[] = {7,2,3,3,5,2,8,2};
+	int tpg[] = {1,2,3,3,4,2,5,6};
+	int group0[] = {6};
+	int group1[] = {0};
+	int group2[] = {4};
+	int group3[] = {2,3};
+	int group4[] = {1,5};
+	int group5[] = {7};
+	int *groups[] = {group0, group1, group2, group3,
+			  group4, group5};
+	int group_size[] = {1,1,1,2,2,1};
+
+	set_priority(p8, prio, 8);
+	set_tpg(p8, tpg, 8);
+	mp8.pgpolicyfn = group_by_tpg;
+	assert_int_equal(group_paths(&mp8, 0), 0);
+	verify_pathgroups(&mp8, p8, groups, group_size, NULL, 6);
+}
+
+static void test_group_by_tpg_3_groups8(void **state)
+{
+	int prio[] = {1,2,2,1,2,1,1,2};
+	int tpg[] = {1,2,2,1,3,1,1,3};
+	int group0[] = {1,2};
+	int group1[] = {4,7};
+	int group2[] = {0,3,5,6};
+	int *groups[] = {group0, group1, group2};
+	int group_size[] = {2,2,4};
+
+	set_priority(p8, prio, 8);
+	set_tpg(p8, tpg, 8);
+	mp8.pgpolicyfn = group_by_tpg;
+	assert_int_equal(group_paths(&mp8, 0), 0);
+	verify_pathgroups(&mp8, p8, groups, group_size, NULL, 3);
+}
+
+static void test_group_by_tpg_2_groups4(void **state)
+{
+	int prio[] = {2,1,1,2};
+	int tpg[] = {1,2,2,1};
+	int group0[] = {0,3};
+	int group1[] = {1,2};
+	int *groups[] = {group0, group1};
+	int group_size[] = {2,2};
+
+	set_priority(p4, prio, 4);
+	set_tpg(p4, tpg, 4);
+	mp4.pgpolicyfn = group_by_tpg;
+	assert_int_equal(group_paths(&mp4, 0), 0);
+	verify_pathgroups(&mp4, p4, groups, group_size, NULL, 2);
+}
+
+static void test_group_by_tpg1(void **state)
+{
+	int paths[] = {0};
+	int *groups[] = {paths};
+	int group_size[] = {1};
+
+	mp1.pgpolicyfn = group_by_tpg;
+	assert_int_equal(group_paths(&mp1, 0), 0);
+	verify_pathgroups(&mp1, p1, groups, group_size, NULL, 1);
+}
+
+static void test_group_by_tpg0(void **state)
+{
+	mp0.pgpolicyfn = group_by_tpg;
+	assert_int_equal(group_paths(&mp0, 0), 0);
+	verify_pathgroups(&mp0, NULL, NULL, NULL, NULL, 0);
+}
+
+static void test_group_by_tpg_null(void **state)
+{
+	mp_null.pgpolicyfn = group_by_tpg;
+	assert_int_equal(group_paths(&mp_null, 0), 0);
+	verify_pathgroups(&mp_null, NULL, NULL, NULL, NULL, 0);
+}
+
+static void test_group_by_tpg_mixed_all_marginal8(void **state)
+{
+	int prio[] = {7,2,3,3,5,2,8,2};
+	int tpg[] = {1,2,3,3,4,2,5,6};
+	int marginal[] = {1,1,1,1,1,1,1,1};
+	int group0[] = {6};
+	int group1[] = {0};
+	int group2[] = {4};
+	int group3[] = {2,3};
+	int group4[] = {1,5};
+	int group5[] = {7};
+	int *groups[] = {group0, group1, group2, group3,
+			  group4, group5};
+	int group_size[] = {1,1,1,2,2,1};
+	int group_marginal[] = {1,1,1,1,1,1};
+
+	set_priority(p8, prio, 8);
+	set_tpg(p8, tpg, 8);
+	set_marginal(p8, marginal, 8);
+	mp8.pgpolicyfn = group_by_tpg;
+	assert_int_equal(group_paths(&mp8, 1), 0);
+	verify_pathgroups(&mp8, p8, groups, group_size, group_marginal, 6);
+}
+
+static void test_group_by_tpg_mixed_half_marginal8(void **state)
+{
+	int prio[] = {7,1,3,3,3,2,8,2};
+	int tpg[] = {1,2,3,4,5,6,7,6};
+	int marginal[] = {0,0,0,1,0,1,1,1};
+	int group0[] = {0};
+	int group1[] = {2};
+	int group2[] = {4};
+	int group3[] = {1};
+	int group4[] = {6};
+	int group5[] = {3};
+	int group6[] = {5,7};
+	int *groups[] = {group0, group1, group2, group3,
+			  group4, group5, group6};
+	int group_size[] = {1,1,1,1,1,1,2};
+	int group_marginal[] = {0,0,0,0,1,1,1};
+
+	set_priority(p8, prio, 8);
+	set_tpg(p8, tpg, 8);
+	set_marginal(p8, marginal, 8);
+	mp8.pgpolicyfn = group_by_tpg;
+	assert_int_equal(group_paths(&mp8, 1), 0);
+	verify_pathgroups(&mp8, p8, groups, group_size, group_marginal, 7);
+}
+
+static void test_group_by_tpg_mixed_one_marginal8(void **state)
+{
+	int prio[] = {7,1,3,3,5,2,8,2};
+	int tpg[]  = {1,2,3,3,4,5,6,5};
+	int marginal[] = {0,0,0,0,0,1,0,0};
+	int group0[] = {6};
+	int group1[] = {0};
+	int group2[] = {4};
+	int group3[] = {2,3};
+	int group4[] = {7};
+	int group5[] = {1};
+	int group6[] = {5};
+	int *groups[] = {group0, group1, group2, group3,
+			  group4, group5, group6};
+	int group_size[] = {1,1,1,2,1,1,1};
+	int group_marginal[] = {0,0,0,0,0,0,1};
+
+	set_priority(p8, prio, 8);
+	set_tpg(p8, tpg, 8);
+	set_marginal(p8, marginal, 8);
+	mp8.pgpolicyfn = group_by_tpg;
+	assert_int_equal(group_paths(&mp8, 1), 0);
+	verify_pathgroups(&mp8, p8, groups, group_size, group_marginal, 7);
+}
+
+
 static void test_group_by_node_name_same8(void **state)
 {
 	char *node_name[] = {"a","a","a","a","a","a","a","a"};
@@ -1002,6 +1192,17 @@ int test_pgpolicies(void)
 		setup_test(test_group_by_prio_mixed_all_marginal, 8),
 		setup_test(test_group_by_prio_mixed_half_marginal, 8),
 		setup_test(test_group_by_prio_mixed_one_marginal, 8),
+		setup_test(test_group_by_tpg_same, 8),
+		setup_test(test_group_by_tpg_different, 8),
+		setup_test(test_group_by_tpg_mixed, 8),
+		setup_test(test_group_by_tpg_3_groups, 8),
+		setup_test(test_group_by_tpg_2_groups, 4),
+		setup_test(test_group_by_tpg, 1),
+		setup_test(test_group_by_tpg, 0),
+		setup_test(test_group_by_tpg, _null),
+		setup_test(test_group_by_tpg_mixed_all_marginal, 8),
+		setup_test(test_group_by_tpg_mixed_half_marginal, 8),
+		setup_test(test_group_by_tpg_mixed_one_marginal, 8),
 		setup_test(test_group_by_node_name_same, 8),
 		setup_test(test_group_by_node_name_increasing, 8),
 		setup_test(test_group_by_node_name_3_groups, 8),
-- 
2.17.2

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

[dm-devel] [PATCH V2 05/11] libmultipath: add "detect_pgpolicy" config option

[Benjamin Marzinski]

This allows configuations to use "group_by_prio" if alua is autodetected
and another policy if it isn't, so they can work with detect_prio.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/config.c      |  2 ++
 libmultipath/config.h      |  2 ++
 libmultipath/configure.c   |  1 +
 libmultipath/defaults.h    |  1 +
 libmultipath/dict.c        | 11 +++++++++++
 libmultipath/hwtable.c     |  1 +
 libmultipath/propsel.c     | 20 ++++++++++++++++++++
 libmultipath/propsel.h     |  1 +
 libmultipath/structs.h     |  7 +++++++
 multipath/multipath.conf.5 | 12 ++++++++++++
 10 files changed, 58 insertions(+)

diff --git a/libmultipath/config.c b/libmultipath/config.c
index 5c5c0726..2e742373 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -452,6 +452,7 @@ merge_hwe (struct hwentry * dst, struct hwentry * src)
 	merge_num(retain_hwhandler);
 	merge_num(detect_prio);
 	merge_num(detect_checker);
+	merge_num(detect_pgpolicy);
 	merge_num(deferred_remove);
 	merge_num(delay_watch_checks);
 	merge_num(delay_wait_checks);
@@ -617,6 +618,7 @@ store_hwe (vector hwtable, struct hwentry * dhwe)
 	hwe->retain_hwhandler = dhwe->retain_hwhandler;
 	hwe->detect_prio = dhwe->detect_prio;
 	hwe->detect_checker = dhwe->detect_checker;
+	hwe->detect_pgpolicy = dhwe->detect_pgpolicy;
 	hwe->ghost_delay = dhwe->ghost_delay;
 	hwe->vpd_vendor_id = dhwe->vpd_vendor_id;
 
diff --git a/libmultipath/config.h b/libmultipath/config.h
index 87947469..014c6849 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -76,6 +76,7 @@ struct hwentry {
 	int retain_hwhandler;
 	int detect_prio;
 	int detect_checker;
+	int detect_pgpolicy;
 	int deferred_remove;
 	int delay_watch_checks;
 	int delay_wait_checks;
@@ -171,6 +172,7 @@ struct config {
 	int retain_hwhandler;
 	int detect_prio;
 	int detect_checker;
+	int detect_pgpolicy;
 	int force_sync;
 	int deferred_remove;
 	int processed_main_config;
diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 4a1c28bb..366b166f 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -304,6 +304,7 @@ int setup_map(struct multipath *mpp, char **params, struct vectors *vecs)
 	pthread_cleanup_push(put_multipath_config, conf);
 
 	select_pgfailback(conf, mpp);
+	select_detect_pgpolicy(conf, mpp);
 	select_pgpolicy(conf, mpp);
 
 	/*
diff --git a/libmultipath/defaults.h b/libmultipath/defaults.h
index a5e9ea0c..8f2d2f05 100644
--- a/libmultipath/defaults.h
+++ b/libmultipath/defaults.h
@@ -29,6 +29,7 @@
 #define DEFAULT_RETAIN_HWHANDLER RETAIN_HWHANDLER_ON
 #define DEFAULT_DETECT_PRIO	DETECT_PRIO_ON
 #define DEFAULT_DETECT_CHECKER	DETECT_CHECKER_ON
+#define DEFAULT_DETECT_PGPOLICY	DETECT_PGPOLICY_ON
 #define DEFAULT_DEFERRED_REMOVE	DEFERRED_REMOVE_OFF
 #define DEFAULT_DELAY_CHECKS	NU_NO
 #define DEFAULT_ERR_CHECKS	NU_NO
diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index dddd3cd6..edd4923d 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -868,6 +868,14 @@ declare_ovr_snprint(detect_checker, print_yes_no_undef)
 declare_hw_handler(detect_checker, set_yes_no_undef)
 declare_hw_snprint(detect_checker, print_yes_no_undef)
 
+declare_def_handler(detect_pgpolicy, set_yes_no_undef)
+declare_def_snprint_defint(detect_pgpolicy, print_yes_no_undef,
+			   DEFAULT_DETECT_PGPOLICY)
+declare_ovr_handler(detect_pgpolicy, set_yes_no_undef)
+declare_ovr_snprint(detect_pgpolicy, print_yes_no_undef)
+declare_hw_handler(detect_pgpolicy, set_yes_no_undef)
+declare_hw_snprint(detect_pgpolicy, print_yes_no_undef)
+
 declare_def_handler(force_sync, set_yes_no)
 declare_def_snprint(force_sync, print_yes_no)
 
@@ -2112,6 +2120,7 @@ init_keywords(vector keywords)
 	install_keyword("retain_attached_hw_handler", &def_retain_hwhandler_handler, &snprint_def_retain_hwhandler);
 	install_keyword("detect_prio", &def_detect_prio_handler, &snprint_def_detect_prio);
 	install_keyword("detect_checker", &def_detect_checker_handler, &snprint_def_detect_checker);
+	install_keyword("detect_pgpolicy", &def_detect_pgpolicy_handler, &snprint_def_detect_pgpolicy);
 	install_keyword("force_sync", &def_force_sync_handler, &snprint_def_force_sync);
 	install_keyword("strict_timing", &def_strict_timing_handler, &snprint_def_strict_timing);
 	install_keyword("deferred_remove", &def_deferred_remove_handler, &snprint_def_deferred_remove);
@@ -2202,6 +2211,7 @@ init_keywords(vector keywords)
 	install_keyword("retain_attached_hw_handler", &hw_retain_hwhandler_handler, &snprint_hw_retain_hwhandler);
 	install_keyword("detect_prio", &hw_detect_prio_handler, &snprint_hw_detect_prio);
 	install_keyword("detect_checker", &hw_detect_checker_handler, &snprint_hw_detect_checker);
+	install_keyword("detect_pgpolicy", &hw_detect_pgpolicy_handler, &snprint_hw_detect_pgpolicy);
 	install_keyword("deferred_remove", &hw_deferred_remove_handler, &snprint_hw_deferred_remove);
 	install_keyword("delay_watch_checks", &hw_delay_watch_checks_handler, &snprint_hw_delay_watch_checks);
 	install_keyword("delay_wait_checks", &hw_delay_wait_checks_handler, &snprint_hw_delay_wait_checks);
@@ -2244,6 +2254,7 @@ init_keywords(vector keywords)
 	install_keyword("retain_attached_hw_handler", &ovr_retain_hwhandler_handler, &snprint_ovr_retain_hwhandler);
 	install_keyword("detect_prio", &ovr_detect_prio_handler, &snprint_ovr_detect_prio);
 	install_keyword("detect_checker", &ovr_detect_checker_handler, &snprint_ovr_detect_checker);
+	install_keyword("detect_pgpolicy", &ovr_detect_pgpolicy_handler, &snprint_ovr_detect_pgpolicy);
 	install_keyword("deferred_remove", &ovr_deferred_remove_handler, &snprint_ovr_deferred_remove);
 	install_keyword("delay_watch_checks", &ovr_delay_watch_checks_handler, &snprint_ovr_delay_watch_checks);
 	install_keyword("delay_wait_checks", &ovr_delay_wait_checks_handler, &snprint_ovr_delay_wait_checks);
diff --git a/libmultipath/hwtable.c b/libmultipath/hwtable.c
index c3d93f09..0b3e7c49 100644
--- a/libmultipath/hwtable.c
+++ b/libmultipath/hwtable.c
@@ -67,6 +67,7 @@
 		.retain_hwhandler = RETAIN_HWHANDLER_ON,
 		.detect_prio   = DETECT_PRIO_ON,
 		.detect_checker = DETECT_CHECKER_ON,
+		.detect_pgpolicy = DETECT_PGPOLICY_ON,
 		.deferred_remove = DEFERRED_REMOVE_OFF,
 		.delay_watch_checks = DELAY_CHECKS_OFF,
 		.delay_wait_checks = DELAY_CHECKS_OFF,
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index d214281b..10b84948 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -265,6 +265,21 @@ verify_alua_prio(struct multipath *mp)
 	return true;
 }
 
+int select_detect_pgpolicy(struct config *conf, struct multipath *mp)
+{
+	const char *origin;
+
+	mp_set_ovr(detect_pgpolicy);
+	mp_set_hwe(detect_pgpolicy);
+	mp_set_conf(detect_pgpolicy);
+	mp_set_default(detect_pgpolicy, DEFAULT_DETECT_PGPOLICY);
+out:
+	condlog(3, "%s: detect_pgpolicy = %s %s", mp->alias,
+		(mp->detect_pgpolicy == DETECT_PGPOLICY_ON)? "yes" : "no",
+		 origin);
+	return 0;
+}
+
 int select_pgpolicy(struct config *conf, struct multipath * mp)
 {
 	const char *origin;
@@ -275,6 +290,11 @@ int select_pgpolicy(struct config *conf, struct multipath * mp)
 		origin = cmdline_origin;
 		goto out;
 	}
+	if (mp->detect_pgpolicy == DETECT_PGPOLICY_ON && verify_alua_prio(mp)) {
+		mp->pgpolicy = GROUP_BY_PRIO;
+		origin = autodetect_origin;
+		goto out;
+	}
 	mp_set_mpe(pgpolicy);
 	mp_set_ovr(pgpolicy);
 	mp_set_hwe(pgpolicy);
diff --git a/libmultipath/propsel.h b/libmultipath/propsel.h
index 152ca44c..513ee6ac 100644
--- a/libmultipath/propsel.h
+++ b/libmultipath/propsel.h
@@ -1,5 +1,6 @@
 int select_rr_weight (struct config *conf, struct multipath * mp);
 int select_pgfailback (struct config *conf, struct multipath * mp);
+int select_detect_pgpolicy (struct config *conf, struct multipath * mp);
 int select_pgpolicy (struct config *conf, struct multipath * mp);
 int select_selector (struct config *conf, struct multipath * mp);
 int select_alias (struct config *conf, struct multipath * mp);
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 0eea19b4..682a7e17 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -143,6 +143,12 @@ enum detect_checker_states {
 	DETECT_CHECKER_ON = YNU_YES,
 };
 
+enum detect_pgpolicy_states {
+	DETECT_PGPOLICY_UNDEF = YNU_UNDEF,
+	DETECT_PGPOLICY_OFF = YNU_NO,
+	DETECT_PGPOLICY_ON = YNU_YES,
+};
+
 enum deferred_remove_states {
 	DEFERRED_REMOVE_UNDEF = YNU_UNDEF,
 	DEFERRED_REMOVE_OFF = YNU_NO,
@@ -389,6 +395,7 @@ enum prflag_value {
 struct multipath {
 	char wwid[WWID_SIZE];
 	char alias_old[WWID_SIZE];
+	int detect_pgpolicy;
 	int pgpolicy;
 	pgpolicyfn *pgpolicyfn;
 	int nextpg;
diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
index b65a543d..3b25b5d4 100644
--- a/multipath/multipath.conf.5
+++ b/multipath/multipath.conf.5
@@ -242,6 +242,18 @@ The default is: \fBfailover\fR
 .
 .
 .TP
+.B detect_pgpolicy
+If set to \fIyes\fR and all path devcices are configured with either the
+\fIalua\fR or \fIsysfs\fR prioritizer, the multipath device will automatically
+use the \fIgroup_by_prio\fR path_grouping_policy. If set to \fIno\fR, the
+path_grouping_policy will be selected as usual.
+.RS
+.TP
+The default is: \fIyes\fR
+.RE
+.
+.
+.TP
 .B pg_timeout
 (Deprecated) This option is not supported any more, and the value is ignored.
 .
-- 
2.17.2

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

[dm-devel] [PATCH V2 06/11] libmultipath: add "detect_pgpolicy_use_tpg" config option

[Benjamin Marzinski]

If this and "detect_pgpolicy" are both selected and ALUA is
autodetected, the multipath device will use the "group_by_tpg" policy
instead of the "group_by_prio" policy.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/config.c             |  2 ++
 libmultipath/config.h             |  2 ++
 libmultipath/configure.c          |  1 +
 libmultipath/defaults.h           |  1 +
 libmultipath/dict.c               | 11 +++++++++++
 libmultipath/hwtable.c            |  1 +
 libmultipath/libmultipath.version | 10 +++-------
 libmultipath/propsel.c            | 24 ++++++++++++++++++++++--
 libmultipath/propsel.h            |  1 +
 libmultipath/structs.h            |  7 +++++++
 multipath/multipath.conf.5        | 13 +++++++++++++
 11 files changed, 64 insertions(+), 9 deletions(-)

diff --git a/libmultipath/config.c b/libmultipath/config.c
index 2e742373..7b207590 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -453,6 +453,7 @@ merge_hwe (struct hwentry * dst, struct hwentry * src)
 	merge_num(detect_prio);
 	merge_num(detect_checker);
 	merge_num(detect_pgpolicy);
+	merge_num(detect_pgpolicy_use_tpg);
 	merge_num(deferred_remove);
 	merge_num(delay_watch_checks);
 	merge_num(delay_wait_checks);
@@ -619,6 +620,7 @@ store_hwe (vector hwtable, struct hwentry * dhwe)
 	hwe->detect_prio = dhwe->detect_prio;
 	hwe->detect_checker = dhwe->detect_checker;
 	hwe->detect_pgpolicy = dhwe->detect_pgpolicy;
+	hwe->detect_pgpolicy_use_tpg = dhwe->detect_pgpolicy_use_tpg;
 	hwe->ghost_delay = dhwe->ghost_delay;
 	hwe->vpd_vendor_id = dhwe->vpd_vendor_id;
 
diff --git a/libmultipath/config.h b/libmultipath/config.h
index 014c6849..0a2c297b 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -77,6 +77,7 @@ struct hwentry {
 	int detect_prio;
 	int detect_checker;
 	int detect_pgpolicy;
+	int detect_pgpolicy_use_tpg;
 	int deferred_remove;
 	int delay_watch_checks;
 	int delay_wait_checks;
@@ -173,6 +174,7 @@ struct config {
 	int detect_prio;
 	int detect_checker;
 	int detect_pgpolicy;
+	int detect_pgpolicy_use_tpg;
 	int force_sync;
 	int deferred_remove;
 	int processed_main_config;
diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 366b166f..ac99edc8 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -305,6 +305,7 @@ int setup_map(struct multipath *mpp, char **params, struct vectors *vecs)
 
 	select_pgfailback(conf, mpp);
 	select_detect_pgpolicy(conf, mpp);
+	select_detect_pgpolicy_use_tpg(conf, mpp);
 	select_pgpolicy(conf, mpp);
 
 	/*
diff --git a/libmultipath/defaults.h b/libmultipath/defaults.h
index 8f2d2f05..b3f11d4c 100644
--- a/libmultipath/defaults.h
+++ b/libmultipath/defaults.h
@@ -30,6 +30,7 @@
 #define DEFAULT_DETECT_PRIO	DETECT_PRIO_ON
 #define DEFAULT_DETECT_CHECKER	DETECT_CHECKER_ON
 #define DEFAULT_DETECT_PGPOLICY	DETECT_PGPOLICY_ON
+#define DEFAULT_DETECT_PGPOLICY_USE_TPG	DETECT_PGPOLICY_USE_TPG_OFF
 #define DEFAULT_DEFERRED_REMOVE	DEFERRED_REMOVE_OFF
 #define DEFAULT_DELAY_CHECKS	NU_NO
 #define DEFAULT_ERR_CHECKS	NU_NO
diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index edd4923d..6b3e04a3 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -876,6 +876,14 @@ declare_ovr_snprint(detect_pgpolicy, print_yes_no_undef)
 declare_hw_handler(detect_pgpolicy, set_yes_no_undef)
 declare_hw_snprint(detect_pgpolicy, print_yes_no_undef)
 
+declare_def_handler(detect_pgpolicy_use_tpg, set_yes_no_undef)
+declare_def_snprint_defint(detect_pgpolicy_use_tpg, print_yes_no_undef,
+			   DEFAULT_DETECT_PGPOLICY_USE_TPG)
+declare_ovr_handler(detect_pgpolicy_use_tpg, set_yes_no_undef)
+declare_ovr_snprint(detect_pgpolicy_use_tpg, print_yes_no_undef)
+declare_hw_handler(detect_pgpolicy_use_tpg, set_yes_no_undef)
+declare_hw_snprint(detect_pgpolicy_use_tpg, print_yes_no_undef)
+
 declare_def_handler(force_sync, set_yes_no)
 declare_def_snprint(force_sync, print_yes_no)
 
@@ -2121,6 +2129,7 @@ init_keywords(vector keywords)
 	install_keyword("detect_prio", &def_detect_prio_handler, &snprint_def_detect_prio);
 	install_keyword("detect_checker", &def_detect_checker_handler, &snprint_def_detect_checker);
 	install_keyword("detect_pgpolicy", &def_detect_pgpolicy_handler, &snprint_def_detect_pgpolicy);
+	install_keyword("detect_pgpolicy_use_tpg", &def_detect_pgpolicy_use_tpg_handler, &snprint_def_detect_pgpolicy_use_tpg);
 	install_keyword("force_sync", &def_force_sync_handler, &snprint_def_force_sync);
 	install_keyword("strict_timing", &def_strict_timing_handler, &snprint_def_strict_timing);
 	install_keyword("deferred_remove", &def_deferred_remove_handler, &snprint_def_deferred_remove);
@@ -2212,6 +2221,7 @@ init_keywords(vector keywords)
 	install_keyword("detect_prio", &hw_detect_prio_handler, &snprint_hw_detect_prio);
 	install_keyword("detect_checker", &hw_detect_checker_handler, &snprint_hw_detect_checker);
 	install_keyword("detect_pgpolicy", &hw_detect_pgpolicy_handler, &snprint_hw_detect_pgpolicy);
+	install_keyword("detect_pgpolicy_use_tpg", &hw_detect_pgpolicy_use_tpg_handler, &snprint_hw_detect_pgpolicy_use_tpg);
 	install_keyword("deferred_remove", &hw_deferred_remove_handler, &snprint_hw_deferred_remove);
 	install_keyword("delay_watch_checks", &hw_delay_watch_checks_handler, &snprint_hw_delay_watch_checks);
 	install_keyword("delay_wait_checks", &hw_delay_wait_checks_handler, &snprint_hw_delay_wait_checks);
@@ -2255,6 +2265,7 @@ init_keywords(vector keywords)
 	install_keyword("detect_prio", &ovr_detect_prio_handler, &snprint_ovr_detect_prio);
 	install_keyword("detect_checker", &ovr_detect_checker_handler, &snprint_ovr_detect_checker);
 	install_keyword("detect_pgpolicy", &ovr_detect_pgpolicy_handler, &snprint_ovr_detect_pgpolicy);
+	install_keyword("detect_pgpolicy_use_tpg", &ovr_detect_pgpolicy_use_tpg_handler, &snprint_ovr_detect_pgpolicy_use_tpg);
 	install_keyword("deferred_remove", &ovr_deferred_remove_handler, &snprint_ovr_deferred_remove);
 	install_keyword("delay_watch_checks", &ovr_delay_watch_checks_handler, &snprint_ovr_delay_watch_checks);
 	install_keyword("delay_wait_checks", &ovr_delay_wait_checks_handler, &snprint_ovr_delay_wait_checks);
diff --git a/libmultipath/hwtable.c b/libmultipath/hwtable.c
index 0b3e7c49..9a98a7ba 100644
--- a/libmultipath/hwtable.c
+++ b/libmultipath/hwtable.c
@@ -68,6 +68,7 @@
 		.detect_prio   = DETECT_PRIO_ON,
 		.detect_checker = DETECT_CHECKER_ON,
 		.detect_pgpolicy = DETECT_PGPOLICY_ON,
+		.detect_pgpolicy_use_tpg = DETECT_PGPOLICY_USE_TPG_ON,
 		.deferred_remove = DEFERRED_REMOVE_OFF,
 		.delay_watch_checks = DELAY_CHECKS_OFF,
 		.delay_wait_checks = DELAY_CHECKS_OFF,
diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
index aba1a30e..8f72c452 100644
--- a/libmultipath/libmultipath.version
+++ b/libmultipath/libmultipath.version
@@ -43,7 +43,7 @@ LIBMPATHCOMMON_1.0.0 {
 	put_multipath_config;
 };
 
-LIBMULTIPATH_18.0.0 {
+LIBMULTIPATH_19.0.0 {
 global:
 	/* symbols referenced by multipath and multipathd */
 	add_foreign;
@@ -116,6 +116,7 @@ global:
 	get_refwwid;
 	get_state;
 	get_udev_device;
+	get_uid;
 	get_used_hwes;
 	get_vpd_sgio;
 	group_by_prio;
@@ -141,6 +142,7 @@ global:
 	pathcount;
 	path_discovery;
 	path_get_tpgs;
+	pathinfo;
 	path_offline;
 	print_all_paths;
 	print_foreign_topology;
@@ -235,9 +237,3 @@ global:
 local:
 	*;
 };
-
-LIBMULTIPATH_18.1.0 {
-global:
-	get_uid;
-	pathinfo;
-} LIBMULTIPATH_18.0.0;
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index 10b84948..df10a68f 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -280,6 +280,22 @@ out:
 	return 0;
 }
 
+int select_detect_pgpolicy_use_tpg(struct config *conf, struct multipath *mp)
+{
+	const char *origin;
+
+	mp_set_ovr(detect_pgpolicy_use_tpg);
+	mp_set_hwe(detect_pgpolicy_use_tpg);
+	mp_set_conf(detect_pgpolicy_use_tpg);
+	mp_set_default(detect_pgpolicy_use_tpg,
+		       DEFAULT_DETECT_PGPOLICY_USE_TPG);
+out:
+	condlog(3, "%s: detect_pgpolicy_use_tpg = %s %s", mp->alias,
+		(mp->detect_pgpolicy_use_tpg == DETECT_PGPOLICY_USE_TPG_ON)?
+		"yes" : "no", origin);
+	return 0;
+}
+
 int select_pgpolicy(struct config *conf, struct multipath * mp)
 {
 	const char *origin;
@@ -291,7 +307,10 @@ int select_pgpolicy(struct config *conf, struct multipath * mp)
 		goto out;
 	}
 	if (mp->detect_pgpolicy == DETECT_PGPOLICY_ON && verify_alua_prio(mp)) {
-		mp->pgpolicy = GROUP_BY_PRIO;
+		if (mp->detect_pgpolicy_use_tpg == DETECT_PGPOLICY_USE_TPG_ON)
+			mp->pgpolicy = GROUP_BY_TPG;
+		else
+			mp->pgpolicy = GROUP_BY_PRIO;
 		origin = autodetect_origin;
 		goto out;
 	}
@@ -301,7 +320,8 @@ int select_pgpolicy(struct config *conf, struct multipath * mp)
 	mp_set_conf(pgpolicy);
 	mp_set_default(pgpolicy, DEFAULT_PGPOLICY);
 out:
-	if (mp->pgpolicy == GROUP_BY_TPG && !verify_alua_prio(mp)) {
+	if (mp->pgpolicy == GROUP_BY_TPG && origin != autodetect_origin &&
+	    !verify_alua_prio(mp)) {
 		mp->pgpolicy = DEFAULT_PGPOLICY;
 		origin = "(setting: emergency fallback - not all paths use alua prio)";
 		log_prio = 1;
diff --git a/libmultipath/propsel.h b/libmultipath/propsel.h
index 513ee6ac..73615c2f 100644
--- a/libmultipath/propsel.h
+++ b/libmultipath/propsel.h
@@ -1,6 +1,7 @@
 int select_rr_weight (struct config *conf, struct multipath * mp);
 int select_pgfailback (struct config *conf, struct multipath * mp);
 int select_detect_pgpolicy (struct config *conf, struct multipath * mp);
+int select_detect_pgpolicy_use_tpg (struct config *conf, struct multipath * mp);
 int select_pgpolicy (struct config *conf, struct multipath * mp);
 int select_selector (struct config *conf, struct multipath * mp);
 int select_alias (struct config *conf, struct multipath * mp);
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 682a7e17..97f9de5a 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -149,6 +149,12 @@ enum detect_pgpolicy_states {
 	DETECT_PGPOLICY_ON = YNU_YES,
 };
 
+enum detect_pgpolicy_use_tpg_states {
+	DETECT_PGPOLICY_USE_TPG_UNDEF = YNU_UNDEF,
+	DETECT_PGPOLICY_USE_TPG_OFF = YNU_NO,
+	DETECT_PGPOLICY_USE_TPG_ON = YNU_YES,
+};
+
 enum deferred_remove_states {
 	DEFERRED_REMOVE_UNDEF = YNU_UNDEF,
 	DEFERRED_REMOVE_OFF = YNU_NO,
@@ -396,6 +402,7 @@ struct multipath {
 	char wwid[WWID_SIZE];
 	char alias_old[WWID_SIZE];
 	int detect_pgpolicy;
+	int detect_pgpolicy_use_tpg;
 	int pgpolicy;
 	pgpolicyfn *pgpolicyfn;
 	int nextpg;
diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
index 3b25b5d4..8a0ff0d8 100644
--- a/multipath/multipath.conf.5
+++ b/multipath/multipath.conf.5
@@ -254,6 +254,19 @@ The default is: \fIyes\fR
 .
 .
 .TP
+.B detect_pgpolicy_use_tpg
+If both this and \fIdetect_pgpolicy\fR are set to \fIyes\fR and all path
+devcices are configured with either the \fIalua\fR or \fIsysfs\fR prioritizer,
+the multipath device will automatically use the \fIgroup_by_tpg\fR
+path_grouping_policy. If set to \fIno\fR, the path_grouping_policy will be
+selected by the method described for \fIdetect_pgpolicy\fR above.
+.RS
+.TP
+The default is: \fIno\fR
+.RE
+.
+.
+.TP
 .B pg_timeout
 (Deprecated) This option is not supported any more, and the value is ignored.
 .
-- 
2.17.2

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

[dm-devel] [PATCH V2 07/11] libmultipath: don't count PRIO_UNDEF paths for pathgroup priority

[Benjamin Marzinski]

When multipath is not set to group_by_prio, different paths in a
pathgroup can have different priorities. If there is a problem getting
the priority of an active path, its priority will be set to PRIO_UNDEF.
This will change the priority of the whole pathgroup, even though it's
likely that this is simply a temporary error. Instead, do not count
PRIO_UNDEF paths towards to priority of the path group, unless there are
no paths that have an actual priority. This will not effect the priority
of multipath devices with group_by_prio, since all paths in a pathgroup
will have the same priority.

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

diff --git a/libmultipath/switchgroup.c b/libmultipath/switchgroup.c
index 6fdfcfa7..b1e1f39b 100644
--- a/libmultipath/switchgroup.c
+++ b/libmultipath/switchgroup.c
@@ -12,6 +12,7 @@ void path_group_prio_update(struct pathgroup *pgp)
 	int i;
 	int priority = 0;
 	int marginal = 0;
+	int defined_prios = 0;
 	struct path * pp;
 
 	pgp->enabled_paths = 0;
@@ -24,12 +25,17 @@ void path_group_prio_update(struct pathgroup *pgp)
 			marginal++;
 		if (pp->state == PATH_UP ||
 		    pp->state == PATH_GHOST) {
-			priority += pp->priority;
+			if (pp->priority != PRIO_UNDEF) {
+				defined_prios++;
+				priority += pp->priority;
+			}
 			pgp->enabled_paths++;
 		}
 	}
-	if (pgp->enabled_paths)
-		pgp->priority = priority / pgp->enabled_paths;
+	if (defined_prios)
+		pgp->priority = priority / defined_prios;
+	else if (pgp->enabled_paths)
+		pgp->priority = PRIO_UNDEF;
 	else
 		pgp->priority = 0;
 	if (marginal && marginal == i)
-- 
2.17.2

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

[dm-devel] [PATCH V2 08/11] multipath-tools tests: add tests to verify PRIO_UDEF changes

[Benjamin Marzinski]

Make sure that pathgroups that include paths with a prio_UNDEF priority
are properly sorted.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 tests/pgpolicy.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/tests/pgpolicy.c b/tests/pgpolicy.c
index 85fa30ce..ccf29bc9 100644
--- a/tests/pgpolicy.c
+++ b/tests/pgpolicy.c
@@ -648,6 +648,26 @@ static void test_group_by_prio_mixed_one_marginal8(void **state)
 	verify_pathgroups(&mp8, p8, groups, group_size, group_marginal, 7);
 }
 
+static void test_group_by_prio_mixed_undef8(void **state)
+{
+	int prio[] = {7,1,3,-1,5,2,8,2};
+	int group0[] = {6};
+	int group1[] = {0};
+	int group2[] = {4};
+	int group3[] = {2};
+	int group4[] = {5,7};
+	int group5[] = {1};
+	int group6[] = {3};
+	int *groups[] = {group0, group1, group2, group3,
+			  group4, group5, group6};
+	int group_size[] = {1,1,1,1,2,1,1};
+
+	set_priority(p8, prio, 8);
+	mp8.pgpolicyfn = group_by_prio;
+	assert_int_equal(group_paths(&mp8, 0), 0);
+	verify_pathgroups(&mp8, p8, groups, group_size, NULL, 7);
+}
+
 static void test_group_by_tpg_same8(void **state)
 {
 	int paths[] = {0,1,2,3,4,5,6,7};
@@ -828,6 +848,26 @@ static void test_group_by_tpg_mixed_one_marginal8(void **state)
 	verify_pathgroups(&mp8, p8, groups, group_size, group_marginal, 7);
 }
 
+static void test_group_by_tpg_mixed_undef8(void **state)
+{
+	int prio[] = {-1,2,3,-1,5,2,8,2};
+	int tpg[] = {1,2,3,3,4,2,5,6};
+	int group0[] = {6};
+	int group1[] = {4};
+	int group2[] = {2,3};
+	int group3[] = {1,5};
+	int group4[] = {7};
+	int group5[] = {0};
+	int *groups[] = {group0, group1, group2, group3,
+			  group4, group5};
+	int group_size[] = {1,1,2,2,1,1};
+
+	set_priority(p8, prio, 8);
+	set_tpg(p8, tpg, 8);
+	mp8.pgpolicyfn = group_by_tpg;
+	assert_int_equal(group_paths(&mp8, 0), 0);
+	verify_pathgroups(&mp8, p8, groups, group_size, NULL, 6);
+}
 
 static void test_group_by_node_name_same8(void **state)
 {
@@ -1192,6 +1232,7 @@ int test_pgpolicies(void)
 		setup_test(test_group_by_prio_mixed_all_marginal, 8),
 		setup_test(test_group_by_prio_mixed_half_marginal, 8),
 		setup_test(test_group_by_prio_mixed_one_marginal, 8),
+		setup_test(test_group_by_prio_mixed_undef, 8),
 		setup_test(test_group_by_tpg_same, 8),
 		setup_test(test_group_by_tpg_different, 8),
 		setup_test(test_group_by_tpg_mixed, 8),
@@ -1203,6 +1244,7 @@ int test_pgpolicies(void)
 		setup_test(test_group_by_tpg_mixed_all_marginal, 8),
 		setup_test(test_group_by_tpg_mixed_half_marginal, 8),
 		setup_test(test_group_by_tpg_mixed_one_marginal, 8),
+		setup_test(test_group_by_tpg_mixed_undef, 8),
 		setup_test(test_group_by_node_name_same, 8),
 		setup_test(test_group_by_node_name_increasing, 8),
 		setup_test(test_group_by_node_name_3_groups, 8),
-- 
2.17.2

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

[dm-devel] [PATCH V2 09/11] multipathd: only refresh priorities in update_prio()

[Benjamin Marzinski]

Multipathd previously had various rules to update priorities in
update_prio(), need_switch_pathgroup(), and reload_map(). Instead, only
update the priority in update_prio().  To cover the cases where the
priorities were getting updated in other functions, update_prio() now
always updates all paths' priorities, if the priority on the path that
it was called with changes.

Also, do not try to update a path's priority if it is down. Previously,
when refreshing all the paths' priorities, a path could have its
priority updated when it was in the PATH_DOWN state, as long as its
priority was PRIO_UNDEF. But if a path is down, and the last time
multipath tried to get its priority, it failed, it seems likely that the
prioritizer will just fail again.

Finally, skip updating all paths' priorities in
deferred_failback_tick().  Now that all the paths' priorities will get
updated when one changes before starting the deferred failback count,
it's no longer necessary to update them all again when the failback
timeout expires.  The checker loop will continue to update them
correctly while the timeout is going on.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/cli_handlers.c  |   8 +--
 multipathd/fpin_handlers.c |   4 +-
 multipathd/main.c          | 109 +++++++++++++------------------------
 multipathd/main.h          |   3 +-
 4 files changed, 45 insertions(+), 79 deletions(-)

diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 44bf43df..c9addfbb 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -810,7 +810,7 @@ cli_reload(void *v, struct strbuf *reply, void *data)
 		return 1;
 	}
 
-	return reload_and_sync_map(mpp, vecs, 0);
+	return reload_and_sync_map(mpp, vecs);
 }
 
 static int resize_map(struct multipath *mpp, unsigned long long size,
@@ -1449,7 +1449,7 @@ static int cli_set_marginal(void * v, struct strbuf *reply, void * data)
 	}
 	pp->marginal = 1;
 
-	return reload_and_sync_map(pp->mpp, vecs, 0);
+	return reload_and_sync_map(pp->mpp, vecs);
 }
 
 static int cli_unset_marginal(void * v, struct strbuf *reply, void * data)
@@ -1476,7 +1476,7 @@ static int cli_unset_marginal(void * v, struct strbuf *reply, void * data)
 	}
 	pp->marginal = 0;
 
-	return reload_and_sync_map(pp->mpp, vecs, 0);
+	return reload_and_sync_map(pp->mpp, vecs);
 }
 
 static int cli_unset_all_marginal(void * v, struct strbuf *reply, void * data)
@@ -1513,7 +1513,7 @@ static int cli_unset_all_marginal(void * v, struct strbuf *reply, void * data)
 		vector_foreach_slot (pgp->paths, pp, j)
 			pp->marginal = 0;
 
-	return reload_and_sync_map(mpp, vecs, 0);
+	return reload_and_sync_map(mpp, vecs);
 }
 
 #define HANDLER(x) x
diff --git a/multipathd/fpin_handlers.c b/multipathd/fpin_handlers.c
index 8f464f04..aa0f63c9 100644
--- a/multipathd/fpin_handlers.c
+++ b/multipathd/fpin_handlers.c
@@ -127,7 +127,7 @@ empty:
 	/* walk backwards because reload_and_sync_map() can remove mpp */
 	vector_foreach_slot_backwards(vecs->mpvec, mpp, i) {
 		if (mpp->fpin_must_reload) {
-			ret = reload_and_sync_map(mpp, vecs, 0);
+			ret = reload_and_sync_map(mpp, vecs);
 			if (ret == 2)
 				condlog(2, "map removed during reload");
 			else
@@ -262,7 +262,7 @@ unref:
 	/* walk backwards because reload_and_sync_map() can remove mpp */
 	vector_foreach_slot_backwards(vecs->mpvec, mpp, i) {
 		if (mpp->fpin_must_reload) {
-			ret = reload_and_sync_map(mpp, vecs, 0);
+			ret = reload_and_sync_map(mpp, vecs);
 			if (ret == 2)
 				condlog(2, "map removed during reload");
 			else
diff --git a/multipathd/main.c b/multipathd/main.c
index bdeffe76..f603d143 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -396,32 +396,13 @@ void put_multipath_config(__attribute__((unused)) void *arg)
 }
 
 static int
-need_switch_pathgroup (struct multipath * mpp, int refresh)
+need_switch_pathgroup (struct multipath * mpp)
 {
-	struct pathgroup * pgp;
-	struct path * pp;
-	unsigned int i, j;
-	struct config *conf;
 	int bestpg;
 
 	if (!mpp)
 		return 0;
 
-	/*
-	 * Refresh path priority values
-	 */
-	if (refresh) {
-		vector_foreach_slot (mpp->pg, pgp, i) {
-			vector_foreach_slot (pgp->paths, pp, j) {
-				conf = get_multipath_config();
-				pthread_cleanup_push(put_multipath_config,
-						     conf);
-				pathinfo(pp, conf, DI_PRIO);
-				pthread_cleanup_pop(1);
-			}
-		}
-	}
-
 	if (!mpp->pg || VECTOR_SIZE(mpp->paths) == 0)
 		return 0;
 
@@ -1594,7 +1575,7 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
 			else {
 				if (ro == 1)
 					pp->mpp->force_readonly = 1;
-				retval = reload_and_sync_map(mpp, vecs, 0);
+				retval = reload_and_sync_map(mpp, vecs);
 				if (retval == 2)
 					condlog(2, "%s: map removed during reload", pp->dev);
 				else {
@@ -1994,7 +1975,7 @@ deferred_failback_tick (vector mpvec)
 		if (mpp->pgfailback > 0 && mpp->failback_tick > 0) {
 			mpp->failback_tick--;
 
-			if (!mpp->failback_tick && need_switch_pathgroup(mpp, 1))
+			if (!mpp->failback_tick && need_switch_pathgroup(mpp))
 				switch_pathgroup(mpp);
 		}
 	}
@@ -2051,54 +2032,40 @@ int update_prio(struct path *pp, int refresh_all)
 	int i, j, changed = 0;
 	struct config *conf;
 
-	if (refresh_all) {
-		vector_foreach_slot (pp->mpp->pg, pgp, i) {
-			vector_foreach_slot (pgp->paths, pp1, j) {
-				oldpriority = pp1->priority;
-				conf = get_multipath_config();
-				pthread_cleanup_push(put_multipath_config,
-						     conf);
-				pathinfo(pp1, conf, DI_PRIO);
-				pthread_cleanup_pop(1);
-				if (pp1->priority != oldpriority)
-					changed = 1;
-			}
-		}
-		return changed;
-	}
 	oldpriority = pp->priority;
-	conf = get_multipath_config();
-	pthread_cleanup_push(put_multipath_config, conf);
-	if (pp->state != PATH_DOWN)
+	if (pp->state != PATH_DOWN) {
+		conf = get_multipath_config();
+		pthread_cleanup_push(put_multipath_config, conf);
 		pathinfo(pp, conf, DI_PRIO);
-	pthread_cleanup_pop(1);
+		pthread_cleanup_pop(1);
+	}
 
-	if (pp->priority == oldpriority)
+	if (pp->priority == oldpriority && !refresh_all)
 		return 0;
-	return 1;
+
+	vector_foreach_slot (pp->mpp->pg, pgp, i) {
+		vector_foreach_slot (pgp->paths, pp1, j) {
+			if (pp1 == pp || pp1->state == PATH_DOWN)
+				continue;
+			oldpriority = pp1->priority;
+			conf = get_multipath_config();
+			pthread_cleanup_push(put_multipath_config, conf);
+			pathinfo(pp1, conf, DI_PRIO);
+			pthread_cleanup_pop(1);
+			if (pp1->priority != oldpriority)
+				changed = 1;
+		}
+	}
+	return changed;
 }
 
-static int reload_map(struct vectors *vecs, struct multipath *mpp, int refresh,
+static int reload_map(struct vectors *vecs, struct multipath *mpp,
 		      int is_daemon)
 {
 	char *params __attribute__((cleanup(cleanup_charp))) = NULL;
-	struct path *pp;
-	int i, r;
+	int r;
 
 	update_mpp_paths(mpp, vecs->pathvec);
-	if (refresh) {
-		vector_foreach_slot (mpp->paths, pp, i) {
-			struct config *conf = get_multipath_config();
-			pthread_cleanup_push(put_multipath_config, conf);
-			r = pathinfo(pp, conf, DI_PRIO);
-			pthread_cleanup_pop(1);
-			if (r) {
-				condlog(2, "%s: failed to refresh pathinfo",
-					mpp->alias);
-				return 1;
-			}
-		}
-	}
 	if (setup_map(mpp, &params, vecs)) {
 		condlog(0, "%s: failed to setup map", mpp->alias);
 		return 1;
@@ -2115,10 +2082,9 @@ static int reload_map(struct vectors *vecs, struct multipath *mpp, int refresh,
 	return 0;
 }
 
-int reload_and_sync_map(struct multipath *mpp,
-			struct vectors *vecs, int refresh)
+int reload_and_sync_map(struct multipath *mpp, struct vectors *vecs)
 {
-	if (reload_map(vecs, mpp, refresh, 1))
+	if (reload_map(vecs, mpp, 1))
 		return 1;
 	if (setup_multipath(vecs, mpp) != 0)
 		return 2;
@@ -2573,25 +2539,26 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 	 */
 	condlog(4, "path prio refresh");
 
-	if (marginal_changed)
-		reload_and_sync_map(pp->mpp, vecs, 1);
-	else if (update_prio(pp, new_path_up) &&
-	    (pp->mpp->pgpolicyfn == (pgpolicyfn *)group_by_prio) &&
-	     pp->mpp->pgfailback == -FAILBACK_IMMEDIATE) {
+	if (marginal_changed) {
+		update_prio(pp, 1);
+		reload_and_sync_map(pp->mpp, vecs);
+	} else if (update_prio(pp, new_path_up) &&
+		   pp->mpp->pgpolicyfn == (pgpolicyfn *)group_by_prio &&
+		   pp->mpp->pgfailback == -FAILBACK_IMMEDIATE) {
 		condlog(2, "%s: path priorities changed. reloading",
 			pp->mpp->alias);
-		reload_and_sync_map(pp->mpp, vecs, !new_path_up);
-	} else if (need_switch_pathgroup(pp->mpp, 0)) {
+		reload_and_sync_map(pp->mpp, vecs);
+	} else if (need_switch_pathgroup(pp->mpp)) {
 		if (pp->mpp->pgfailback > 0 &&
 		    (new_path_up || pp->mpp->failback_tick <= 0))
-			pp->mpp->failback_tick =
-				pp->mpp->pgfailback + 1;
+			pp->mpp->failback_tick = pp->mpp->pgfailback + 1;
 		else if (pp->mpp->pgfailback == -FAILBACK_IMMEDIATE ||
 			 (chkr_new_path_up && followover_should_failback(pp)))
 			switch_pathgroup(pp->mpp);
 	}
 	return 1;
 }
+
 enum checker_state {
 	CHECKER_STARTING,
 	CHECKER_RUNNING,
diff --git a/multipathd/main.h b/multipathd/main.h
index e8bee8e6..a253d186 100644
--- a/multipathd/main.h
+++ b/multipathd/main.h
@@ -47,8 +47,7 @@ int __setup_multipath (struct vectors * vecs, struct multipath * mpp,
 		       int reset);
 #define setup_multipath(vecs, mpp) __setup_multipath(vecs, mpp, 1)
 int update_multipath (struct vectors *vecs, char *mapname, int reset);
-int reload_and_sync_map(struct multipath *mpp, struct vectors *vecs,
-			int refresh);
+int reload_and_sync_map(struct multipath *mpp, struct vectors *vecs);
 
 void handle_path_wwid_change(struct path *pp, struct vectors *vecs);
 bool check_path_wwid_change(struct path *pp);
-- 
2.17.2

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

[dm-devel] [PATCH V2 10/11] multipathd: reload map if the path groups are out of order

[Benjamin Marzinski]

need_switch_pathgroup() only checks if the currently used pathgroup is
not the highest priority pathgroup. If it isn't, all multipathd does is
instruct the kernel to switch to the correct pathgroup.  However, the
kernel treats the pathgroups as if they were ordered by priority. When
the kernel runs out of paths to use in the currently selected pathgroup,
it will start checking the pathgroups in order until it finds one with
usable paths.

need_switch_pathgroup() should also check if the pathgroups are out of
order, and if so, multipathd should reload the map to reorder them
correctly.

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

diff --git a/multipathd/main.c b/multipathd/main.c
index f603d143..05c74e9e 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -395,11 +395,46 @@ void put_multipath_config(__attribute__((unused)) void *arg)
 	rcu_read_unlock();
 }
 
+/*
+ * The path group orderings that this function finds acceptible are different
+ * from now select_path_group determines the best pathgroup. The idea here is
+ * to only trigger a kernel reload when it is obvious that the pathgroups would
+ * be out of order, even if all the paths were usable. Thus pathgroups with
+ * PRIO_UNDEF are skipped, and the number of enabled paths doesn't matter here.
+ */
+bool path_groups_in_order(struct multipath *mpp)
+{
+	int i;
+	struct pathgroup *pgp;
+	bool seen_marginal_pg = false;
+	int last_prio = INT_MAX;
+
+	if (VECTOR_SIZE(mpp->pg) < 2)
+		return true;
+
+	vector_foreach_slot(mpp->pg, pgp, i) {
+		/* skip pgs with PRIO_UNDEF, since this is likely temporary */
+		if (!pgp->paths || pgp->priority == PRIO_UNDEF)
+			continue;
+		if (pgp->marginal && !seen_marginal_pg) {
+			last_prio = INT_MAX;
+			continue;
+		}
+		if (seen_marginal_pg && !pgp->marginal)
+			return false;
+		if (pgp->priority > last_prio)
+			return false;
+		last_prio = pgp->priority;
+	}
+	return true;
+}
+
 static int
-need_switch_pathgroup (struct multipath * mpp)
+need_switch_pathgroup (struct multipath * mpp, bool *need_reload)
 {
 	int bestpg;
 
+	*need_reload = false;
 	if (!mpp)
 		return 0;
 
@@ -411,10 +446,9 @@ need_switch_pathgroup (struct multipath * mpp)
 		return 0;
 
 	mpp->bestpg = bestpg;
-	if (mpp->bestpg != mpp->nextpg)
-		return 1;
+	*need_reload = !path_groups_in_order(mpp);
 
-	return 0;
+	return (*need_reload || mpp->bestpg != mpp->nextpg);
 }
 
 static void
@@ -1963,20 +1997,26 @@ ghost_delay_tick(struct vectors *vecs)
 }
 
 static void
-deferred_failback_tick (vector mpvec)
+deferred_failback_tick (struct vectors *vecs)
 {
 	struct multipath * mpp;
 	unsigned int i;
+	bool need_reload;
 
-	vector_foreach_slot (mpvec, mpp, i) {
+	vector_foreach_slot (vecs->mpvec, mpp, i) {
 		/*
 		 * deferred failback getting sooner
 		 */
 		if (mpp->pgfailback > 0 && mpp->failback_tick > 0) {
 			mpp->failback_tick--;
 
-			if (!mpp->failback_tick && need_switch_pathgroup(mpp))
-				switch_pathgroup(mpp);
+			if (!mpp->failback_tick &&
+			    need_switch_pathgroup(mpp, &need_reload)) {
+				if (need_reload)
+					reload_and_sync_map(mpp, vecs);
+				else
+					switch_pathgroup(mpp);
+			}
 		}
 	}
 }
@@ -2219,6 +2259,7 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 	struct config *conf;
 	int marginal_pathgroups, marginal_changed = 0;
 	int ret;
+	bool need_reload;
 
 	if (((pp->initialized == INIT_OK || pp->initialized == INIT_PARTIAL ||
 	      pp->initialized == INIT_REQUESTED_UDEV) && !pp->mpp) ||
@@ -2548,13 +2589,17 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 		condlog(2, "%s: path priorities changed. reloading",
 			pp->mpp->alias);
 		reload_and_sync_map(pp->mpp, vecs);
-	} else if (need_switch_pathgroup(pp->mpp)) {
+	} else if (need_switch_pathgroup(pp->mpp, &need_reload)) {
 		if (pp->mpp->pgfailback > 0 &&
 		    (new_path_up || pp->mpp->failback_tick <= 0))
 			pp->mpp->failback_tick = pp->mpp->pgfailback + 1;
 		else if (pp->mpp->pgfailback == -FAILBACK_IMMEDIATE ||
-			 (chkr_new_path_up && followover_should_failback(pp)))
-			switch_pathgroup(pp->mpp);
+			 (chkr_new_path_up && followover_should_failback(pp))) {
+			if (need_reload)
+				reload_and_sync_map(pp->mpp, vecs);
+			else
+				switch_pathgroup(pp->mpp);
+		}
 	}
 	return 1;
 }
@@ -2680,7 +2725,7 @@ unlock:
 		pthread_cleanup_push(cleanup_lock, &vecs->lock);
 		lock(&vecs->lock);
 		pthread_testcancel();
-		deferred_failback_tick(vecs->mpvec);
+		deferred_failback_tick(vecs);
 		retry_count_tick(vecs->mpvec);
 		missing_uev_wait_tick(vecs);
 		ghost_delay_tick(vecs);
-- 
2.17.2

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

[dm-devel] [PATCH V2 11/11] multipathd: don't assume mpp->paths will exist in need_switch_pathgroup

[Benjamin Marzinski]

When need_switch_pathgroup() is called by deferred_failback_tick(),
there is a chance that mpp->paths will be NULL, even if there are paths
in the multipath device's pathgroups. Instead check if there are
multiple pathgroups, since multipath can't be using the wrong pathgroup
if there is one or none.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 05c74e9e..539cff54 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -438,7 +438,7 @@ need_switch_pathgroup (struct multipath * mpp, bool *need_reload)
 	if (!mpp)
 		return 0;
 
-	if (!mpp->pg || VECTOR_SIZE(mpp->paths) == 0)
+	if (VECTOR_SIZE(mpp->pg) < 2)
 		return 0;
 
 	bestpg = select_path_group(mpp);
-- 
2.17.2

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

Re: [dm-devel] [PATCH V2 01/11] libmultipath: add group_by_tpg path_grouping_policy

[Martin Wilck]

On Tue, 2023-06-06 at 15:13 -0500, Benjamin Marzinski wrote:
> When we group paths by prio and the priority changes, paths can end
> up
> temporarily in the wrong path groups.  This usually happens when some
> paths are down, so their priority can't be updated. To avoid this for
> ALUA paths, group them by their target port group instead. The path
> groups chosen by this policy won't always match with those chosen by
> group_by_prio, since it is possible for multiple ALUA target port
> groups to have the same priority.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/discovery.c         |  1 +
>  libmultipath/pgpolicies.c        | 19 +++++++++++++++++++
>  libmultipath/pgpolicies.h        |  4 +++-
>  libmultipath/prioritizers/alua.c |  1 +
>  libmultipath/propsel.c           | 27 +++++++++++++++++++++++++--
>  libmultipath/structs.c           |  1 +
>  libmultipath/structs.h           |  3 +++
>  multipath/main.c                 |  1 +
>  multipath/multipath.conf.5       |  4 ++++
>  9 files changed, 58 insertions(+), 3 deletions(-)
> 
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 6865cd92..2dcafe5d 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -1051,6 +1051,7 @@ detect_alua(struct path * pp)
>                 return;
>         }
>         pp->tpgs = tpgs;
> +       pp->tpg_id = ret;
>  }
>  
>  int path_get_tpgs(struct path *pp)
> diff --git a/libmultipath/pgpolicies.c b/libmultipath/pgpolicies.c
> index 10b44d37..e14da8cc 100644
> --- a/libmultipath/pgpolicies.c
> +++ b/libmultipath/pgpolicies.c
> @@ -25,6 +25,8 @@ int get_pgpolicy_id(char * str)
>                 return GROUP_BY_PRIO;
>         if (0 == strncmp(str, "group_by_node_name", 18))
>                 return GROUP_BY_NODE_NAME;
> +       if (0 == strncmp(str, "group_by_tpg", 12))
> +               return GROUP_BY_TPG;
>  
>         return IOPOLICY_UNDEF;
>  }
> @@ -49,6 +51,9 @@ int get_pgpolicy_name(char * buff, int len, int id)
>         case GROUP_BY_NODE_NAME:
>                 s = "group_by_node_name";
>                 break;
> +       case GROUP_BY_TPG:
> +               s = "group_by_tpg";
> +               break;
>         default:
>                 s = "undefined";
>                 break;
> @@ -191,6 +196,12 @@ prios_match(struct path *pp1, struct path *pp2)
>         return (pp1->priority == pp2->priority);
>  }
>  
> +bool
> +tpg_match(struct path *pp1, struct path *pp2)
> +{
> +       return (pp1->tpg_id == pp2->tpg_id);
> +}
> +
>  int group_by_match(struct multipath * mp, vector paths,
>                    bool (*path_match_fn)(struct path *, struct path
> *))
>  {
> @@ -279,6 +290,14 @@ int group_by_prio(struct multipath *mp, vector
> paths)
>         return group_by_match(mp, paths, prios_match);
>  }
>  
> +/*
> + * One path group per alua target port group present in the path
> vector
> + */
> +int group_by_tpg(struct multipath *mp, vector paths)
> +{
> +       return group_by_match(mp, paths, tpg_match);
> +}
> +
>  int one_path_per_group(struct multipath *mp, vector paths)
>  {
>         int i;
> diff --git a/libmultipath/pgpolicies.h b/libmultipath/pgpolicies.h
> index 15927610..d3ab2f35 100644
> --- a/libmultipath/pgpolicies.h
> +++ b/libmultipath/pgpolicies.h
> @@ -16,7 +16,8 @@ enum iopolicies {
>         MULTIBUS,
>         GROUP_BY_SERIAL,
>         GROUP_BY_PRIO,
> -       GROUP_BY_NODE_NAME
> +       GROUP_BY_NODE_NAME,
> +       GROUP_BY_TPG,
>  };
>  
>  int get_pgpolicy_id(char *);
> @@ -30,5 +31,6 @@ int one_group(struct multipath *, vector);
>  int group_by_serial(struct multipath *, vector);
>  int group_by_prio(struct multipath *, vector);
>  int group_by_node_name(struct multipath *, vector);
> +int group_by_tpg(struct multipath *, vector);
>  
>  #endif
> diff --git a/libmultipath/prioritizers/alua.c
> b/libmultipath/prioritizers/alua.c
> index 0ab06e2b..4888a974 100644
> --- a/libmultipath/prioritizers/alua.c
> +++ b/libmultipath/prioritizers/alua.c
> @@ -65,6 +65,7 @@ get_alua_info(struct path * pp, unsigned int
> timeout)
>                         return -ALUA_PRIO_NOT_SUPPORTED;
>                 return -ALUA_PRIO_RTPG_FAILED;
>         }
> +       pp->tpg_id = tpg;
>         condlog(3, "%s: reported target port group is %i", pp->dev,


I still think that we should log a change here. Perhaps we should keep
the existing condlog() and just use prio 2 if the tpg_id changed, and
prio 4 if it didn't (the current 3 clutters the logs quite a bit).

Regards
Martin



> tpg);
>         rc = get_asymmetric_access_state(pp, tpg, timeout);
>         if (rc < 0) {
> diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
> index a25cc921..841fa247 100644
> --- a/libmultipath/propsel.c
> +++ b/libmultipath/propsel.c
> @@ -35,7 +35,8 @@ pgpolicyfn *pgpolicies[] = {
>         one_group,
>         group_by_serial,
>         group_by_prio,
> -       group_by_node_name
> +       group_by_node_name,
> +       group_by_tpg,
>  };
>  
>  #define do_set(var, src, dest,
> msg)                                    \
> @@ -249,10 +250,26 @@ out:
>         return 0;
>  }
>  
> +static bool
> +verify_alua_prio(struct multipath *mp)
> +{
> +       int i;
> +       struct path *pp;
> +
> +       vector_foreach_slot(mp->paths, pp, i) {
> +               const char *name = prio_name(&pp->prio);
> +               if (strncmp(name, PRIO_ALUA, PRIO_NAME_LEN) &&
> +                   strncmp(name, PRIO_SYSFS, PRIO_NAME_LEN))
> +                        return false;
> +       }
> +       return true;
> +}
> +
>  int select_pgpolicy(struct config *conf, struct multipath * mp)
>  {
>         const char *origin;
>         char buff[POLICY_NAME_SIZE];
> +       int log_prio = 3;
>  
>         if (conf->pgpolicy_flag > 0) {
>                 mp->pgpolicy = conf->pgpolicy_flag;
> @@ -265,9 +282,15 @@ int select_pgpolicy(struct config *conf, struct
> multipath * mp)
>         mp_set_conf(pgpolicy);
>         mp_set_default(pgpolicy, DEFAULT_PGPOLICY);
>  out:
> +       if (mp->pgpolicy == GROUP_BY_TPG && !verify_alua_prio(mp)) {
> +               mp->pgpolicy = DEFAULT_PGPOLICY;
> +               origin = "(setting: emergency fallback - not all
> paths use alua prio)";
> +               log_prio = 1;
> +       }
>         mp->pgpolicyfn = pgpolicies[mp->pgpolicy];
>         get_pgpolicy_name(buff, POLICY_NAME_SIZE, mp->pgpolicy);
> -       condlog(3, "%s: path_grouping_policy = %s %s", mp->alias,
> buff, origin);
> +       condlog(log_prio, "%s: path_grouping_policy = %s %s", mp-
> >alias, buff,
> +               origin);
>         return 0;
>  }
>  
> diff --git a/libmultipath/structs.c b/libmultipath/structs.c
> index 87e84d5d..39504dca 100644
> --- a/libmultipath/structs.c
> +++ b/libmultipath/structs.c
> @@ -125,6 +125,7 @@ alloc_path (void)
>                 pp->sg_id.proto_id = PROTOCOL_UNSET;
>                 pp->fd = -1;
>                 pp->tpgs = TPGS_UNDEF;
> +               pp->tpg_id = GROUP_ID_UNDEF;
>                 pp->priority = PRIO_UNDEF;
>                 pp->checkint = CHECKINT_UNDEF;
>                 checker_clear(&pp->checker);
> diff --git a/libmultipath/structs.h b/libmultipath/structs.h
> index a1208751..0eea19b4 100644
> --- a/libmultipath/structs.h
> +++ b/libmultipath/structs.h
> @@ -317,6 +317,8 @@ struct hd_geometry {
>  };
>  #endif
>  
> +#define GROUP_ID_UNDEF -1
> +
>  struct path {
>         char dev[FILE_NAME_SIZE];
>         char dev_t[BLK_DEV_SIZE];
> @@ -372,6 +374,7 @@ struct path {
>         /* configlet pointers */
>         vector hwe;
>         struct gen_path generic_path;
> +       int tpg_id;
>  };
>  
>  typedef int (pgpolicyfn) (struct multipath *, vector);
> diff --git a/multipath/main.c b/multipath/main.c
> index 90f940f1..b78f3162 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -157,6 +157,7 @@ usage (char * progname)
>                 "          . group_by_serial     one priority group
> per serial\n"
>                 "          . group_by_prio       one priority group
> per priority lvl\n"
>                 "          . group_by_node_name  one priority group
> per target node\n"
> +               "          . group_by_tpg        one priority group
> per ALUA target port group\n"
>                 "  -v lvl  verbosity level:\n"
>                 "          . 0 no output\n"
>                 "          . 1 print created devmap names only\n"
> diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
> index b4dccd1b..b65a543d 100644
> --- a/multipath/multipath.conf.5
> +++ b/multipath/multipath.conf.5
> @@ -233,6 +233,10 @@ per-multipath option in the configuration file.
>  One priority group per target node name. Target node names are
> fetched
>  in \fI/sys/class/fc_transport/target*/node_name\fR.
>  .TP
> +.I group_by_tpg
> +One priority group per ALUA target port group. In order to use this
> policy,
> +all paths in the multipath device must have \fIprio\fR set to
> \fBalua\fR.
> +.TP
>  The default is: \fBfailover\fR
>  .RE
>  .

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

Re: [dm-devel] [PATCH V2 08/11] multipath-tools tests: add tests to verify PRIO_UDEF changes

[Martin Wilck]

On Tue, 2023-06-06 at 15:13 -0500, Benjamin Marzinski wrote:
> Make sure that pathgroups that include paths with a prio_UNDEF
> priority
> are properly sorted.
>  
> Reviewed-by: Martin Wilck <mwilck@suse.com>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

Nit: typo in subject. I'll fix this when the series is applied to the
queue branch.

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

Re: [dm-devel] [PATCH V2 04/11] multipath-tools tests: add tests for group_by_tpg policy

[Martin Wilck]

On Tue, 2023-06-06 at 15:13 -0500, Benjamin Marzinski wrote:
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

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

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

Re: [dm-devel] [PATCH V2 05/11] libmultipath: add "detect_pgpolicy" config option

[Martin Wilck]

On Tue, 2023-06-06 at 15:13 -0500, Benjamin Marzinski wrote:
> This allows configuations to use "group_by_prio" if alua is
> autodetected
> and another policy if it isn't, so they can work with detect_prio.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

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

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

Re: [dm-devel] [PATCH V2 00/11] multipath: Add a group_by_tgp pgpolicy

[Martin Wilck]

On Tue, 2023-06-06 at 15:13 -0500, Benjamin Marzinski wrote:
> This patchset is a combination of my previous two patchsets
> 
> [PATCH 0/5] multipath: Add a group_by_tgp path grouping policy
> [PATCH 0/5] priority and pathgroup switching changes
> 
> The first part adds a new path grouping policy that can be used with
> ALUA devices. The goal is to avoid the temporary incorrect path
> groupings that can happen when paths change priorities.
> 

Lest I forget: it'd be nice to add functionality (in a later set) to
print the TPG id of a pathgroup in the topology output if group_by_tpg
was used.

Thanks,
Martin

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

Re: [dm-devel] [PATCH V2 01/11] libmultipath: add group_by_tpg path_grouping_policy

[Benjamin Marzinski]

On Wed, Jun 07, 2023 at 06:31:19PM +0000, Martin Wilck wrote:
> On Tue, 2023-06-06 at 15:13 -0500, Benjamin Marzinski wrote:
> >  #endif
> > diff --git a/libmultipath/prioritizers/alua.c
> > b/libmultipath/prioritizers/alua.c
> > index 0ab06e2b..4888a974 100644
> > --- a/libmultipath/prioritizers/alua.c
> > +++ b/libmultipath/prioritizers/alua.c
> > @@ -65,6 +65,7 @@ get_alua_info(struct path * pp, unsigned int
> > timeout)
> >                         return -ALUA_PRIO_NOT_SUPPORTED;
> >                 return -ALUA_PRIO_RTPG_FAILED;
> >         }
> > +       pp->tpg_id = tpg;
> >         condlog(3, "%s: reported target port group is %i", pp->dev,
> 
> 
> I still think that we should log a change here. Perhaps we should keep
> the existing condlog() and just use prio 2 if the tpg_id changed, and
> prio 4 if it didn't (the current 3 clutters the logs quite a bit).

Oops. My Bad. I meant to add that. I'll send a follow-on patch. 

-Ben

> 
> Regards
> Martin
> 
> 
> 
> > tpg);
> >         rc = get_asymmetric_access_state(pp, tpg, timeout);
> >         if (rc < 0) {
> > diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
> > index a25cc921..841fa247 100644
> > --- a/libmultipath/propsel.c
> > +++ b/libmultipath/propsel.c
> > @@ -35,7 +35,8 @@ pgpolicyfn *pgpolicies[] = {
> >         one_group,
> >         group_by_serial,
> >         group_by_prio,
> > -       group_by_node_name
> > +       group_by_node_name,
> > +       group_by_tpg,
> >  };
> >  
> >  #define do_set(var, src, dest,
> > msg)                                    \
> > @@ -249,10 +250,26 @@ out:
> >         return 0;
> >  }
> >  
> > +static bool
> > +verify_alua_prio(struct multipath *mp)
> > +{
> > +       int i;
> > +       struct path *pp;
> > +
> > +       vector_foreach_slot(mp->paths, pp, i) {
> > +               const char *name = prio_name(&pp->prio);
> > +               if (strncmp(name, PRIO_ALUA, PRIO_NAME_LEN) &&
> > +                   strncmp(name, PRIO_SYSFS, PRIO_NAME_LEN))
> > +                        return false;
> > +       }
> > +       return true;
> > +}
> > +
> >  int select_pgpolicy(struct config *conf, struct multipath * mp)
> >  {
> >         const char *origin;
> >         char buff[POLICY_NAME_SIZE];
> > +       int log_prio = 3;
> >  
> >         if (conf->pgpolicy_flag > 0) {
> >                 mp->pgpolicy = conf->pgpolicy_flag;
> > @@ -265,9 +282,15 @@ int select_pgpolicy(struct config *conf, struct
> > multipath * mp)
> >         mp_set_conf(pgpolicy);
> >         mp_set_default(pgpolicy, DEFAULT_PGPOLICY);
> >  out:
> > +       if (mp->pgpolicy == GROUP_BY_TPG && !verify_alua_prio(mp)) {
> > +               mp->pgpolicy = DEFAULT_PGPOLICY;
> > +               origin = "(setting: emergency fallback - not all
> > paths use alua prio)";
> > +               log_prio = 1;
> > +       }
> >         mp->pgpolicyfn = pgpolicies[mp->pgpolicy];
> >         get_pgpolicy_name(buff, POLICY_NAME_SIZE, mp->pgpolicy);
> > -       condlog(3, "%s: path_grouping_policy = %s %s", mp->alias,
> > buff, origin);
> > +       condlog(log_prio, "%s: path_grouping_policy = %s %s", mp-
> > >alias, buff,
> > +               origin);
> >         return 0;
> >  }
> >  
> > diff --git a/libmultipath/structs.c b/libmultipath/structs.c
> > index 87e84d5d..39504dca 100644
> > --- a/libmultipath/structs.c
> > +++ b/libmultipath/structs.c
> > @@ -125,6 +125,7 @@ alloc_path (void)
> >                 pp->sg_id.proto_id = PROTOCOL_UNSET;
> >                 pp->fd = -1;
> >                 pp->tpgs = TPGS_UNDEF;
> > +               pp->tpg_id = GROUP_ID_UNDEF;
> >                 pp->priority = PRIO_UNDEF;
> >                 pp->checkint = CHECKINT_UNDEF;
> >                 checker_clear(&pp->checker);
> > diff --git a/libmultipath/structs.h b/libmultipath/structs.h
> > index a1208751..0eea19b4 100644
> > --- a/libmultipath/structs.h
> > +++ b/libmultipath/structs.h
> > @@ -317,6 +317,8 @@ struct hd_geometry {
> >  };
> >  #endif
> >  
> > +#define GROUP_ID_UNDEF -1
> > +
> >  struct path {
> >         char dev[FILE_NAME_SIZE];
> >         char dev_t[BLK_DEV_SIZE];
> > @@ -372,6 +374,7 @@ struct path {
> >         /* configlet pointers */
> >         vector hwe;
> >         struct gen_path generic_path;
> > +       int tpg_id;
> >  };
> >  
> >  typedef int (pgpolicyfn) (struct multipath *, vector);
> > diff --git a/multipath/main.c b/multipath/main.c
> > index 90f940f1..b78f3162 100644
> > --- a/multipath/main.c
> > +++ b/multipath/main.c
> > @@ -157,6 +157,7 @@ usage (char * progname)
> >                 "          . group_by_serial     one priority group
> > per serial\n"
> >                 "          . group_by_prio       one priority group
> > per priority lvl\n"
> >                 "          . group_by_node_name  one priority group
> > per target node\n"
> > +               "          . group_by_tpg        one priority group
> > per ALUA target port group\n"
> >                 "  -v lvl  verbosity level:\n"
> >                 "          . 0 no output\n"
> >                 "          . 1 print created devmap names only\n"
> > diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
> > index b4dccd1b..b65a543d 100644
> > --- a/multipath/multipath.conf.5
> > +++ b/multipath/multipath.conf.5
> > @@ -233,6 +233,10 @@ per-multipath option in the configuration file.
> >  One priority group per target node name. Target node names are
> > fetched
> >  in \fI/sys/class/fc_transport/target*/node_name\fR.
> >  .TP
> > +.I group_by_tpg
> > +One priority group per ALUA target port group. In order to use this
> > policy,
> > +all paths in the multipath device must have \fIprio\fR set to
> > \fBalua\fR.
> > +.TP
> >  The default is: \fBfailover\fR
> >  .RE
> >  .
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH V2 06/11] libmultipath: add "detect_pgpolicy_use_tpg" config option

[Martin Wilck]

On Tue, 2023-06-06 at 15:13 -0500, Benjamin Marzinski wrote:
> If this and "detect_pgpolicy" are both selected and ALUA is
> autodetected, the multipath device will use the "group_by_tpg" policy
> instead of the "group_by_prio" policy.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

One minor remark below.

> ---
>  libmultipath/config.c             |  2 ++
>  libmultipath/config.h             |  2 ++
>  libmultipath/configure.c          |  1 +
>  libmultipath/defaults.h           |  1 +
>  libmultipath/dict.c               | 11 +++++++++++
>  libmultipath/hwtable.c            |  1 +
>  libmultipath/libmultipath.version | 10 +++-------
>  libmultipath/propsel.c            | 24 ++++++++++++++++++++++--
>  libmultipath/propsel.h            |  1 +
>  libmultipath/structs.h            |  7 +++++++
>  multipath/multipath.conf.5        | 13 +++++++++++++
>  11 files changed, 64 insertions(+), 9 deletions(-)
> 
> diff --git a/libmultipath/config.c b/libmultipath/config.c
> index 2e742373..7b207590 100644
> --- a/libmultipath/config.c
> +++ b/libmultipath/config.c
> @@ -453,6 +453,7 @@ merge_hwe (struct hwentry * dst, struct hwentry *
> src)
>         merge_num(detect_prio);
>         merge_num(detect_checker);
>         merge_num(detect_pgpolicy);
> +       merge_num(detect_pgpolicy_use_tpg);
>         merge_num(deferred_remove);
>         merge_num(delay_watch_checks);
>         merge_num(delay_wait_checks);
> @@ -619,6 +620,7 @@ store_hwe (vector hwtable, struct hwentry * dhwe)
>         hwe->detect_prio = dhwe->detect_prio;
>         hwe->detect_checker = dhwe->detect_checker;
>         hwe->detect_pgpolicy = dhwe->detect_pgpolicy;
> +       hwe->detect_pgpolicy_use_tpg = dhwe->detect_pgpolicy_use_tpg;
>         hwe->ghost_delay = dhwe->ghost_delay;
>         hwe->vpd_vendor_id = dhwe->vpd_vendor_id;
>  
> diff --git a/libmultipath/config.h b/libmultipath/config.h
> index 014c6849..0a2c297b 100644
> --- a/libmultipath/config.h
> +++ b/libmultipath/config.h
> @@ -77,6 +77,7 @@ struct hwentry {
>         int detect_prio;
>         int detect_checker;
>         int detect_pgpolicy;
> +       int detect_pgpolicy_use_tpg;
>         int deferred_remove;
>         int delay_watch_checks;
>         int delay_wait_checks;
> @@ -173,6 +174,7 @@ struct config {
>         int detect_prio;
>         int detect_checker;
>         int detect_pgpolicy;
> +       int detect_pgpolicy_use_tpg;
>         int force_sync;
>         int deferred_remove;
>         int processed_main_config;
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index 366b166f..ac99edc8 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -305,6 +305,7 @@ int setup_map(struct multipath *mpp, char
> **params, struct vectors *vecs)
>  
>         select_pgfailback(conf, mpp);
>         select_detect_pgpolicy(conf, mpp);
> +       select_detect_pgpolicy_use_tpg(conf, mpp);
>         select_pgpolicy(conf, mpp);
>  
>         /*
> diff --git a/libmultipath/defaults.h b/libmultipath/defaults.h
> index 8f2d2f05..b3f11d4c 100644
> --- a/libmultipath/defaults.h
> +++ b/libmultipath/defaults.h
> @@ -30,6 +30,7 @@
>  #define DEFAULT_DETECT_PRIO    DETECT_PRIO_ON
>  #define DEFAULT_DETECT_CHECKER DETECT_CHECKER_ON
>  #define DEFAULT_DETECT_PGPOLICY        DETECT_PGPOLICY_ON
> +#define
> DEFAULT_DETECT_PGPOLICY_USE_TPG        DETECT_PGPOLICY_USE_TPG_OFF
>  #define DEFAULT_DEFERRED_REMOVE        DEFERRED_REMOVE_OFF
>  #define DEFAULT_DELAY_CHECKS   NU_NO
>  #define DEFAULT_ERR_CHECKS     NU_NO
> diff --git a/libmultipath/dict.c b/libmultipath/dict.c
> index edd4923d..6b3e04a3 100644
> --- a/libmultipath/dict.c
> +++ b/libmultipath/dict.c
> @@ -876,6 +876,14 @@ declare_ovr_snprint(detect_pgpolicy,
> print_yes_no_undef)
>  declare_hw_handler(detect_pgpolicy, set_yes_no_undef)
>  declare_hw_snprint(detect_pgpolicy, print_yes_no_undef)
>  
> +declare_def_handler(detect_pgpolicy_use_tpg, set_yes_no_undef)
> +declare_def_snprint_defint(detect_pgpolicy_use_tpg,
> print_yes_no_undef,
> +                          DEFAULT_DETECT_PGPOLICY_USE_TPG)
> +declare_ovr_handler(detect_pgpolicy_use_tpg, set_yes_no_undef)
> +declare_ovr_snprint(detect_pgpolicy_use_tpg, print_yes_no_undef)
> +declare_hw_handler(detect_pgpolicy_use_tpg, set_yes_no_undef)
> +declare_hw_snprint(detect_pgpolicy_use_tpg, print_yes_no_undef)
> +
>  declare_def_handler(force_sync, set_yes_no)
>  declare_def_snprint(force_sync, print_yes_no)
>  
> @@ -2121,6 +2129,7 @@ init_keywords(vector keywords)
>         install_keyword("detect_prio", &def_detect_prio_handler,
> &snprint_def_detect_prio);
>         install_keyword("detect_checker",
> &def_detect_checker_handler, &snprint_def_detect_checker);
>         install_keyword("detect_pgpolicy",
> &def_detect_pgpolicy_handler, &snprint_def_detect_pgpolicy);
> +       install_keyword("detect_pgpolicy_use_tpg",
> &def_detect_pgpolicy_use_tpg_handler,
> &snprint_def_detect_pgpolicy_use_tpg);
>         install_keyword("force_sync", &def_force_sync_handler,
> &snprint_def_force_sync);
>         install_keyword("strict_timing", &def_strict_timing_handler,
> &snprint_def_strict_timing);
>         install_keyword("deferred_remove",
> &def_deferred_remove_handler, &snprint_def_deferred_remove);
> @@ -2212,6 +2221,7 @@ init_keywords(vector keywords)
>         install_keyword("detect_prio", &hw_detect_prio_handler,
> &snprint_hw_detect_prio);
>         install_keyword("detect_checker", &hw_detect_checker_handler,
> &snprint_hw_detect_checker);
>         install_keyword("detect_pgpolicy",
> &hw_detect_pgpolicy_handler, &snprint_hw_detect_pgpolicy);
> +       install_keyword("detect_pgpolicy_use_tpg",
> &hw_detect_pgpolicy_use_tpg_handler,
> &snprint_hw_detect_pgpolicy_use_tpg);
>         install_keyword("deferred_remove",
> &hw_deferred_remove_handler, &snprint_hw_deferred_remove);
>         install_keyword("delay_watch_checks",
> &hw_delay_watch_checks_handler, &snprint_hw_delay_watch_checks);
>         install_keyword("delay_wait_checks",
> &hw_delay_wait_checks_handler, &snprint_hw_delay_wait_checks);
> @@ -2255,6 +2265,7 @@ init_keywords(vector keywords)
>         install_keyword("detect_prio", &ovr_detect_prio_handler,
> &snprint_ovr_detect_prio);
>         install_keyword("detect_checker",
> &ovr_detect_checker_handler, &snprint_ovr_detect_checker);
>         install_keyword("detect_pgpolicy",
> &ovr_detect_pgpolicy_handler, &snprint_ovr_detect_pgpolicy);
> +       install_keyword("detect_pgpolicy_use_tpg",
> &ovr_detect_pgpolicy_use_tpg_handler,
> &snprint_ovr_detect_pgpolicy_use_tpg);
>         install_keyword("deferred_remove",
> &ovr_deferred_remove_handler, &snprint_ovr_deferred_remove);
>         install_keyword("delay_watch_checks",
> &ovr_delay_watch_checks_handler, &snprint_ovr_delay_watch_checks);
>         install_keyword("delay_wait_checks",
> &ovr_delay_wait_checks_handler, &snprint_ovr_delay_wait_checks);
> diff --git a/libmultipath/hwtable.c b/libmultipath/hwtable.c
> index 0b3e7c49..9a98a7ba 100644
> --- a/libmultipath/hwtable.c
> +++ b/libmultipath/hwtable.c
> @@ -68,6 +68,7 @@
>                 .detect_prio   = DETECT_PRIO_ON,
>                 .detect_checker = DETECT_CHECKER_ON,
>                 .detect_pgpolicy = DETECT_PGPOLICY_ON,
> +               .detect_pgpolicy_use_tpg =
> DETECT_PGPOLICY_USE_TPG_ON,

This is supposed to be the default, i.e. DETECT_PGPOLICY_USE_TPG_OFF




>                 .deferred_remove = DEFERRED_REMOVE_OFF,
>                 .delay_watch_checks = DELAY_CHECKS_OFF,
>                 .delay_wait_checks = DELAY_CHECKS_OFF,
> diff --git a/libmultipath/libmultipath.version
> b/libmultipath/libmultipath.version
> index aba1a30e..8f72c452 100644
> --- a/libmultipath/libmultipath.version
> +++ b/libmultipath/libmultipath.version
> @@ -43,7 +43,7 @@ LIBMPATHCOMMON_1.0.0 {
>         put_multipath_config;
>  };
>  
> -LIBMULTIPATH_18.0.0 {
> +LIBMULTIPATH_19.0.0 {
>  global:
>         /* symbols referenced by multipath and multipathd */
>         add_foreign;
> @@ -116,6 +116,7 @@ global:
>         get_refwwid;
>         get_state;
>         get_udev_device;
> +       get_uid;
>         get_used_hwes;
>         get_vpd_sgio;
>         group_by_prio;
> @@ -141,6 +142,7 @@ global:
>         pathcount;
>         path_discovery;
>         path_get_tpgs;
> +       pathinfo;
>         path_offline;
>         print_all_paths;
>         print_foreign_topology;
> @@ -235,9 +237,3 @@ global:
>  local:
>         *;
>  };
> -
> -LIBMULTIPATH_18.1.0 {
> -global:
> -       get_uid;
> -       pathinfo;
> -} LIBMULTIPATH_18.0.0;
> diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
> index 10b84948..df10a68f 100644
> --- a/libmultipath/propsel.c
> +++ b/libmultipath/propsel.c
> @@ -280,6 +280,22 @@ out:
>         return 0;
>  }
>  
> +int select_detect_pgpolicy_use_tpg(struct config *conf, struct
> multipath *mp)
> +{
> +       const char *origin;
> +
> +       mp_set_ovr(detect_pgpolicy_use_tpg);
> +       mp_set_hwe(detect_pgpolicy_use_tpg);
> +       mp_set_conf(detect_pgpolicy_use_tpg);
> +       mp_set_default(detect_pgpolicy_use_tpg,
> +                      DEFAULT_DETECT_PGPOLICY_USE_TPG);
> +out:
> +       condlog(3, "%s: detect_pgpolicy_use_tpg = %s %s", mp->alias,
> +               (mp->detect_pgpolicy_use_tpg ==
> DETECT_PGPOLICY_USE_TPG_ON)?
> +               "yes" : "no", origin);
> +       return 0;
> +}
> +
>  int select_pgpolicy(struct config *conf, struct multipath * mp)
>  {
>         const char *origin;
> @@ -291,7 +307,10 @@ int select_pgpolicy(struct config *conf, struct
> multipath * mp)
>                 goto out;
>         }
>         if (mp->detect_pgpolicy == DETECT_PGPOLICY_ON &&
> verify_alua_prio(mp)) {
> -               mp->pgpolicy = GROUP_BY_PRIO;
> +               if (mp->detect_pgpolicy_use_tpg ==
> DETECT_PGPOLICY_USE_TPG_ON)
> +                       mp->pgpolicy = GROUP_BY_TPG;
> +               else
> +                       mp->pgpolicy = GROUP_BY_PRIO;
>                 origin = autodetect_origin;
>                 goto out;
>         }
> @@ -301,7 +320,8 @@ int select_pgpolicy(struct config *conf, struct
> multipath * mp)
>         mp_set_conf(pgpolicy);
>         mp_set_default(pgpolicy, DEFAULT_PGPOLICY);
>  out:
> -       if (mp->pgpolicy == GROUP_BY_TPG && !verify_alua_prio(mp)) {
> +       if (mp->pgpolicy == GROUP_BY_TPG && origin !=
> autodetect_origin &&
> +           !verify_alua_prio(mp)) {
>                 mp->pgpolicy = DEFAULT_PGPOLICY;
>                 origin = "(setting: emergency fallback - not all
> paths use alua prio)";
>                 log_prio = 1;
> diff --git a/libmultipath/propsel.h b/libmultipath/propsel.h
> index 513ee6ac..73615c2f 100644
> --- a/libmultipath/propsel.h
> +++ b/libmultipath/propsel.h
> @@ -1,6 +1,7 @@
>  int select_rr_weight (struct config *conf, struct multipath * mp);
>  int select_pgfailback (struct config *conf, struct multipath * mp);
>  int select_detect_pgpolicy (struct config *conf, struct multipath *
> mp);
> +int select_detect_pgpolicy_use_tpg (struct config *conf, struct
> multipath * mp);
>  int select_pgpolicy (struct config *conf, struct multipath * mp);
>  int select_selector (struct config *conf, struct multipath * mp);
>  int select_alias (struct config *conf, struct multipath * mp);
> diff --git a/libmultipath/structs.h b/libmultipath/structs.h
> index 682a7e17..97f9de5a 100644
> --- a/libmultipath/structs.h
> +++ b/libmultipath/structs.h
> @@ -149,6 +149,12 @@ enum detect_pgpolicy_states {
>         DETECT_PGPOLICY_ON = YNU_YES,
>  };
>  
> +enum detect_pgpolicy_use_tpg_states {
> +       DETECT_PGPOLICY_USE_TPG_UNDEF = YNU_UNDEF,
> +       DETECT_PGPOLICY_USE_TPG_OFF = YNU_NO,
> +       DETECT_PGPOLICY_USE_TPG_ON = YNU_YES,
> +};
> +
>  enum deferred_remove_states {
>         DEFERRED_REMOVE_UNDEF = YNU_UNDEF,
>         DEFERRED_REMOVE_OFF = YNU_NO,
> @@ -396,6 +402,7 @@ struct multipath {
>         char wwid[WWID_SIZE];
>         char alias_old[WWID_SIZE];
>         int detect_pgpolicy;
> +       int detect_pgpolicy_use_tpg;
>         int pgpolicy;
>         pgpolicyfn *pgpolicyfn;
>         int nextpg;
> diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
> index 3b25b5d4..8a0ff0d8 100644
> --- a/multipath/multipath.conf.5
> +++ b/multipath/multipath.conf.5
> @@ -254,6 +254,19 @@ The default is: \fIyes\fR
>  .
>  .
>  .TP
> +.B detect_pgpolicy_use_tpg
> +If both this and \fIdetect_pgpolicy\fR are set to \fIyes\fR and all
> path
> +devcices are configured with either the \fIalua\fR or \fIsysfs\fR
> prioritizer,
> +the multipath device will automatically use the \fIgroup_by_tpg\fR
> +path_grouping_policy. If set to \fIno\fR, the path_grouping_policy
> will be
> +selected by the method described for \fIdetect_pgpolicy\fR above.
> +.RS
> +.TP
> +The default is: \fIno\fR
> +.RE
> +.
> +.
> +.TP
>  .B pg_timeout
>  (Deprecated) This option is not supported any more, and the value is
> ignored.
>  .

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

Re: [dm-devel] [PATCH V2 10/11] multipathd: reload map if the path groups are out of order

[Martin Wilck]

On Tue, 2023-06-06 at 15:13 -0500, Benjamin Marzinski wrote:
> need_switch_pathgroup() only checks if the currently used pathgroup
> is
> not the highest priority pathgroup. If it isn't, all multipathd does
> is
> instruct the kernel to switch to the correct pathgroup.  However, the
> kernel treats the pathgroups as if they were ordered by priority.
> When
> the kernel runs out of paths to use in the currently selected
> pathgroup,
> it will start checking the pathgroups in order until it finds one
> with
> usable paths.
> 
> need_switch_pathgroup() should also check if the pathgroups are out
> of
> order, and if so, multipathd should reload the map to reorder them
> correctly.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  multipathd/main.c | 69 ++++++++++++++++++++++++++++++++++++++-------
> --
>  1 file changed, 57 insertions(+), 12 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index f603d143..05c74e9e 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -395,11 +395,46 @@ void
> put_multipath_config(__attribute__((unused)) void *arg)
>         rcu_read_unlock();
>  }
>  
> +/*
> + * The path group orderings that this function finds acceptible are
> different
> + * from now select_path_group determines the best pathgroup. The
> idea here is
> + * to only trigger a kernel reload when it is obvious that the
> pathgroups would
> + * be out of order, even if all the paths were usable. Thus
> pathgroups with
> + * PRIO_UNDEF are skipped, and the number of enabled paths doesn't
> matter here.
> + */
> +bool path_groups_in_order(struct multipath *mpp)
> +{
> +       int i;
> +       struct pathgroup *pgp;
> +       bool seen_marginal_pg = false;
> +       int last_prio = INT_MAX;
> +
> +       if (VECTOR_SIZE(mpp->pg) < 2)
> +               return true;
> +
> +       vector_foreach_slot(mpp->pg, pgp, i) {
> +               /* skip pgs with PRIO_UNDEF, since this is likely
> temporary */
> +               if (!pgp->paths || pgp->priority == PRIO_UNDEF)
> +                       continue;
> +               if (pgp->marginal && !seen_marginal_pg) {
> +                       last_prio = INT_MAX;
> +                       continue;
> +               }
> +               if (seen_marginal_pg && !pgp->marginal)
> +                       return false;
> +               if (pgp->priority > last_prio)
> +                       return false;
> +               last_prio = pgp->priority;
> +       }
> +       return true;
> +}

I still don't get the logic here. What's the point of using
seen_marginal_pg if it is always false? See my previous reply to v1 of
this patch.

Regards
Martin



> +
>  static int
> -need_switch_pathgroup (struct multipath * mpp)
> +need_switch_pathgroup (struct multipath * mpp, bool *need_reload)
>  {
>         int bestpg;
>  
> +       *need_reload = false;
>         if (!mpp)
>                 return 0;
>  
> @@ -411,10 +446,9 @@ need_switch_pathgroup (struct multipath * mpp)
>                 return 0;
>  
>         mpp->bestpg = bestpg;
> -       if (mpp->bestpg != mpp->nextpg)
> -               return 1;
> +       *need_reload = !path_groups_in_order(mpp);
>  
> -       return 0;
> +       return (*need_reload || mpp->bestpg != mpp->nextpg);
>  }
>  
>  static void
> @@ -1963,20 +1997,26 @@ ghost_delay_tick(struct vectors *vecs)
>  }
>  
>  static void
> -deferred_failback_tick (vector mpvec)
> +deferred_failback_tick (struct vectors *vecs)
>  {
>         struct multipath * mpp;
>         unsigned int i;
> +       bool need_reload;
>  
> -       vector_foreach_slot (mpvec, mpp, i) {
> +       vector_foreach_slot (vecs->mpvec, mpp, i) {
>                 /*
>                  * deferred failback getting sooner
>                  */
>                 if (mpp->pgfailback > 0 && mpp->failback_tick > 0) {
>                         mpp->failback_tick--;
>  
> -                       if (!mpp->failback_tick &&
> need_switch_pathgroup(mpp))
> -                               switch_pathgroup(mpp);
> +                       if (!mpp->failback_tick &&
> +                           need_switch_pathgroup(mpp, &need_reload))
> {
> +                               if (need_reload)
> +                                       reload_and_sync_map(mpp,
> vecs);
> +                               else
> +                                       switch_pathgroup(mpp);
> +                       }
>                 }
>         }
>  }
> @@ -2219,6 +2259,7 @@ check_path (struct vectors * vecs, struct path
> * pp, unsigned int ticks)
>         struct config *conf;
>         int marginal_pathgroups, marginal_changed = 0;
>         int ret;
> +       bool need_reload;
>  
>         if (((pp->initialized == INIT_OK || pp->initialized ==
> INIT_PARTIAL ||
>               pp->initialized == INIT_REQUESTED_UDEV) && !pp->mpp) ||
> @@ -2548,13 +2589,17 @@ check_path (struct vectors * vecs, struct
> path * pp, unsigned int ticks)
>                 condlog(2, "%s: path priorities changed. reloading",
>                         pp->mpp->alias);
>                 reload_and_sync_map(pp->mpp, vecs);
> -       } else if (need_switch_pathgroup(pp->mpp)) {
> +       } else if (need_switch_pathgroup(pp->mpp, &need_reload)) {
>                 if (pp->mpp->pgfailback > 0 &&
>                     (new_path_up || pp->mpp->failback_tick <= 0))
>                         pp->mpp->failback_tick = pp->mpp->pgfailback
> + 1;
>                 else if (pp->mpp->pgfailback == -FAILBACK_IMMEDIATE
> ||
> -                        (chkr_new_path_up &&
> followover_should_failback(pp)))
> -                       switch_pathgroup(pp->mpp);
> +                        (chkr_new_path_up &&
> followover_should_failback(pp))) {
> +                       if (need_reload)
> +                               reload_and_sync_map(pp->mpp, vecs);
> +                       else
> +                               switch_pathgroup(pp->mpp);
> +               }
>         }
>         return 1;
>  }
> @@ -2680,7 +2725,7 @@ unlock:
>                 pthread_cleanup_push(cleanup_lock, &vecs->lock);
>                 lock(&vecs->lock);
>                 pthread_testcancel();
> -               deferred_failback_tick(vecs->mpvec);
> +               deferred_failback_tick(vecs);
>                 retry_count_tick(vecs->mpvec);
>                 missing_uev_wait_tick(vecs);
>                 ghost_delay_tick(vecs);



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

Re: [dm-devel] [PATCH V2 09/11] multipathd: only refresh priorities in update_prio()

[Martin Wilck]

On Tue, 2023-06-06 at 15:13 -0500, Benjamin Marzinski wrote:
> Multipathd previously had various rules to update priorities in
> update_prio(), need_switch_pathgroup(), and reload_map(). Instead,
> only
> update the priority in update_prio().  To cover the cases where the
> priorities were getting updated in other functions, update_prio() now
> always updates all paths' priorities, if the priority on the path
> that
> it was called with changes.
> 
> Also, do not try to update a path's priority if it is down.
> Previously,
> when refreshing all the paths' priorities, a path could have its
> priority updated when it was in the PATH_DOWN state, as long as its
> priority was PRIO_UNDEF. But if a path is down, and the last time
> multipath tried to get its priority, it failed, it seems likely that
> the
> prioritizer will just fail again.
> 
> Finally, skip updating all paths' priorities in
> deferred_failback_tick().  Now that all the paths' priorities will
> get
> updated when one changes before starting the deferred failback count,
> it's no longer necessary to update them all again when the failback
> timeout expires.  The checker loop will continue to update them
> correctly while the timeout is going on.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

Nice, thanks for doing this.

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

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

Re: [dm-devel] [PATCH V2 10/11] multipathd: reload map if the path groups are out of order

[Benjamin Marzinski]

On Wed, Jun 07, 2023 at 06:59:21PM +0000, Martin Wilck wrote:
> On Tue, 2023-06-06 at 15:13 -0500, Benjamin Marzinski wrote:
> > need_switch_pathgroup() only checks if the currently used pathgroup
> > is
> > not the highest priority pathgroup. If it isn't, all multipathd does
> > is
> > instruct the kernel to switch to the correct pathgroup.  However, the
> > kernel treats the pathgroups as if they were ordered by priority.
> > When
> > the kernel runs out of paths to use in the currently selected
> > pathgroup,
> > it will start checking the pathgroups in order until it finds one
> > with
> > usable paths.
> > 
> > need_switch_pathgroup() should also check if the pathgroups are out
> > of
> > order, and if so, multipathd should reload the map to reorder them
> > correctly.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  multipathd/main.c | 69 ++++++++++++++++++++++++++++++++++++++-------
> > --
> >  1 file changed, 57 insertions(+), 12 deletions(-)
> > 
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index f603d143..05c74e9e 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -395,11 +395,46 @@ void
> > put_multipath_config(__attribute__((unused)) void *arg)
> >         rcu_read_unlock();
> >  }
> >  
> > +/*
> > + * The path group orderings that this function finds acceptible are
> > different
> > + * from now select_path_group determines the best pathgroup. The
> > idea here is
> > + * to only trigger a kernel reload when it is obvious that the
> > pathgroups would
> > + * be out of order, even if all the paths were usable. Thus
> > pathgroups with
> > + * PRIO_UNDEF are skipped, and the number of enabled paths doesn't
> > matter here.
> > + */
> > +bool path_groups_in_order(struct multipath *mpp)
> > +{
> > +       int i;
> > +       struct pathgroup *pgp;
> > +       bool seen_marginal_pg = false;
> > +       int last_prio = INT_MAX;
> > +
> > +       if (VECTOR_SIZE(mpp->pg) < 2)
> > +               return true;
> > +
> > +       vector_foreach_slot(mpp->pg, pgp, i) {
> > +               /* skip pgs with PRIO_UNDEF, since this is likely
> > temporary */
> > +               if (!pgp->paths || pgp->priority == PRIO_UNDEF)
> > +                       continue;
> > +               if (pgp->marginal && !seen_marginal_pg) {
> > +                       last_prio = INT_MAX;
> > +                       continue;
> > +               }
> > +               if (seen_marginal_pg && !pgp->marginal)
> > +                       return false;
> > +               if (pgp->priority > last_prio)
> > +                       return false;
> > +               last_prio = pgp->priority;
> > +       }
> > +       return true;
> > +}
> 
> I still don't get the logic here. What's the point of using
> seen_marginal_pg if it is always false? See my previous reply to v1 of
> this patch.

That's because the logic's all messed up. It should be:

               if (pgp->marginal && !seen_marginal_pg) {
                       seen_marginal_pg = true;
                       last_prio = pgp->priority;
                       continue;
               }

You reset the priority to make sure that the marginal pgs are in
priority order as well, but you need to reset it to the first marginal
pg's priority. And yes, once you see a marginal pg, you should set
seen_marginal_pg.

Also the (seen_marginal_pg && !pgp->marginal) check should happen first
in the loop. Even if the path group doesn't currently have any usable
paths, non-marginal pgs should always be before marginal pgs.

I'll post a V3 patchset.

-Ben

> 
> Regards
> Martin
> 
> 
> 
> > +
> >  static int
> > -need_switch_pathgroup (struct multipath * mpp)
> > +need_switch_pathgroup (struct multipath * mpp, bool *need_reload)
> >  {
> >         int bestpg;
> >  
> > +       *need_reload = false;
> >         if (!mpp)
> >                 return 0;
> >  
> > @@ -411,10 +446,9 @@ need_switch_pathgroup (struct multipath * mpp)
> >                 return 0;
> >  
> >         mpp->bestpg = bestpg;
> > -       if (mpp->bestpg != mpp->nextpg)
> > -               return 1;
> > +       *need_reload = !path_groups_in_order(mpp);
> >  
> > -       return 0;
> > +       return (*need_reload || mpp->bestpg != mpp->nextpg);
> >  }
> >  
> >  static void
> > @@ -1963,20 +1997,26 @@ ghost_delay_tick(struct vectors *vecs)
> >  }
> >  
> >  static void
> > -deferred_failback_tick (vector mpvec)
> > +deferred_failback_tick (struct vectors *vecs)
> >  {
> >         struct multipath * mpp;
> >         unsigned int i;
> > +       bool need_reload;
> >  
> > -       vector_foreach_slot (mpvec, mpp, i) {
> > +       vector_foreach_slot (vecs->mpvec, mpp, i) {
> >                 /*
> >                  * deferred failback getting sooner
> >                  */
> >                 if (mpp->pgfailback > 0 && mpp->failback_tick > 0) {
> >                         mpp->failback_tick--;
> >  
> > -                       if (!mpp->failback_tick &&
> > need_switch_pathgroup(mpp))
> > -                               switch_pathgroup(mpp);
> > +                       if (!mpp->failback_tick &&
> > +                           need_switch_pathgroup(mpp, &need_reload))
> > {
> > +                               if (need_reload)
> > +                                       reload_and_sync_map(mpp,
> > vecs);
> > +                               else
> > +                                       switch_pathgroup(mpp);
> > +                       }
> >                 }
> >         }
> >  }
> > @@ -2219,6 +2259,7 @@ check_path (struct vectors * vecs, struct path
> > * pp, unsigned int ticks)
> >         struct config *conf;
> >         int marginal_pathgroups, marginal_changed = 0;
> >         int ret;
> > +       bool need_reload;
> >  
> >         if (((pp->initialized == INIT_OK || pp->initialized ==
> > INIT_PARTIAL ||
> >               pp->initialized == INIT_REQUESTED_UDEV) && !pp->mpp) ||
> > @@ -2548,13 +2589,17 @@ check_path (struct vectors * vecs, struct
> > path * pp, unsigned int ticks)
> >                 condlog(2, "%s: path priorities changed. reloading",
> >                         pp->mpp->alias);
> >                 reload_and_sync_map(pp->mpp, vecs);
> > -       } else if (need_switch_pathgroup(pp->mpp)) {
> > +       } else if (need_switch_pathgroup(pp->mpp, &need_reload)) {
> >                 if (pp->mpp->pgfailback > 0 &&
> >                     (new_path_up || pp->mpp->failback_tick <= 0))
> >                         pp->mpp->failback_tick = pp->mpp->pgfailback
> > + 1;
> >                 else if (pp->mpp->pgfailback == -FAILBACK_IMMEDIATE
> > ||
> > -                        (chkr_new_path_up &&
> > followover_should_failback(pp)))
> > -                       switch_pathgroup(pp->mpp);
> > +                        (chkr_new_path_up &&
> > followover_should_failback(pp))) {
> > +                       if (need_reload)
> > +                               reload_and_sync_map(pp->mpp, vecs);
> > +                       else
> > +                               switch_pathgroup(pp->mpp);
> > +               }
> >         }
> >         return 1;
> >  }
> > @@ -2680,7 +2725,7 @@ unlock:
> >                 pthread_cleanup_push(cleanup_lock, &vecs->lock);
> >                 lock(&vecs->lock);
> >                 pthread_testcancel();
> > -               deferred_failback_tick(vecs->mpvec);
> > +               deferred_failback_tick(vecs);
> >                 retry_count_tick(vecs->mpvec);
> >                 missing_uev_wait_tick(vecs);
> >                 ghost_delay_tick(vecs);
> 
> 
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel