* [dm-devel] [PATCH 0/5] multipath: Add a group_by_tgp path grouping policy
@ 2023-05-19 23:02 Benjamin Marzinski
2023-05-19 23:02 ` [dm-devel] [PATCH 1/5] libmultipath: add group_by_tpg path_grouping_policy Benjamin Marzinski
` (5 more replies)
0 siblings, 6 replies; 18+ messages in thread
From: Benjamin Marzinski @ 2023-05-19 23:02 UTC (permalink / raw)
To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck
This patchset is 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.
There is one thing that I'm not sure of. Is there any possiblity of a
path device changing the target port group it belongs to while it use?
If so, then we would need code to check for this and reload the device
if it occurs.
Benjamin Marzinski (5):
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/config.c | 2 +
libmultipath/config.h | 2 +
libmultipath/configure.c | 1 +
libmultipath/defaults.h | 1 +
libmultipath/dict.c | 17 ++-
libmultipath/discovery.c | 1 +
libmultipath/hwtable.c | 1 +
libmultipath/libmultipath.version | 10 +-
libmultipath/pgpolicies.c | 42 ++++---
libmultipath/pgpolicies.h | 6 +-
libmultipath/print.c | 9 ++
libmultipath/prioritizers/alua.c | 1 +
libmultipath/propsel.c | 50 +++++++-
libmultipath/propsel.h | 1 +
libmultipath/structs.c | 1 +
libmultipath/structs.h | 10 ++
multipath/main.c | 1 +
multipath/multipath.conf.5 | 16 +++
tests/pgpolicy.c | 201 ++++++++++++++++++++++++++++++
19 files changed, 338 insertions(+), 35 deletions(-)
--
2.17.2
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* [dm-devel] [PATCH 1/5] libmultipath: add group_by_tpg path_grouping_policy
2023-05-19 23:02 [dm-devel] [PATCH 0/5] multipath: Add a group_by_tgp path grouping policy Benjamin Marzinski
@ 2023-05-19 23:02 ` Benjamin Marzinski
2023-05-31 15:19 ` Martin Wilck
2023-05-19 23:02 ` [dm-devel] [PATCH 2/5] libmultipath: don't copy pgpolicy string in get_pgpolicy_name Benjamin Marzinski
` (4 subsequent siblings)
5 siblings, 1 reply; 18+ messages in thread
From: Benjamin Marzinski @ 2023-05-19 23:02 UTC (permalink / raw)
To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [dm-devel] [PATCH 2/5] libmultipath: don't copy pgpolicy string in get_pgpolicy_name
2023-05-19 23:02 [dm-devel] [PATCH 0/5] multipath: Add a group_by_tgp path grouping policy Benjamin Marzinski
2023-05-19 23:02 ` [dm-devel] [PATCH 1/5] libmultipath: add group_by_tpg path_grouping_policy Benjamin Marzinski
@ 2023-05-19 23:02 ` Benjamin Marzinski
2023-05-31 15:19 ` Martin Wilck
2023-05-19 23:02 ` [dm-devel] [PATCH 3/5] libmultipath: add ALUA tpg path wildcard Benjamin Marzinski
` (3 subsequent siblings)
5 siblings, 1 reply; 18+ messages in thread
From: Benjamin Marzinski @ 2023-05-19 23:02 UTC (permalink / raw)
To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck
copying the value into a passed in buffer doesn't help any of the
callers of this function. It's just wasted work.
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [dm-devel] [PATCH 3/5] libmultipath: add ALUA tpg path wildcard
2023-05-19 23:02 [dm-devel] [PATCH 0/5] multipath: Add a group_by_tgp path grouping policy Benjamin Marzinski
2023-05-19 23:02 ` [dm-devel] [PATCH 1/5] libmultipath: add group_by_tpg path_grouping_policy Benjamin Marzinski
2023-05-19 23:02 ` [dm-devel] [PATCH 2/5] libmultipath: don't copy pgpolicy string in get_pgpolicy_name Benjamin Marzinski
@ 2023-05-19 23:02 ` Benjamin Marzinski
2023-05-31 15:21 ` Martin Wilck
2023-05-19 23:02 ` [dm-devel] [PATCH 4/5] multipath-tools tests: add tests for group_by_tpg policy Benjamin Marzinski
` (2 subsequent siblings)
5 siblings, 1 reply; 18+ messages in thread
From: Benjamin Marzinski @ 2023-05-19 23:02 UTC (permalink / raw)
To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck
Make it possible to easily check a path's target port group.
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [dm-devel] [PATCH 4/5] multipath-tools tests: add tests for group_by_tpg policy
2023-05-19 23:02 [dm-devel] [PATCH 0/5] multipath: Add a group_by_tgp path grouping policy Benjamin Marzinski
` (2 preceding siblings ...)
2023-05-19 23:02 ` [dm-devel] [PATCH 3/5] libmultipath: add ALUA tpg path wildcard Benjamin Marzinski
@ 2023-05-19 23:02 ` Benjamin Marzinski
2023-05-31 15:30 ` Martin Wilck
2023-05-19 23:02 ` [dm-devel] [PATCH 5/5] libmultipath: add "detect_pgpolicy" config option Benjamin Marzinski
2023-05-31 15:45 ` [dm-devel] [PATCH 0/5] multipath: Add a group_by_tgp path grouping policy Martin Wilck
5 siblings, 1 reply; 18+ messages in thread
From: Benjamin Marzinski @ 2023-05-19 23:02 UTC (permalink / raw)
To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [dm-devel] [PATCH 5/5] libmultipath: add "detect_pgpolicy" config option
2023-05-19 23:02 [dm-devel] [PATCH 0/5] multipath: Add a group_by_tgp path grouping policy Benjamin Marzinski
` (3 preceding siblings ...)
2023-05-19 23:02 ` [dm-devel] [PATCH 4/5] multipath-tools tests: add tests for group_by_tpg policy Benjamin Marzinski
@ 2023-05-19 23:02 ` Benjamin Marzinski
2023-05-31 15:44 ` Martin Wilck
2023-05-31 15:45 ` [dm-devel] [PATCH 0/5] multipath: Add a group_by_tgp path grouping policy Martin Wilck
5 siblings, 1 reply; 18+ messages in thread
From: Benjamin Marzinski @ 2023-05-19 23:02 UTC (permalink / raw)
To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck
This allows configuations to use "group_by_tpg" 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/libmultipath.version | 10 +++-------
libmultipath/propsel.c | 23 ++++++++++++++++++++++-
libmultipath/propsel.h | 1 +
libmultipath/structs.h | 7 +++++++
multipath/multipath.conf.5 | 12 ++++++++++++
11 files changed, 63 insertions(+), 8 deletions(-)
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..090baa5c 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_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 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 65bca744..803230c1 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_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 d214281b..12f4825d 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,13 +290,19 @@ 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_TPG;
+ origin = autodetect_origin;
+ goto out;
+ }
mp_set_mpe(pgpolicy);
mp_set_ovr(pgpolicy);
mp_set_hwe(pgpolicy);
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 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..9f8be510 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_tpg\fR path_grouping_policy. If set to \fIno\fR, the
+path_grouping_policy will be selected as usual.
+.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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [dm-devel] [PATCH 2/5] libmultipath: don't copy pgpolicy string in get_pgpolicy_name
2023-05-19 23:02 ` [dm-devel] [PATCH 2/5] libmultipath: don't copy pgpolicy string in get_pgpolicy_name Benjamin Marzinski
@ 2023-05-31 15:19 ` Martin Wilck
0 siblings, 0 replies; 18+ messages in thread
From: Martin Wilck @ 2023-05-31 15:19 UTC (permalink / raw)
To: bmarzins@redhat.com, christophe.varoqui@opensvc.com; +Cc: dm-devel@redhat.com
On Fri, 2023-05-19 at 18:02 -0500, Benjamin Marzinski wrote:
> copying the value into a passed in buffer doesn't help any of the
> callers of this function. It's just wasted work.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Martin Wilck <mwilck@suse.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;
> }
>
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [dm-devel] [PATCH 1/5] libmultipath: add group_by_tpg path_grouping_policy
2023-05-19 23:02 ` [dm-devel] [PATCH 1/5] libmultipath: add group_by_tpg path_grouping_policy Benjamin Marzinski
@ 2023-05-31 15:19 ` Martin Wilck
2023-06-01 17:47 ` Benjamin Marzinski
0 siblings, 1 reply; 18+ messages in thread
From: Martin Wilck @ 2023-05-31 15:19 UTC (permalink / raw)
To: bmarzins@redhat.com, christophe.varoqui@opensvc.com; +Cc: dm-devel@redhat.com
On Fri, 2023-05-19 at 18:02 -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;
In view of the discussion from the cover letter:
tpg_id should have already been set by detect_alua(). I'm not sure if
it's good to overwrite it silently here. Shouldn't we rather check
if the value is unchanged, or refrain from setting it more than once?
> 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;
> + }
I don't understand this logic. Why don't you simply check pp->tpgs?
> 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [dm-devel] [PATCH 3/5] libmultipath: add ALUA tpg path wildcard
2023-05-19 23:02 ` [dm-devel] [PATCH 3/5] libmultipath: add ALUA tpg path wildcard Benjamin Marzinski
@ 2023-05-31 15:21 ` Martin Wilck
0 siblings, 0 replies; 18+ messages in thread
From: Martin Wilck @ 2023-05-31 15:21 UTC (permalink / raw)
To: bmarzins@redhat.com, christophe.varoqui@opensvc.com; +Cc: dm-devel@redhat.com
On Fri, 2023-05-19 at 18:02 -0500, Benjamin Marzinski wrote:
> Make it possible to easily check a path's target port group.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Martin Wilck <mwilck@suse.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[] = {
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [dm-devel] [PATCH 4/5] multipath-tools tests: add tests for group_by_tpg policy
2023-05-19 23:02 ` [dm-devel] [PATCH 4/5] multipath-tools tests: add tests for group_by_tpg policy Benjamin Marzinski
@ 2023-05-31 15:30 ` Martin Wilck
2023-06-01 17:51 ` Benjamin Marzinski
0 siblings, 1 reply; 18+ messages in thread
From: Martin Wilck @ 2023-05-31 15:30 UTC (permalink / raw)
To: bmarzins@redhat.com, christophe.varoqui@opensvc.com; +Cc: dm-devel@redhat.com
On Fri, 2023-05-19 at 18:02 -0500, Benjamin Marzinski wrote:
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
I wonder if it might make sense for group_by_tpg to mock calls to
getprio (assigning the prio from the path's TPG ID) rather than calling
set_priority() directly.
> ---
> 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),
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [dm-devel] [PATCH 5/5] libmultipath: add "detect_pgpolicy" config option
2023-05-19 23:02 ` [dm-devel] [PATCH 5/5] libmultipath: add "detect_pgpolicy" config option Benjamin Marzinski
@ 2023-05-31 15:44 ` Martin Wilck
2023-06-01 18:17 ` Benjamin Marzinski
0 siblings, 1 reply; 18+ messages in thread
From: Martin Wilck @ 2023-05-31 15:44 UTC (permalink / raw)
To: bmarzins@redhat.com, christophe.varoqui@opensvc.com; +Cc: dm-devel@redhat.com
On Fri, 2023-05-19 at 18:02 -0500, Benjamin Marzinski wrote:
> This allows configuations to use "group_by_tpg" if alua is
> autodetected
> and another policy if it isn't, so they can work with detect_prio.
This is a bit confusing. We might have introduced this kind of
autodetection without group_by_tpg; using group_by_prio for arrays with
ALUA support would have made quite a bit of sense.
What this patch really does is to make multipath-tools prefer
group_by_tpg over group_by_prio if it finds that ALUA is supported.
Should this be a separate option, perhaps?
- detect_pgpolicy: use an ALUA-based pgpolicy if available
- detect_pgpolicy_prefer_tpg: prefer group_by_tpg over group_by_prio
for arrays supporting ALUA.
This way users could benefit from ALUA autodetection without switching
to the TPG algorithm automatically.
Or do we have good arguments that group_by_tpg is always "better" than
group_by_prio if ALUA is supported? I guess it might be, but it still
needs to prove its usefulness it practice.
Also, if we add the auto-detection feature, I think it should default
to ON, at least upstream.
Regards,
Martin
>
> 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 | 23 ++++++++++++++++++++++-
> libmultipath/propsel.h | 1 +
> libmultipath/structs.h | 7 +++++++
> multipath/multipath.conf.5 | 12 ++++++++++++
> 11 files changed, 63 insertions(+), 8 deletions(-)
>
> 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..090baa5c 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_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 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 65bca744..803230c1 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_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 d214281b..12f4825d 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,13 +290,19 @@ 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_TPG;
> + origin = autodetect_origin;
> + goto out;
> + }
> mp_set_mpe(pgpolicy);
> mp_set_ovr(pgpolicy);
> mp_set_hwe(pgpolicy);
> 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 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..9f8be510 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_tpg\fR path_grouping_policy. If set to \fIno\fR,
> the
> +path_grouping_policy will be selected as usual.
> +.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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [dm-devel] [PATCH 0/5] multipath: Add a group_by_tgp path grouping policy
2023-05-19 23:02 [dm-devel] [PATCH 0/5] multipath: Add a group_by_tgp path grouping policy Benjamin Marzinski
` (4 preceding siblings ...)
2023-05-19 23:02 ` [dm-devel] [PATCH 5/5] libmultipath: add "detect_pgpolicy" config option Benjamin Marzinski
@ 2023-05-31 15:45 ` Martin Wilck
5 siblings, 0 replies; 18+ messages in thread
From: Martin Wilck @ 2023-05-31 15:45 UTC (permalink / raw)
To: bmarzins@redhat.com, christophe.varoqui@opensvc.com; +Cc: dm-devel@redhat.com
On Fri, 2023-05-19 at 18:02 -0500, Benjamin Marzinski wrote:
> This patchset is 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.
>
Thanks a lot for doing this. It has been on my todo list for a long
time. And sorry to make you wait so long for the review.
> There is one thing that I'm not sure of. Is there any possiblity of
> a
> path device changing the target port group it belongs to while it
> use?
> If so, then we would need code to check for this and reload the
> device
> if it occurs.
The kernel ALUA code seems to be written to deal with such changes. The
spec is unclear about it. The spec says that the primary TPG is a set
of target ports that have the same state _at all times_, which we might
boldly interpret such that the TPG can't change (if it did, the target
ports wouldn't have the same state "at all times" any more). But well,
I don't think we should rely upon that.
So yes, I think we should be prepared to handle TPG change, even if
it's very unlikely to happen in practice. But it doesn't need to be in
this patch set. After all, the worst thing that can happen is that the
grouping might be wrong in a very unusual situation. With group-by-
prio, that happens all the time, although only temporarily.
Regards
Martin
>
> Benjamin Marzinski (5):
> 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/config.c | 2 +
> libmultipath/config.h | 2 +
> libmultipath/configure.c | 1 +
> libmultipath/defaults.h | 1 +
> libmultipath/dict.c | 17 ++-
> libmultipath/discovery.c | 1 +
> libmultipath/hwtable.c | 1 +
> libmultipath/libmultipath.version | 10 +-
> libmultipath/pgpolicies.c | 42 ++++---
> libmultipath/pgpolicies.h | 6 +-
> libmultipath/print.c | 9 ++
> libmultipath/prioritizers/alua.c | 1 +
> libmultipath/propsel.c | 50 +++++++-
> libmultipath/propsel.h | 1 +
> libmultipath/structs.c | 1 +
> libmultipath/structs.h | 10 ++
> multipath/main.c | 1 +
> multipath/multipath.conf.5 | 16 +++
> tests/pgpolicy.c | 201
> ++++++++++++++++++++++++++++++
> 19 files changed, 338 insertions(+), 35 deletions(-)
>
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [dm-devel] [PATCH 1/5] libmultipath: add group_by_tpg path_grouping_policy
2023-05-31 15:19 ` Martin Wilck
@ 2023-06-01 17:47 ` Benjamin Marzinski
0 siblings, 0 replies; 18+ messages in thread
From: Benjamin Marzinski @ 2023-06-01 17:47 UTC (permalink / raw)
To: Martin Wilck; +Cc: dm-devel@redhat.com
On Wed, May 31, 2023 at 03:19:55PM +0000, Martin Wilck wrote:
> On Fri, 2023-05-19 at 18:02 -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;
>
> In view of the discussion from the cover letter:
>
> tpg_id should have already been set by detect_alua(). I'm not sure if
> it's good to overwrite it silently here. Shouldn't we rather check
> if the value is unchanged, or refrain from setting it more than once?
We can certainly log a message here, but like you said in your reply to
my cover letter, it does appear possible the a path can change the
Target Port Group it belongs to. In this case, we should update the
tpg_id if we notice the change, so the path gets placed in correct
group. With my second patchset, which makes multipath do more to update
the priorities of all the paths in non-group-by-prio setups, this should
mostly keep the paths in the correct pathgroup, if they change TPG.
Obviously the paths that are down won't get updated, but that's no worse
than we have now, and changing TPGs is a corner case.
>
> > 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;
> > + }
>
> I don't understand this logic. Why don't you simply check pp->tpgs?
The call to verify_alua_prio is because the prio is set per path, and it
is possible that not all paths have the ALUA prioritizer. Regardless of
how that occured (and I admit that it's really only possible due to a
failure while trying to get the tpg), we can't really group the paths by
tpg if not all the paths have it set.
As for why verfy_alua_prio() doesn't check pp->tpgs, there are multiple
cases where a path that is configured with alua can fail detect_prio()
with what appears to be a temporary error. In that case the tpgs will
remain TPGS_UNDEF. So if pp->tgpgs == UNDEF, the code would still need
to check the prio name to see if the path is configured with ALUA. On
the other hand, if we check the prio name first, then that's the
definitive answer, and it doesn't matter if p->tpgs wasn't set yet.
> > 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [dm-devel] [PATCH 4/5] multipath-tools tests: add tests for group_by_tpg policy
2023-05-31 15:30 ` Martin Wilck
@ 2023-06-01 17:51 ` Benjamin Marzinski
2023-06-02 8:49 ` Martin Wilck
0 siblings, 1 reply; 18+ messages in thread
From: Benjamin Marzinski @ 2023-06-01 17:51 UTC (permalink / raw)
To: Martin Wilck; +Cc: dm-devel@redhat.com
On Wed, May 31, 2023 at 03:30:02PM +0000, Martin Wilck wrote:
> On Fri, 2023-05-19 at 18:02 -0500, Benjamin Marzinski wrote:
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
>
> I wonder if it might make sense for group_by_tpg to mock calls to
> getprio (assigning the prio from the path's TPG ID) rather than calling
> set_priority() directly.
It seems like it should be. I just continued with what the existing tests were
already doing. Can improving these tests happen outside of this patchset?
-Ben
>
>
> > ---
> > 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),
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [dm-devel] [PATCH 5/5] libmultipath: add "detect_pgpolicy" config option
2023-05-31 15:44 ` Martin Wilck
@ 2023-06-01 18:17 ` Benjamin Marzinski
2023-06-02 16:16 ` Martin Wilck
0 siblings, 1 reply; 18+ messages in thread
From: Benjamin Marzinski @ 2023-06-01 18:17 UTC (permalink / raw)
To: Martin Wilck; +Cc: dm-devel@redhat.com
On Wed, May 31, 2023 at 03:44:58PM +0000, Martin Wilck wrote:
> On Fri, 2023-05-19 at 18:02 -0500, Benjamin Marzinski wrote:
> > This allows configuations to use "group_by_tpg" if alua is
> > autodetected
> > and another policy if it isn't, so they can work with detect_prio.
>
> This is a bit confusing. We might have introduced this kind of
> autodetection without group_by_tpg; using group_by_prio for arrays with
> ALUA support would have made quite a bit of sense.
I guess that all depends on what the autodetection is for. If the goal
for ALUA autodetection is to make it possible to write configs that
support arrays which have an optional ALUA mode, then I don't think this
is necessary. All those arrays should be configured with group_by_prio,
regardless of whether or not they are in ALUA mode.
But we've moved more towards adding autodetection to make multipath work
correctly, even without a config for a specific array. In this case,
yes, if we autodetect ALUA, if would be nice if we could
automatically set group_by_prio.
> What this patch really does is to make multipath-tools prefer
> group_by_tpg over group_by_prio if it finds that ALUA is supported.
> Should this be a separate option, perhaps?
>
> - detect_pgpolicy: use an ALUA-based pgpolicy if available
> - detect_pgpolicy_prefer_tpg: prefer group_by_tpg over group_by_prio
> for arrays supporting ALUA.
>
> This way users could benefit from ALUA autodetection without switching
> to the TPG algorithm automatically.
Sure. Lets go with that. I'll respin this.
> Or do we have good arguments that group_by_tpg is always "better" than
> group_by_prio if ALUA is supported? I guess it might be, but it still
> needs to prove its usefulness it practice.
I would also rather it proved itself first. That's why I had it disabled
by default. We can always switch the default later.
> Also, if we add the auto-detection feature, I think it should default
> to ON, at least upstream.
I don't know of any case where you would need FAILOVER, when you have an
ALUA device. I can imagine someone wanting to be able to turn off
load-balancing, but I think it makes sense to enable this by default
upstream.
>
> Regards,
> Martin
>
> >
> > 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 | 23 ++++++++++++++++++++++-
> > libmultipath/propsel.h | 1 +
> > libmultipath/structs.h | 7 +++++++
> > multipath/multipath.conf.5 | 12 ++++++++++++
> > 11 files changed, 63 insertions(+), 8 deletions(-)
> >
> > 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..090baa5c 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_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 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 65bca744..803230c1 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_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 d214281b..12f4825d 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,13 +290,19 @@ 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_TPG;
> > + origin = autodetect_origin;
> > + goto out;
> > + }
> > mp_set_mpe(pgpolicy);
> > mp_set_ovr(pgpolicy);
> > mp_set_hwe(pgpolicy);
> > 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 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..9f8be510 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_tpg\fR path_grouping_policy. If set to \fIno\fR,
> > the
> > +path_grouping_policy will be selected as usual.
> > +.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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [dm-devel] [PATCH 4/5] multipath-tools tests: add tests for group_by_tpg policy
2023-06-01 17:51 ` Benjamin Marzinski
@ 2023-06-02 8:49 ` Martin Wilck
0 siblings, 0 replies; 18+ messages in thread
From: Martin Wilck @ 2023-06-02 8:49 UTC (permalink / raw)
To: bmarzins@redhat.com; +Cc: dm-devel@redhat.com
On Thu, 2023-06-01 at 12:51 -0500, Benjamin Marzinski wrote:
> On Wed, May 31, 2023 at 03:30:02PM +0000, Martin Wilck wrote:
> > On Fri, 2023-05-19 at 18:02 -0500, Benjamin Marzinski wrote:
> > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> >
> > I wonder if it might make sense for group_by_tpg to mock calls to
> > getprio (assigning the prio from the path's TPG ID) rather than
> > calling
> > set_priority() directly.
>
> It seems like it should be. I just continued with what the existing
> tests were
> already doing. Can improving these tests happen outside of this
> patchset?
>
Yes.
Martin
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [dm-devel] [PATCH 5/5] libmultipath: add "detect_pgpolicy" config option
2023-06-01 18:17 ` Benjamin Marzinski
@ 2023-06-02 16:16 ` Martin Wilck
2023-06-05 14:14 ` Benjamin Marzinski
0 siblings, 1 reply; 18+ messages in thread
From: Martin Wilck @ 2023-06-02 16:16 UTC (permalink / raw)
To: Benjamin Marzinski, Xose Vazquez Perez; +Cc: dm-devel@redhat.com
On Thu, 2023-06-01 at 13:17 -0500, Benjamin Marzinski wrote:
> On Wed, May 31, 2023 at 03:44:58PM +0000, Martin Wilck wrote:
> > On Fri, 2023-05-19 at 18:02 -0500, Benjamin Marzinski wrote:
> > > This allows configuations to use "group_by_tpg" if alua is
> > > autodetected
> > > and another policy if it isn't, so they can work with
> > > detect_prio.
> >
> > This is a bit confusing. We might have introduced this kind of
> > autodetection without group_by_tpg; using group_by_prio for arrays
> > with
> > ALUA support would have made quite a bit of sense.
>
> I guess that all depends on what the autodetection is for. If the
> goal
> for ALUA autodetection is to make it possible to write configs that
> support arrays which have an optional ALUA mode, then I don't think
> this
> is necessary. All those arrays should be configured with
> group_by_prio,
> regardless of whether or not they are in ALUA mode.
Hm, but are they, really? Xose has changed some defaults from multibus
to group_by_prio lately, but I am not sure if that covers all. If we
set group_by_prio automatically without ALUA, we'd probably end up
effectively with MULTIBUS always, as the prio would likely be constant.
But some arrays would prefer FAILOVER, I suppose, perhaps even some of
those with optional ALUA? I am not sure. But this is what really
matters: whether the array should work in active-active or active-
passive mode when there is no ALUA to detect it.
> But we've moved more towards adding autodetection to make multipath
> work
> correctly, even without a config for a specific array. In this case,
> yes, if we autodetect ALUA, if would be nice if we could
> automatically set group_by_prio.
>
> > What this patch really does is to make multipath-tools prefer
> > group_by_tpg over group_by_prio if it finds that ALUA is
> > supported.
> > Should this be a separate option, perhaps?
> >
> > - detect_pgpolicy: use an ALUA-based pgpolicy if available
> > - detect_pgpolicy_prefer_tpg: prefer group_by_tpg over
> > group_by_prio
> > for arrays supporting ALUA.
> >
> > This way users could benefit from ALUA autodetection without
> > switching
> > to the TPG algorithm automatically.
>
> Sure. Lets go with that. I'll respin this.
Perhaps you can come up with a better name than
"detect_pgpolicy_prefer_tpg" :-)
To make sure we're on the same boat:
- detect_pgpolicy defaults to ON
- detect_pgpolicy_prefer_tpg defaults to OFF for now.
Right?
> > Or do we have good arguments that group_by_tpg is always "better"
> > than
> > group_by_prio if ALUA is supported? I guess it might be, but it
> > still
> > needs to prove its usefulness it practice.
>
> I would also rather it proved itself first. That's why I had it
> disabled
> by default. We can always switch the default later.
>
> > Also, if we add the auto-detection feature, I think it should
> > default
> > to ON, at least upstream.
>
> I don't know of any case where you would need FAILOVER, when you have
> an
> ALUA device. I can imagine someone wanting to be able to turn off
> load-balancing, but I think it makes sense to enable this by default
> upstream.
I expect that active-passive arrays would use STANDBY state for the
passive side, or at least NON-OPTIMIZED. This would effectively be a
failover mode. Anyway, it'll be possible to deactivate the
autodetection. That's kind of awkward for users, as we now from
detect_checker etc., but it works, and fits the way we did this for
other options.
Regards
Martin
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [dm-devel] [PATCH 5/5] libmultipath: add "detect_pgpolicy" config option
2023-06-02 16:16 ` Martin Wilck
@ 2023-06-05 14:14 ` Benjamin Marzinski
0 siblings, 0 replies; 18+ messages in thread
From: Benjamin Marzinski @ 2023-06-05 14:14 UTC (permalink / raw)
To: Martin Wilck; +Cc: dm-devel@redhat.com, Xose Vazquez Perez
On Fri, Jun 02, 2023 at 06:16:08PM +0200, Martin Wilck wrote:
> On Thu, 2023-06-01 at 13:17 -0500, Benjamin Marzinski wrote:
> > On Wed, May 31, 2023 at 03:44:58PM +0000, Martin Wilck wrote:
> > > On Fri, 2023-05-19 at 18:02 -0500, Benjamin Marzinski wrote:
> > > > This allows configuations to use "group_by_tpg" if alua is
> > > > autodetected
> > > > and another policy if it isn't, so they can work with
> > > > detect_prio.
> > >
> > > This is a bit confusing. We might have introduced this kind of
> > > autodetection without group_by_tpg; using group_by_prio for arrays
> > > with
> > > ALUA support would have made quite a bit of sense.
> >
> > I guess that all depends on what the autodetection is for. If the
> > goal
> > for ALUA autodetection is to make it possible to write configs that
> > support arrays which have an optional ALUA mode, then I don't think
> > this
> > is necessary. All those arrays should be configured with
> > group_by_prio,
> > regardless of whether or not they are in ALUA mode.
>
> Hm, but are they, really? Xose has changed some defaults from multibus
> to group_by_prio lately, but I am not sure if that covers all. If we
> set group_by_prio automatically without ALUA, we'd probably end up
> effectively with MULTIBUS always, as the prio would likely be constant.
> But some arrays would prefer FAILOVER, I suppose, perhaps even some of
> those with optional ALUA? I am not sure. But this is what really
> matters: whether the array should work in active-active or active-
> passive mode when there is no ALUA to detect it.
There are no builtin device configs that set path grouping policy to
FAILOVER. Out of all our builtin configs, the only SCSI one that doesn't
specifically set the path grouping policy is the IBM 3303 NVDISK and I
don't believe that one supports ALUA. I don't know of any vendor who
wants to have a builtin device config for their array, but can't write
an optimal one because they need a different pgpolicy depending on
whether ALUA is or isn't present. So all I was saying is that I don't
think having detect_pgplicy would enable us to add new builtin configs
for arrays that we couldn't handle correctly in all cases before.
I do agree that there are likely people who don't bother to edit
multipath.conf for their device, and multipath autodetects alua but
still uses FAILOVER, when it would be better for it to use
GROUP_BY_PRIO. That's why I think your proposal is a good one.
> > But we've moved more towards adding autodetection to make multipath
> > work
> > correctly, even without a config for a specific array. In this case,
> > yes, if we autodetect ALUA, if would be nice if we could
> > automatically set group_by_prio.
> >
> > > What this patch really does is to make multipath-tools prefer
> > > group_by_tpg over group_by_prio if it finds that ALUA is
> > > supported.
> > > Should this be a separate option, perhaps?
> > >
> > > - detect_pgpolicy: use an ALUA-based pgpolicy if available
> > > - detect_pgpolicy_prefer_tpg: prefer group_by_tpg over
> > > group_by_prio
> > > for arrays supporting ALUA.
> > >
> > > This way users could benefit from ALUA autodetection without
> > > switching
> > > to the TPG algorithm automatically.
> >
> > Sure. Lets go with that. I'll respin this.
>
> Perhaps you can come up with a better name than
> "detect_pgpolicy_prefer_tpg" :-)
>
> To make sure we're on the same boat:
>
> - detect_pgpolicy defaults to ON
> - detect_pgpolicy_prefer_tpg defaults to OFF for now.
>
> Right?
>
Right.
> > > Or do we have good arguments that group_by_tpg is always "better"
> > > than
> > > group_by_prio if ALUA is supported? I guess it might be, but it
> > > still
> > > needs to prove its usefulness it practice.
> >
> > I would also rather it proved itself first. That's why I had it
> > disabled
> > by default. We can always switch the default later.
> >
> > > Also, if we add the auto-detection feature, I think it should
> > > default
> > > to ON, at least upstream.
> >
> > I don't know of any case where you would need FAILOVER, when you have
> > an
> > ALUA device. I can imagine someone wanting to be able to turn off
> > load-balancing, but I think it makes sense to enable this by default
> > upstream.
>
> I expect that active-passive arrays would use STANDBY state for the
> passive side, or at least NON-OPTIMIZED. This would effectively be a
> failover mode. Anyway, it'll be possible to deactivate the
> autodetection. That's kind of awkward for users, as we now from
> detect_checker etc., but it works, and fits the way we did this for
> other options.
>
> Regards
> Martin
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2023-06-05 14:15 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-19 23:02 [dm-devel] [PATCH 0/5] multipath: Add a group_by_tgp path grouping policy Benjamin Marzinski
2023-05-19 23:02 ` [dm-devel] [PATCH 1/5] libmultipath: add group_by_tpg path_grouping_policy Benjamin Marzinski
2023-05-31 15:19 ` Martin Wilck
2023-06-01 17:47 ` Benjamin Marzinski
2023-05-19 23:02 ` [dm-devel] [PATCH 2/5] libmultipath: don't copy pgpolicy string in get_pgpolicy_name Benjamin Marzinski
2023-05-31 15:19 ` Martin Wilck
2023-05-19 23:02 ` [dm-devel] [PATCH 3/5] libmultipath: add ALUA tpg path wildcard Benjamin Marzinski
2023-05-31 15:21 ` Martin Wilck
2023-05-19 23:02 ` [dm-devel] [PATCH 4/5] multipath-tools tests: add tests for group_by_tpg policy Benjamin Marzinski
2023-05-31 15:30 ` Martin Wilck
2023-06-01 17:51 ` Benjamin Marzinski
2023-06-02 8:49 ` Martin Wilck
2023-05-19 23:02 ` [dm-devel] [PATCH 5/5] libmultipath: add "detect_pgpolicy" config option Benjamin Marzinski
2023-05-31 15:44 ` Martin Wilck
2023-06-01 18:17 ` Benjamin Marzinski
2023-06-02 16:16 ` Martin Wilck
2023-06-05 14:14 ` Benjamin Marzinski
2023-05-31 15:45 ` [dm-devel] [PATCH 0/5] multipath: Add a group_by_tgp path grouping policy Martin Wilck
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).