* [PATCH 01/16] libmultipath: make vector_foreach_slot_backwards work as expected
2019-08-02 16:33 [PATCH 00/16] multipath marginal pathgroups Benjamin Marzinski
@ 2019-08-02 16:33 ` Benjamin Marzinski
2019-08-14 21:35 ` Martin Wilck
2019-08-02 16:33 ` [PATCH 02/16] libmultipath: add marginal paths and groups infrastructure Benjamin Marzinski
` (14 subsequent siblings)
15 siblings, 1 reply; 41+ messages in thread
From: Benjamin Marzinski @ 2019-08-02 16:33 UTC (permalink / raw)
To: Christophe Varoqui
Cc: device-mapper development, Martin Wilck, Muneendra Kumar
All of the code that uses vector_foreach_slot_backwards() treats "i" as
the index of the entry "p", but the way it was coded, that wasn't the
case. "i" was the number of the entry counting from 1, not 0.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/vector.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libmultipath/vector.h b/libmultipath/vector.h
index 41d2b896..344dffd5 100644
--- a/libmultipath/vector.h
+++ b/libmultipath/vector.h
@@ -40,7 +40,7 @@ typedef struct _vector *vector;
#define vector_foreach_slot_after(v,p,i) \
for (; (v) && i < VECTOR_SIZE(v) && ((p) = (v)->slot[i]); i++)
#define vector_foreach_slot_backwards(v,p,i) \
- for (i = VECTOR_SIZE(v); i > 0 && ((p) = (v)->slot[i-1]); i--)
+ for (i = VECTOR_SIZE(v) - 1; (int)i >= 0 && ((p) = (v)->slot[i]); i--)
#define identity(x) (x)
/*
--
2.17.2
^ permalink raw reply related [flat|nested] 41+ messages in thread* [PATCH 02/16] libmultipath: add marginal paths and groups infrastructure
2019-08-02 16:33 [PATCH 00/16] multipath marginal pathgroups Benjamin Marzinski
2019-08-02 16:33 ` [PATCH 01/16] libmultipath: make vector_foreach_slot_backwards work as expected Benjamin Marzinski
@ 2019-08-02 16:33 ` Benjamin Marzinski
2019-08-14 21:37 ` Martin Wilck
2019-08-02 16:33 ` [PATCH 03/16] tests: add path grouping policy unit tests Benjamin Marzinski
` (13 subsequent siblings)
15 siblings, 1 reply; 41+ messages in thread
From: Benjamin Marzinski @ 2019-08-02 16:33 UTC (permalink / raw)
To: Christophe Varoqui
Cc: device-mapper development, Martin Wilck, Muneendra Kumar
This commit adds a marginal variable ot the paths and pathgroups structs.
The marginal paths variable can be set by
multipathd path <path> setmarginal
and cleared by
multipathd path <path> unsetmarginal
All of the marginal paths on a multipath device can be cleared by
multipathd map <map> unsetmarginal
Currently, the marginal variable of a pathgroup will not change. This
will be added by a future commit. The marginal state of a path or
pathgroup is printable with the %M wildcard, and is displayed in the
json output.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/print.c | 18 ++++++++
libmultipath/print.h | 6 ++-
libmultipath/structs.h | 2 +
multipathd/cli.c | 5 +++
multipathd/cli.h | 4 ++
multipathd/cli_handlers.c | 91 +++++++++++++++++++++++++++++++++++++++
multipathd/cli_handlers.h | 3 ++
multipathd/main.c | 3 ++
8 files changed, 130 insertions(+), 2 deletions(-)
diff --git a/libmultipath/print.c b/libmultipath/print.c
index fc40b0f0..907469ad 100644
--- a/libmultipath/print.c
+++ b/libmultipath/print.c
@@ -502,6 +502,14 @@ snprint_pg_state (char * buff, size_t len, const struct pathgroup * pgp)
}
}
+static int
+snprint_pg_marginal (char * buff, size_t len, const struct pathgroup * pgp)
+{
+ if (pgp->marginal)
+ return snprintf(buff, len, "marginal");
+ return snprintf(buff, len, "normal");
+}
+
static int
snprint_path_size (char * buff, size_t len, const struct path * pp)
{
@@ -672,6 +680,14 @@ snprint_path_protocol(char * buff, size_t len, const struct path * pp)
}
}
+int
+snprint_path_marginal(char * buff, size_t len, const struct path * pp)
+{
+ if (pp->marginal)
+ return snprintf(buff, len, "marginal");
+ return snprintf(buff, len, "normal");
+}
+
struct multipath_data mpd[] = {
{'n', "name", 0, snprint_name},
{'w', "uuid", 0, snprint_multipath_uuid},
@@ -713,6 +729,7 @@ struct path_data pd[] = {
{'p', "pri", 0, snprint_pri},
{'S', "size", 0, snprint_path_size},
{'z', "serial", 0, snprint_path_serial},
+ {'M', "marginal_st", 0, snprint_path_marginal},
{'m', "multipath", 0, snprint_path_mpp},
{'N', "host WWNN", 0, snprint_host_wwnn},
{'n', "target WWNN", 0, snprint_tgt_wwnn},
@@ -729,6 +746,7 @@ struct pathgroup_data pgd[] = {
{'s', "selector", 0, snprint_pg_selector},
{'p', "pri", 0, snprint_pg_pri},
{'t', "dm_st", 0, snprint_pg_state},
+ {'M', "marginal_st", 0, snprint_pg_marginal},
{0, NULL, 0 , NULL}
};
diff --git a/libmultipath/print.h b/libmultipath/print.h
index e2fb865c..7e36ec63 100644
--- a/libmultipath/print.h
+++ b/libmultipath/print.h
@@ -50,7 +50,8 @@
#define PRINT_JSON_GROUP "{\n" \
" \"selector\" : \"%s\",\n" \
" \"pri\" : %p,\n" \
- " \"dm_st\" : \"%t\","
+ " \"dm_st\" : \"%t\",\n" \
+ " \"marginal_st\" : \"%M\","
#define PRINT_JSON_GROUP_NUM " \"group\" : %d,\n"
@@ -66,7 +67,8 @@
" \"target_wwnn\" : \"%n\",\n" \
" \"host_wwpn\" : \"%R\",\n" \
" \"target_wwpn\" : \"%r\",\n" \
- " \"host_adapter\" : \"%a\""
+ " \"host_adapter\" : \"%a\",\n" \
+ " \"marginal_st\" : \"%M\""
#define MAX_LINE_LEN 80
#define MAX_LINES 64
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 7879d763..1a3d827b 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -289,6 +289,7 @@ struct path {
int io_err_pathfail_cnt;
int io_err_pathfail_starttime;
int find_multipaths_timeout;
+ int marginal;
/* configlet pointers */
vector hwe;
struct gen_path generic_path;
@@ -403,6 +404,7 @@ struct pathgroup {
int status;
int priority;
int enabled_paths;
+ int marginal;
vector paths;
struct multipath *mpp;
struct gen_pathgroup generic_pg;
diff --git a/multipathd/cli.c b/multipathd/cli.c
index 17795b61..800c0fbe 100644
--- a/multipathd/cli.c
+++ b/multipathd/cli.c
@@ -215,6 +215,8 @@ load_keys (void)
r += add_key(keys, "unsetprkey", UNSETPRKEY, 0);
r += add_key(keys, "key", KEY, 1);
r += add_key(keys, "local", LOCAL, 0);
+ r += add_key(keys, "setmarginal", SETMARGINAL, 0);
+ r += add_key(keys, "unsetmarginal", UNSETMARGINAL, 0);
if (r) {
@@ -589,6 +591,9 @@ cli_init (void) {
add_handler(UNSETPRKEY+MAP, NULL);
add_handler(FORCEQ+DAEMON, NULL);
add_handler(RESTOREQ+DAEMON, NULL);
+ add_handler(SETMARGINAL+PATH, NULL);
+ add_handler(UNSETMARGINAL+PATH, NULL);
+ add_handler(UNSETMARGINAL+MAP, NULL);
return 0;
}
diff --git a/multipathd/cli.h b/multipathd/cli.h
index 32dcffac..fdfb9aed 100644
--- a/multipathd/cli.h
+++ b/multipathd/cli.h
@@ -45,6 +45,8 @@ enum {
__UNSETPRKEY,
__KEY,
__LOCAL,
+ __SETMARGINAL,
+ __UNSETMARGINAL,
};
#define LIST (1 << __LIST)
@@ -89,6 +91,8 @@ enum {
#define UNSETPRKEY (1ULL << __UNSETPRKEY)
#define KEY (1ULL << __KEY)
#define LOCAL (1ULL << __LOCAL)
+#define SETMARGINAL (1ULL << __SETMARGINAL)
+#define UNSETMARGINAL (1ULL << __UNSETMARGINAL)
#define INITIAL_REPLY_LEN 1200
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 4c32d953..8a899049 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -1537,3 +1537,94 @@ cli_setprkey(void * v, char ** reply, int * len, void * data)
return ret;
}
+
+int cli_set_marginal(void * v, char ** reply, int * len, void * data)
+{
+ struct vectors * vecs = (struct vectors *)data;
+ char * param = get_keyparam(v, PATH);
+ struct path * pp;
+
+ param = convert_dev(param, 1);
+ pp = find_path_by_dev(vecs->pathvec, param);
+
+ if (!pp)
+ pp = find_path_by_devt(vecs->pathvec, param);
+
+ if (!pp || !pp->mpp || !pp->mpp->alias)
+ return 1;
+
+ condlog(2, "%s: set marginal path %s (operator)",
+ pp->mpp->alias, pp->dev_t);
+ if (pp->mpp->wait_for_udev) {
+ condlog(2, "%s: device not fully created, failing set marginal",
+ pp->mpp->alias);
+ return 1;
+ }
+ pp->marginal = 1;
+
+ return update_path_groups(pp->mpp, vecs, 0);
+}
+
+int cli_unset_marginal(void * v, char ** reply, int * len, void * data)
+{
+ struct vectors * vecs = (struct vectors *)data;
+ char * param = get_keyparam(v, PATH);
+ struct path * pp;
+
+ param = convert_dev(param, 1);
+ pp = find_path_by_dev(vecs->pathvec, param);
+
+ if (!pp)
+ pp = find_path_by_devt(vecs->pathvec, param);
+
+ if (!pp || !pp->mpp || !pp->mpp->alias)
+ return 1;
+
+ condlog(2, "%s: unset marginal path %s (operator)",
+ pp->mpp->alias, pp->dev_t);
+ if (pp->mpp->wait_for_udev) {
+ condlog(2, "%s: device not fully created, "
+ "failing unset marginal", pp->mpp->alias);
+ return 1;
+ }
+ pp->marginal = 0;
+
+ return update_path_groups(pp->mpp, vecs, 0);
+}
+
+int cli_unset_all_marginal(void * v, char ** reply, int * len, void * data)
+{
+ struct vectors * vecs = (struct vectors *)data;
+ char * mapname = get_keyparam(v, MAP);
+ struct multipath *mpp;
+ struct pathgroup * pgp;
+ struct path * pp;
+ unsigned int i, j;
+ int minor;
+
+ mapname = convert_dev(mapname, 0);
+ condlog(2, "%s: unset all marginal paths (operator)",
+ mapname);
+
+ if (sscanf(mapname, "dm-%d", &minor) == 1)
+ mpp = find_mp_by_minor(vecs->mpvec, minor);
+ else
+ mpp = find_mp_by_alias(vecs->mpvec, mapname);
+
+ if (!mpp) {
+ condlog(0, "%s: invalid map name. "
+ "cannot unset marginal paths", mapname);
+ return 1;
+ }
+ if (mpp->wait_for_udev) {
+ condlog(2, "%s: device not fully created, "
+ "failing unset all marginal", mpp->alias);
+ return 1;
+ }
+
+ vector_foreach_slot (mpp->pg, pgp, i)
+ vector_foreach_slot (pgp->paths, pp, j)
+ pp->marginal = 0;
+
+ return update_path_groups(mpp, vecs, 0);
+}
diff --git a/multipathd/cli_handlers.h b/multipathd/cli_handlers.h
index edbdf063..0f451064 100644
--- a/multipathd/cli_handlers.h
+++ b/multipathd/cli_handlers.h
@@ -49,3 +49,6 @@ int cli_unsetprstatus(void * v, char ** reply, int * len, void * data);
int cli_getprkey(void * v, char ** reply, int * len, void * data);
int cli_setprkey(void * v, char ** reply, int * len, void * data);
int cli_unsetprkey(void * v, char ** reply, int * len, void * data);
+int cli_set_marginal(void * v, char ** reply, int * len, void * data);
+int cli_unset_marginal(void * v, char ** reply, int * len, void * data);
+int cli_unset_all_marginal(void * v, char ** reply, int * len, void * data);
diff --git a/multipathd/main.c b/multipathd/main.c
index 7a5cd115..7db15736 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1609,6 +1609,9 @@ uxlsnrloop (void * ap)
set_handler_callback(GETPRKEY+MAP, cli_getprkey);
set_handler_callback(SETPRKEY+MAP+KEY, cli_setprkey);
set_handler_callback(UNSETPRKEY+MAP, cli_unsetprkey);
+ set_handler_callback(SETMARGINAL+PATH, cli_set_marginal);
+ set_handler_callback(UNSETMARGINAL+PATH, cli_unset_marginal);
+ set_handler_callback(UNSETMARGINAL+MAP, cli_unset_all_marginal);
umask(077);
uxsock_listen(&uxsock_trigger, ux_sock, ap);
--
2.17.2
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [PATCH 02/16] libmultipath: add marginal paths and groups infrastructure
2019-08-02 16:33 ` [PATCH 02/16] libmultipath: add marginal paths and groups infrastructure Benjamin Marzinski
@ 2019-08-14 21:37 ` Martin Wilck
0 siblings, 0 replies; 41+ messages in thread
From: Martin Wilck @ 2019-08-14 21:37 UTC (permalink / raw)
To: bmarzins, christophe.varoqui; +Cc: dm-devel, mkumar
On Fri, 2019-08-02 at 11:33 -0500, Benjamin Marzinski wrote:
> This commit adds a marginal variable ot the paths and pathgroups
> structs.
> The marginal paths variable can be set by
>
> multipathd path <path> setmarginal
>
> and cleared by
>
> multipathd path <path> unsetmarginal
>
> All of the marginal paths on a multipath device can be cleared by
>
> multipathd map <map> unsetmarginal
>
> Currently, the marginal variable of a pathgroup will not change. This
> will be added by a future commit. The marginal state of a path or
> pathgroup is printable with the %M wildcard, and is displayed in the
> json output.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Martin Wilck <mwilck@suse.com>
(I'd like different semantics for these commands, but that doesn't
affect this patch).
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 03/16] tests: add path grouping policy unit tests.
2019-08-02 16:33 [PATCH 00/16] multipath marginal pathgroups Benjamin Marzinski
2019-08-02 16:33 ` [PATCH 01/16] libmultipath: make vector_foreach_slot_backwards work as expected Benjamin Marzinski
2019-08-02 16:33 ` [PATCH 02/16] libmultipath: add marginal paths and groups infrastructure Benjamin Marzinski
@ 2019-08-02 16:33 ` Benjamin Marzinski
2019-08-14 21:22 ` Martin Wilck
2019-08-14 21:38 ` Martin Wilck
2019-08-02 16:33 ` [PATCH 04/16] libmultipath: add wrapper function around pgpolicyfn Benjamin Marzinski
` (12 subsequent siblings)
15 siblings, 2 replies; 41+ messages in thread
From: Benjamin Marzinski @ 2019-08-02 16:33 UTC (permalink / raw)
To: Christophe Varoqui
Cc: device-mapper development, Martin Wilck, Muneendra Kumar
In preparation for changing the path grouping code, add some unit tests
to verify that it works correctly. The only test that currently fails
(and so it being skipped) is using MULTIBUS when mp->paths is empty. All
the other path grouping policies free mp->paths, even if it is empty.
one_group() should as well. This will be fixed when the path grouping
code is updated.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
tests/Makefile | 2 +-
tests/pgpolicy.c | 708 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 709 insertions(+), 1 deletion(-)
create mode 100644 tests/pgpolicy.c
diff --git a/tests/Makefile b/tests/Makefile
index bf159b2d..a5cdf390 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -3,7 +3,7 @@ include ../Makefile.inc
CFLAGS += $(BIN_CFLAGS) -I$(multipathdir) -I$(mpathcmddir)
LIBDEPS += -L$(multipathdir) -lmultipath -lcmocka
-TESTS := uevent parser util dmevents hwtable blacklist unaligned vpd
+TESTS := uevent parser util dmevents hwtable blacklist unaligned vpd pgpolicy
.SILENT: $(TESTS:%=%.o)
.PRECIOUS: $(TESTS:%=%-test)
diff --git a/tests/pgpolicy.c b/tests/pgpolicy.c
new file mode 100644
index 00000000..fbb8589e
--- /dev/null
+++ b/tests/pgpolicy.c
@@ -0,0 +1,708 @@
+/*
+ * Copyright (c) 2018 Benjamin Marzinski, Redhat
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <https://www.gnu.org/licenses/>.
+ *
+ */
+
+#include <stdint.h>
+#include <stdbool.h>
+#include <stdarg.h>
+#include <stddef.h>
+#include <setjmp.h>
+#include <stdlib.h>
+#include <cmocka.h>
+
+#include "globals.c"
+#include "pgpolicies.h"
+
+struct multipath mp8, mp4, mp1, mp0, mp_null;
+struct path p8[8], p4[4], p1[1];
+
+
+static void set_priority(struct path *pp, int *prio, int size)
+{
+ int i;
+
+ for (i = 0; i < size; i++) {
+ pp[i].priority = prio[i];
+ }
+}
+
+static void set_tgt_node_name(struct path *pp, char **tgt_node_name, int size)
+{
+ int i;
+
+ for (i = 0; i < size; i++) {
+ strcpy(pp[i].tgt_node_name, tgt_node_name[i]);
+ }
+}
+
+static void set_serial(struct path *pp, char **serial, int size)
+{
+ int i;
+
+ for (i = 0; i < size; i++) {
+ strcpy(pp[i].serial, serial[i]);
+ }
+}
+
+static int setup(void **state)
+{
+ int i;
+
+ for (i = 0; i < 8; i++) {
+ sprintf(p8[i].dev, "p8_%d", i);
+ sprintf(p8[i].dev_t, "8:%d", i);
+ p8[i].state = PATH_UP;
+ }
+ for (i = 0; i < 4; i++) {
+ sprintf(p4[i].dev, "p4_%d", i);
+ sprintf(p4[i].dev_t, "4:%d", i);
+ p4[i].state = PATH_UP;
+ }
+ sprintf(p1[0].dev, "p1_0");
+ sprintf(p1[0].dev_t, "4:0");
+ p1[0].state = PATH_UP;
+ return 0;
+}
+
+static int setupX(struct multipath *mp, struct path *pp, int size)
+{
+ int i;
+ int prio[8] = {10, 10, 10, 10, 10, 10, 10, 10};
+
+ mp->paths = vector_alloc();
+ if (!mp->paths)
+ return -1;
+ for (i = 0; i < size; i++) {
+ if (!vector_alloc_slot(mp->paths))
+ return -1;
+ vector_set_slot(mp->paths, &pp[i]);
+ }
+ set_priority(pp, prio, size);
+ return 0;
+}
+
+static int setup8(void **state)
+{
+ return setupX(&mp8, p8, 8);
+}
+
+static int setup4(void **state)
+{
+ return setupX(&mp4, p4, 4);
+}
+
+static int setup1(void **state)
+{
+ return setupX(&mp1, p1, 1);
+}
+
+static int setup0(void **state)
+{
+ return setupX(&mp0, NULL, 0);
+}
+
+static int setup_null(void **state)
+{
+ return 0;
+}
+
+static int teardownX(struct multipath *mp)
+{
+ free_pgvec(mp->pg, KEEP_PATHS);
+ mp->pg = NULL;
+ return 0;
+}
+
+static int teardown8(void **state)
+{
+ return teardownX(&mp8);
+}
+
+static int teardown4(void **state)
+{
+ return teardownX(&mp4);
+}
+
+static int teardown1(void **state)
+{
+ return teardownX(&mp1);
+}
+
+static int teardown0(void **state)
+{
+ return teardownX(&mp0);
+}
+
+static int teardown_null(void **state)
+{
+ return teardownX(&mp_null);
+}
+
+static void
+verify_pathgroups(struct multipath *mp, struct path *pp, int **groups,
+ int *group_size, int size)
+{
+ int i, j;
+ struct pathgroup *pgp;
+
+ assert_null(mp->paths);
+ assert_non_null(mp->pg);
+ assert_int_equal(VECTOR_SIZE(mp->pg), size);
+ for (i = 0; i < size; i++) {
+ pgp = VECTOR_SLOT(mp->pg, i);
+ assert_non_null(pgp);
+ assert_non_null(pgp->paths);
+ assert_int_equal(VECTOR_SIZE(pgp->paths), group_size[i]);
+ for (j = 0; j < group_size[i]; j++) {
+ int path_nr = groups[i][j];
+ struct path *pgp_path = VECTOR_SLOT(pgp->paths, j);
+ struct path *pp_path = &pp[path_nr];
+ /* Test names instead of pointers to get a more
+ * useful error message */
+ assert_string_equal(pgp_path->dev, pp_path->dev);
+ /* This test is just a backkup in case the
+ * something wenth wrong naming the paths */
+ assert_ptr_equal(pgp_path, pp_path);
+ }
+ }
+}
+
+static void test_one_group8(void **state)
+{
+ int paths[] = {0,1,2,3,4,5,6,7};
+ int *groups[] = {paths};
+ int group_size[] = {8};
+
+ assert_int_equal(one_group(&mp8), 0);
+ verify_pathgroups(&mp8, p8, groups, group_size, 1);
+}
+
+static void test_one_group4(void **state)
+{
+ int paths[] = {0,1,2,3};
+ int *groups[] = {paths};
+ int group_size[] = {4};
+
+ assert_int_equal(one_group(&mp4), 0);
+ verify_pathgroups(&mp4, p4, groups, group_size, 1);
+}
+
+static void test_one_group1(void **state)
+{
+ int paths[] = {0};
+ int *groups[] = {paths};
+ int group_size[] = {1};
+
+ assert_int_equal(one_group(&mp1), 0);
+ verify_pathgroups(&mp1, p1, groups, group_size, 1);
+}
+
+static void test_one_group0(void **state)
+{
+ assert_int_equal(one_group(&mp0), 0);
+ skip(); /* BROKEN */
+ verify_pathgroups(&mp0, NULL, NULL, NULL, 0);
+}
+
+static void test_one_group_null(void **state)
+{
+ assert_int_equal(one_group(&mp_null), 0);
+ verify_pathgroups(&mp_null, NULL, NULL, NULL, 0);
+}
+
+static void test_one_path_per_group_same8(void **state)
+{
+ int paths[] = {0,1,2,3,4,5,6,7};
+ 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};
+
+ assert_int_equal(one_path_per_group(&mp8), 0);
+ verify_pathgroups(&mp8, p8, groups, group_size, 8);
+}
+
+static void test_one_path_per_group_increasing8(void **state)
+{
+ int prio[] = {1,2,3,4,5,6,7,8};
+ 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);
+ assert_int_equal(one_path_per_group(&mp8), 0);
+ verify_pathgroups(&mp8, p8, groups, group_size, 8);
+}
+
+static void test_one_path_per_group_decreasing8(void **state)
+{
+ int prio[] = {8,7,6,5,4,3,2,1};
+ int paths[] = {0,1,2,3,4,5,6,7};
+ 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);
+ assert_int_equal(one_path_per_group(&mp8), 0);
+ verify_pathgroups(&mp8, p8, groups, group_size, 8);
+}
+
+static void test_one_path_per_group_mixed8(void **state)
+{
+ int prio[] = {7,1,3,3,5,2,8,2};
+ int paths[] = {6,0,4,2,3,5,7,1};
+ 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);
+ assert_int_equal(one_path_per_group(&mp8), 0);
+ verify_pathgroups(&mp8, p8, groups, group_size, 8);
+}
+
+static void test_one_path_per_group4(void **state)
+{
+ int paths[] = {0,1,2,3};
+ int *groups[] = {&paths[0], &paths[1], &paths[2], &paths[3]};
+ int group_size[] = {1,1,1,1};
+
+ assert_int_equal(one_path_per_group(&mp4), 0);
+ verify_pathgroups(&mp4, p4, groups, group_size, 4);
+}
+
+static void test_one_path_per_group1(void **state)
+{
+ int paths[] = {0};
+ int *groups[] = {paths};
+ int group_size[] = {1};
+
+ assert_int_equal(one_path_per_group(&mp1), 0);
+ verify_pathgroups(&mp1, p1, groups, group_size, 1);
+}
+
+static void test_one_path_per_group0(void **state)
+{
+ assert_int_equal(one_path_per_group(&mp0), 0);
+ verify_pathgroups(&mp0, NULL, NULL, NULL, 0);
+}
+
+static void test_one_path_per_group_null(void **state)
+{
+ assert_int_equal(one_path_per_group(&mp_null), 0);
+ verify_pathgroups(&mp_null, NULL, NULL, NULL, 0);
+}
+
+static void test_group_by_prio_same8(void **state)
+{
+ int paths[] = {0,1,2,3,4,5,6,7};
+ int *groups[] = {paths};
+ int group_size[] = {8};
+
+ assert_int_equal(group_by_prio(&mp8), 0);
+ verify_pathgroups(&mp8, p8, groups, group_size, 1);
+}
+
+static void test_group_by_prio_increasing8(void **state)
+{
+ int prio[] = {1,2,3,4,5,6,7,8};
+ 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);
+ assert_int_equal(group_by_prio(&mp8), 0);
+ verify_pathgroups(&mp8, p8, groups, group_size, 8);
+}
+
+static void test_group_by_prio_decreasing8(void **state)
+{
+ int prio[] = {8,7,6,5,4,3,2,1};
+ int paths[] = {0,1,2,3,4,5,6,7};
+ 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);
+ assert_int_equal(group_by_prio(&mp8), 0);
+ verify_pathgroups(&mp8, p8, groups, group_size, 8);
+}
+
+static void test_group_by_prio_mixed8(void **state)
+{
+ int prio[] = {7,1,3,3,5,2,8,2};
+ int group0[] = {6};
+ int group1[] = {0};
+ int group2[] = {4};
+ int group3[] = {2,3};
+ int group4[] = {5,7};
+ int group5[] = {1};
+ int *groups[] = {group0, group1, group2, group3,
+ group4, group5};
+ int group_size[] = {1,1,1,2,2,1};
+
+ set_priority(p8, prio, 8);
+ assert_int_equal(group_by_prio(&mp8), 0);
+ verify_pathgroups(&mp8, p8, groups, group_size, 6);
+}
+
+static void test_group_by_prio_2_groups8(void **state)
+{
+ int prio[] = {1,2,2,1,2,1,1,2};
+ int group0[] = {1,2,4,7};
+ int group1[] = {0,3,5,6};
+ int *groups[] = {group0, group1};
+ int group_size[] = {4,4};
+
+ set_priority(p8, prio, 8);
+ assert_int_equal(group_by_prio(&mp8), 0);
+ verify_pathgroups(&mp8, p8, groups, group_size, 2);
+}
+
+static void test_group_by_prio_mixed4(void **state)
+{
+ int prio[] = {2,3,1,3};
+ int group0[] = {1,3};
+ int group1[] = {0};
+ int group2[] = {2};
+ int *groups[] = {group0, group1, group2};
+ int group_size[] = {2,1,1};
+
+ set_priority(p4, prio, 4);
+ assert_int_equal(group_by_prio(&mp4), 0);
+ verify_pathgroups(&mp4, p4, groups, group_size, 3);
+}
+
+static void test_group_by_prio_2_groups4(void **state)
+{
+ int prio[] = {2,1,1,2};
+ int group0[] = {0,3};
+ int group1[] = {1,2};
+ int *groups[] = {group0, group1};
+ int group_size[] = {2,2};
+
+ set_priority(p4, prio, 4);
+ assert_int_equal(group_by_prio(&mp4), 0);
+ verify_pathgroups(&mp4, p4, groups, group_size, 2);
+}
+
+static void test_group_by_prio1(void **state)
+{
+ int paths[] = {0};
+ int *groups[] = {paths};
+ int group_size[] = {1};
+
+ assert_int_equal(group_by_prio(&mp1), 0);
+ verify_pathgroups(&mp1, p1, groups, group_size, 1);
+}
+
+static void test_group_by_prio0(void **state)
+{
+ assert_int_equal(group_by_prio(&mp0), 0);
+ verify_pathgroups(&mp0, NULL, NULL, NULL, 0);
+}
+
+static void test_group_by_prio_null(void **state)
+{
+ assert_int_equal(group_by_prio(&mp_null), 0);
+ verify_pathgroups(&mp_null, NULL, NULL, NULL, 0);
+}
+
+static void test_group_by_node_name_same8(void **state)
+{
+ char *node_name[] = {"a","a","a","a","a","a","a","a"};
+ int paths[] = {0,1,2,3,4,5,6,7};
+ int *groups[] = {paths};
+ int group_size[] = {8};
+
+ set_tgt_node_name(p8, node_name, 8);
+ assert_int_equal(group_by_node_name(&mp8), 0);
+ verify_pathgroups(&mp8, p8, groups, group_size, 1);
+}
+
+static void test_group_by_node_name_increasing8(void **state)
+{
+ char *node_name[] = {"a","b","c","d","e","f","g","h"};
+ int prio[] = {1,2,3,4,5,6,7,8};
+ 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_tgt_node_name(p8, node_name, 8);
+ assert_int_equal(group_by_node_name(&mp8), 0);
+ verify_pathgroups(&mp8, p8, groups, group_size, 8);
+}
+
+static void test_group_by_node_name_3_groups8(void **state)
+{
+ char *node_name[] = {"a","b","a","c","b","c","c","a"};
+ int prio[] = {4,1,4,1,1,1,1,4};
+ int group0[] = {0,2,7};
+ int group1[] = {3,5,6};
+ int group2[] = {1,4};
+ int *groups[] = {group0, group1, group2};
+ int group_size[] = {3,3,2};
+
+ set_priority(p8, prio, 8);
+ set_tgt_node_name(p8, node_name, 8);
+ assert_int_equal(group_by_node_name(&mp8), 0);
+ verify_pathgroups(&mp8, p8, groups, group_size, 3);
+}
+
+static void test_group_by_node_name_2_groups8(void **state)
+{
+ char *node_name[] = {"a", "a", "b", "a", "b", "b", "b", "a"};
+ int prio[] = {4,1,2,1,2,2,2,1};
+ int group0[] = {2,4,5,6};
+ int group1[] = {0,1,3,7};
+ int *groups[] = {group0, group1};
+ int group_size[] = {4,4};
+
+ set_priority(p8, prio, 8);
+ set_tgt_node_name(p8, node_name, 8);
+ assert_int_equal(group_by_node_name(&mp8), 0);
+ verify_pathgroups(&mp8, p8, groups, group_size, 2);
+}
+
+static void test_group_by_node_name_3_groups4(void **state)
+{
+ char *node_name[] = {"a","b","c","a"};
+ int prio[] = {3,1,3,1};
+ int group0[] = {2};
+ int group1[] = {0,3};
+ int group2[] = {1};
+ int *groups[] = {group0, group1, group2};
+ int group_size[] = {1,2,1};
+
+ set_priority(p4, prio, 4);
+ set_tgt_node_name(p4, node_name, 4);
+ assert_int_equal(group_by_node_name(&mp4), 0);
+ verify_pathgroups(&mp4, p4, groups, group_size, 3);
+}
+
+static void test_group_by_node_name_2_groups4(void **state)
+{
+ char *node_name[] = {"a","b","b","a"};
+ int prio[] = {2,1,1,2};
+ int group0[] = {0,3};
+ int group1[] = {1,2};
+ int *groups[] = {group0, group1};
+ int group_size[] = {2,2};
+
+ set_priority(p4, prio, 4);
+ set_tgt_node_name(p4, node_name, 4);
+ assert_int_equal(group_by_node_name(&mp4), 0);
+ verify_pathgroups(&mp4, p4, groups, group_size, 2);
+}
+
+static void test_group_by_node_name1(void **state)
+{
+ char *node_name[] = {"a"};
+ int paths[] = {0};
+ int *groups[] = {paths};
+ int group_size[] = {1};
+
+ set_tgt_node_name(p1, node_name, 1);
+ assert_int_equal(group_by_node_name(&mp1), 0);
+ verify_pathgroups(&mp1, p1, groups, group_size, 1);
+}
+
+static void test_group_by_node_name0(void **state)
+{
+ assert_int_equal(group_by_node_name(&mp0), 0);
+ verify_pathgroups(&mp0, NULL, NULL, NULL, 0);
+}
+
+static void test_group_by_node_name_null(void **state)
+{
+ assert_int_equal(group_by_node_name(&mp_null), 0);
+ verify_pathgroups(&mp_null, NULL, NULL, NULL, 0);
+}
+
+static void test_group_by_serial_same8(void **state)
+{
+ char *serial[] = {"1","1","1","1","1","1","1","1"};
+ int paths[] = {0,1,2,3,4,5,6,7};
+ int *groups[] = {paths};
+ int group_size[] = {8};
+
+ set_serial(p8, serial, 8);
+ assert_int_equal(group_by_serial(&mp8), 0);
+ verify_pathgroups(&mp8, p8, groups, group_size, 1);
+}
+
+static void test_group_by_serial_increasing8(void **state)
+{
+ char *serial[] = {"1","2","3","4","5","6","7","8"};
+ int prio[] = {1,2,3,4,5,6,7,8};
+ 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_serial(p8, serial, 8);
+ assert_int_equal(group_by_serial(&mp8), 0);
+ verify_pathgroups(&mp8, p8, groups, group_size, 8);
+}
+
+static void test_group_by_serial_3_groups8(void **state)
+{
+ char *serial[] = {"1","2","1","3","2","3","2","1"};
+ int prio[] = {4,1,4,3,1,3,1,4};
+ int group0[] = {0,2,7};
+ int group1[] = {3,5};
+ int group2[] = {1,4,6};
+ int *groups[] = {group0, group1, group2};
+ int group_size[] = {3,2,3};
+
+ set_priority(p8, prio, 8);
+ set_serial(p8, serial, 8);
+ assert_int_equal(group_by_serial(&mp8), 0);
+ verify_pathgroups(&mp8, p8, groups, group_size, 3);
+}
+
+static void test_group_by_serial_2_groups8(void **state)
+{
+ char *serial[] = {"1", "2", "1", "1", "2", "2", "1", "2"};
+ int prio[] = {3,2,2,1,2,2,1,2};
+ int group0[] = {1,4,5,7};
+ int group1[] = {0,2,3,6};
+ int *groups[] = {group0, group1};
+ int group_size[] = {4,4};
+
+ set_priority(p8, prio, 8);
+ set_serial(p8, serial, 8);
+ assert_int_equal(group_by_serial(&mp8), 0);
+ verify_pathgroups(&mp8, p8, groups, group_size, 2);
+}
+
+static void test_group_by_serial_3_groups4(void **state)
+{
+ char *serial[] = {"1","2","3","2"};
+ int prio[] = {3,1,3,1};
+ int group0[] = {0};
+ int group1[] = {2};
+ int group2[] = {1,3};
+ int *groups[] = {group0, group1, group2};
+ int group_size[] = {1,1,2};
+
+ set_priority(p4, prio, 4);
+ set_serial(p4, serial, 4);
+ assert_int_equal(group_by_serial(&mp4), 0);
+ verify_pathgroups(&mp4, p4, groups, group_size, 3);
+}
+
+static void test_group_by_serial_2_groups4(void **state)
+{
+ char *serial[] = {"1","2","1","2"};
+ int prio[] = {3,1,3,1};
+ int group0[] = {0,2};
+ int group1[] = {1,3};
+ int *groups[] = {group0, group1};
+ int group_size[] = {2,2};
+
+ set_priority(p4, prio, 4);
+ set_serial(p4, serial, 4);
+ assert_int_equal(group_by_serial(&mp4), 0);
+ verify_pathgroups(&mp4, p4, groups, group_size, 2);
+}
+
+static void test_group_by_serial1(void **state)
+{
+ char *serial[1] = {"1"};
+ int paths[1] = {0};
+ int *groups[1] = {paths};
+ int group_size[1] = {1};
+
+ set_serial(p1, serial, 1);
+ assert_int_equal(group_by_serial(&mp1), 0);
+ verify_pathgroups(&mp1, p1, groups, group_size, 1);
+}
+
+static void test_group_by_serial0(void **state)
+{
+ assert_int_equal(group_by_serial(&mp0), 0);
+ verify_pathgroups(&mp0, NULL, NULL, NULL, 0);
+}
+
+static void test_group_by_serial_null(void **state)
+{
+ assert_int_equal(group_by_serial(&mp_null), 0);
+ verify_pathgroups(&mp_null, NULL, NULL, NULL, 0);
+}
+
+#define setup_test(name, nr) \
+cmocka_unit_test_setup_teardown(name ## nr, setup ## nr, teardown ## nr)
+
+int test_pgpolicies(void)
+{
+ const struct CMUnitTest tests[] = {
+ setup_test(test_one_group, 8),
+ setup_test(test_one_group, 4),
+ setup_test(test_one_group, 1),
+ setup_test(test_one_group, 0),
+ setup_test(test_one_group, _null),
+ setup_test(test_one_path_per_group_same, 8),
+ setup_test(test_one_path_per_group_increasing, 8),
+ setup_test(test_one_path_per_group_decreasing, 8),
+ setup_test(test_one_path_per_group_mixed, 8),
+ setup_test(test_one_path_per_group, 4),
+ setup_test(test_one_path_per_group, 1),
+ setup_test(test_one_path_per_group, 0),
+ setup_test(test_one_path_per_group, _null),
+ setup_test(test_group_by_prio_same, 8),
+ setup_test(test_group_by_prio_increasing, 8),
+ setup_test(test_group_by_prio_decreasing, 8),
+ setup_test(test_group_by_prio_mixed, 8),
+ setup_test(test_group_by_prio_2_groups, 8),
+ setup_test(test_group_by_prio_mixed, 4),
+ setup_test(test_group_by_prio_2_groups, 4),
+ setup_test(test_group_by_prio, 1),
+ setup_test(test_group_by_prio, 0),
+ setup_test(test_group_by_prio, _null),
+ 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),
+ setup_test(test_group_by_node_name_2_groups, 8),
+ setup_test(test_group_by_node_name_3_groups, 4),
+ setup_test(test_group_by_node_name_2_groups, 4),
+ setup_test(test_group_by_node_name, 1),
+ setup_test(test_group_by_node_name, 0),
+ setup_test(test_group_by_node_name, _null),
+ setup_test(test_group_by_serial_same, 8),
+ setup_test(test_group_by_serial_increasing, 8),
+ setup_test(test_group_by_serial_3_groups, 8),
+ setup_test(test_group_by_serial_2_groups, 8),
+ setup_test(test_group_by_serial_3_groups, 4),
+ setup_test(test_group_by_serial_2_groups, 4),
+ setup_test(test_group_by_serial, 1),
+ setup_test(test_group_by_serial, 0),
+ setup_test(test_group_by_serial, _null),
+ };
+ return cmocka_run_group_tests(tests, setup, NULL);
+}
+
+int main(void)
+{
+ int ret = 0;
+
+ ret += test_pgpolicies();
+ return ret;
+}
--
2.17.2
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [PATCH 03/16] tests: add path grouping policy unit tests.
2019-08-02 16:33 ` [PATCH 03/16] tests: add path grouping policy unit tests Benjamin Marzinski
@ 2019-08-14 21:22 ` Martin Wilck
2019-08-16 21:01 ` Benjamin Marzinski
2019-08-14 21:38 ` Martin Wilck
1 sibling, 1 reply; 41+ messages in thread
From: Martin Wilck @ 2019-08-14 21:22 UTC (permalink / raw)
To: bmarzins, christophe.varoqui; +Cc: dm-devel, mkumar
On Fri, 2019-08-02 at 11:33 -0500, Benjamin Marzinski wrote:
> In preparation for changing the path grouping code, add some unit
> tests
> to verify that it works correctly. The only test that currently fails
> (and so it being skipped) is using MULTIBUS when mp->paths is empty.
> All
> the other path grouping policies free mp->paths, even if it is empty.
> one_group() should as well. This will be fixed when the path grouping
> code is updated.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> tests/Makefile | 2 +-
> tests/pgpolicy.c | 708
> +++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 709 insertions(+), 1 deletion(-)
> create mode 100644 tests/pgpolicy.c
>
> +
> +static void test_group_by_node_name_3_groups4(void **state)
> +{
> + char *node_name[] = {"a","b","c","a"};
> + int prio[] = {3,1,3,1};
> + int group0[] = {2};
> + int group1[] = {0,3};
> + int group2[] = {1};
> + int *groups[] = {group0, group1, group2};
> + int group_size[] = {1,2,1};
> +
> + set_priority(p4, prio, 4);
> + set_tgt_node_name(p4, node_name, 4);
> + assert_int_equal(group_by_node_name(&mp4), 0);
> + verify_pathgroups(&mp4, p4, groups, group_size, 3);
> +}
Nothing wrong with your code, but this is one example where I would say
our prio group ordering is counter-intuitive. Does it really make sense
to order the group of two paths with prio {3, 1} *after* the group with
just one prio 3 path?
Regards,
Martin
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 03/16] tests: add path grouping policy unit tests.
2019-08-14 21:22 ` Martin Wilck
@ 2019-08-16 21:01 ` Benjamin Marzinski
2019-08-19 9:34 ` Martin Wilck
0 siblings, 1 reply; 41+ messages in thread
From: Benjamin Marzinski @ 2019-08-16 21:01 UTC (permalink / raw)
To: Martin Wilck; +Cc: dm-devel, mkumar
On Wed, Aug 14, 2019 at 09:22:17PM +0000, Martin Wilck wrote:
> On Fri, 2019-08-02 at 11:33 -0500, Benjamin Marzinski wrote:
> > In preparation for changing the path grouping code, add some unit
> > tests
> > to verify that it works correctly. The only test that currently fails
> > (and so it being skipped) is using MULTIBUS when mp->paths is empty.
> > All
> > the other path grouping policies free mp->paths, even if it is empty.
> > one_group() should as well. This will be fixed when the path grouping
> > code is updated.
> >
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> > tests/Makefile | 2 +-
> > tests/pgpolicy.c | 708
> > +++++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 709 insertions(+), 1 deletion(-)
> > create mode 100644 tests/pgpolicy.c
> >
> > +
> > +static void test_group_by_node_name_3_groups4(void **state)
> > +{
> > + char *node_name[] = {"a","b","c","a"};
> > + int prio[] = {3,1,3,1};
> > + int group0[] = {2};
> > + int group1[] = {0,3};
> > + int group2[] = {1};
> > + int *groups[] = {group0, group1, group2};
> > + int group_size[] = {1,2,1};
> > +
> > + set_priority(p4, prio, 4);
> > + set_tgt_node_name(p4, node_name, 4);
> > + assert_int_equal(group_by_node_name(&mp4), 0);
> > + verify_pathgroups(&mp4, p4, groups, group_size, 3);
> > +}
>
> Nothing wrong with your code, but this is one example where I would say
> our prio group ordering is counter-intuitive. Does it really make sense
> to order the group of two paths with prio {3, 1} *after* the group with
> just one prio 3 path?
That's kind of tricky, because with the round-robin path selector, just
having one fast path in the group might be the correct answer, while
with the dynamic path selectors, it seems like having both paths would
be better. My person issue with out path grouping is that I don't think
that what group_by_prio is what many devices want. For many devices,
the paths are going to be grouped by something static, like what
controller the paths go to. In that case, all the overhead of remaking
that pathgroups whenever the priority changes is a bunch of wasted
overhead. Also, it can cause situations where is multipathd notices
a priority change at the wrong moment, it will temporarily make garbage
pathgroups, with some paths using their old priority, and some using
their new priority (which multipathd hasn't noticed yet).
-Ben
>
> Regards,
> Martin
>
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 03/16] tests: add path grouping policy unit tests.
2019-08-16 21:01 ` Benjamin Marzinski
@ 2019-08-19 9:34 ` Martin Wilck
0 siblings, 0 replies; 41+ messages in thread
From: Martin Wilck @ 2019-08-19 9:34 UTC (permalink / raw)
To: bmarzins; +Cc: dm-devel, mkumar
On Fri, 2019-08-16 at 16:01 -0500, Benjamin Marzinski wrote:
> On Wed, Aug 14, 2019 at 09:22:17PM +0000, Martin Wilck wrote:
> > On Fri, 2019-08-02 at 11:33 -0500, Benjamin Marzinski wrote:
> > > In preparation for changing the path grouping code, add some unit
> > > tests
> > > to verify that it works correctly. The only test that currently
> > > fails
> > > (and so it being skipped) is using MULTIBUS when mp->paths is
> > > empty.
> > > All
> > > the other path grouping policies free mp->paths, even if it is
> > > empty.
> > > one_group() should as well. This will be fixed when the path
> > > grouping
> > > code is updated.
> > >
> > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > > ---
> > > tests/Makefile | 2 +-
> > > tests/pgpolicy.c | 708
> > > +++++++++++++++++++++++++++++++++++++++++++++++
> > > 2 files changed, 709 insertions(+), 1 deletion(-)
> > > create mode 100644 tests/pgpolicy.c
> > >
> > > +
> > > +static void test_group_by_node_name_3_groups4(void **state)
> > > +{
> > > + char *node_name[] = {"a","b","c","a"};
> > > + int prio[] = {3,1,3,1};
> > > + int group0[] = {2};
> > > + int group1[] = {0,3};
> > > + int group2[] = {1};
> > > + int *groups[] = {group0, group1, group2};
> > > + int group_size[] = {1,2,1};
> > > +
> > > + set_priority(p4, prio, 4);
> > > + set_tgt_node_name(p4, node_name, 4);
> > > + assert_int_equal(group_by_node_name(&mp4), 0);
> > > + verify_pathgroups(&mp4, p4, groups, group_size, 3);
> > > +}
> >
> > Nothing wrong with your code, but this is one example where I would
> > say
> > our prio group ordering is counter-intuitive. Does it really make
> > sense
> > to order the group of two paths with prio {3, 1} *after* the group
> > with
> > just one prio 3 path?
>
> That's kind of tricky, because with the round-robin path selector,
> just
> having one fast path in the group might be the correct answer, while
> with the dynamic path selectors, it seems like having both paths
> would
> be better.
FTR, the semantics were changed from summing to averaging almost
exactly 10 years ago:
91270ef "Use Average path priority value for path switching"
At the time, the patch author's rationale (blessed by Hannes' approval)
was that a group of 1 x prio=8 should not have higher precedence than a
group of 2 x 3 (details in
https://dm-devel.redhat.narkive.com/dmvqjPHA/rdac-priority-checker-changing-priorities).
While in the specific case the argument was valid, I doubt that it
applies in all situations.
> My person issue with out path grouping is that I don't think
> that what group_by_prio is what many devices want. For many devices,
> the paths are going to be grouped by something static, like what
> controller the paths go to.
True. In most situation that I have seen, "group_by_prio" is basically
a way to express storage states such as ALUA "optimized" vs. "non-
optimized" and "preferred". For a while I've been pondering to use
these states for grouping directly rather than the priorities derived
from them. So far I haven't come down to it.
Regards,
Martin
> In that case, all the overhead of remaking
> that pathgroups whenever the priority changes is a bunch of wasted
> overhead. Also, it can cause situations where is multipathd notices
> a priority change at the wrong moment, it will temporarily make
> garbage
> pathgroups, with some paths using their old priority, and some using
> their new priority (which multipathd hasn't noticed yet).
>
> -Ben
>
> > Regards,
> > Martin
> >
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 03/16] tests: add path grouping policy unit tests.
2019-08-02 16:33 ` [PATCH 03/16] tests: add path grouping policy unit tests Benjamin Marzinski
2019-08-14 21:22 ` Martin Wilck
@ 2019-08-14 21:38 ` Martin Wilck
1 sibling, 0 replies; 41+ messages in thread
From: Martin Wilck @ 2019-08-14 21:38 UTC (permalink / raw)
To: bmarzins, christophe.varoqui; +Cc: dm-devel, mkumar
On Fri, 2019-08-02 at 11:33 -0500, Benjamin Marzinski wrote:
> In preparation for changing the path grouping code, add some unit
> tests
> to verify that it works correctly. The only test that currently fails
> (and so it being skipped) is using MULTIBUS when mp->paths is empty.
> All
> the other path grouping policies free mp->paths, even if it is empty.
> one_group() should as well. This will be fixed when the path grouping
> code is updated.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Martin Wilck <mwilck@suse.com>
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 04/16] libmultipath: add wrapper function around pgpolicyfn
2019-08-02 16:33 [PATCH 00/16] multipath marginal pathgroups Benjamin Marzinski
` (2 preceding siblings ...)
2019-08-02 16:33 ` [PATCH 03/16] tests: add path grouping policy unit tests Benjamin Marzinski
@ 2019-08-02 16:33 ` Benjamin Marzinski
2019-08-14 21:39 ` Martin Wilck
2019-08-02 16:33 ` [PATCH 05/16] libmultipath: fix double free in pgpolicyfn error paths Benjamin Marzinski
` (11 subsequent siblings)
15 siblings, 1 reply; 41+ messages in thread
From: Benjamin Marzinski @ 2019-08-02 16:33 UTC (permalink / raw)
To: Christophe Varoqui
Cc: device-mapper development, Martin Wilck, Muneendra Kumar
group_paths() is a wrapper around the pgpolicy functions, that pulls out
the common code from the beginning and the end. However since
one_group() didn't free the mp->paths vector, it has to set it to NULL,
to avoid having the wrapper code do that. Also, the pathgroups in
group_by_prio are now needlessly sorted afterwards.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/configure.c | 2 +-
libmultipath/pgpolicies.c | 65 +++++++++++++--------------------------
libmultipath/pgpolicies.h | 2 +-
3 files changed, 23 insertions(+), 46 deletions(-)
diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index b09ef529..3c309d64 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -387,7 +387,7 @@ int setup_map(struct multipath *mpp, char *params, int params_size,
vector_free(mpp->pg);
mpp->pg = NULL;
}
- if (mpp->pgpolicyfn && mpp->pgpolicyfn(mpp))
+ if (group_paths(mpp))
return 1;
/*
diff --git a/libmultipath/pgpolicies.c b/libmultipath/pgpolicies.c
index 660768a4..21d4f122 100644
--- a/libmultipath/pgpolicies.c
+++ b/libmultipath/pgpolicies.c
@@ -84,6 +84,26 @@ sort_pathgroups (struct multipath *mp) {
}
+int group_paths(struct multipath *mp)
+{
+ if (!mp->pg)
+ mp->pg = vector_alloc();
+ if (!mp->pg)
+ return 1;
+
+ if (VECTOR_SIZE(mp->paths) > 0 &&
+ (!mp->pgpolicyfn || mp->pgpolicyfn(mp))) {
+ vector_free(mp->pg);
+ mp->pg = NULL;
+ return 1;
+ }
+
+ sort_pathgroups(mp);
+ vector_free(mp->paths);
+ mp->paths = NULL;
+ return 0;
+}
+
/*
* One path group per unique tgt_node_name present in the path vector
*/
@@ -95,12 +115,6 @@ int group_by_node_name(struct multipath * mp)
struct pathgroup * pgp;
struct path * pp2;
- if (!mp->pg)
- mp->pg = vector_alloc();
-
- if (!mp->pg)
- return 1;
-
/* init the bitmap */
bitmap = (int *)MALLOC(VECTOR_SIZE(mp->paths) * sizeof (int));
@@ -146,9 +160,6 @@ int group_by_node_name(struct multipath * mp)
}
}
FREE(bitmap);
- sort_pathgroups(mp);
- free_pathvec(mp->paths, KEEP_PATHS);
- mp->paths = NULL;
return 0;
out2:
free_pathgroup(pgp, KEEP_PATHS);
@@ -171,12 +182,6 @@ int group_by_serial(struct multipath * mp)
struct pathgroup * pgp;
struct path * pp2;
- if (!mp->pg)
- mp->pg = vector_alloc();
-
- if (!mp->pg)
- return 1;
-
/* init the bitmap */
bitmap = (int *)MALLOC(VECTOR_SIZE(mp->paths) * sizeof (int));
@@ -221,9 +226,6 @@ int group_by_serial(struct multipath * mp)
}
}
FREE(bitmap);
- sort_pathgroups(mp);
- free_pathvec(mp->paths, KEEP_PATHS);
- mp->paths = NULL;
return 0;
out2:
free_pathgroup(pgp, KEEP_PATHS);
@@ -241,12 +243,6 @@ int one_path_per_group(struct multipath *mp)
struct path * pp;
struct pathgroup * pgp;
- if (!mp->pg)
- mp->pg = vector_alloc();
-
- if (!mp->pg)
- return 1;
-
for (i = 0; i < VECTOR_SIZE(mp->paths); i++) {
pp = VECTOR_SLOT(mp->paths, i);
pgp = alloc_pathgroup();
@@ -260,9 +256,6 @@ int one_path_per_group(struct multipath *mp)
if (store_path(pgp->paths, pp))
goto out1;
}
- sort_pathgroups(mp);
- free_pathvec(mp->paths, KEEP_PATHS);
- mp->paths = NULL;
return 0;
out1:
free_pathgroup(pgp, KEEP_PATHS);
@@ -276,15 +269,6 @@ int one_group(struct multipath *mp) /* aka multibus */
{
struct pathgroup * pgp;
- if (VECTOR_SIZE(mp->paths) < 0)
- return 0;
-
- if (!mp->pg)
- mp->pg = vector_alloc();
-
- if (!mp->pg)
- return 1;
-
if (VECTOR_SIZE(mp->paths) > 0) {
pgp = alloc_pathgroup();
@@ -297,6 +281,7 @@ int one_group(struct multipath *mp) /* aka multibus */
goto out1;
pgp->paths = mp->paths;
+ /* Do this to avoid freeing vector in group_paths */
mp->paths = NULL;
}
@@ -317,12 +302,6 @@ int group_by_prio(struct multipath *mp)
struct pathgroup * pgp;
vector pathvec = NULL;
- if (!mp->pg)
- mp->pg = vector_alloc();
-
- if (!mp->pg)
- return 1;
-
pathvec = vector_alloc();
if (!pathvec)
goto out;
@@ -387,8 +366,6 @@ int group_by_prio(struct multipath *mp)
}
}
free_pathvec(pathvec, KEEP_PATHS);
- free_pathvec(mp->paths, KEEP_PATHS);
- mp->paths = NULL;
return 0;
out2:
free_pathgroup(pgp, KEEP_PATHS);
diff --git a/libmultipath/pgpolicies.h b/libmultipath/pgpolicies.h
index c0eaa7f4..11834011 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);
-
+int group_paths(struct multipath *);
/*
* policies
*/
--
2.17.2
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [PATCH 04/16] libmultipath: add wrapper function around pgpolicyfn
2019-08-02 16:33 ` [PATCH 04/16] libmultipath: add wrapper function around pgpolicyfn Benjamin Marzinski
@ 2019-08-14 21:39 ` Martin Wilck
2019-08-16 21:02 ` Benjamin Marzinski
0 siblings, 1 reply; 41+ messages in thread
From: Martin Wilck @ 2019-08-14 21:39 UTC (permalink / raw)
To: bmarzins, christophe.varoqui; +Cc: dm-devel, mkumar
On Fri, 2019-08-02 at 11:33 -0500, Benjamin Marzinski wrote:
> group_paths() is a wrapper around the pgpolicy functions, that pulls
> out
> the common code from the beginning and the end. However since
> one_group() didn't free the mp->paths vector, it has to set it to
> NULL,
> to avoid having the wrapper code do that. Also, the pathgroups in
> group_by_prio are now needlessly sorted afterwards.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Martin Wilck <mwilck@suse.com>
(Suggestion: squash this with 07/16, and possibly also 05/16 and
06/16).
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 04/16] libmultipath: add wrapper function around pgpolicyfn
2019-08-14 21:39 ` Martin Wilck
@ 2019-08-16 21:02 ` Benjamin Marzinski
0 siblings, 0 replies; 41+ messages in thread
From: Benjamin Marzinski @ 2019-08-16 21:02 UTC (permalink / raw)
To: Martin Wilck; +Cc: dm-devel, mkumar
On Wed, Aug 14, 2019 at 09:39:27PM +0000, Martin Wilck wrote:
> On Fri, 2019-08-02 at 11:33 -0500, Benjamin Marzinski wrote:
> > group_paths() is a wrapper around the pgpolicy functions, that pulls
> > out
> > the common code from the beginning and the end. However since
> > one_group() didn't free the mp->paths vector, it has to set it to
> > NULL,
> > to avoid having the wrapper code do that. Also, the pathgroups in
> > group_by_prio are now needlessly sorted afterwards.
> >
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
>
> Reviewed-by: Martin Wilck <mwilck@suse.com>
>
> (Suggestion: squash this with 07/16, and possibly also 05/16 and
> 06/16).
>
Sure.
-Ben
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 05/16] libmultipath: fix double free in pgpolicyfn error paths
2019-08-02 16:33 [PATCH 00/16] multipath marginal pathgroups Benjamin Marzinski
` (3 preceding siblings ...)
2019-08-02 16:33 ` [PATCH 04/16] libmultipath: add wrapper function around pgpolicyfn Benjamin Marzinski
@ 2019-08-02 16:33 ` Benjamin Marzinski
2019-08-14 21:21 ` Martin Wilck
2019-08-14 21:39 ` Martin Wilck
2019-08-02 16:33 ` [PATCH 06/16] libmultipath: remove store_pathgroup Benjamin Marzinski
` (10 subsequent siblings)
15 siblings, 2 replies; 41+ messages in thread
From: Benjamin Marzinski @ 2019-08-02 16:33 UTC (permalink / raw)
To: Christophe Varoqui
Cc: device-mapper development, Martin Wilck, Muneendra Kumar
In the pgpolicy functions, if an error is encountered after
alloc_pathgroup() is called, but before the path group is added to a
multipath device with add_pathgroup(), the pathgroup needs to be cleaned
up by calling free_pathgroup(). However, after the pathgroup has been
added to the multipath device, calling free_pgvec() will clean it up. In
this case, if free_pathgroup() is called first, the recently added
pathgroup will be freed twice.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/pgpolicies.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/libmultipath/pgpolicies.c b/libmultipath/pgpolicies.c
index 21d4f122..d447c46e 100644
--- a/libmultipath/pgpolicies.c
+++ b/libmultipath/pgpolicies.c
@@ -139,7 +139,7 @@ int group_by_node_name(struct multipath * mp)
/* feed the first path */
if (store_path(pgp->paths, pp))
- goto out2;
+ goto out1;
bitmap[i] = 1;
@@ -153,7 +153,7 @@ int group_by_node_name(struct multipath * mp)
if (!strncmp(pp->tgt_node_name, pp2->tgt_node_name,
NODE_NAME_SIZE)) {
if (store_path(pgp->paths, pp2))
- goto out2;
+ goto out1;
bitmap[j] = 1;
}
@@ -206,7 +206,7 @@ int group_by_serial(struct multipath * mp)
/* feed the first path */
if (store_path(pgp->paths, pp))
- goto out2;
+ goto out1;
bitmap[i] = 1;
@@ -219,7 +219,7 @@ int group_by_serial(struct multipath * mp)
if (0 == strcmp(pp->serial, pp2->serial)) {
if (store_path(pgp->paths, pp2))
- goto out2;
+ goto out1;
bitmap[j] = 1;
}
@@ -254,7 +254,7 @@ int one_path_per_group(struct multipath *mp)
goto out1;
if (store_path(pgp->paths, pp))
- goto out1;
+ goto out;
}
return 0;
out1:
@@ -358,7 +358,7 @@ int group_by_prio(struct multipath *mp)
vector_foreach_slot(pathvec, pp, i) {
if (pp->priority == prio) {
if (store_path(pgp->paths, pp))
- goto out2;
+ goto out1;
vector_del_slot(pathvec, i);
i--;
--
2.17.2
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [PATCH 05/16] libmultipath: fix double free in pgpolicyfn error paths
2019-08-02 16:33 ` [PATCH 05/16] libmultipath: fix double free in pgpolicyfn error paths Benjamin Marzinski
@ 2019-08-14 21:21 ` Martin Wilck
2019-08-14 21:39 ` Martin Wilck
1 sibling, 0 replies; 41+ messages in thread
From: Martin Wilck @ 2019-08-14 21:21 UTC (permalink / raw)
To: bmarzins, christophe.varoqui; +Cc: dm-devel, mkumar
On Fri, 2019-08-02 at 11:33 -0500, Benjamin Marzinski wrote:
> In the pgpolicy functions, if an error is encountered after
> alloc_pathgroup() is called, but before the path group is added to a
> multipath device with add_pathgroup(), the pathgroup needs to be
> cleaned
> up by calling free_pathgroup(). However, after the pathgroup has been
> added to the multipath device, calling free_pgvec() will clean it up.
> In
> this case, if free_pathgroup() is called first, the recently added
> pathgroup will be freed twice.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> libmultipath/pgpolicies.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
This is correct, but the code remains quite horrible. One day I'd love
to see "pgp" being a local variable inside the loop. That can be done
after your series though.
Martin
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 05/16] libmultipath: fix double free in pgpolicyfn error paths
2019-08-02 16:33 ` [PATCH 05/16] libmultipath: fix double free in pgpolicyfn error paths Benjamin Marzinski
2019-08-14 21:21 ` Martin Wilck
@ 2019-08-14 21:39 ` Martin Wilck
1 sibling, 0 replies; 41+ messages in thread
From: Martin Wilck @ 2019-08-14 21:39 UTC (permalink / raw)
To: bmarzins, christophe.varoqui; +Cc: dm-devel, mkumar
On Fri, 2019-08-02 at 11:33 -0500, Benjamin Marzinski wrote:
> In the pgpolicy functions, if an error is encountered after
> alloc_pathgroup() is called, but before the path group is added to a
> multipath device with add_pathgroup(), the pathgroup needs to be
> cleaned
> up by calling free_pathgroup(). However, after the pathgroup has been
> added to the multipath device, calling free_pgvec() will clean it up.
> In
> this case, if free_pathgroup() is called first, the recently added
> pathgroup will be freed twice.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Martin Wilck <mwilck@suse.com>
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 06/16] libmultipath: remove store_pathgroup
2019-08-02 16:33 [PATCH 00/16] multipath marginal pathgroups Benjamin Marzinski
` (4 preceding siblings ...)
2019-08-02 16:33 ` [PATCH 05/16] libmultipath: fix double free in pgpolicyfn error paths Benjamin Marzinski
@ 2019-08-02 16:33 ` Benjamin Marzinski
2019-08-14 21:40 ` Martin Wilck
2019-08-02 16:33 ` [PATCH 07/16] libmultipath: make one_group allocate a new vector Benjamin Marzinski
` (9 subsequent siblings)
15 siblings, 1 reply; 41+ messages in thread
From: Benjamin Marzinski @ 2019-08-02 16:33 UTC (permalink / raw)
To: Christophe Varoqui
Cc: device-mapper development, Martin Wilck, Muneendra Kumar
store_pathgroup() is only called by add_pathgroup(), and doesn't need to
exist as a seperate function.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/structs.c | 16 +++-------------
libmultipath/structs.h | 1 -
2 files changed, 3 insertions(+), 14 deletions(-)
diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index fee899bd..bf7fdd73 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -318,23 +318,13 @@ store_path (vector pathvec, struct path * pp)
return 0;
}
-int
-store_pathgroup (vector pgvec, struct pathgroup * pgp)
+int add_pathgroup(struct multipath *mpp, struct pathgroup *pgp)
{
- if (!vector_alloc_slot(pgvec))
+ if (!vector_alloc_slot(mpp->pg))
return 1;
- vector_set_slot(pgvec, pgp);
-
- return 0;
-}
-
-int add_pathgroup(struct multipath *mpp, struct pathgroup *pgp)
-{
- int ret = store_pathgroup(mpp->pg, pgp);
+ vector_set_slot(mpp->pg, pgp);
- if (ret)
- return ret;
pgp->mpp = mpp;
return 0;
}
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 1a3d827b..893074eb 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -445,7 +445,6 @@ int store_adaptergroup(vector adapters, struct adapter_group *agp);
int store_hostgroup(vector hostgroupvec, struct host_group *hgp);
int store_path (vector pathvec, struct path * pp);
-int store_pathgroup (vector pgvec, struct pathgroup * pgp);
int add_pathgroup(struct multipath*, struct pathgroup *);
struct multipath * find_mp_by_alias (const struct _vector *mp, const char *alias);
--
2.17.2
^ permalink raw reply related [flat|nested] 41+ messages in thread* [PATCH 07/16] libmultipath: make one_group allocate a new vector
2019-08-02 16:33 [PATCH 00/16] multipath marginal pathgroups Benjamin Marzinski
` (5 preceding siblings ...)
2019-08-02 16:33 ` [PATCH 06/16] libmultipath: remove store_pathgroup Benjamin Marzinski
@ 2019-08-02 16:33 ` Benjamin Marzinski
2019-08-14 21:40 ` Martin Wilck
2019-08-02 16:33 ` [PATCH 08/16] libmultipath: consolidate group_by_* functions Benjamin Marzinski
` (8 subsequent siblings)
15 siblings, 1 reply; 41+ messages in thread
From: Benjamin Marzinski @ 2019-08-02 16:33 UTC (permalink / raw)
To: Christophe Varoqui
Cc: device-mapper development, Martin Wilck, Muneendra Kumar
All the pgpolicy functions besides one_group() allocate a new vector for
the pathgroups. If one_group() works the same, it is easier to factor
out the common code.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/pgpolicies.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/libmultipath/pgpolicies.c b/libmultipath/pgpolicies.c
index d447c46e..1af42f52 100644
--- a/libmultipath/pgpolicies.c
+++ b/libmultipath/pgpolicies.c
@@ -267,24 +267,24 @@ out:
int one_group(struct multipath *mp) /* aka multibus */
{
+ int i;
+ struct path * pp;
struct pathgroup * pgp;
- if (VECTOR_SIZE(mp->paths) > 0) {
- pgp = alloc_pathgroup();
+ pgp = alloc_pathgroup();
- if (!pgp)
- goto out;
+ if (!pgp)
+ goto out;
- vector_free(pgp->paths);
+ if (add_pathgroup(mp, pgp))
+ goto out1;
- if (add_pathgroup(mp, pgp))
- goto out1;
+ for (i = 0; i < VECTOR_SIZE(mp->paths); i++) {
+ pp = VECTOR_SLOT(mp->paths, i);
- pgp->paths = mp->paths;
- /* Do this to avoid freeing vector in group_paths */
- mp->paths = NULL;
+ if (store_path(pgp->paths, pp))
+ goto out;
}
-
return 0;
out1:
free_pathgroup(pgp, KEEP_PATHS);
--
2.17.2
^ permalink raw reply related [flat|nested] 41+ messages in thread* [PATCH 08/16] libmultipath: consolidate group_by_* functions
2019-08-02 16:33 [PATCH 00/16] multipath marginal pathgroups Benjamin Marzinski
` (6 preceding siblings ...)
2019-08-02 16:33 ` [PATCH 07/16] libmultipath: make one_group allocate a new vector Benjamin Marzinski
@ 2019-08-02 16:33 ` Benjamin Marzinski
2019-08-14 21:40 ` Martin Wilck
2019-08-02 16:33 ` [PATCH 09/16] tests: update pgpolicy tests to work with group_paths() Benjamin Marzinski
` (7 subsequent siblings)
15 siblings, 1 reply; 41+ messages in thread
From: Benjamin Marzinski @ 2019-08-02 16:33 UTC (permalink / raw)
To: Christophe Varoqui
Cc: device-mapper development, Martin Wilck, Muneendra Kumar
group_by_node_name() and group_by_serial() are exactly the same except
for how the paths are compared. group_by_prio() is different but its
pathvec solves the same issue as the bitmap from the other two
functions, and since we are always running sort_pathgroups() after
calling pgpriorityfn, there is no need to sort the pathgroups in
group_by_prio(). This means that all three functions can be replaced
with one function, group_by_match() that takes a match function as an
argument.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/pgpolicies.c | 189 +++++++++-----------------------------
1 file changed, 41 insertions(+), 148 deletions(-)
diff --git a/libmultipath/pgpolicies.c b/libmultipath/pgpolicies.c
index 1af42f52..2e4db74c 100644
--- a/libmultipath/pgpolicies.c
+++ b/libmultipath/pgpolicies.c
@@ -4,6 +4,7 @@
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
+#include <stdbool.h>
#include "checkers.h"
#include "util.h"
@@ -104,10 +105,29 @@ int group_paths(struct multipath *mp)
return 0;
}
-/*
- * One path group per unique tgt_node_name present in the path vector
- */
-int group_by_node_name(struct multipath * mp)
+typedef bool (path_match_fn)(struct path *pp1, struct path *pp2);
+
+bool
+node_names_match(struct path *pp1, struct path *pp2)
+{
+ return (strncmp(pp1->tgt_node_name, pp2->tgt_node_name,
+ NODE_NAME_SIZE) == 0);
+}
+
+bool
+serials_match(struct path *pp1, struct path *pp2)
+{
+ return (strncmp(pp1->serial, pp2->serial, SERIAL_SIZE) == 0);
+}
+
+bool
+prios_match(struct path *pp1, struct path *pp2)
+{
+ return (pp1->priority == pp2->priority);
+}
+
+int group_by_match(struct multipath * mp,
+ bool (*path_match_fn)(struct path *, struct path *))
{
int i, j;
int * bitmap;
@@ -150,8 +170,7 @@ int group_by_node_name(struct multipath * mp)
pp2 = VECTOR_SLOT(mp->paths, j);
- if (!strncmp(pp->tgt_node_name, pp2->tgt_node_name,
- NODE_NAME_SIZE)) {
+ if (path_match_fn(pp, pp2)) {
if (store_path(pgp->paths, pp2))
goto out1;
@@ -171,70 +190,28 @@ out:
return 1;
}
+/*
+ * One path group per unique tgt_node_name present in the path vector
+ */
+int group_by_node_name(struct multipath * mp)
+{
+ return group_by_match(mp, node_names_match);
+}
+
/*
* One path group per unique serial number present in the path vector
*/
int group_by_serial(struct multipath * mp)
{
- int i, j;
- int * bitmap;
- struct path * pp;
- struct pathgroup * pgp;
- struct path * pp2;
-
- /* init the bitmap */
- bitmap = (int *)MALLOC(VECTOR_SIZE(mp->paths) * sizeof (int));
-
- if (!bitmap)
- goto out;
-
- for (i = 0; i < VECTOR_SIZE(mp->paths); i++) {
-
- if (bitmap[i])
- continue;
-
- pp = VECTOR_SLOT(mp->paths, i);
-
- /* here, we really got a new pg */
- pgp = alloc_pathgroup();
-
- if (!pgp)
- goto out1;
-
- if (add_pathgroup(mp, pgp))
- goto out2;
-
- /* feed the first path */
- if (store_path(pgp->paths, pp))
- goto out1;
-
- bitmap[i] = 1;
-
- for (j = i + 1; j < VECTOR_SIZE(mp->paths); j++) {
-
- if (bitmap[j])
- continue;
-
- pp2 = VECTOR_SLOT(mp->paths, j);
-
- if (0 == strcmp(pp->serial, pp2->serial)) {
- if (store_path(pgp->paths, pp2))
- goto out1;
+ return group_by_match(mp, serials_match);
+}
- bitmap[j] = 1;
- }
- }
- }
- FREE(bitmap);
- return 0;
-out2:
- free_pathgroup(pgp, KEEP_PATHS);
-out1:
- FREE(bitmap);
-out:
- free_pgvec(mp->pg, KEEP_PATHS);
- mp->pg = NULL;
- return 1;
+/*
+ * One path group per priority present in the path vector
+ */
+int group_by_prio(struct multipath *mp)
+{
+ return group_by_match(mp, prios_match);
}
int one_path_per_group(struct multipath *mp)
@@ -293,87 +270,3 @@ out:
mp->pg = NULL;
return 1;
}
-
-int group_by_prio(struct multipath *mp)
-{
- int i;
- int prio;
- struct path * pp;
- struct pathgroup * pgp;
- vector pathvec = NULL;
-
- pathvec = vector_alloc();
- if (!pathvec)
- goto out;
-
- vector_foreach_slot(mp->paths, pp, i) {
- if (!vector_alloc_slot(pathvec))
- goto out1;
- vector_set_slot(pathvec, pp);
- }
-
- while (VECTOR_SIZE(pathvec) > 0) {
- pp = VECTOR_SLOT(pathvec, 0);
- prio = pp->priority;
-
- /*
- * Find the position to insert the new path group. All groups
- * are ordered by the priority value (higher value first).
- */
- vector_foreach_slot(mp->pg, pgp, i) {
- pp = VECTOR_SLOT(pgp->paths, 0);
-
- if (prio > pp->priority)
- break;
- }
-
- /*
- * Initialize the new path group.
- */
- pgp = alloc_pathgroup();
-
- if (!pgp)
- goto out1;
-
- if (store_path(pgp->paths, VECTOR_SLOT(pathvec, 0)))
- goto out2;
-
- vector_del_slot(pathvec, 0);
-
- /*
- * Store the new path group into the vector.
- */
- if (i < VECTOR_SIZE(mp->pg)) {
- if (!vector_insert_slot(mp->pg, i, pgp))
- goto out2;
- pgp->mpp = mp;
- } else {
- if (add_pathgroup(mp, pgp))
- goto out2;
- }
-
- /*
- * add the other paths with the same prio
- */
- vector_foreach_slot(pathvec, pp, i) {
- if (pp->priority == prio) {
- if (store_path(pgp->paths, pp))
- goto out1;
-
- vector_del_slot(pathvec, i);
- i--;
- }
- }
- }
- free_pathvec(pathvec, KEEP_PATHS);
- return 0;
-out2:
- free_pathgroup(pgp, KEEP_PATHS);
-out1:
- free_pathvec(pathvec, KEEP_PATHS);
-out:
- free_pgvec(mp->pg, KEEP_PATHS);
- mp->pg = NULL;
- return 1;
-
-}
--
2.17.2
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [PATCH 08/16] libmultipath: consolidate group_by_* functions
2019-08-02 16:33 ` [PATCH 08/16] libmultipath: consolidate group_by_* functions Benjamin Marzinski
@ 2019-08-14 21:40 ` Martin Wilck
0 siblings, 0 replies; 41+ messages in thread
From: Martin Wilck @ 2019-08-14 21:40 UTC (permalink / raw)
To: bmarzins, christophe.varoqui; +Cc: dm-devel, mkumar
On Fri, 2019-08-02 at 11:33 -0500, Benjamin Marzinski wrote:
> group_by_node_name() and group_by_serial() are exactly the same
> except
> for how the paths are compared. group_by_prio() is different but its
> pathvec solves the same issue as the bitmap from the other two
> functions, and since we are always running sort_pathgroups() after
> calling pgpriorityfn, there is no need to sort the pathgroups in
> group_by_prio(). This means that all three functions can be replaced
> with one function, group_by_match() that takes a match function as an
> argument.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Martin Wilck <mwilck@suse.com>
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 09/16] tests: update pgpolicy tests to work with group_paths()
2019-08-02 16:33 [PATCH 00/16] multipath marginal pathgroups Benjamin Marzinski
` (7 preceding siblings ...)
2019-08-02 16:33 ` [PATCH 08/16] libmultipath: consolidate group_by_* functions Benjamin Marzinski
@ 2019-08-02 16:33 ` Benjamin Marzinski
2019-08-14 21:20 ` Martin Wilck
2019-08-14 21:41 ` Martin Wilck
2019-08-02 16:33 ` [PATCH 10/16] libmultipath: make pgpolicyfn take a paths vector Benjamin Marzinski
` (6 subsequent siblings)
15 siblings, 2 replies; 41+ messages in thread
From: Benjamin Marzinski @ 2019-08-02 16:33 UTC (permalink / raw)
To: Christophe Varoqui
Cc: device-mapper development, Martin Wilck, Muneendra Kumar
The pgpolicy unit tests now work again, using group_paths().
test_one_group0(), which was skipped with the old path grouping code
because it failed, is now working correctly.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
tests/pgpolicy.c | 125 +++++++++++++++++++++++++++++++----------------
1 file changed, 83 insertions(+), 42 deletions(-)
diff --git a/tests/pgpolicy.c b/tests/pgpolicy.c
index fbb8589e..04a77c4c 100644
--- a/tests/pgpolicy.c
+++ b/tests/pgpolicy.c
@@ -92,6 +92,7 @@ static int setupX(struct multipath *mp, struct path *pp, int size)
vector_set_slot(mp->paths, &pp[i]);
}
set_priority(pp, prio, size);
+ mp->pgpolicyfn = NULL;
return 0;
}
@@ -187,7 +188,8 @@ static void test_one_group8(void **state)
int *groups[] = {paths};
int group_size[] = {8};
- assert_int_equal(one_group(&mp8), 0);
+ mp8.pgpolicyfn = one_group;
+ assert_int_equal(group_paths(&mp8), 0);
verify_pathgroups(&mp8, p8, groups, group_size, 1);
}
@@ -197,7 +199,8 @@ static void test_one_group4(void **state)
int *groups[] = {paths};
int group_size[] = {4};
- assert_int_equal(one_group(&mp4), 0);
+ mp4.pgpolicyfn = one_group;
+ assert_int_equal(group_paths(&mp4), 0);
verify_pathgroups(&mp4, p4, groups, group_size, 1);
}
@@ -207,20 +210,22 @@ static void test_one_group1(void **state)
int *groups[] = {paths};
int group_size[] = {1};
- assert_int_equal(one_group(&mp1), 0);
+ mp1.pgpolicyfn = one_group;
+ assert_int_equal(group_paths(&mp1), 0);
verify_pathgroups(&mp1, p1, groups, group_size, 1);
}
static void test_one_group0(void **state)
{
- assert_int_equal(one_group(&mp0), 0);
- skip(); /* BROKEN */
+ mp0.pgpolicyfn = one_group;
+ assert_int_equal(group_paths(&mp0), 0);
verify_pathgroups(&mp0, NULL, NULL, NULL, 0);
}
static void test_one_group_null(void **state)
{
- assert_int_equal(one_group(&mp_null), 0);
+ mp_null.pgpolicyfn = one_group;
+ assert_int_equal(group_paths(&mp_null), 0);
verify_pathgroups(&mp_null, NULL, NULL, NULL, 0);
}
@@ -231,7 +236,8 @@ static void test_one_path_per_group_same8(void **state)
&paths[4], &paths[5], &paths[6], &paths[7]};
int group_size[] = {1,1,1,1,1,1,1,1};
- assert_int_equal(one_path_per_group(&mp8), 0);
+ mp8.pgpolicyfn = one_path_per_group;
+ assert_int_equal(group_paths(&mp8), 0);
verify_pathgroups(&mp8, p8, groups, group_size, 8);
}
@@ -244,7 +250,8 @@ static void test_one_path_per_group_increasing8(void **state)
int group_size[] = {1,1,1,1,1,1,1,1};
set_priority(p8, prio, 8);
- assert_int_equal(one_path_per_group(&mp8), 0);
+ mp8.pgpolicyfn = one_path_per_group;
+ assert_int_equal(group_paths(&mp8), 0);
verify_pathgroups(&mp8, p8, groups, group_size, 8);
}
@@ -257,7 +264,8 @@ static void test_one_path_per_group_decreasing8(void **state)
int group_size[] = {1,1,1,1,1,1,1,1};
set_priority(p8, prio, 8);
- assert_int_equal(one_path_per_group(&mp8), 0);
+ mp8.pgpolicyfn = one_path_per_group;
+ assert_int_equal(group_paths(&mp8), 0);
verify_pathgroups(&mp8, p8, groups, group_size, 8);
}
@@ -270,7 +278,8 @@ static void test_one_path_per_group_mixed8(void **state)
int group_size[] = {1,1,1,1,1,1,1,1};
set_priority(p8, prio, 8);
- assert_int_equal(one_path_per_group(&mp8), 0);
+ mp8.pgpolicyfn = one_path_per_group;
+ assert_int_equal(group_paths(&mp8), 0);
verify_pathgroups(&mp8, p8, groups, group_size, 8);
}
@@ -280,7 +289,8 @@ static void test_one_path_per_group4(void **state)
int *groups[] = {&paths[0], &paths[1], &paths[2], &paths[3]};
int group_size[] = {1,1,1,1};
- assert_int_equal(one_path_per_group(&mp4), 0);
+ mp4.pgpolicyfn = one_path_per_group;
+ assert_int_equal(group_paths(&mp4), 0);
verify_pathgroups(&mp4, p4, groups, group_size, 4);
}
@@ -290,19 +300,22 @@ static void test_one_path_per_group1(void **state)
int *groups[] = {paths};
int group_size[] = {1};
- assert_int_equal(one_path_per_group(&mp1), 0);
+ mp1.pgpolicyfn = one_path_per_group;
+ assert_int_equal(group_paths(&mp1), 0);
verify_pathgroups(&mp1, p1, groups, group_size, 1);
}
static void test_one_path_per_group0(void **state)
{
- assert_int_equal(one_path_per_group(&mp0), 0);
+ mp0.pgpolicyfn = one_path_per_group;
+ assert_int_equal(group_paths(&mp0), 0);
verify_pathgroups(&mp0, NULL, NULL, NULL, 0);
}
static void test_one_path_per_group_null(void **state)
{
- assert_int_equal(one_path_per_group(&mp_null), 0);
+ mp_null.pgpolicyfn = one_path_per_group;
+ assert_int_equal(group_paths(&mp_null), 0);
verify_pathgroups(&mp_null, NULL, NULL, NULL, 0);
}
@@ -312,7 +325,8 @@ static void test_group_by_prio_same8(void **state)
int *groups[] = {paths};
int group_size[] = {8};
- assert_int_equal(group_by_prio(&mp8), 0);
+ mp8.pgpolicyfn = group_by_prio;
+ assert_int_equal(group_paths(&mp8), 0);
verify_pathgroups(&mp8, p8, groups, group_size, 1);
}
@@ -325,7 +339,8 @@ static void test_group_by_prio_increasing8(void **state)
int group_size[] = {1,1,1,1,1,1,1,1};
set_priority(p8, prio, 8);
- assert_int_equal(group_by_prio(&mp8), 0);
+ mp8.pgpolicyfn = group_by_prio;
+ assert_int_equal(group_paths(&mp8), 0);
verify_pathgroups(&mp8, p8, groups, group_size, 8);
}
@@ -338,7 +353,8 @@ static void test_group_by_prio_decreasing8(void **state)
int group_size[] = {1,1,1,1,1,1,1,1};
set_priority(p8, prio, 8);
- assert_int_equal(group_by_prio(&mp8), 0);
+ mp8.pgpolicyfn = group_by_prio;
+ assert_int_equal(group_paths(&mp8), 0);
verify_pathgroups(&mp8, p8, groups, group_size, 8);
}
@@ -356,7 +372,8 @@ static void test_group_by_prio_mixed8(void **state)
int group_size[] = {1,1,1,2,2,1};
set_priority(p8, prio, 8);
- assert_int_equal(group_by_prio(&mp8), 0);
+ mp8.pgpolicyfn = group_by_prio;
+ assert_int_equal(group_paths(&mp8), 0);
verify_pathgroups(&mp8, p8, groups, group_size, 6);
}
@@ -369,7 +386,8 @@ static void test_group_by_prio_2_groups8(void **state)
int group_size[] = {4,4};
set_priority(p8, prio, 8);
- assert_int_equal(group_by_prio(&mp8), 0);
+ mp8.pgpolicyfn = group_by_prio;
+ assert_int_equal(group_paths(&mp8), 0);
verify_pathgroups(&mp8, p8, groups, group_size, 2);
}
@@ -383,7 +401,8 @@ static void test_group_by_prio_mixed4(void **state)
int group_size[] = {2,1,1};
set_priority(p4, prio, 4);
- assert_int_equal(group_by_prio(&mp4), 0);
+ mp4.pgpolicyfn = group_by_prio;
+ assert_int_equal(group_paths(&mp4), 0);
verify_pathgroups(&mp4, p4, groups, group_size, 3);
}
@@ -396,7 +415,8 @@ static void test_group_by_prio_2_groups4(void **state)
int group_size[] = {2,2};
set_priority(p4, prio, 4);
- assert_int_equal(group_by_prio(&mp4), 0);
+ mp4.pgpolicyfn = group_by_prio;
+ assert_int_equal(group_paths(&mp4), 0);
verify_pathgroups(&mp4, p4, groups, group_size, 2);
}
@@ -406,19 +426,22 @@ static void test_group_by_prio1(void **state)
int *groups[] = {paths};
int group_size[] = {1};
- assert_int_equal(group_by_prio(&mp1), 0);
+ mp1.pgpolicyfn = group_by_prio;
+ assert_int_equal(group_paths(&mp1), 0);
verify_pathgroups(&mp1, p1, groups, group_size, 1);
}
static void test_group_by_prio0(void **state)
{
- assert_int_equal(group_by_prio(&mp0), 0);
+ mp0.pgpolicyfn = group_by_prio;
+ assert_int_equal(group_paths(&mp0), 0);
verify_pathgroups(&mp0, NULL, NULL, NULL, 0);
}
static void test_group_by_prio_null(void **state)
{
- assert_int_equal(group_by_prio(&mp_null), 0);
+ mp_null.pgpolicyfn = group_by_prio;
+ assert_int_equal(group_paths(&mp_null), 0);
verify_pathgroups(&mp_null, NULL, NULL, NULL, 0);
}
@@ -430,7 +453,8 @@ static void test_group_by_node_name_same8(void **state)
int group_size[] = {8};
set_tgt_node_name(p8, node_name, 8);
- assert_int_equal(group_by_node_name(&mp8), 0);
+ mp8.pgpolicyfn = group_by_node_name;
+ assert_int_equal(group_paths(&mp8), 0);
verify_pathgroups(&mp8, p8, groups, group_size, 1);
}
@@ -445,7 +469,8 @@ static void test_group_by_node_name_increasing8(void **state)
set_priority(p8, prio, 8);
set_tgt_node_name(p8, node_name, 8);
- assert_int_equal(group_by_node_name(&mp8), 0);
+ mp8.pgpolicyfn = group_by_node_name;
+ assert_int_equal(group_paths(&mp8), 0);
verify_pathgroups(&mp8, p8, groups, group_size, 8);
}
@@ -461,7 +486,8 @@ static void test_group_by_node_name_3_groups8(void **state)
set_priority(p8, prio, 8);
set_tgt_node_name(p8, node_name, 8);
- assert_int_equal(group_by_node_name(&mp8), 0);
+ mp8.pgpolicyfn = group_by_node_name;
+ assert_int_equal(group_paths(&mp8), 0);
verify_pathgroups(&mp8, p8, groups, group_size, 3);
}
@@ -476,7 +502,8 @@ static void test_group_by_node_name_2_groups8(void **state)
set_priority(p8, prio, 8);
set_tgt_node_name(p8, node_name, 8);
- assert_int_equal(group_by_node_name(&mp8), 0);
+ mp8.pgpolicyfn = group_by_node_name;
+ assert_int_equal(group_paths(&mp8), 0);
verify_pathgroups(&mp8, p8, groups, group_size, 2);
}
@@ -492,7 +519,8 @@ static void test_group_by_node_name_3_groups4(void **state)
set_priority(p4, prio, 4);
set_tgt_node_name(p4, node_name, 4);
- assert_int_equal(group_by_node_name(&mp4), 0);
+ mp4.pgpolicyfn = group_by_node_name;
+ assert_int_equal(group_paths(&mp4), 0);
verify_pathgroups(&mp4, p4, groups, group_size, 3);
}
@@ -507,7 +535,8 @@ static void test_group_by_node_name_2_groups4(void **state)
set_priority(p4, prio, 4);
set_tgt_node_name(p4, node_name, 4);
- assert_int_equal(group_by_node_name(&mp4), 0);
+ mp4.pgpolicyfn = group_by_node_name;
+ assert_int_equal(group_paths(&mp4), 0);
verify_pathgroups(&mp4, p4, groups, group_size, 2);
}
@@ -519,19 +548,22 @@ static void test_group_by_node_name1(void **state)
int group_size[] = {1};
set_tgt_node_name(p1, node_name, 1);
- assert_int_equal(group_by_node_name(&mp1), 0);
+ mp1.pgpolicyfn = group_by_node_name;
+ assert_int_equal(group_paths(&mp1), 0);
verify_pathgroups(&mp1, p1, groups, group_size, 1);
}
static void test_group_by_node_name0(void **state)
{
- assert_int_equal(group_by_node_name(&mp0), 0);
+ mp0.pgpolicyfn = group_by_node_name;
+ assert_int_equal(group_paths(&mp0), 0);
verify_pathgroups(&mp0, NULL, NULL, NULL, 0);
}
static void test_group_by_node_name_null(void **state)
{
- assert_int_equal(group_by_node_name(&mp_null), 0);
+ mp_null.pgpolicyfn = group_by_node_name;
+ assert_int_equal(group_paths(&mp_null), 0);
verify_pathgroups(&mp_null, NULL, NULL, NULL, 0);
}
@@ -543,7 +575,8 @@ static void test_group_by_serial_same8(void **state)
int group_size[] = {8};
set_serial(p8, serial, 8);
- assert_int_equal(group_by_serial(&mp8), 0);
+ mp8.pgpolicyfn = group_by_serial;
+ assert_int_equal(group_paths(&mp8), 0);
verify_pathgroups(&mp8, p8, groups, group_size, 1);
}
@@ -558,7 +591,8 @@ static void test_group_by_serial_increasing8(void **state)
set_priority(p8, prio, 8);
set_serial(p8, serial, 8);
- assert_int_equal(group_by_serial(&mp8), 0);
+ mp8.pgpolicyfn = group_by_serial;
+ assert_int_equal(group_paths(&mp8), 0);
verify_pathgroups(&mp8, p8, groups, group_size, 8);
}
@@ -574,7 +608,8 @@ static void test_group_by_serial_3_groups8(void **state)
set_priority(p8, prio, 8);
set_serial(p8, serial, 8);
- assert_int_equal(group_by_serial(&mp8), 0);
+ mp8.pgpolicyfn = group_by_serial;
+ assert_int_equal(group_paths(&mp8), 0);
verify_pathgroups(&mp8, p8, groups, group_size, 3);
}
@@ -589,7 +624,8 @@ static void test_group_by_serial_2_groups8(void **state)
set_priority(p8, prio, 8);
set_serial(p8, serial, 8);
- assert_int_equal(group_by_serial(&mp8), 0);
+ mp8.pgpolicyfn = group_by_serial;
+ assert_int_equal(group_paths(&mp8), 0);
verify_pathgroups(&mp8, p8, groups, group_size, 2);
}
@@ -605,7 +641,8 @@ static void test_group_by_serial_3_groups4(void **state)
set_priority(p4, prio, 4);
set_serial(p4, serial, 4);
- assert_int_equal(group_by_serial(&mp4), 0);
+ mp4.pgpolicyfn = group_by_serial;
+ assert_int_equal(group_paths(&mp4), 0);
verify_pathgroups(&mp4, p4, groups, group_size, 3);
}
@@ -620,7 +657,8 @@ static void test_group_by_serial_2_groups4(void **state)
set_priority(p4, prio, 4);
set_serial(p4, serial, 4);
- assert_int_equal(group_by_serial(&mp4), 0);
+ mp4.pgpolicyfn = group_by_serial;
+ assert_int_equal(group_paths(&mp4), 0);
verify_pathgroups(&mp4, p4, groups, group_size, 2);
}
@@ -632,19 +670,22 @@ static void test_group_by_serial1(void **state)
int group_size[1] = {1};
set_serial(p1, serial, 1);
- assert_int_equal(group_by_serial(&mp1), 0);
+ mp1.pgpolicyfn = group_by_serial;
+ assert_int_equal(group_paths(&mp1), 0);
verify_pathgroups(&mp1, p1, groups, group_size, 1);
}
static void test_group_by_serial0(void **state)
{
- assert_int_equal(group_by_serial(&mp0), 0);
+ mp0.pgpolicyfn = group_by_serial;
+ assert_int_equal(group_paths(&mp0), 0);
verify_pathgroups(&mp0, NULL, NULL, NULL, 0);
}
static void test_group_by_serial_null(void **state)
{
- assert_int_equal(group_by_serial(&mp_null), 0);
+ mp_null.pgpolicyfn = group_by_serial;
+ assert_int_equal(group_paths(&mp_null), 0);
verify_pathgroups(&mp_null, NULL, NULL, NULL, 0);
}
--
2.17.2
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [PATCH 09/16] tests: update pgpolicy tests to work with group_paths()
2019-08-02 16:33 ` [PATCH 09/16] tests: update pgpolicy tests to work with group_paths() Benjamin Marzinski
@ 2019-08-14 21:20 ` Martin Wilck
2019-08-14 21:41 ` Martin Wilck
1 sibling, 0 replies; 41+ messages in thread
From: Martin Wilck @ 2019-08-14 21:20 UTC (permalink / raw)
To: bmarzins, christophe.varoqui; +Cc: dm-devel, mkumar
On Fri, 2019-08-02 at 11:33 -0500, Benjamin Marzinski wrote:
> The pgpolicy unit tests now work again, using group_paths().
> test_one_group0(), which was skipped with the old path grouping code
> because it failed, is now working correctly.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> tests/pgpolicy.c | 125 +++++++++++++++++++++++++++++++------------
> ----
Minor nit: The current patch order breaks the tests until this one is
applied. I'd have liked to see this patch right after 04/16.
Martin
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 09/16] tests: update pgpolicy tests to work with group_paths()
2019-08-02 16:33 ` [PATCH 09/16] tests: update pgpolicy tests to work with group_paths() Benjamin Marzinski
2019-08-14 21:20 ` Martin Wilck
@ 2019-08-14 21:41 ` Martin Wilck
1 sibling, 0 replies; 41+ messages in thread
From: Martin Wilck @ 2019-08-14 21:41 UTC (permalink / raw)
To: bmarzins, christophe.varoqui; +Cc: dm-devel, mkumar
On Fri, 2019-08-02 at 11:33 -0500, Benjamin Marzinski wrote:
> The pgpolicy unit tests now work again, using group_paths().
> test_one_group0(), which was skipped with the old path grouping code
> because it failed, is now working correctly.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Martin Wilck <mwilck@suse.com>
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 10/16] libmultipath: make pgpolicyfn take a paths vector
2019-08-02 16:33 [PATCH 00/16] multipath marginal pathgroups Benjamin Marzinski
` (8 preceding siblings ...)
2019-08-02 16:33 ` [PATCH 09/16] tests: update pgpolicy tests to work with group_paths() Benjamin Marzinski
@ 2019-08-02 16:33 ` Benjamin Marzinski
2019-08-14 22:05 ` Martin Wilck
2019-08-02 16:33 ` [PATCH 11/16] libmultipath: make group_paths handle marginal paths Benjamin Marzinski
` (5 subsequent siblings)
15 siblings, 1 reply; 41+ messages in thread
From: Benjamin Marzinski @ 2019-08-02 16:33 UTC (permalink / raw)
To: Christophe Varoqui
Cc: device-mapper development, Martin Wilck, Muneendra Kumar
To enable future changes, mp->pgpolicyfn() now takes a vector of
paths instead of always using mp->paths.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/pgpolicies.c | 38 +++++++++++++++++++-------------------
libmultipath/pgpolicies.h | 10 +++++-----
libmultipath/structs.h | 2 +-
3 files changed, 25 insertions(+), 25 deletions(-)
diff --git a/libmultipath/pgpolicies.c b/libmultipath/pgpolicies.c
index 2e4db74c..2e7c0502 100644
--- a/libmultipath/pgpolicies.c
+++ b/libmultipath/pgpolicies.c
@@ -93,7 +93,7 @@ int group_paths(struct multipath *mp)
return 1;
if (VECTOR_SIZE(mp->paths) > 0 &&
- (!mp->pgpolicyfn || mp->pgpolicyfn(mp))) {
+ (!mp->pgpolicyfn || mp->pgpolicyfn(mp, mp->paths))) {
vector_free(mp->pg);
mp->pg = NULL;
return 1;
@@ -126,7 +126,7 @@ prios_match(struct path *pp1, struct path *pp2)
return (pp1->priority == pp2->priority);
}
-int group_by_match(struct multipath * mp,
+int group_by_match(struct multipath * mp, vector paths,
bool (*path_match_fn)(struct path *, struct path *))
{
int i, j;
@@ -136,17 +136,17 @@ int group_by_match(struct multipath * mp,
struct path * pp2;
/* init the bitmap */
- bitmap = (int *)MALLOC(VECTOR_SIZE(mp->paths) * sizeof (int));
+ bitmap = (int *)MALLOC(VECTOR_SIZE(paths) * sizeof (int));
if (!bitmap)
goto out;
- for (i = 0; i < VECTOR_SIZE(mp->paths); i++) {
+ for (i = 0; i < VECTOR_SIZE(paths); i++) {
if (bitmap[i])
continue;
- pp = VECTOR_SLOT(mp->paths, i);
+ pp = VECTOR_SLOT(paths, i);
/* here, we really got a new pg */
pgp = alloc_pathgroup();
@@ -163,12 +163,12 @@ int group_by_match(struct multipath * mp,
bitmap[i] = 1;
- for (j = i + 1; j < VECTOR_SIZE(mp->paths); j++) {
+ for (j = i + 1; j < VECTOR_SIZE(paths); j++) {
if (bitmap[j])
continue;
- pp2 = VECTOR_SLOT(mp->paths, j);
+ pp2 = VECTOR_SLOT(paths, j);
if (path_match_fn(pp, pp2)) {
if (store_path(pgp->paths, pp2))
@@ -193,35 +193,35 @@ out:
/*
* One path group per unique tgt_node_name present in the path vector
*/
-int group_by_node_name(struct multipath * mp)
+int group_by_node_name(struct multipath * mp, vector paths)
{
- return group_by_match(mp, node_names_match);
+ return group_by_match(mp, paths, node_names_match);
}
/*
* One path group per unique serial number present in the path vector
*/
-int group_by_serial(struct multipath * mp)
+int group_by_serial(struct multipath * mp, vector paths)
{
- return group_by_match(mp, serials_match);
+ return group_by_match(mp, paths, serials_match);
}
/*
* One path group per priority present in the path vector
*/
-int group_by_prio(struct multipath *mp)
+int group_by_prio(struct multipath *mp, vector paths)
{
- return group_by_match(mp, prios_match);
+ return group_by_match(mp, paths, prios_match);
}
-int one_path_per_group(struct multipath *mp)
+int one_path_per_group(struct multipath *mp, vector paths)
{
int i;
struct path * pp;
struct pathgroup * pgp;
- for (i = 0; i < VECTOR_SIZE(mp->paths); i++) {
- pp = VECTOR_SLOT(mp->paths, i);
+ for (i = 0; i < VECTOR_SIZE(paths); i++) {
+ pp = VECTOR_SLOT(paths, i);
pgp = alloc_pathgroup();
if (!pgp)
@@ -242,7 +242,7 @@ out:
return 1;
}
-int one_group(struct multipath *mp) /* aka multibus */
+int one_group(struct multipath *mp, vector paths) /* aka multibus */
{
int i;
struct path * pp;
@@ -256,8 +256,8 @@ int one_group(struct multipath *mp) /* aka multibus */
if (add_pathgroup(mp, pgp))
goto out1;
- for (i = 0; i < VECTOR_SIZE(mp->paths); i++) {
- pp = VECTOR_SLOT(mp->paths, i);
+ for (i = 0; i < VECTOR_SIZE(paths); i++) {
+ pp = VECTOR_SLOT(paths, i);
if (store_path(pgp->paths, pp))
goto out;
diff --git a/libmultipath/pgpolicies.h b/libmultipath/pgpolicies.h
index 11834011..7532d75f 100644
--- a/libmultipath/pgpolicies.h
+++ b/libmultipath/pgpolicies.h
@@ -25,10 +25,10 @@ int group_paths(struct multipath *);
/*
* policies
*/
-int one_path_per_group(struct multipath *);
-int one_group(struct multipath *);
-int group_by_serial(struct multipath *);
-int group_by_prio(struct multipath *);
-int group_by_node_name(struct multipath *);
+int one_path_per_group(struct multipath *, vector);
+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);
#endif
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 893074eb..a8b9d325 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -295,7 +295,7 @@ struct path {
struct gen_path generic_path;
};
-typedef int (pgpolicyfn) (struct multipath *);
+typedef int (pgpolicyfn) (struct multipath *, vector);
struct multipath {
char wwid[WWID_SIZE];
--
2.17.2
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [PATCH 10/16] libmultipath: make pgpolicyfn take a paths vector
2019-08-02 16:33 ` [PATCH 10/16] libmultipath: make pgpolicyfn take a paths vector Benjamin Marzinski
@ 2019-08-14 22:05 ` Martin Wilck
2019-08-16 21:28 ` Benjamin Marzinski
0 siblings, 1 reply; 41+ messages in thread
From: Martin Wilck @ 2019-08-14 22:05 UTC (permalink / raw)
To: bmarzins, christophe.varoqui; +Cc: dm-devel, mkumar, Hannes Reinecke
On Fri, 2019-08-02 at 11:33 -0500, Benjamin Marzinski wrote:
> To enable future changes, mp->pgpolicyfn() now takes a vector of
> paths instead of always using mp->paths.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> libmultipath/pgpolicies.c | 38 +++++++++++++++++++----------------
> ---
> libmultipath/pgpolicies.h | 10 +++++-----
> libmultipath/structs.h | 2 +-
> 3 files changed, 25 insertions(+), 25 deletions(-)
>
The following applies to this patch and its successors (11-13, 15).
This is technically correct, beautiful code, but I'm not sure if this
is where we want to go. Do we really need that strict separation
between "normal" and "marginal" path groups?
As I already noted in my reply to 14/16, I'd favor an approach where
we would factor in the "marginality" of a path when calculating the
priority and the grouping. For example, when working with ALUA and
group_by_prio, rather than lumping all marginal paths together, we may
want to group like this:
- active/optimized, healthy
- active/non-optimized, healthy
- active/optimized, marginal
- active/non-optimized, marginal
- standby
- other
The priorities of these groups would be up to discussion. Today I'm not
even sure if "marginal" property should take precedence over
"optimized" property, or vice-versa. It might actually depend on the
situation and the degree of "shakiness" ...
OK: This is future material. But if we take this patch and its
successors, be'd have it cast in stone that "marginal/normal" is the
main distinction, taking precedence over everything else, and I'm not
convinced that that's the way to go.
We could obtain the semantics of your current patch set by assigning a
negative prio bias to marginal paths, and changing the grouping
algorithms (except group_by_prio) such that they take the marginal
property into account (e.g. group_by_node_name would pretend that all
marginal paths have a common "node name" and lump them together). This
would allow us to keep our current simple mpp->pg vector and represent
the marginal paths simply by one (the last) PG in this vector.
The benefit would be that this model would be more flexible for more
sophisticated priority models in the future.
Regards,
Martin
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 10/16] libmultipath: make pgpolicyfn take a paths vector
2019-08-14 22:05 ` Martin Wilck
@ 2019-08-16 21:28 ` Benjamin Marzinski
2019-08-20 22:55 ` Benjamin Marzinski
0 siblings, 1 reply; 41+ messages in thread
From: Benjamin Marzinski @ 2019-08-16 21:28 UTC (permalink / raw)
To: Martin Wilck; +Cc: dm-devel, mkumar, Hannes Reinecke
On Wed, Aug 14, 2019 at 10:05:45PM +0000, Martin Wilck wrote:
> On Fri, 2019-08-02 at 11:33 -0500, Benjamin Marzinski wrote:
> > To enable future changes, mp->pgpolicyfn() now takes a vector of
> > paths instead of always using mp->paths.
> >
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> > libmultipath/pgpolicies.c | 38 +++++++++++++++++++----------------
> > ---
> > libmultipath/pgpolicies.h | 10 +++++-----
> > libmultipath/structs.h | 2 +-
> > 3 files changed, 25 insertions(+), 25 deletions(-)
> >
>
> The following applies to this patch and its successors (11-13, 15).
>
> This is technically correct, beautiful code, but I'm not sure if this
> is where we want to go. Do we really need that strict separation
> between "normal" and "marginal" path groups?
>
> As I already noted in my reply to 14/16, I'd favor an approach where
> we would factor in the "marginality" of a path when calculating the
> priority and the grouping. For example, when working with ALUA and
> group_by_prio, rather than lumping all marginal paths together, we may
> want to group like this:
>
> - active/optimized, healthy
> - active/non-optimized, healthy
> - active/optimized, marginal
> - active/non-optimized, marginal
> - standby
> - other
>
> The priorities of these groups would be up to discussion. Today I'm not
> even sure if "marginal" property should take precedence over
> "optimized" property, or vice-versa. It might actually depend on the
> situation and the degree of "shakiness" ...
Possibly, if we include some way measuring the degree of shakiness. In
my experience, by far the most common reason that a path is declared
marginal is because something has gone wrong where the path_checker is
saying the the path is good, but virtually no I/O to the path is
succeeding. In this case you just want to keep that path from
continually being brought back and then failing. That's why marking the
path as down was a pretty decent idea. In fact, one of my worries with
the marginal pathgroups code is that it makes it impossible for you to
ever trigger your no_path_retry limit in this case, which some customers
may still want. If your only path is on that fails virtually all IO,
then it's the same having no paths.
All this is to say that I agree that with our current marginal paths
algorithms, you can make a case that you could detect a path that you
might not want to use if there are better options, but which doesn't
deserve to be the absolute last resort. On the other hand, I'm not
convinced that we'd run into this case enough for it to be worth adding
too much complexity to the code. Instead, I'm still more worried about
the opposite issue, where you would rather have the multipath device
timeout and throw an error, rather than continue to try to use a
marginal path.
> OK: This is future material. But if we take this patch and its
> successors, be'd have it cast in stone that "marginal/normal" is the
> main distinction, taking precedence over everything else, and I'm not
> convinced that that's the way to go.
>
> We could obtain the semantics of your current patch set by assigning a
> negative prio bias to marginal paths, and changing the grouping
> algorithms (except group_by_prio) such that they take the marginal
> property into account (e.g. group_by_node_name would pretend that all
> marginal paths have a common "node name" and lump them together). This
> would allow us to keep our current simple mpp->pg vector and represent
> the marginal paths simply by one (the last) PG in this vector.
>
> The benefit would be that this model would be more flexible for more
> sophisticated priority models in the future.
I'll take a look at doing this in a way that would make it easier to
fine tune this in the future.
-Ben
> Regards,
> Martin
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 10/16] libmultipath: make pgpolicyfn take a paths vector
2019-08-16 21:28 ` Benjamin Marzinski
@ 2019-08-20 22:55 ` Benjamin Marzinski
2019-08-21 10:28 ` Martin Wilck
0 siblings, 1 reply; 41+ messages in thread
From: Benjamin Marzinski @ 2019-08-20 22:55 UTC (permalink / raw)
To: Martin Wilck; +Cc: dm-devel, mkumar, Hannes Reinecke
On Fri, Aug 16, 2019 at 04:28:37PM -0500, Benjamin Marzinski wrote:
> On Wed, Aug 14, 2019 at 10:05:45PM +0000, Martin Wilck wrote:
> > OK: This is future material. But if we take this patch and its
> > successors, be'd have it cast in stone that "marginal/normal" is the
> > main distinction, taking precedence over everything else, and I'm not
> > convinced that that's the way to go.
> >
> > We could obtain the semantics of your current patch set by assigning a
> > negative prio bias to marginal paths, and changing the grouping
> > algorithms (except group_by_prio) such that they take the marginal
> > property into account (e.g. group_by_node_name would pretend that all
> > marginal paths have a common "node name" and lump them together). This
> > would allow us to keep our current simple mpp->pg vector and represent
> > the marginal paths simply by one (the last) PG in this vector.
> >
> > The benefit would be that this model would be more flexible for more
> > sophisticated priority models in the future.
>
> I'll take a look at doing this in a way that would make it easier to
> fine tune this in the future.
So, I've looked at this, and I'd like to make the case that these
patches don't put multipath into an inflexible method of dealing with
path groups. In my mind, there are three issues with path grouping that
my patches deal with
1. How the paths are grouped together
2. How the pathgroups are ordered
3. How the best pathgroup is chosen
I'd argue that the only issue where my patches really adds some
significant code is on 1. I think that my choice of groups for 1 is
correct, and I have a suggestion for making issue 3 go away.
In regards to the first issue, how the paths are grouped together, my
patch basically follows two rules:
1. Marginal paths shouldn't be grouped with non-marginal paths.
Obviously, if you want the marginal paths to stay in the same group as
they would normally be in, then you don't want marginal path detection
at all. Further, I don't see a scenario where we would like the marginal
paths to be grouped with non-marginal paths that they wouldn't normally
be grouped with.
2. Marginal paths should be grouped with other marginal paths using the
same rules as for non-marginal paths.
There are often very good reasons why paths are grouped the way they
are. For instance, if the marginal path is passive (I'm not even sure
that we should keep paths in the marginal state if they are PATH_GHOST,
but we currently do), we really don't want to put it in a pathgroup with
active paths. There probably are cases where it is safe to put all, or
some, of the marginal paths together, but it's not easy to know when
that is, and I don't think there is much benefit from doing the work to
figure it out, and it is always safe to group them like you would if
they were non-marginal paths.
I don't see a better way to set up the groups than what my patch does,
so I'm not particularly worried about all the code involved in making
sure that the groups look like this.
As for the second issue, how the pathgroups are ordered, I don't think
my code locks us in at all. The functions that order the pathgroups are
path_group_prio_update() and sort_pathgroups(). If you wanted to make
multipath just change the priority of marginal pathgroups, you would
just need to set that priority in path_group_prio_update(), and if you
only wanted to use the priority for sorting them, you would just change
sort_pathgroups() to only sort by priority. If you wanted to change
pgp->marginal from something binary to something that reflected how
marginal a path was, and you want to have that to change how a path was
sorted vs other paths with different marginal and priority values, you
could do all of that by simply changing the values set in
path_group_prio_update() and how those values are sorted in
sort_pathgroups(). I don't think my code does anything to make this less
flexible.
As for the third issue, how the best pathgroup is chosen, this is also a
case where changing things just involves changing how things are done in
one function, select_path_group(), to match what's done in
sort_pathgroups(). But since the pathgroups are already in order from
the best one to use to the worst, the multipath userspace tools could
just work how the kernel works, and pick the first usable pathgroup.
This won't always give the same answer that multipath currently does,
since the current code looks at the number of enabled paths. But the
only times when it will be different is when there are multiple
pathgroups with the same priority, and the first one has some failed
paths. Since we rarely have multiple pathgroups with the same priorty
except when using the failover policy (group_by_serial and
group_by_node_name being rarely used), and you will always have one path
per pathgroup with failover, in practice this will almost never occur.
Oh, I did notice a bug in my select_path_group() code. I should be
calling path_group_prio_update() before checking if the pathgroup
is marginal or not. I'll fix that.
So, I'm not against making this all work with priorities if there is a
reason to do so, but doing that will just involve changes in 3
straightforward functions, or 2 if we decide to simplify
select_path_group() so it just uses the order from sort_pathgroups() as
a guide.
If you feel strongly about doing this with priorities, I can add a patch
that changes this. But if it gets us the same results, then I don't see
much benefit. We can always change it laster if we want to change how
the groups actually end up.
-Ben
> -Ben
>
> > Regards,
> > Martin
> >
>
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 10/16] libmultipath: make pgpolicyfn take a paths vector
2019-08-20 22:55 ` Benjamin Marzinski
@ 2019-08-21 10:28 ` Martin Wilck
0 siblings, 0 replies; 41+ messages in thread
From: Martin Wilck @ 2019-08-21 10:28 UTC (permalink / raw)
To: bmarzins; +Cc: dm-devel, mkumar, Hannes Reinecke
On Tue, 2019-08-20 at 17:55 -0500, Benjamin Marzinski wrote:
>
> So, I've looked at this, and I'd like to make the case that these
> patches don't put multipath into an inflexible method of dealing with
> path groups. In my mind, there are three issues with path grouping
> that
> my patches deal with
>
> 1. How the paths are grouped together
> 2. How the pathgroups are ordered
> 3. How the best pathgroup is chosen
>
> I'd argue that the only issue where my patches really adds some
> significant code is on 1. I think that my choice of groups for 1 is
> correct, and I have a suggestion for making issue 3 go away.
>
> In regards to the first issue, how the paths are grouped together, my
> patch basically follows two rules:
>
> 1. Marginal paths shouldn't be grouped with non-marginal paths.
>
> Obviously, if you want the marginal paths to stay in the same group
> as
> they would normally be in, then you don't want marginal path
> detection
> at all.
Well, this is how multipathd used to behave - the "marginal" property
didn't have any influence on the path grouping. But this kind of
grouping can still be used by setting marginal_pathgroups to "no".
And if someone wants marginal path detection to be active and to affect
path grouping, indeed she doesn't want marginal and non-marginal paths
in one path group.
So yes, I think you are right here.
> >Further, I don't see a scenario where we would like the marginal
> paths to be grouped with non-marginal paths that they wouldn't
> normally
> be grouped with.
Right.
> 2. Marginal paths should be grouped with other marginal paths using
> the
> same rules as for non-marginal paths.
>
> There are often very good reasons why paths are grouped the way they
> are. For instance, if the marginal path is passive (I'm not even
> sure
> that we should keep paths in the marginal state if they are
> PATH_GHOST,
> but we currently do), we really don't want to put it in a pathgroup
> with
> active paths. There probably are cases where it is safe to put all,
> or
> some, of the marginal paths together, but it's not easy to know when
> that is, and I don't think there is much benefit from doing the work
> to
> figure it out, and it is always safe to group them like you would if
> they were non-marginal paths.
>
> I don't see a better way to set up the groups than what my patch
> does,
> so I'm not particularly worried about all the code involved in making
> sure that the groups look like this.
Right. Note that your description of "marginal_pathgroups" in the man
page is a bit misleading, as it talks about "a separate pathgroup",
which could be read as "one separate pathgroup". The paragraph could be
understood such that all marginal paths are lumped together into one
PG, wich is not the case.
You have logically changed the path group vector into a 2-dimensional
Matrix. In the past, similar problems have been solved by simply
scaling the prio values (think ALUA with "exclusive_pref_bit" set), but
that obviously works for grouping only with "group_by_prio".
In fact, your new approach is more powerful, and we might consider
using it in a generalized form in the future.
> As for the second issue, how the pathgroups are ordered, I don't
> think
> my code locks us in at all. The functions that order the pathgroups
> are
> path_group_prio_update() and sort_pathgroups(). If you wanted to
> make
> multipath just change the priority of marginal pathgroups, you would
> just need to set that priority in path_group_prio_update(), and if
> you
> only wanted to use the priority for sorting them, you would just
> change
> sort_pathgroups() to only sort by priority. If you wanted to change
> pgp->marginal from something binary to something that reflected how
> marginal a path was, and you want to have that to change how a path
> was
> sorted vs other paths with different marginal and priority values,
> you
> could do all of that by simply changing the values set in
> path_group_prio_update() and how those values are sorted in
> sort_pathgroups(). I don't think my code does anything to make this
> less
> flexible.
I guess you are right here, too. (*)
> As for the third issue, how the best pathgroup is chosen, this is
> also a
> case where changing things just involves changing how things are done
> in
> one function, select_path_group(), to match what's done in
> sort_pathgroups(). But since the pathgroups are already in order from
> the best one to use to the worst, the multipath userspace tools could
> just work how the kernel works, and pick the first usable pathgroup.
> This won't always give the same answer that multipath currently does,
> since the current code looks at the number of enabled paths. But the
> only times when it will be different is when there are multiple
> pathgroups with the same priority, and the first one has some failed
> paths. Since we rarely have multiple pathgroups with the same
> priorty
> except when using the failover policy (group_by_serial and
> group_by_node_name being rarely used), and you will always have one
> path
> per pathgroup with failover, in practice this will almost never
> occur.
I don't see an inconsistency here. sort_pathgroups() also takes the
number of enabled paths into account; it's just not always called when
paths are failed or reinstated. The point is just that in the corner
case you describe, we run a lightweight "switch_group" ioctl rather
than reloading the map with a new PG ordering, which is good. (**)
> Oh, I did notice a bug in my select_path_group() code. I should be
> calling path_group_prio_update() before checking if the pathgroup
> is marginal or not. I'll fix that.
>
> So, I'm not against making this all work with priorities if there is
> a
> reason to do so, but doing that will just involve changes in 3
> straightforward functions, or 2 if we decide to simplify
> select_path_group() so it just uses the order from sort_pathgroups()
> as
> a guide.
>
> If you feel strongly about doing this with priorities, I can add a
> patch
> that changes this. But if it gets us the same results, then I don't
> see
> much benefit. We can always change it laster if we want to change how
> the groups actually end up.
I had a discussion with Hannes, and it went rather the opposite way of
what I'd written in my previous post (consider multi-dimentional
priorities/grouping for the future). He strongly supported your notion
that "marginality" should be the most important grouping criterion /
priority factor, taking precedence over other factors.
So yes, you've mostly convinced me. Please re-post with the fixes you
mentioned, and I'll ack the series. I still have a number of open
issues with how we do prioritizing and grouping in general, but I agree
your series is a step in the right direction.
Cheers, Martin
(*) Note to self: It just occured to me that rather than using sorting,
we could use dm-multipaths's concept of "disabled" or "bypassed" path
groups to represent marginal groups. When switching PGs, the kernel
tries bypassed PGs only if all others have no valid paths, so this fits
your marginal model quite well. But then, we wouldn't have the
flexibility that PG sorting provides, as you argued. Moreover, PGs
can't start out bypassed when a map is reloaded, opening a short race
window where the kernel could try using these PGs. Finally, the kernel
sets the bypassed flag when a SCSI DH returns SCSI_DH_DEV_TEMP_BUSY,
which is used by the EMC and ALUA DHs, and at least by the latter in a
questionable way (indicating unspecific errors). I find this whole
kernel logic weird - why disable the entire PG if a DH error occurs on
a single path? - but that's a different issue.
(**) In the corner-case-in-corner-case where all paths of this selected
PG (which is now not the first) fail, the kernel might come to a
different fallback conclusion as if we had re-sorted PGs. But I think
this can be ignored).
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 11/16] libmultipath: make group_paths handle marginal paths
2019-08-02 16:33 [PATCH 00/16] multipath marginal pathgroups Benjamin Marzinski
` (9 preceding siblings ...)
2019-08-02 16:33 ` [PATCH 10/16] libmultipath: make pgpolicyfn take a paths vector Benjamin Marzinski
@ 2019-08-02 16:33 ` Benjamin Marzinski
2019-08-02 16:33 ` [PATCH 12/16] tests: add tests for grouping " Benjamin Marzinski
` (4 subsequent siblings)
15 siblings, 0 replies; 41+ messages in thread
From: Benjamin Marzinski @ 2019-08-02 16:33 UTC (permalink / raw)
To: Christophe Varoqui
Cc: device-mapper development, Martin Wilck, Muneendra Kumar
group_paths() will now create seperate path groups for marginal and
normal paths, and place all of the marginal path groups after the normal
ones, in order by priority.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/pgpolicies.c | 83 +++++++++++++++++++++++++++++++++-----
libmultipath/switchgroup.c | 15 ++++++-
2 files changed, 88 insertions(+), 10 deletions(-)
diff --git a/libmultipath/pgpolicies.c b/libmultipath/pgpolicies.c
index 2e7c0502..6fb2d28a 100644
--- a/libmultipath/pgpolicies.c
+++ b/libmultipath/pgpolicies.c
@@ -72,9 +72,11 @@ sort_pathgroups (struct multipath *mp) {
pgp2 = VECTOR_SLOT(mp->pg, j);
if (!pgp2)
continue;
- if (pgp2->priority > pgp1->priority ||
- (pgp2->priority == pgp1->priority &&
- pgp2->enabled_paths >= pgp1->enabled_paths)) {
+ if (pgp2->marginal < pgp1->marginal ||
+ (pgp2->marginal == pgp1->marginal &&
+ (pgp2->priority > pgp1->priority ||
+ (pgp2->priority == pgp1->priority &&
+ pgp2->enabled_paths >= pgp1->enabled_paths)))) {
vector_move_up(mp->pg, i, j + 1);
break;
}
@@ -84,25 +86,88 @@ sort_pathgroups (struct multipath *mp) {
}
}
+static int
+split_marginal_paths(vector paths, vector *normal_p, vector *marginal_p)
+{
+ int i;
+ int has_marginal = 0;
+ int has_normal = 0;
+ struct path *pp;
+ vector normal = NULL;
+ vector marginal = NULL;
+
+ *normal_p = *marginal_p = NULL;
+ vector_foreach_slot(paths, pp, i) {
+ if (pp->marginal)
+ has_marginal = 1;
+ else
+ has_normal = 1;
+ }
+
+ if (!has_marginal || !has_normal)
+ return -1;
+
+ normal = vector_alloc();
+ marginal = vector_alloc();
+ if (!normal || !marginal)
+ goto fail;
+
+ vector_foreach_slot(paths, pp, i) {
+ if (pp->marginal) {
+ if (store_path(marginal, pp))
+ goto fail;
+ }
+ else {
+ if (store_path(normal, pp))
+ goto fail;
+ }
+ }
+ *normal_p = normal;
+ *marginal_p = marginal;
+ return 0;
+fail:
+ vector_free(normal);
+ vector_free(marginal);
+ return -1;
+}
int group_paths(struct multipath *mp)
{
+ vector normal, marginal;
+
if (!mp->pg)
mp->pg = vector_alloc();
if (!mp->pg)
return 1;
- if (VECTOR_SIZE(mp->paths) > 0 &&
- (!mp->pgpolicyfn || mp->pgpolicyfn(mp, mp->paths))) {
- vector_free(mp->pg);
- mp->pg = NULL;
- return 1;
+ if (VECTOR_SIZE(mp->paths) == 0)
+ goto out;
+ if (!mp->pgpolicyfn)
+ goto fail;
+
+ if (split_marginal_paths(mp->paths, &normal, &marginal) != 0) {
+ if (mp->pgpolicyfn(mp, mp->paths) != 0)
+ goto fail;
+ } else {
+ if (mp->pgpolicyfn(mp, normal) != 0)
+ goto fail_marginal;
+ if (mp->pgpolicyfn(mp, marginal) != 0)
+ goto fail_marginal;
+ vector_free(normal);
+ vector_free(marginal);
}
-
sort_pathgroups(mp);
+out:
vector_free(mp->paths);
mp->paths = NULL;
return 0;
+fail_marginal:
+ vector_free(normal);
+ vector_free(marginal);
+fail:
+ vector_free(mp->pg);
+ mp->pg = NULL;
+ return 1;
}
typedef bool (path_match_fn)(struct path *pp1, struct path *pp2);
diff --git a/libmultipath/switchgroup.c b/libmultipath/switchgroup.c
index 9632ce2d..d03cb6fd 100644
--- a/libmultipath/switchgroup.c
+++ b/libmultipath/switchgroup.c
@@ -11,6 +11,7 @@ void path_group_prio_update(struct pathgroup *pgp)
{
int i;
int priority = 0;
+ int marginal = 0;
struct path * pp;
pgp->enabled_paths = 0;
@@ -19,6 +20,8 @@ void path_group_prio_update(struct pathgroup *pgp)
return;
}
vector_foreach_slot (pgp->paths, pp, i) {
+ if (pp->marginal)
+ marginal++;
if (pp->state == PATH_UP ||
pp->state == PATH_GHOST) {
priority += pp->priority;
@@ -29,11 +32,14 @@ void path_group_prio_update(struct pathgroup *pgp)
pgp->priority = priority / pgp->enabled_paths;
else
pgp->priority = 0;
+ if (marginal && marginal == i)
+ pgp->marginal = 1;
}
int select_path_group(struct multipath *mpp)
{
int i;
+ int normal_pgp = 0;
int max_priority = 0;
int bestpg = 1;
int max_enabled_paths = 1;
@@ -45,10 +51,17 @@ int select_path_group(struct multipath *mpp)
vector_foreach_slot (mpp->pg, pgp, i) {
if (!pgp->paths)
continue;
+ if (pgp->marginal && normal_pgp)
+ continue;
path_group_prio_update(pgp);
if (pgp->enabled_paths) {
- if (pgp->priority > max_priority) {
+ if (!pgp->marginal && !normal_pgp) {
+ normal_pgp = 1;
+ max_priority = pgp->priority;
+ max_enabled_paths = pgp->enabled_paths;
+ bestpg = i + 1;
+ } else if (pgp->priority > max_priority) {
max_priority = pgp->priority;
max_enabled_paths = pgp->enabled_paths;
bestpg = i + 1;
--
2.17.2
^ permalink raw reply related [flat|nested] 41+ messages in thread* [PATCH 12/16] tests: add tests for grouping marginal paths.
2019-08-02 16:33 [PATCH 00/16] multipath marginal pathgroups Benjamin Marzinski
` (10 preceding siblings ...)
2019-08-02 16:33 ` [PATCH 11/16] libmultipath: make group_paths handle marginal paths Benjamin Marzinski
@ 2019-08-02 16:33 ` Benjamin Marzinski
2019-08-02 16:33 ` [PATCH 13/16] libmultipath: add marginal_pathgroups config option Benjamin Marzinski
` (3 subsequent siblings)
15 siblings, 0 replies; 41+ messages in thread
From: Benjamin Marzinski @ 2019-08-02 16:33 UTC (permalink / raw)
To: Christophe Varoqui
Cc: device-mapper development, Martin Wilck, Muneendra Kumar
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
tests/pgpolicy.c | 337 +++++++++++++++++++++++++++++++++++++++++------
1 file changed, 295 insertions(+), 42 deletions(-)
diff --git a/tests/pgpolicy.c b/tests/pgpolicy.c
index 04a77c4c..ab09f91c 100644
--- a/tests/pgpolicy.c
+++ b/tests/pgpolicy.c
@@ -40,6 +40,15 @@ static void set_priority(struct path *pp, int *prio, int size)
}
}
+static void set_marginal(struct path *pp, int *marginal, int size)
+{
+ int i;
+
+ for (i = 0; i < size; i++) {
+ pp[i].marginal = marginal[i];
+ }
+}
+
static void set_tgt_node_name(struct path *pp, char **tgt_node_name, int size)
{
int i;
@@ -82,6 +91,7 @@ static int setupX(struct multipath *mp, struct path *pp, int size)
{
int i;
int prio[8] = {10, 10, 10, 10, 10, 10, 10, 10};
+ int marginal[8] = {0, 0, 0, 0, 0, 0, 0, 0};
mp->paths = vector_alloc();
if (!mp->paths)
@@ -92,6 +102,7 @@ static int setupX(struct multipath *mp, struct path *pp, int size)
vector_set_slot(mp->paths, &pp[i]);
}
set_priority(pp, prio, size);
+ set_marginal(pp, marginal, size);
mp->pgpolicyfn = NULL;
return 0;
}
@@ -155,7 +166,7 @@ static int teardown_null(void **state)
static void
verify_pathgroups(struct multipath *mp, struct path *pp, int **groups,
- int *group_size, int size)
+ int *group_size, int *marginal, int size)
{
int i, j;
struct pathgroup *pgp;
@@ -168,6 +179,10 @@ verify_pathgroups(struct multipath *mp, struct path *pp, int **groups,
assert_non_null(pgp);
assert_non_null(pgp->paths);
assert_int_equal(VECTOR_SIZE(pgp->paths), group_size[i]);
+ if (marginal)
+ assert_int_equal(pgp->marginal, marginal[i]);
+ else
+ assert_int_equal(pgp->marginal, 0);
for (j = 0; j < group_size[i]; j++) {
int path_nr = groups[i][j];
struct path *pgp_path = VECTOR_SLOT(pgp->paths, j);
@@ -190,7 +205,7 @@ static void test_one_group8(void **state)
mp8.pgpolicyfn = one_group;
assert_int_equal(group_paths(&mp8), 0);
- verify_pathgroups(&mp8, p8, groups, group_size, 1);
+ verify_pathgroups(&mp8, p8, groups, group_size, NULL, 1);
}
static void test_one_group4(void **state)
@@ -201,7 +216,7 @@ static void test_one_group4(void **state)
mp4.pgpolicyfn = one_group;
assert_int_equal(group_paths(&mp4), 0);
- verify_pathgroups(&mp4, p4, groups, group_size, 1);
+ verify_pathgroups(&mp4, p4, groups, group_size, NULL, 1);
}
static void test_one_group1(void **state)
@@ -212,21 +227,65 @@ static void test_one_group1(void **state)
mp1.pgpolicyfn = one_group;
assert_int_equal(group_paths(&mp1), 0);
- verify_pathgroups(&mp1, p1, groups, group_size, 1);
+ verify_pathgroups(&mp1, p1, groups, group_size, NULL, 1);
}
static void test_one_group0(void **state)
{
mp0.pgpolicyfn = one_group;
assert_int_equal(group_paths(&mp0), 0);
- verify_pathgroups(&mp0, NULL, NULL, NULL, 0);
+ verify_pathgroups(&mp0, NULL, NULL, NULL, NULL, 0);
}
static void test_one_group_null(void **state)
{
mp_null.pgpolicyfn = one_group;
assert_int_equal(group_paths(&mp_null), 0);
- verify_pathgroups(&mp_null, NULL, NULL, NULL, 0);
+ verify_pathgroups(&mp_null, NULL, NULL, NULL, NULL, 0);
+}
+
+static void test_one_group_all_marginal8(void **state)
+{
+ int paths[] = {0,1,2,3,4,5,6,7};
+ int marginal[] = {1,1,1,1,1,1,1,1};
+ int *groups[] = {paths};
+ int group_size[] = {8};
+ int group_marginal[] = {1};
+
+ set_marginal(p8, marginal, 8);
+ mp8.pgpolicyfn = one_group;
+ assert_int_equal(group_paths(&mp8), 0);
+ verify_pathgroups(&mp8, p8, groups, group_size, group_marginal, 1);
+}
+
+static void test_one_group_half_marginal8(void **state)
+{
+ int marginal[] = {1,0,1,0,1,1,0,0};
+ int group0[] = {1,3,6,7};
+ int group1[] = {0,2,4,5};
+ int *groups[] = {group0, group1};
+ int group_size[] = {4,4};
+ int group_marginal[] = {0,1};
+
+ set_marginal(p8, marginal, 8);
+ mp8.pgpolicyfn = one_group;
+ assert_int_equal(group_paths(&mp8), 0);
+ verify_pathgroups(&mp8, p8, groups, group_size, group_marginal, 2);
+}
+
+static void test_one_group_one_marginal8(void **state)
+{
+ int marginal[] = {0,0,0,0,0,1,0,0};
+ int group0[] = {0,1,2,3,4,6,7};
+ int group1[] = {5};
+ int *groups[] = {group0, group1};
+ int group_size[] = {7,1};
+ int group_marginal[] = {0,1};
+
+ set_marginal(p8, marginal, 8);
+ mp8.pgpolicyfn = one_group;
+ assert_int_equal(group_paths(&mp8), 0);
+ verify_pathgroups(&mp8, p8, groups, group_size, group_marginal, 2);
}
static void test_one_path_per_group_same8(void **state)
@@ -238,7 +297,7 @@ static void test_one_path_per_group_same8(void **state)
mp8.pgpolicyfn = one_path_per_group;
assert_int_equal(group_paths(&mp8), 0);
- verify_pathgroups(&mp8, p8, groups, group_size, 8);
+ verify_pathgroups(&mp8, p8, groups, group_size, NULL, 8);
}
static void test_one_path_per_group_increasing8(void **state)
@@ -252,7 +311,7 @@ static void test_one_path_per_group_increasing8(void **state)
set_priority(p8, prio, 8);
mp8.pgpolicyfn = one_path_per_group;
assert_int_equal(group_paths(&mp8), 0);
- verify_pathgroups(&mp8, p8, groups, group_size, 8);
+ verify_pathgroups(&mp8, p8, groups, group_size, NULL, 8);
}
static void test_one_path_per_group_decreasing8(void **state)
@@ -266,7 +325,7 @@ static void test_one_path_per_group_decreasing8(void **state)
set_priority(p8, prio, 8);
mp8.pgpolicyfn = one_path_per_group;
assert_int_equal(group_paths(&mp8), 0);
- verify_pathgroups(&mp8, p8, groups, group_size, 8);
+ verify_pathgroups(&mp8, p8, groups, group_size, NULL, 8);
}
static void test_one_path_per_group_mixed8(void **state)
@@ -280,7 +339,7 @@ static void test_one_path_per_group_mixed8(void **state)
set_priority(p8, prio, 8);
mp8.pgpolicyfn = one_path_per_group;
assert_int_equal(group_paths(&mp8), 0);
- verify_pathgroups(&mp8, p8, groups, group_size, 8);
+ verify_pathgroups(&mp8, p8, groups, group_size, NULL, 8);
}
static void test_one_path_per_group4(void **state)
@@ -291,7 +350,7 @@ static void test_one_path_per_group4(void **state)
mp4.pgpolicyfn = one_path_per_group;
assert_int_equal(group_paths(&mp4), 0);
- verify_pathgroups(&mp4, p4, groups, group_size, 4);
+ verify_pathgroups(&mp4, p4, groups, group_size, NULL, 4);
}
static void test_one_path_per_group1(void **state)
@@ -302,21 +361,55 @@ static void test_one_path_per_group1(void **state)
mp1.pgpolicyfn = one_path_per_group;
assert_int_equal(group_paths(&mp1), 0);
- verify_pathgroups(&mp1, p1, groups, group_size, 1);
+ verify_pathgroups(&mp1, p1, groups, group_size, NULL, 1);
}
static void test_one_path_per_group0(void **state)
{
mp0.pgpolicyfn = one_path_per_group;
assert_int_equal(group_paths(&mp0), 0);
- verify_pathgroups(&mp0, NULL, NULL, NULL, 0);
+ verify_pathgroups(&mp0, NULL, NULL, NULL, NULL, 0);
}
static void test_one_path_per_group_null(void **state)
{
mp_null.pgpolicyfn = one_path_per_group;
assert_int_equal(group_paths(&mp_null), 0);
- verify_pathgroups(&mp_null, NULL, NULL, NULL, 0);
+ verify_pathgroups(&mp_null, NULL, NULL, NULL, NULL, 0);
+}
+
+static void test_one_path_per_group_mixed_all_marginal8(void **state)
+{
+ int prio[] = {7,1,3,3,5,2,8,2};
+ int marginal[] = {1,1,1,1,1,1,1,1};
+ int paths[] = {6,0,4,2,3,5,7,1};
+ 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};
+ int group_marginal[] = {1,1,1,1,1,1,1,1};
+
+ set_priority(p8, prio, 8);
+ set_marginal(p8, marginal, 8);
+ mp8.pgpolicyfn = one_path_per_group;
+ assert_int_equal(group_paths(&mp8), 0);
+ verify_pathgroups(&mp8, p8, groups, group_size, group_marginal, 8);
+}
+
+static void test_one_path_per_group_mixed_half_marginal8(void **state)
+{
+ int prio[] = {7,1,3,3,5,2,8,2};
+ int marginal[] = {0,1,1,0,0,0,1,1};
+ int paths[] = {0,4,3,5,6,2,7,1};
+ 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};
+ int group_marginal[] = {0,0,0,0,1,1,1,1};
+
+ set_priority(p8, prio, 8);
+ set_marginal(p8, marginal, 8);
+ mp8.pgpolicyfn = one_path_per_group;
+ assert_int_equal(group_paths(&mp8), 0);
+ verify_pathgroups(&mp8, p8, groups, group_size, group_marginal, 8);
}
static void test_group_by_prio_same8(void **state)
@@ -327,7 +420,7 @@ static void test_group_by_prio_same8(void **state)
mp8.pgpolicyfn = group_by_prio;
assert_int_equal(group_paths(&mp8), 0);
- verify_pathgroups(&mp8, p8, groups, group_size, 1);
+ verify_pathgroups(&mp8, p8, groups, group_size, NULL, 1);
}
static void test_group_by_prio_increasing8(void **state)
@@ -341,7 +434,7 @@ static void test_group_by_prio_increasing8(void **state)
set_priority(p8, prio, 8);
mp8.pgpolicyfn = group_by_prio;
assert_int_equal(group_paths(&mp8), 0);
- verify_pathgroups(&mp8, p8, groups, group_size, 8);
+ verify_pathgroups(&mp8, p8, groups, group_size, NULL, 8);
}
static void test_group_by_prio_decreasing8(void **state)
@@ -355,7 +448,7 @@ static void test_group_by_prio_decreasing8(void **state)
set_priority(p8, prio, 8);
mp8.pgpolicyfn = group_by_prio;
assert_int_equal(group_paths(&mp8), 0);
- verify_pathgroups(&mp8, p8, groups, group_size, 8);
+ verify_pathgroups(&mp8, p8, groups, group_size, NULL, 8);
}
static void test_group_by_prio_mixed8(void **state)
@@ -374,7 +467,7 @@ static void test_group_by_prio_mixed8(void **state)
set_priority(p8, prio, 8);
mp8.pgpolicyfn = group_by_prio;
assert_int_equal(group_paths(&mp8), 0);
- verify_pathgroups(&mp8, p8, groups, group_size, 6);
+ verify_pathgroups(&mp8, p8, groups, group_size, NULL, 6);
}
static void test_group_by_prio_2_groups8(void **state)
@@ -388,7 +481,7 @@ static void test_group_by_prio_2_groups8(void **state)
set_priority(p8, prio, 8);
mp8.pgpolicyfn = group_by_prio;
assert_int_equal(group_paths(&mp8), 0);
- verify_pathgroups(&mp8, p8, groups, group_size, 2);
+ verify_pathgroups(&mp8, p8, groups, group_size, NULL, 2);
}
static void test_group_by_prio_mixed4(void **state)
@@ -403,7 +496,7 @@ static void test_group_by_prio_mixed4(void **state)
set_priority(p4, prio, 4);
mp4.pgpolicyfn = group_by_prio;
assert_int_equal(group_paths(&mp4), 0);
- verify_pathgroups(&mp4, p4, groups, group_size, 3);
+ verify_pathgroups(&mp4, p4, groups, group_size, NULL, 3);
}
static void test_group_by_prio_2_groups4(void **state)
@@ -417,7 +510,7 @@ static void test_group_by_prio_2_groups4(void **state)
set_priority(p4, prio, 4);
mp4.pgpolicyfn = group_by_prio;
assert_int_equal(group_paths(&mp4), 0);
- verify_pathgroups(&mp4, p4, groups, group_size, 2);
+ verify_pathgroups(&mp4, p4, groups, group_size, NULL, 2);
}
static void test_group_by_prio1(void **state)
@@ -428,21 +521,89 @@ static void test_group_by_prio1(void **state)
mp1.pgpolicyfn = group_by_prio;
assert_int_equal(group_paths(&mp1), 0);
- verify_pathgroups(&mp1, p1, groups, group_size, 1);
+ verify_pathgroups(&mp1, p1, groups, group_size, NULL, 1);
}
static void test_group_by_prio0(void **state)
{
mp0.pgpolicyfn = group_by_prio;
assert_int_equal(group_paths(&mp0), 0);
- verify_pathgroups(&mp0, NULL, NULL, NULL, 0);
+ verify_pathgroups(&mp0, NULL, NULL, NULL, NULL, 0);
}
static void test_group_by_prio_null(void **state)
{
mp_null.pgpolicyfn = group_by_prio;
assert_int_equal(group_paths(&mp_null), 0);
- verify_pathgroups(&mp_null, NULL, NULL, NULL, 0);
+ verify_pathgroups(&mp_null, NULL, NULL, NULL, NULL, 0);
+}
+
+static void test_group_by_prio_mixed_all_marginal8(void **state)
+{
+ int prio[] = {7,1,3,3,5,2,8,2};
+ 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[] = {5,7};
+ int group5[] = {1};
+ 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_marginal(p8, marginal, 8);
+ mp8.pgpolicyfn = group_by_prio;
+ assert_int_equal(group_paths(&mp8), 0);
+ verify_pathgroups(&mp8, p8, groups, group_size, group_marginal, 6);
+}
+
+static void test_group_by_prio_mixed_half_marginal8(void **state)
+{
+ int prio[] = {7,1,3,3,5,2,8,2};
+ int marginal[] = {0,0,0,1,0,1,1,1};
+ int group0[] = {0};
+ int group1[] = {4};
+ int group2[] = {2};
+ 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_marginal(p8, marginal, 8);
+ mp8.pgpolicyfn = group_by_prio;
+ assert_int_equal(group_paths(&mp8), 0);
+ verify_pathgroups(&mp8, p8, groups, group_size, group_marginal, 7);
+}
+
+static void test_group_by_prio_mixed_one_marginal8(void **state)
+{
+ int prio[] = {7,1,3,3,5,2,8,2};
+ 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_marginal(p8, marginal, 8);
+ mp8.pgpolicyfn = group_by_prio;
+ assert_int_equal(group_paths(&mp8), 0);
+ verify_pathgroups(&mp8, p8, groups, group_size, group_marginal, 7);
}
static void test_group_by_node_name_same8(void **state)
@@ -455,7 +616,7 @@ static void test_group_by_node_name_same8(void **state)
set_tgt_node_name(p8, node_name, 8);
mp8.pgpolicyfn = group_by_node_name;
assert_int_equal(group_paths(&mp8), 0);
- verify_pathgroups(&mp8, p8, groups, group_size, 1);
+ verify_pathgroups(&mp8, p8, groups, group_size, NULL, 1);
}
static void test_group_by_node_name_increasing8(void **state)
@@ -471,7 +632,7 @@ static void test_group_by_node_name_increasing8(void **state)
set_tgt_node_name(p8, node_name, 8);
mp8.pgpolicyfn = group_by_node_name;
assert_int_equal(group_paths(&mp8), 0);
- verify_pathgroups(&mp8, p8, groups, group_size, 8);
+ verify_pathgroups(&mp8, p8, groups, group_size, NULL, 8);
}
static void test_group_by_node_name_3_groups8(void **state)
@@ -488,7 +649,7 @@ static void test_group_by_node_name_3_groups8(void **state)
set_tgt_node_name(p8, node_name, 8);
mp8.pgpolicyfn = group_by_node_name;
assert_int_equal(group_paths(&mp8), 0);
- verify_pathgroups(&mp8, p8, groups, group_size, 3);
+ verify_pathgroups(&mp8, p8, groups, group_size, NULL, 3);
}
static void test_group_by_node_name_2_groups8(void **state)
@@ -504,7 +665,7 @@ static void test_group_by_node_name_2_groups8(void **state)
set_tgt_node_name(p8, node_name, 8);
mp8.pgpolicyfn = group_by_node_name;
assert_int_equal(group_paths(&mp8), 0);
- verify_pathgroups(&mp8, p8, groups, group_size, 2);
+ verify_pathgroups(&mp8, p8, groups, group_size, NULL, 2);
}
static void test_group_by_node_name_3_groups4(void **state)
@@ -521,7 +682,7 @@ static void test_group_by_node_name_3_groups4(void **state)
set_tgt_node_name(p4, node_name, 4);
mp4.pgpolicyfn = group_by_node_name;
assert_int_equal(group_paths(&mp4), 0);
- verify_pathgroups(&mp4, p4, groups, group_size, 3);
+ verify_pathgroups(&mp4, p4, groups, group_size, NULL, 3);
}
static void test_group_by_node_name_2_groups4(void **state)
@@ -537,7 +698,7 @@ static void test_group_by_node_name_2_groups4(void **state)
set_tgt_node_name(p4, node_name, 4);
mp4.pgpolicyfn = group_by_node_name;
assert_int_equal(group_paths(&mp4), 0);
- verify_pathgroups(&mp4, p4, groups, group_size, 2);
+ verify_pathgroups(&mp4, p4, groups, group_size, NULL, 2);
}
static void test_group_by_node_name1(void **state)
@@ -550,21 +711,61 @@ static void test_group_by_node_name1(void **state)
set_tgt_node_name(p1, node_name, 1);
mp1.pgpolicyfn = group_by_node_name;
assert_int_equal(group_paths(&mp1), 0);
- verify_pathgroups(&mp1, p1, groups, group_size, 1);
+ verify_pathgroups(&mp1, p1, groups, group_size, NULL, 1);
}
static void test_group_by_node_name0(void **state)
{
mp0.pgpolicyfn = group_by_node_name;
assert_int_equal(group_paths(&mp0), 0);
- verify_pathgroups(&mp0, NULL, NULL, NULL, 0);
+ verify_pathgroups(&mp0, NULL, NULL, NULL, NULL, 0);
}
static void test_group_by_node_name_null(void **state)
{
mp_null.pgpolicyfn = group_by_node_name;
assert_int_equal(group_paths(&mp_null), 0);
- verify_pathgroups(&mp_null, NULL, NULL, NULL, 0);
+ verify_pathgroups(&mp_null, NULL, NULL, NULL, NULL, 0);
+}
+
+static void test_group_by_node_name_2_groups_all_marginal8(void **state)
+{
+ char *node_name[] = {"a", "a", "b", "a", "b", "b", "b", "a"};
+ int prio[] = {4,1,2,1,2,2,2,1};
+ int marginal[] = {1,1,1,1,1,1,1,1};
+ int group0[] = {2,4,5,6};
+ int group1[] = {0,1,3,7};
+ int *groups[] = {group0, group1};
+ int group_size[] = {4,4};
+ int group_marginal[] = {1,1};
+
+ set_priority(p8, prio, 8);
+ set_marginal(p8, marginal, 8);
+ set_tgt_node_name(p8, node_name, 8);
+ mp8.pgpolicyfn = group_by_node_name;
+ assert_int_equal(group_paths(&mp8), 0);
+ verify_pathgroups(&mp8, p8, groups, group_size, group_marginal, 2);
+}
+
+static void test_group_by_node_name_2_groups_half_marginal8(void **state)
+{
+ char *node_name[] = {"a", "a", "b", "a", "b", "b", "b", "a"};
+ int prio[] = {4,1,2,1,2,2,2,1};
+ int marginal[] = {1,0,1,1,0,1,0,0};
+ int group0[] = {4,6};
+ int group1[] = {1,7};
+ int group2[] = {0,3};
+ int group3[] = {2,5};
+ int *groups[] = {group0, group1, group2, group3};
+ int group_size[] = {2,2,2,2};
+ int group_marginal[] = {0,0,1,1};
+
+ set_priority(p8, prio, 8);
+ set_marginal(p8, marginal, 8);
+ set_tgt_node_name(p8, node_name, 8);
+ mp8.pgpolicyfn = group_by_node_name;
+ assert_int_equal(group_paths(&mp8), 0);
+ verify_pathgroups(&mp8, p8, groups, group_size, group_marginal, 4);
}
static void test_group_by_serial_same8(void **state)
@@ -577,7 +778,7 @@ static void test_group_by_serial_same8(void **state)
set_serial(p8, serial, 8);
mp8.pgpolicyfn = group_by_serial;
assert_int_equal(group_paths(&mp8), 0);
- verify_pathgroups(&mp8, p8, groups, group_size, 1);
+ verify_pathgroups(&mp8, p8, groups, group_size, NULL, 1);
}
static void test_group_by_serial_increasing8(void **state)
@@ -593,7 +794,7 @@ static void test_group_by_serial_increasing8(void **state)
set_serial(p8, serial, 8);
mp8.pgpolicyfn = group_by_serial;
assert_int_equal(group_paths(&mp8), 0);
- verify_pathgroups(&mp8, p8, groups, group_size, 8);
+ verify_pathgroups(&mp8, p8, groups, group_size, NULL, 8);
}
static void test_group_by_serial_3_groups8(void **state)
@@ -610,7 +811,7 @@ static void test_group_by_serial_3_groups8(void **state)
set_serial(p8, serial, 8);
mp8.pgpolicyfn = group_by_serial;
assert_int_equal(group_paths(&mp8), 0);
- verify_pathgroups(&mp8, p8, groups, group_size, 3);
+ verify_pathgroups(&mp8, p8, groups, group_size, NULL, 3);
}
static void test_group_by_serial_2_groups8(void **state)
@@ -626,7 +827,7 @@ static void test_group_by_serial_2_groups8(void **state)
set_serial(p8, serial, 8);
mp8.pgpolicyfn = group_by_serial;
assert_int_equal(group_paths(&mp8), 0);
- verify_pathgroups(&mp8, p8, groups, group_size, 2);
+ verify_pathgroups(&mp8, p8, groups, group_size, NULL, 2);
}
static void test_group_by_serial_3_groups4(void **state)
@@ -643,7 +844,7 @@ static void test_group_by_serial_3_groups4(void **state)
set_serial(p4, serial, 4);
mp4.pgpolicyfn = group_by_serial;
assert_int_equal(group_paths(&mp4), 0);
- verify_pathgroups(&mp4, p4, groups, group_size, 3);
+ verify_pathgroups(&mp4, p4, groups, group_size, NULL, 3);
}
static void test_group_by_serial_2_groups4(void **state)
@@ -659,7 +860,7 @@ static void test_group_by_serial_2_groups4(void **state)
set_serial(p4, serial, 4);
mp4.pgpolicyfn = group_by_serial;
assert_int_equal(group_paths(&mp4), 0);
- verify_pathgroups(&mp4, p4, groups, group_size, 2);
+ verify_pathgroups(&mp4, p4, groups, group_size, NULL, 2);
}
static void test_group_by_serial1(void **state)
@@ -672,21 +873,61 @@ static void test_group_by_serial1(void **state)
set_serial(p1, serial, 1);
mp1.pgpolicyfn = group_by_serial;
assert_int_equal(group_paths(&mp1), 0);
- verify_pathgroups(&mp1, p1, groups, group_size, 1);
+ verify_pathgroups(&mp1, p1, groups, group_size, NULL, 1);
}
static void test_group_by_serial0(void **state)
{
mp0.pgpolicyfn = group_by_serial;
assert_int_equal(group_paths(&mp0), 0);
- verify_pathgroups(&mp0, NULL, NULL, NULL, 0);
+ verify_pathgroups(&mp0, NULL, NULL, NULL, NULL, 0);
}
static void test_group_by_serial_null(void **state)
{
mp_null.pgpolicyfn = group_by_serial;
assert_int_equal(group_paths(&mp_null), 0);
- verify_pathgroups(&mp_null, NULL, NULL, NULL, 0);
+ verify_pathgroups(&mp_null, NULL, NULL, NULL, NULL, 0);
+}
+
+static void test_group_by_serial_2_groups8_all_marginal8(void **state)
+{
+ char *serial[] = {"1", "2", "1", "1", "2", "2", "1", "2"};
+ int marginal[] = {1,1,1,1,1,1,1,1};
+ int prio[] = {3,2,2,1,2,2,1,2};
+ int group0[] = {1,4,5,7};
+ int group1[] = {0,2,3,6};
+ int *groups[] = {group0, group1};
+ int group_size[] = {4,4};
+ int group_marginal[] = {1,1};
+
+ set_priority(p8, prio, 8);
+ set_serial(p8, serial, 8);
+ set_marginal(p8, marginal, 8);
+ mp8.pgpolicyfn = group_by_serial;
+ assert_int_equal(group_paths(&mp8), 0);
+ verify_pathgroups(&mp8, p8, groups, group_size, group_marginal, 2);
+}
+
+static void test_group_by_serial_2_groups8_half_marginal8(void **state)
+{
+ char *serial[] = {"1", "2", "1", "1", "2", "2", "1", "2"};
+ int marginal[] = {0,0,1,1,1,1,0,0};
+ int prio[] = {3,2,2,1,2,2,1,2};
+ int group0[] = {0,6};
+ int group1[] = {1,7};
+ int group2[] = {4,5};
+ int group3[] = {2,3};
+ int *groups[] = {group0, group1, group2, group3};
+ int group_size[] = {2,2,2,2};
+ int group_marginal[] = {0,0,1,1};
+
+ set_priority(p8, prio, 8);
+ set_serial(p8, serial, 8);
+ set_marginal(p8, marginal, 8);
+ mp8.pgpolicyfn = group_by_serial;
+ assert_int_equal(group_paths(&mp8), 0);
+ verify_pathgroups(&mp8, p8, groups, group_size, group_marginal, 4);
}
#define setup_test(name, nr) \
@@ -700,6 +941,9 @@ int test_pgpolicies(void)
setup_test(test_one_group, 1),
setup_test(test_one_group, 0),
setup_test(test_one_group, _null),
+ setup_test(test_one_group_all_marginal, 8),
+ setup_test(test_one_group_half_marginal, 8),
+ setup_test(test_one_group_one_marginal, 8),
setup_test(test_one_path_per_group_same, 8),
setup_test(test_one_path_per_group_increasing, 8),
setup_test(test_one_path_per_group_decreasing, 8),
@@ -708,6 +952,8 @@ int test_pgpolicies(void)
setup_test(test_one_path_per_group, 1),
setup_test(test_one_path_per_group, 0),
setup_test(test_one_path_per_group, _null),
+ setup_test(test_one_path_per_group_mixed_all_marginal, 8),
+ setup_test(test_one_path_per_group_mixed_half_marginal, 8),
setup_test(test_group_by_prio_same, 8),
setup_test(test_group_by_prio_increasing, 8),
setup_test(test_group_by_prio_decreasing, 8),
@@ -718,6 +964,9 @@ int test_pgpolicies(void)
setup_test(test_group_by_prio, 1),
setup_test(test_group_by_prio, 0),
setup_test(test_group_by_prio, _null),
+ 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_node_name_same, 8),
setup_test(test_group_by_node_name_increasing, 8),
setup_test(test_group_by_node_name_3_groups, 8),
@@ -727,6 +976,8 @@ int test_pgpolicies(void)
setup_test(test_group_by_node_name, 1),
setup_test(test_group_by_node_name, 0),
setup_test(test_group_by_node_name, _null),
+ setup_test(test_group_by_node_name_2_groups_all_marginal, 8),
+ setup_test(test_group_by_node_name_2_groups_half_marginal, 8),
setup_test(test_group_by_serial_same, 8),
setup_test(test_group_by_serial_increasing, 8),
setup_test(test_group_by_serial_3_groups, 8),
@@ -736,6 +987,8 @@ int test_pgpolicies(void)
setup_test(test_group_by_serial, 1),
setup_test(test_group_by_serial, 0),
setup_test(test_group_by_serial, _null),
+ setup_test(test_group_by_serial_2_groups8_all_marginal, 8),
+ setup_test(test_group_by_serial_2_groups8_half_marginal, 8),
};
return cmocka_run_group_tests(tests, setup, NULL);
}
--
2.17.2
^ permalink raw reply related [flat|nested] 41+ messages in thread* [PATCH 13/16] libmultipath: add marginal_pathgroups config option
2019-08-02 16:33 [PATCH 00/16] multipath marginal pathgroups Benjamin Marzinski
` (11 preceding siblings ...)
2019-08-02 16:33 ` [PATCH 12/16] tests: add tests for grouping " Benjamin Marzinski
@ 2019-08-02 16:33 ` Benjamin Marzinski
2019-08-02 16:33 ` [PATCH 14/16] libmutipath: deprecate delay_*_checks Benjamin Marzinski
` (2 subsequent siblings)
15 siblings, 0 replies; 41+ messages in thread
From: Benjamin Marzinski @ 2019-08-02 16:33 UTC (permalink / raw)
To: Christophe Varoqui
Cc: device-mapper development, Martin Wilck, Muneendra Kumar
group_paths now gets passed this to determine whether to enable
marginal pathgroups. The unit tests have also been updated.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/config.h | 1 +
libmultipath/configure.c | 5 +-
libmultipath/dict.c | 3 +
libmultipath/pgpolicies.c | 5 +-
libmultipath/pgpolicies.h | 2 +-
tests/pgpolicy.c | 140 +++++++++++++++++++++++---------------
6 files changed, 98 insertions(+), 58 deletions(-)
diff --git a/libmultipath/config.h b/libmultipath/config.h
index ff2b4e86..0b978970 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -186,6 +186,7 @@ struct config {
int max_sectors_kb;
int ghost_delay;
int find_multipaths_timeout;
+ int marginal_pathgroups;
unsigned int version[3];
char * multipath_dir;
diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 3c309d64..3238d485 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -297,7 +297,7 @@ int setup_map(struct multipath *mpp, char *params, int params_size,
{
struct pathgroup * pgp;
struct config *conf;
- int i, n_paths;
+ int i, n_paths, marginal_pathgroups;
/*
* don't bother if devmap size is unknown
@@ -357,6 +357,7 @@ int setup_map(struct multipath *mpp, char *params, int params_size,
select_flush_on_last_del(conf, mpp);
sysfs_set_scsi_tmo(mpp, conf->checkint);
+ marginal_pathgroups = conf->marginal_pathgroups;
pthread_cleanup_pop(1);
if (marginal_path_check_enabled(mpp)) {
@@ -387,7 +388,7 @@ int setup_map(struct multipath *mpp, char *params, int params_size,
vector_free(mpp->pg);
mpp->pg = NULL;
}
- if (group_paths(mpp))
+ if (group_paths(mpp, marginal_pathgroups))
return 1;
/*
diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index c6eba0f6..b5feb884 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -1339,6 +1339,8 @@ declare_ovr_snprint(all_tg_pt, print_yes_no_undef)
declare_hw_handler(all_tg_pt, set_yes_no_undef)
declare_hw_snprint(all_tg_pt, print_yes_no_undef)
+declare_def_handler(marginal_pathgroups, set_yes_no)
+declare_def_snprint(marginal_pathgroups, print_yes_no)
static int
def_uxsock_timeout_handler(struct config *conf, vector strvec)
@@ -1710,6 +1712,7 @@ init_keywords(vector keywords)
install_keyword("find_multipaths_timeout",
&def_find_multipaths_timeout_handler,
&snprint_def_find_multipaths_timeout);
+ install_keyword("marginal_pathgroups", &def_marginal_pathgroups_handler, &snprint_def_marginal_pathgroups);
__deprecated install_keyword("default_selector", &def_selector_handler, NULL);
__deprecated install_keyword("default_path_grouping_policy", &def_pgpolicy_handler, NULL);
__deprecated install_keyword("default_uid_attribute", &def_uid_attribute_handler, NULL);
diff --git a/libmultipath/pgpolicies.c b/libmultipath/pgpolicies.c
index 6fb2d28a..8f7c6b13 100644
--- a/libmultipath/pgpolicies.c
+++ b/libmultipath/pgpolicies.c
@@ -131,7 +131,7 @@ fail:
return -1;
}
-int group_paths(struct multipath *mp)
+int group_paths(struct multipath *mp, int marginal_pathgroups)
{
vector normal, marginal;
@@ -145,7 +145,8 @@ int group_paths(struct multipath *mp)
if (!mp->pgpolicyfn)
goto fail;
- if (split_marginal_paths(mp->paths, &normal, &marginal) != 0) {
+ if (!marginal_pathgroups ||
+ split_marginal_paths(mp->paths, &normal, &marginal) != 0) {
if (mp->pgpolicyfn(mp, mp->paths) != 0)
goto fail;
} else {
diff --git a/libmultipath/pgpolicies.h b/libmultipath/pgpolicies.h
index 7532d75f..15927610 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);
-int group_paths(struct multipath *);
+int group_paths(struct multipath *, int);
/*
* policies
*/
diff --git a/tests/pgpolicy.c b/tests/pgpolicy.c
index ab09f91c..3f61b123 100644
--- a/tests/pgpolicy.c
+++ b/tests/pgpolicy.c
@@ -204,7 +204,7 @@ static void test_one_group8(void **state)
int group_size[] = {8};
mp8.pgpolicyfn = one_group;
- assert_int_equal(group_paths(&mp8), 0);
+ assert_int_equal(group_paths(&mp8, 0), 0);
verify_pathgroups(&mp8, p8, groups, group_size, NULL, 1);
}
@@ -215,7 +215,7 @@ static void test_one_group4(void **state)
int group_size[] = {4};
mp4.pgpolicyfn = one_group;
- assert_int_equal(group_paths(&mp4), 0);
+ assert_int_equal(group_paths(&mp4, 0), 0);
verify_pathgroups(&mp4, p4, groups, group_size, NULL, 1);
}
@@ -226,21 +226,21 @@ static void test_one_group1(void **state)
int group_size[] = {1};
mp1.pgpolicyfn = one_group;
- assert_int_equal(group_paths(&mp1), 0);
+ assert_int_equal(group_paths(&mp1, 0), 0);
verify_pathgroups(&mp1, p1, groups, group_size, NULL, 1);
}
static void test_one_group0(void **state)
{
mp0.pgpolicyfn = one_group;
- assert_int_equal(group_paths(&mp0), 0);
+ assert_int_equal(group_paths(&mp0, 0), 0);
verify_pathgroups(&mp0, NULL, NULL, NULL, NULL, 0);
}
static void test_one_group_null(void **state)
{
mp_null.pgpolicyfn = one_group;
- assert_int_equal(group_paths(&mp_null), 0);
+ assert_int_equal(group_paths(&mp_null, 0), 0);
verify_pathgroups(&mp_null, NULL, NULL, NULL, NULL, 0);
}
@@ -254,7 +254,7 @@ static void test_one_group_all_marginal8(void **state)
set_marginal(p8, marginal, 8);
mp8.pgpolicyfn = one_group;
- assert_int_equal(group_paths(&mp8), 0);
+ assert_int_equal(group_paths(&mp8, 1), 0);
verify_pathgroups(&mp8, p8, groups, group_size, group_marginal, 1);
}
@@ -269,10 +269,23 @@ static void test_one_group_half_marginal8(void **state)
set_marginal(p8, marginal, 8);
mp8.pgpolicyfn = one_group;
- assert_int_equal(group_paths(&mp8), 0);
+ assert_int_equal(group_paths(&mp8, 1), 0);
verify_pathgroups(&mp8, p8, groups, group_size, group_marginal, 2);
}
+static void test_one_group_ignore_marginal8(void **state)
+{
+ int marginal[] = {1,0,1,0,1,1,0,0};
+ int paths[] = {0,1,2,3,4,5,6,7};
+ int *groups[] = {paths};
+ int group_size[] = {8};
+
+ set_marginal(p8, marginal, 8);
+ mp8.pgpolicyfn = one_group;
+ assert_int_equal(group_paths(&mp8, 0), 0);
+ verify_pathgroups(&mp8, p8, groups, group_size, NULL, 1);
+}
+
static void test_one_group_one_marginal8(void **state)
{
int marginal[] = {0,0,0,0,0,1,0,0};
@@ -284,7 +297,7 @@ static void test_one_group_one_marginal8(void **state)
set_marginal(p8, marginal, 8);
mp8.pgpolicyfn = one_group;
- assert_int_equal(group_paths(&mp8), 0);
+ assert_int_equal(group_paths(&mp8, 1), 0);
verify_pathgroups(&mp8, p8, groups, group_size, group_marginal, 2);
}
@@ -296,7 +309,7 @@ static void test_one_path_per_group_same8(void **state)
int group_size[] = {1,1,1,1,1,1,1,1};
mp8.pgpolicyfn = one_path_per_group;
- assert_int_equal(group_paths(&mp8), 0);
+ assert_int_equal(group_paths(&mp8, 0), 0);
verify_pathgroups(&mp8, p8, groups, group_size, NULL, 8);
}
@@ -310,7 +323,7 @@ static void test_one_path_per_group_increasing8(void **state)
set_priority(p8, prio, 8);
mp8.pgpolicyfn = one_path_per_group;
- assert_int_equal(group_paths(&mp8), 0);
+ assert_int_equal(group_paths(&mp8, 0), 0);
verify_pathgroups(&mp8, p8, groups, group_size, NULL, 8);
}
@@ -324,7 +337,7 @@ static void test_one_path_per_group_decreasing8(void **state)
set_priority(p8, prio, 8);
mp8.pgpolicyfn = one_path_per_group;
- assert_int_equal(group_paths(&mp8), 0);
+ assert_int_equal(group_paths(&mp8, 0), 0);
verify_pathgroups(&mp8, p8, groups, group_size, NULL, 8);
}
@@ -338,7 +351,7 @@ static void test_one_path_per_group_mixed8(void **state)
set_priority(p8, prio, 8);
mp8.pgpolicyfn = one_path_per_group;
- assert_int_equal(group_paths(&mp8), 0);
+ assert_int_equal(group_paths(&mp8, 0), 0);
verify_pathgroups(&mp8, p8, groups, group_size, NULL, 8);
}
@@ -349,7 +362,7 @@ static void test_one_path_per_group4(void **state)
int group_size[] = {1,1,1,1};
mp4.pgpolicyfn = one_path_per_group;
- assert_int_equal(group_paths(&mp4), 0);
+ assert_int_equal(group_paths(&mp4, 0), 0);
verify_pathgroups(&mp4, p4, groups, group_size, NULL, 4);
}
@@ -360,21 +373,21 @@ static void test_one_path_per_group1(void **state)
int group_size[] = {1};
mp1.pgpolicyfn = one_path_per_group;
- assert_int_equal(group_paths(&mp1), 0);
+ assert_int_equal(group_paths(&mp1, 0), 0);
verify_pathgroups(&mp1, p1, groups, group_size, NULL, 1);
}
static void test_one_path_per_group0(void **state)
{
mp0.pgpolicyfn = one_path_per_group;
- assert_int_equal(group_paths(&mp0), 0);
+ assert_int_equal(group_paths(&mp0, 0), 0);
verify_pathgroups(&mp0, NULL, NULL, NULL, NULL, 0);
}
static void test_one_path_per_group_null(void **state)
{
mp_null.pgpolicyfn = one_path_per_group;
- assert_int_equal(group_paths(&mp_null), 0);
+ assert_int_equal(group_paths(&mp_null, 0), 0);
verify_pathgroups(&mp_null, NULL, NULL, NULL, NULL, 0);
}
@@ -391,7 +404,7 @@ static void test_one_path_per_group_mixed_all_marginal8(void **state)
set_priority(p8, prio, 8);
set_marginal(p8, marginal, 8);
mp8.pgpolicyfn = one_path_per_group;
- assert_int_equal(group_paths(&mp8), 0);
+ assert_int_equal(group_paths(&mp8, 1), 0);
verify_pathgroups(&mp8, p8, groups, group_size, group_marginal, 8);
}
@@ -408,7 +421,7 @@ static void test_one_path_per_group_mixed_half_marginal8(void **state)
set_priority(p8, prio, 8);
set_marginal(p8, marginal, 8);
mp8.pgpolicyfn = one_path_per_group;
- assert_int_equal(group_paths(&mp8), 0);
+ assert_int_equal(group_paths(&mp8, 1), 0);
verify_pathgroups(&mp8, p8, groups, group_size, group_marginal, 8);
}
@@ -419,7 +432,7 @@ static void test_group_by_prio_same8(void **state)
int group_size[] = {8};
mp8.pgpolicyfn = group_by_prio;
- assert_int_equal(group_paths(&mp8), 0);
+ assert_int_equal(group_paths(&mp8, 0), 0);
verify_pathgroups(&mp8, p8, groups, group_size, NULL, 1);
}
@@ -433,7 +446,7 @@ static void test_group_by_prio_increasing8(void **state)
set_priority(p8, prio, 8);
mp8.pgpolicyfn = group_by_prio;
- assert_int_equal(group_paths(&mp8), 0);
+ assert_int_equal(group_paths(&mp8, 0), 0);
verify_pathgroups(&mp8, p8, groups, group_size, NULL, 8);
}
@@ -447,7 +460,7 @@ static void test_group_by_prio_decreasing8(void **state)
set_priority(p8, prio, 8);
mp8.pgpolicyfn = group_by_prio;
- assert_int_equal(group_paths(&mp8), 0);
+ assert_int_equal(group_paths(&mp8, 0), 0);
verify_pathgroups(&mp8, p8, groups, group_size, NULL, 8);
}
@@ -466,7 +479,26 @@ static void test_group_by_prio_mixed8(void **state)
set_priority(p8, prio, 8);
mp8.pgpolicyfn = group_by_prio;
- assert_int_equal(group_paths(&mp8), 0);
+ assert_int_equal(group_paths(&mp8, 0), 0);
+ verify_pathgroups(&mp8, p8, groups, group_size, NULL, 6);
+}
+
+static void test_group_by_prio_mixed_no_marginal8(void **state)
+{
+ int prio[] = {7,1,3,3,5,2,8,2};
+ int group0[] = {6};
+ int group1[] = {0};
+ int group2[] = {4};
+ int group3[] = {2,3};
+ int group4[] = {5,7};
+ int group5[] = {1};
+ int *groups[] = {group0, group1, group2, group3,
+ group4, group5};
+ int group_size[] = {1,1,1,2,2,1};
+
+ set_priority(p8, prio, 8);
+ mp8.pgpolicyfn = group_by_prio;
+ assert_int_equal(group_paths(&mp8, 1), 0);
verify_pathgroups(&mp8, p8, groups, group_size, NULL, 6);
}
@@ -480,7 +512,7 @@ static void test_group_by_prio_2_groups8(void **state)
set_priority(p8, prio, 8);
mp8.pgpolicyfn = group_by_prio;
- assert_int_equal(group_paths(&mp8), 0);
+ assert_int_equal(group_paths(&mp8, 0), 0);
verify_pathgroups(&mp8, p8, groups, group_size, NULL, 2);
}
@@ -495,7 +527,7 @@ static void test_group_by_prio_mixed4(void **state)
set_priority(p4, prio, 4);
mp4.pgpolicyfn = group_by_prio;
- assert_int_equal(group_paths(&mp4), 0);
+ assert_int_equal(group_paths(&mp4, 0), 0);
verify_pathgroups(&mp4, p4, groups, group_size, NULL, 3);
}
@@ -509,7 +541,7 @@ static void test_group_by_prio_2_groups4(void **state)
set_priority(p4, prio, 4);
mp4.pgpolicyfn = group_by_prio;
- assert_int_equal(group_paths(&mp4), 0);
+ assert_int_equal(group_paths(&mp4, 0), 0);
verify_pathgroups(&mp4, p4, groups, group_size, NULL, 2);
}
@@ -520,21 +552,21 @@ static void test_group_by_prio1(void **state)
int group_size[] = {1};
mp1.pgpolicyfn = group_by_prio;
- assert_int_equal(group_paths(&mp1), 0);
+ assert_int_equal(group_paths(&mp1, 0), 0);
verify_pathgroups(&mp1, p1, groups, group_size, NULL, 1);
}
static void test_group_by_prio0(void **state)
{
mp0.pgpolicyfn = group_by_prio;
- assert_int_equal(group_paths(&mp0), 0);
+ assert_int_equal(group_paths(&mp0, 0), 0);
verify_pathgroups(&mp0, NULL, NULL, NULL, NULL, 0);
}
static void test_group_by_prio_null(void **state)
{
mp_null.pgpolicyfn = group_by_prio;
- assert_int_equal(group_paths(&mp_null), 0);
+ assert_int_equal(group_paths(&mp_null, 0), 0);
verify_pathgroups(&mp_null, NULL, NULL, NULL, NULL, 0);
}
@@ -556,7 +588,7 @@ static void test_group_by_prio_mixed_all_marginal8(void **state)
set_priority(p8, prio, 8);
set_marginal(p8, marginal, 8);
mp8.pgpolicyfn = group_by_prio;
- assert_int_equal(group_paths(&mp8), 0);
+ assert_int_equal(group_paths(&mp8, 1), 0);
verify_pathgroups(&mp8, p8, groups, group_size, group_marginal, 6);
}
@@ -579,7 +611,7 @@ static void test_group_by_prio_mixed_half_marginal8(void **state)
set_priority(p8, prio, 8);
set_marginal(p8, marginal, 8);
mp8.pgpolicyfn = group_by_prio;
- assert_int_equal(group_paths(&mp8), 0);
+ assert_int_equal(group_paths(&mp8, 1), 0);
verify_pathgroups(&mp8, p8, groups, group_size, group_marginal, 7);
}
@@ -602,7 +634,7 @@ static void test_group_by_prio_mixed_one_marginal8(void **state)
set_priority(p8, prio, 8);
set_marginal(p8, marginal, 8);
mp8.pgpolicyfn = group_by_prio;
- assert_int_equal(group_paths(&mp8), 0);
+ assert_int_equal(group_paths(&mp8, 1), 0);
verify_pathgroups(&mp8, p8, groups, group_size, group_marginal, 7);
}
@@ -615,7 +647,7 @@ static void test_group_by_node_name_same8(void **state)
set_tgt_node_name(p8, node_name, 8);
mp8.pgpolicyfn = group_by_node_name;
- assert_int_equal(group_paths(&mp8), 0);
+ assert_int_equal(group_paths(&mp8, 0), 0);
verify_pathgroups(&mp8, p8, groups, group_size, NULL, 1);
}
@@ -631,7 +663,7 @@ static void test_group_by_node_name_increasing8(void **state)
set_priority(p8, prio, 8);
set_tgt_node_name(p8, node_name, 8);
mp8.pgpolicyfn = group_by_node_name;
- assert_int_equal(group_paths(&mp8), 0);
+ assert_int_equal(group_paths(&mp8, 0), 0);
verify_pathgroups(&mp8, p8, groups, group_size, NULL, 8);
}
@@ -648,7 +680,7 @@ static void test_group_by_node_name_3_groups8(void **state)
set_priority(p8, prio, 8);
set_tgt_node_name(p8, node_name, 8);
mp8.pgpolicyfn = group_by_node_name;
- assert_int_equal(group_paths(&mp8), 0);
+ assert_int_equal(group_paths(&mp8, 0), 0);
verify_pathgroups(&mp8, p8, groups, group_size, NULL, 3);
}
@@ -664,7 +696,7 @@ static void test_group_by_node_name_2_groups8(void **state)
set_priority(p8, prio, 8);
set_tgt_node_name(p8, node_name, 8);
mp8.pgpolicyfn = group_by_node_name;
- assert_int_equal(group_paths(&mp8), 0);
+ assert_int_equal(group_paths(&mp8, 0), 0);
verify_pathgroups(&mp8, p8, groups, group_size, NULL, 2);
}
@@ -681,7 +713,7 @@ static void test_group_by_node_name_3_groups4(void **state)
set_priority(p4, prio, 4);
set_tgt_node_name(p4, node_name, 4);
mp4.pgpolicyfn = group_by_node_name;
- assert_int_equal(group_paths(&mp4), 0);
+ assert_int_equal(group_paths(&mp4, 0), 0);
verify_pathgroups(&mp4, p4, groups, group_size, NULL, 3);
}
@@ -697,7 +729,7 @@ static void test_group_by_node_name_2_groups4(void **state)
set_priority(p4, prio, 4);
set_tgt_node_name(p4, node_name, 4);
mp4.pgpolicyfn = group_by_node_name;
- assert_int_equal(group_paths(&mp4), 0);
+ assert_int_equal(group_paths(&mp4, 0), 0);
verify_pathgroups(&mp4, p4, groups, group_size, NULL, 2);
}
@@ -710,21 +742,21 @@ static void test_group_by_node_name1(void **state)
set_tgt_node_name(p1, node_name, 1);
mp1.pgpolicyfn = group_by_node_name;
- assert_int_equal(group_paths(&mp1), 0);
+ assert_int_equal(group_paths(&mp1,0), 0);
verify_pathgroups(&mp1, p1, groups, group_size, NULL, 1);
}
static void test_group_by_node_name0(void **state)
{
mp0.pgpolicyfn = group_by_node_name;
- assert_int_equal(group_paths(&mp0), 0);
+ assert_int_equal(group_paths(&mp0, 0), 0);
verify_pathgroups(&mp0, NULL, NULL, NULL, NULL, 0);
}
static void test_group_by_node_name_null(void **state)
{
mp_null.pgpolicyfn = group_by_node_name;
- assert_int_equal(group_paths(&mp_null), 0);
+ assert_int_equal(group_paths(&mp_null, 0), 0);
verify_pathgroups(&mp_null, NULL, NULL, NULL, NULL, 0);
}
@@ -743,7 +775,7 @@ static void test_group_by_node_name_2_groups_all_marginal8(void **state)
set_marginal(p8, marginal, 8);
set_tgt_node_name(p8, node_name, 8);
mp8.pgpolicyfn = group_by_node_name;
- assert_int_equal(group_paths(&mp8), 0);
+ assert_int_equal(group_paths(&mp8, 1), 0);
verify_pathgroups(&mp8, p8, groups, group_size, group_marginal, 2);
}
@@ -764,7 +796,7 @@ static void test_group_by_node_name_2_groups_half_marginal8(void **state)
set_marginal(p8, marginal, 8);
set_tgt_node_name(p8, node_name, 8);
mp8.pgpolicyfn = group_by_node_name;
- assert_int_equal(group_paths(&mp8), 0);
+ assert_int_equal(group_paths(&mp8, 1), 0);
verify_pathgroups(&mp8, p8, groups, group_size, group_marginal, 4);
}
@@ -777,7 +809,7 @@ static void test_group_by_serial_same8(void **state)
set_serial(p8, serial, 8);
mp8.pgpolicyfn = group_by_serial;
- assert_int_equal(group_paths(&mp8), 0);
+ assert_int_equal(group_paths(&mp8, 0), 0);
verify_pathgroups(&mp8, p8, groups, group_size, NULL, 1);
}
@@ -793,7 +825,7 @@ static void test_group_by_serial_increasing8(void **state)
set_priority(p8, prio, 8);
set_serial(p8, serial, 8);
mp8.pgpolicyfn = group_by_serial;
- assert_int_equal(group_paths(&mp8), 0);
+ assert_int_equal(group_paths(&mp8, 0), 0);
verify_pathgroups(&mp8, p8, groups, group_size, NULL, 8);
}
@@ -810,7 +842,7 @@ static void test_group_by_serial_3_groups8(void **state)
set_priority(p8, prio, 8);
set_serial(p8, serial, 8);
mp8.pgpolicyfn = group_by_serial;
- assert_int_equal(group_paths(&mp8), 0);
+ assert_int_equal(group_paths(&mp8, 0), 0);
verify_pathgroups(&mp8, p8, groups, group_size, NULL, 3);
}
@@ -826,7 +858,7 @@ static void test_group_by_serial_2_groups8(void **state)
set_priority(p8, prio, 8);
set_serial(p8, serial, 8);
mp8.pgpolicyfn = group_by_serial;
- assert_int_equal(group_paths(&mp8), 0);
+ assert_int_equal(group_paths(&mp8, 0), 0);
verify_pathgroups(&mp8, p8, groups, group_size, NULL, 2);
}
@@ -843,7 +875,7 @@ static void test_group_by_serial_3_groups4(void **state)
set_priority(p4, prio, 4);
set_serial(p4, serial, 4);
mp4.pgpolicyfn = group_by_serial;
- assert_int_equal(group_paths(&mp4), 0);
+ assert_int_equal(group_paths(&mp4, 0), 0);
verify_pathgroups(&mp4, p4, groups, group_size, NULL, 3);
}
@@ -859,7 +891,7 @@ static void test_group_by_serial_2_groups4(void **state)
set_priority(p4, prio, 4);
set_serial(p4, serial, 4);
mp4.pgpolicyfn = group_by_serial;
- assert_int_equal(group_paths(&mp4), 0);
+ assert_int_equal(group_paths(&mp4, 0), 0);
verify_pathgroups(&mp4, p4, groups, group_size, NULL, 2);
}
@@ -872,21 +904,21 @@ static void test_group_by_serial1(void **state)
set_serial(p1, serial, 1);
mp1.pgpolicyfn = group_by_serial;
- assert_int_equal(group_paths(&mp1), 0);
+ assert_int_equal(group_paths(&mp1, 0), 0);
verify_pathgroups(&mp1, p1, groups, group_size, NULL, 1);
}
static void test_group_by_serial0(void **state)
{
mp0.pgpolicyfn = group_by_serial;
- assert_int_equal(group_paths(&mp0), 0);
+ assert_int_equal(group_paths(&mp0, 0), 0);
verify_pathgroups(&mp0, NULL, NULL, NULL, NULL, 0);
}
static void test_group_by_serial_null(void **state)
{
mp_null.pgpolicyfn = group_by_serial;
- assert_int_equal(group_paths(&mp_null), 0);
+ assert_int_equal(group_paths(&mp_null, 0), 0);
verify_pathgroups(&mp_null, NULL, NULL, NULL, NULL, 0);
}
@@ -905,7 +937,7 @@ static void test_group_by_serial_2_groups8_all_marginal8(void **state)
set_serial(p8, serial, 8);
set_marginal(p8, marginal, 8);
mp8.pgpolicyfn = group_by_serial;
- assert_int_equal(group_paths(&mp8), 0);
+ assert_int_equal(group_paths(&mp8, 1), 0);
verify_pathgroups(&mp8, p8, groups, group_size, group_marginal, 2);
}
@@ -926,7 +958,7 @@ static void test_group_by_serial_2_groups8_half_marginal8(void **state)
set_serial(p8, serial, 8);
set_marginal(p8, marginal, 8);
mp8.pgpolicyfn = group_by_serial;
- assert_int_equal(group_paths(&mp8), 0);
+ assert_int_equal(group_paths(&mp8, 1), 0);
verify_pathgroups(&mp8, p8, groups, group_size, group_marginal, 4);
}
@@ -943,6 +975,7 @@ int test_pgpolicies(void)
setup_test(test_one_group, _null),
setup_test(test_one_group_all_marginal, 8),
setup_test(test_one_group_half_marginal, 8),
+ setup_test(test_one_group_ignore_marginal, 8),
setup_test(test_one_group_one_marginal, 8),
setup_test(test_one_path_per_group_same, 8),
setup_test(test_one_path_per_group_increasing, 8),
@@ -958,6 +991,7 @@ int test_pgpolicies(void)
setup_test(test_group_by_prio_increasing, 8),
setup_test(test_group_by_prio_decreasing, 8),
setup_test(test_group_by_prio_mixed, 8),
+ setup_test(test_group_by_prio_mixed_no_marginal, 8),
setup_test(test_group_by_prio_2_groups, 8),
setup_test(test_group_by_prio_mixed, 4),
setup_test(test_group_by_prio_2_groups, 4),
--
2.17.2
^ permalink raw reply related [flat|nested] 41+ messages in thread* [PATCH 14/16] libmutipath: deprecate delay_*_checks
2019-08-02 16:33 [PATCH 00/16] multipath marginal pathgroups Benjamin Marzinski
` (12 preceding siblings ...)
2019-08-02 16:33 ` [PATCH 13/16] libmultipath: add marginal_pathgroups config option Benjamin Marzinski
@ 2019-08-02 16:33 ` Benjamin Marzinski
2019-08-14 21:20 ` Martin Wilck
2019-08-02 16:33 ` [PATCH 15/16] multipathd: use marginal_pathgroups Benjamin Marzinski
2019-08-02 16:33 ` [PATCH 16/16] multipath: update man pages Benjamin Marzinski
15 siblings, 1 reply; 41+ messages in thread
From: Benjamin Marzinski @ 2019-08-02 16:33 UTC (permalink / raw)
To: Christophe Varoqui
Cc: device-mapper development, Martin Wilck, Muneendra Kumar
The delay_checks shaky paths detection method works the same way as the
san_path_err method, but not as well, with less configurability, and
with the code spread all over check_path(). The only real difference is
that marks the path as marginal for a certain number of path checks
instead of for a specific time. This patch deprecates the delay_checks
method and maps it to the the san_path_err method.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/configure.c | 17 +----------
libmultipath/propsel.c | 62 +++++++++++++++++++++++++++++---------
libmultipath/propsel.h | 2 --
libmultipath/structs.h | 10 ------
multipath/multipath.conf.5 | 41 ++++++++++++++-----------
multipathd/main.c | 34 +++------------------
6 files changed, 76 insertions(+), 90 deletions(-)
diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 3238d485..9443389f 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -342,8 +342,6 @@ int setup_map(struct multipath *mpp, char *params, int params_size,
select_dev_loss(conf, mpp);
select_reservation_key(conf, mpp);
select_deferred_remove(conf, mpp);
- select_delay_watch_checks(conf, mpp);
- select_delay_wait_checks(conf, mpp);
select_marginal_path_err_sample_time(conf, mpp);
select_marginal_path_err_rate_threshold(conf, mpp);
select_marginal_path_err_recheck_gap_time(conf, mpp);
@@ -360,21 +358,8 @@ int setup_map(struct multipath *mpp, char *params, int params_size,
marginal_pathgroups = conf->marginal_pathgroups;
pthread_cleanup_pop(1);
- if (marginal_path_check_enabled(mpp)) {
- if (delay_check_enabled(mpp)) {
- condlog(1, "%s: WARNING: both marginal_path and delay_checks error detection selected",
- mpp->alias);
- condlog(0, "%s: unexpected behavior may occur!",
- mpp->alias);
- }
+ if (marginal_path_check_enabled(mpp))
start_io_err_stat_thread(vecs);
- }
- if (san_path_check_enabled(mpp) && delay_check_enabled(mpp)) {
- condlog(1, "%s: WARNING: both san_path_err and delay_checks error detection selected",
- mpp->alias);
- condlog(0, "%s: unexpected behavior may occur!",
- mpp->alias);
- }
n_paths = VECTOR_SIZE(mpp->paths);
/*
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index 6af2513d..894a52e3 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -85,6 +85,10 @@ static const char autodetect_origin[] =
"(setting: storage device autodetected)";
static const char marginal_path_origin[] =
"(setting: implied by marginal_path check)";
+static const char delay_watch_origin[] =
+ "(setting: implied by delay_watch_checks)";
+static const char delay_wait_origin[] =
+ "(setting: implied by delay_wait_checks)";
#define do_default(dest, value) \
do { \
@@ -877,37 +881,59 @@ out:
return 0;
}
-int select_delay_watch_checks(struct config *conf, struct multipath *mp)
+static int
+use_delay_watch_checks(struct config *conf, struct multipath *mp)
{
+ int value = NU_UNDEF;
const char *origin;
char buff[12];
- mp_set_mpe(delay_watch_checks);
- mp_set_ovr(delay_watch_checks);
- mp_set_hwe(delay_watch_checks);
- mp_set_conf(delay_watch_checks);
- mp_set_default(delay_watch_checks, DEFAULT_DELAY_CHECKS);
+ do_set(delay_watch_checks, mp->mpe, value, multipaths_origin);
+ do_set(delay_watch_checks, conf->overrides, value, overrides_origin);
+ do_set_from_hwe(delay_watch_checks, mp, value, hwe_origin);
+ do_set(delay_watch_checks, conf, value, conf_origin);
out:
- if (print_off_int_undef(buff, 12, mp->delay_watch_checks) != 0)
+ if (value > 0) {
+ mp->san_path_err_forget_rate = value;
+ print_off_int_undef(buff, 12, value);
condlog(3, "%s: delay_watch_checks = %s %s",
mp->alias, buff, origin);
+ if (mp->san_path_err_threshold == NU_NO) {
+ mp->san_path_err_threshold = 1;
+ condlog(3, "%s: san_path_err_threshold = 1 %s",
+ mp->alias, delay_watch_origin);
+ }
+ return 1;
+ }
return 0;
}
-int select_delay_wait_checks(struct config *conf, struct multipath *mp)
+static int
+use_delay_wait_checks(struct config *conf, struct multipath *mp)
{
+ int value = NU_UNDEF;
const char *origin;
char buff[12];
- mp_set_mpe(delay_wait_checks);
- mp_set_ovr(delay_wait_checks);
- mp_set_hwe(delay_wait_checks);
- mp_set_conf(delay_wait_checks);
- mp_set_default(delay_wait_checks, DEFAULT_DELAY_CHECKS);
+ do_set(delay_wait_checks, mp->mpe, value, multipaths_origin);
+ do_set(delay_wait_checks, conf->overrides, value, overrides_origin);
+ do_set_from_hwe(delay_wait_checks, mp, value, hwe_origin);
+ do_set(delay_wait_checks, conf, value, conf_origin);
out:
- if (print_off_int_undef(buff, 12, mp->delay_wait_checks) != 0)
+ if (value > 0) {
+ /* this isn't exactly the same length of time as
+ * delay_wait_checks, but it's a close enough approximation */
+ mp->san_path_err_recovery_time = value * conf->max_checkint;
+ print_off_int_undef(buff, 12, value);
condlog(3, "%s: delay_wait_checks = %s %s",
mp->alias, buff, origin);
+ if (mp->san_path_err_threshold == NU_NO) {
+ mp->san_path_err_threshold = 1;
+ condlog(3, "%s: san_path_err_threshold = 1 %s",
+ mp->alias, delay_wait_origin);
+ }
+ return 1;
+ }
return 0;
}
@@ -960,6 +986,10 @@ int select_san_path_err_forget_rate(struct config *conf, struct multipath *mp)
mp_set_ovr(san_path_err_forget_rate);
mp_set_hwe(san_path_err_forget_rate);
mp_set_conf(san_path_err_forget_rate);
+ if (use_delay_watch_checks(conf, mp)) {
+ origin = delay_watch_origin;
+ goto out;
+ }
mp_set_default(san_path_err_forget_rate, DEFAULT_ERR_CHECKS);
out:
if (print_off_int_undef(buff, 12, mp->san_path_err_forget_rate) != 0)
@@ -984,6 +1014,10 @@ int select_san_path_err_recovery_time(struct config *conf, struct multipath *mp)
mp_set_ovr(san_path_err_recovery_time);
mp_set_hwe(san_path_err_recovery_time);
mp_set_conf(san_path_err_recovery_time);
+ if (use_delay_wait_checks(conf, mp)) {
+ origin = delay_wait_origin;
+ goto out;
+ }
mp_set_default(san_path_err_recovery_time, DEFAULT_ERR_CHECKS);
out:
if (print_off_int_undef(buff, 12, mp->san_path_err_recovery_time) != 0)
diff --git a/libmultipath/propsel.h b/libmultipath/propsel.h
index b352c16a..b99652f0 100644
--- a/libmultipath/propsel.h
+++ b/libmultipath/propsel.h
@@ -22,8 +22,6 @@ int select_retain_hwhandler (struct config *conf, struct multipath * mp);
int select_detect_prio(struct config *conf, struct path * pp);
int select_detect_checker(struct config *conf, struct path * pp);
int select_deferred_remove(struct config *conf, struct multipath *mp);
-int select_delay_watch_checks (struct config *conf, struct multipath * mp);
-int select_delay_wait_checks (struct config *conf, struct multipath * mp);
int select_skip_kpartx (struct config *conf, struct multipath * mp);
int select_max_sectors_kb (struct config *conf, struct multipath * mp);
int select_san_path_err_forget_rate(struct config *conf, struct multipath *mp);
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index a8b9d325..a3adf906 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -268,8 +268,6 @@ struct path {
int pgindex;
int detect_prio;
int detect_checker;
- int watch_checks;
- int wait_checks;
int tpgs;
char * uid_attribute;
char * getuid;
@@ -321,8 +319,6 @@ struct multipath {
int fast_io_fail;
int retain_hwhandler;
int deferred_remove;
- int delay_watch_checks;
- int delay_wait_checks;
int san_path_err_threshold;
int san_path_err_forget_rate;
int san_path_err_recovery_time;
@@ -393,12 +389,6 @@ static inline int san_path_check_enabled(const struct multipath *mpp)
mpp->san_path_err_recovery_time > 0;
}
-static inline int delay_check_enabled(const struct multipath *mpp)
-{
- return mpp->delay_watch_checks != NU_NO ||
- mpp->delay_wait_checks != NU_NO;
-}
-
struct pathgroup {
long id;
int status;
diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
index f7d21b4c..7fccf8f3 100644
--- a/multipath/multipath.conf.5
+++ b/multipath/multipath.conf.5
@@ -1013,10 +1013,13 @@ The default is: \fBno\fR
.
.TP
.B delay_watch_checks
-If set to a value greater than 0, multipathd will watch paths that have
-recently become valid for this many checks. If they fail again while they are
-being watched, when they next become valid, they will not be used until they
-have stayed up for \fIdelay_wait_checks\fR checks. See "Shaky paths detection" below.
+This option is \fBdeprecated\fR, and mapped to \fIsan_path_err_forget_rate\fR.
+If this is set to a value greater than 0 and \fIsan_path_err_forget_rate\fR
+isn't set, \fIsan_path_err_forget_rate\fR will be set to the value of
+\fIdelay_watch_checks\fR. Also, if \fIsan_path_err_threshold\fR isn't set, it
+will be set to 1. See the \fIsan_path_err_forget_rate\fR and
+\fIsan_path_err_threshold\fR options, and "Shaky paths detection" below for
+more information.
.RS
.TP
The default is: \fBno\fR
@@ -1025,10 +1028,14 @@ The default is: \fBno\fR
.
.TP
.B delay_wait_checks
-If set to a value greater than 0, when a device that has recently come back
-online fails again within \fIdelay_watch_checks\fR checks, the next time it
-comes back online, it will marked and delayed, and not used until it has passed
-\fIdelay_wait_checks\fR checks. See "Shaky paths detection" below.
+This option is \fBdeprecated\fR, and mapped to \fIsan_path_err_recovery_time\fR.
+If this is set to a value greater than 0 and \fIsan_path_err_recovery_time\fR
+isn't set, \fIsan_path_err_recovery_time\fR will be set to the value of
+\fIdelay_wait_checks\fR times \fImax_polling_interval\fR. This will give
+approximately the same wait time as delay_wait_checks previously did.
+Also, if \fIsan_path_err_threshold\fR isn't set, it will be set to 1.
+See the \fIsan_path_err_recovery_time\fR and \fIsan_path_err_threshold\fR
+options, and "Shaky paths detection" below for more information.
.RS
.TP
The default is: \fBno\fR
@@ -1689,13 +1696,10 @@ if the healthy state appears to be stable. The logic of determining
differs between the three methods.
.TP 8
.B \(dqdelay_checks\(dq failure tracking
-If a path fails again within a
-\fIdelay_watch_checks\fR interval after a failure, don't
-reinstate it until it passes a \fIdelay_wait_checks\fR interval
-in always good status.
-The intervals are measured in \(dqticks\(dq, i.e. the
-time between path checks by multipathd, which is variable and controlled by the
-\fIpolling_interval\fR and \fImax_polling_interval\fR parameters.
+This method is \fBdeprecated\fR and mapped to the \(dqsan_path_err\(dq method.
+See the \fIdelay_watch_checks\fR and \fIdelay_wait_checks\fR options above
+for more information.
+
.TP
.B \(dqmarginal_path\(dq failure tracking
If a second failure event (good->bad transition) occurs within
@@ -1712,12 +1716,13 @@ in seconds.
.B \(dqsan_path_err\(dq failure tracking
multipathd counts path failures for each path. Once the number of failures
exceeds the value given by \fIsan_path_err_threshold\fR, the path is not
-reinstated for \fIsan_path_err_recovery_time\fR ticks. While counting
+reinstated for \fIsan_path_err_recovery_time\fR seconds. While counting
failures, multipathd \(dqforgets\(dq one past failure every
\(dqsan_path_err_forget_rate\(dq ticks; thus if errors don't occur more
often then once in the forget rate interval, the failure count doesn't
-increase and the threshold is never reached. As for the \fIdelay_xy\fR method,
-intervals are measured in \(dqticks\(dq.
+increase and the threshold is never reached. Ticks are the time between
+path checks by multipathd, which is variable and controlled by the
+\fIpolling_interval\fR and \fImax_polling_interval\fR parameters.
.
.RS 8
.LP
diff --git a/multipathd/main.c b/multipathd/main.c
index 7db15736..dca2214c 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2122,16 +2122,6 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
return 1;
}
- if ((newstate == PATH_UP || newstate == PATH_GHOST) &&
- pp->wait_checks > 0) {
- if (pp->mpp->nr_active > 0) {
- pp->state = PATH_DELAYED;
- pp->wait_checks--;
- return 1;
- } else
- pp->wait_checks = 0;
- }
-
/*
* don't reinstate failed path, if its in stand-by
* and if target supports only implicit tpgs mode.
@@ -2162,19 +2152,10 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
* proactively fail path in the DM
*/
if (oldstate == PATH_UP ||
- oldstate == PATH_GHOST) {
+ oldstate == PATH_GHOST)
fail_path(pp, 1);
- if (pp->mpp->delay_wait_checks > 0 &&
- pp->watch_checks > 0) {
- pp->wait_checks = pp->mpp->delay_wait_checks;
- pp->watch_checks = 0;
- }
- } else {
+ else
fail_path(pp, 0);
- if (pp->wait_checks > 0)
- pp->wait_checks =
- pp->mpp->delay_wait_checks;
- }
/*
* cancel scheduled failback
@@ -2200,15 +2181,10 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
* reinstate this path
*/
if (oldstate != PATH_UP &&
- oldstate != PATH_GHOST) {
- if (pp->mpp->delay_watch_checks > 0)
- pp->watch_checks = pp->mpp->delay_watch_checks;
+ oldstate != PATH_GHOST)
add_active = 1;
- } else {
- if (pp->watch_checks > 0)
- pp->watch_checks--;
+ else
add_active = 0;
- }
if (!disable_reinstate && reinstate_path(pp, add_active)) {
condlog(3, "%s: reload map", pp->dev);
ev_add_path(pp, vecs, 1);
@@ -2253,8 +2229,6 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
condlog(4, "%s: delay next check %is",
pp->dev_t, pp->checkint);
}
- if (pp->watch_checks > 0)
- pp->watch_checks--;
pp->tick = pp->checkint;
}
}
--
2.17.2
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [PATCH 14/16] libmutipath: deprecate delay_*_checks
2019-08-02 16:33 ` [PATCH 14/16] libmutipath: deprecate delay_*_checks Benjamin Marzinski
@ 2019-08-14 21:20 ` Martin Wilck
2019-08-16 20:47 ` Benjamin Marzinski
0 siblings, 1 reply; 41+ messages in thread
From: Martin Wilck @ 2019-08-14 21:20 UTC (permalink / raw)
To: bmarzins, christophe.varoqui; +Cc: dm-devel, mkumar
On Fri, 2019-08-02 at 11:33 -0500, Benjamin Marzinski wrote:
> The delay_checks shaky paths detection method works the same way as
> the
> san_path_err method, but not as well, with less configurability, and
> with the code spread all over check_path(). The only real difference
> is
> that marks the path as marginal for a certain number of path checks
> instead of for a specific time. This patch deprecates the
> delay_checks
> method and maps it to the the san_path_err method.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> libmultipath/configure.c | 17 +----------
> libmultipath/propsel.c | 62 +++++++++++++++++++++++++++++-------
I suppose that quite a few users are working with "delay_*_checks" in
production. If we remove the option, we should at least clearly
document how to map existing delay_*_checks parameters to san_path_err*
parameters.
IIUC, to (roughly) imitate the settings
delay_watch_checks = C
delay_wait_checks = W
I need to set
san_path_err_threshold = 2
san_path_err_forget_rate = C
san_path_err_recovery_time = W
Correct? Or can it be done better?
(It's not exactly the same, as delay_watch_checks starts counting when
a path is reinstated after a failure, while san_path_err_threshold
counts good->bad transitions, and the threshold would be reached if a
path fails more often than every C ticks _on average_).
If the above is fine, we might as well map these settings in the code
directly. IOW, instead of ignoring "delay_*_checks" altogether, we
should ignore it only if either san_path_* or marginal_path_*
parameters are set; Otherwise, we could simply map the delay_*_checks
parameters as shown above.
That would be a bit more user friendly in terms of backwards
compatibility.
Regards
Martin
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 14/16] libmutipath: deprecate delay_*_checks
2019-08-14 21:20 ` Martin Wilck
@ 2019-08-16 20:47 ` Benjamin Marzinski
2019-08-16 21:51 ` Martin Wilck
0 siblings, 1 reply; 41+ messages in thread
From: Benjamin Marzinski @ 2019-08-16 20:47 UTC (permalink / raw)
To: Martin Wilck; +Cc: dm-devel, mkumar
On Wed, Aug 14, 2019 at 09:20:46PM +0000, Martin Wilck wrote:
> On Fri, 2019-08-02 at 11:33 -0500, Benjamin Marzinski wrote:
> > The delay_checks shaky paths detection method works the same way as
> > the
> > san_path_err method, but not as well, with less configurability, and
> > with the code spread all over check_path(). The only real difference
> > is
> > that marks the path as marginal for a certain number of path checks
> > instead of for a specific time. This patch deprecates the
> > delay_checks
> > method and maps it to the the san_path_err method.
> >
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> > libmultipath/configure.c | 17 +----------
> > libmultipath/propsel.c | 62 +++++++++++++++++++++++++++++-------
>
> I suppose that quite a few users are working with "delay_*_checks" in
> production. If we remove the option, we should at least clearly
> document how to map existing delay_*_checks parameters to san_path_err*
> parameters.
Didn't I? My patch does include changes to the man page that tells how
it gets remapped.
> IIUC, to (roughly) imitate the settings
>
> delay_watch_checks = C
> delay_wait_checks = W
>
> I need to set
>
> san_path_err_threshold = 2
> san_path_err_forget_rate = C
> san_path_err_recovery_time = W
>
> Correct? Or can it be done better?
Well, the code uses
san_path_err_threshold = 1 (since checks for a number of errors greater
than this threshold)
san_path_err_forget_rate = C
san_path_err_recovery_time = W * polling_interval
> (It's not exactly the same, as delay_watch_checks starts counting when
> a path is reinstated after a failure, while san_path_err_threshold
> counts good->bad transitions, and the threshold would be reached if a
> path fails more often than every C ticks _on average_).
> If the above is fine, we might as well map these settings in the code
> directly. IOW, instead of ignoring "delay_*_checks" altogether, we
> should ignore it only if either san_path_* or marginal_path_*
> parameters are set; Otherwise, we could simply map the delay_*_checks
> parameters as shown above.
Err.. This patch does do the remapping in code (in propsel.c) just as
you suggest.. right?
I'm confused here.
-Ben
> That would be a bit more user friendly in terms of backwards
> compatibility.
>
> Regards
> Martin
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 14/16] libmutipath: deprecate delay_*_checks
2019-08-16 20:47 ` Benjamin Marzinski
@ 2019-08-16 21:51 ` Martin Wilck
0 siblings, 0 replies; 41+ messages in thread
From: Martin Wilck @ 2019-08-16 21:51 UTC (permalink / raw)
To: bmarzins; +Cc: dm-devel, mkumar
On Fri, 2019-08-16 at 15:47 -0500, Benjamin Marzinski wrote:
>
> Err.. This patch does do the remapping in code (in propsel.c) just as
> you suggest.. right?
I'm truly sorry. It was late when I looked at this patch, and obviously
I got it all wrong.
So, I almost ACK this patch; but I'd like to simplify the logic by
using the delay_* parameters only if _none_ of the san_path_ parameters
have been set by the user (and also, of course, if no marginal_path
parameters have been set); similar to the logic of c8775cf "multipathd:
marginal_path overrides san_path_err".
Users should IMO not be able to mix these parameters.
Cheers,
Martin
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 15/16] multipathd: use marginal_pathgroups
2019-08-02 16:33 [PATCH 00/16] multipath marginal pathgroups Benjamin Marzinski
` (13 preceding siblings ...)
2019-08-02 16:33 ` [PATCH 14/16] libmutipath: deprecate delay_*_checks Benjamin Marzinski
@ 2019-08-02 16:33 ` Benjamin Marzinski
2019-08-02 16:33 ` [PATCH 16/16] multipath: update man pages Benjamin Marzinski
15 siblings, 0 replies; 41+ messages in thread
From: Benjamin Marzinski @ 2019-08-02 16:33 UTC (permalink / raw)
To: Christophe Varoqui
Cc: device-mapper development, Martin Wilck, Muneendra Kumar
This commit makes the marginal_pathgroups option work with the
existing methods for determining marginal paths. It also merges the
code for the marginal_path and sand_path_err methods. This has the
side effect of making the marginal_path code set a marginal path's state
to "delayed" instead of "shaky" like it previously did.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
multipathd/main.c | 53 +++++++++++++++++++++++++++++++++--------------
1 file changed, 38 insertions(+), 15 deletions(-)
diff --git a/multipathd/main.c b/multipathd/main.c
index dca2214c..04b2b56a 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1960,6 +1960,18 @@ reinstate_path:
return 0;
}
+static int
+should_skip_path(struct path *pp){
+ if (marginal_path_check_enabled(pp->mpp)) {
+ if (pp->io_err_disable_reinstate && need_io_err_check(pp))
+ return 1;
+ } else if (san_path_check_enabled(pp->mpp)) {
+ if (check_path_reinstate_state(pp))
+ return 1;
+ }
+ return 0;
+}
+
/*
* Returns '1' if the path has been checked, '-1' if it was blacklisted
* and '0' otherwise
@@ -1975,6 +1987,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
int oldchkrstate = pp->chkrstate;
int retrigger_tries, checkint, max_checkint, verbosity;
struct config *conf;
+ int marginal_pathgroups, marginal_changed = 0;
int ret;
if ((pp->initialized == INIT_OK ||
@@ -1991,6 +2004,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
checkint = conf->checkint;
max_checkint = conf->max_checkint;
verbosity = conf->verbosity;
+ marginal_pathgroups = conf->marginal_pathgroups;
put_multipath_config(conf);
if (pp->checkint == CHECKINT_UNDEF) {
@@ -2106,20 +2120,27 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
set_no_path_retry(pp->mpp);
if ((newstate == PATH_UP || newstate == PATH_GHOST) &&
- check_path_reinstate_state(pp)) {
- pp->state = PATH_DELAYED;
- return 1;
- }
-
- if ((newstate == PATH_UP || newstate == PATH_GHOST) &&
- pp->io_err_disable_reinstate && need_io_err_check(pp)) {
- pp->state = PATH_SHAKY;
- /*
- * to reschedule as soon as possible,so that this path can
- * be recoverd in time
- */
- pp->tick = 1;
- return 1;
+ (san_path_check_enabled(pp->mpp) ||
+ marginal_path_check_enabled(pp->mpp))) {
+ int was_marginal = pp->marginal;
+ if (should_skip_path(pp)) {
+ if (!marginal_pathgroups) {
+ if (marginal_path_check_enabled(pp->mpp))
+ /* to reschedule as soon as possible,
+ * so that this path can be recovered
+ * in time */
+ pp->tick = 1;
+ pp->state = PATH_DELAYED;
+ return 1;
+ }
+ if (!was_marginal) {
+ pp->marginal = 1;
+ marginal_changed = 1;
+ }
+ } else if (marginal_pathgroups && was_marginal) {
+ pp->marginal = 0;
+ marginal_changed = 1;
+ }
}
/*
@@ -2258,7 +2279,9 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
*/
condlog(4, "path prio refresh");
- if (update_prio(pp, new_path_up) &&
+ if (marginal_changed)
+ update_path_groups(pp->mpp, vecs, 1);
+ else if (update_prio(pp, new_path_up) &&
(pp->mpp->pgpolicyfn == (pgpolicyfn *)group_by_prio) &&
pp->mpp->pgfailback == -FAILBACK_IMMEDIATE)
update_path_groups(pp->mpp, vecs, !new_path_up);
--
2.17.2
^ permalink raw reply related [flat|nested] 41+ messages in thread* [PATCH 16/16] multipath: update man pages
2019-08-02 16:33 [PATCH 00/16] multipath marginal pathgroups Benjamin Marzinski
` (14 preceding siblings ...)
2019-08-02 16:33 ` [PATCH 15/16] multipathd: use marginal_pathgroups Benjamin Marzinski
@ 2019-08-02 16:33 ` Benjamin Marzinski
2019-08-14 21:21 ` Martin Wilck
15 siblings, 1 reply; 41+ messages in thread
From: Benjamin Marzinski @ 2019-08-02 16:33 UTC (permalink / raw)
To: Christophe Varoqui
Cc: device-mapper development, Martin Wilck, Muneendra Kumar
Add documentation for the marginal_pathgroups option and the
(un)setmarginal commands.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
multipath/multipath.conf.5 | 34 ++++++++++++++++++++++++++++++----
multipathd/multipathd.8 | 19 +++++++++++++++++++
2 files changed, 49 insertions(+), 4 deletions(-)
diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
index 7fccf8f3..a85a8a60 100644
--- a/multipath/multipath.conf.5
+++ b/multipath/multipath.conf.5
@@ -1043,6 +1043,28 @@ The default is: \fBno\fR
.
.
.TP
+.B marginal_pathgroups
+If set to \fIno\fR, the \fIdelay_*_checks\fR, \fImarginal_path_*\fR, and
+\fIsan_path_err_*\fR options will keep marginal, or \(dqshaky\(dq, paths from
+being reinstated until they have been monitored for some time. This can cause
+situations where all non-marginal paths are down, and no paths are usable
+until multipathd detects this and reinstates a marginal path. If the multipath
+device is not configured to queue IO in this case, it can cause IO errors to
+occur, even though there are marginal paths available. However, if this
+option is set to \fIyes\fR, when one of the marginal path detecting methods
+determines that a path is marginal, it will be reinstated and placed in a
+seperate pathgroup that will only be used after all the non-marginal pathgroups
+have been tried first. This prevents the possibility of IO errors occuring
+while marginal paths are still usable. After the path has been monitored
+for the configured time, and is declared healthy, it will be returned to its
+normal pathgroup. See "Shaky paths detection" below for more information.
+.RS
+.TP
+The default is: \fBno\fR
+.RE
+.
+.
+.TP
.B find_multipaths
This option controls whether multipath and multipathd try to create multipath
maps over non-blacklisted devices they encounter. This matters a) when a device is
@@ -1690,10 +1712,14 @@ events. \fImultipathd\fR supports three different methods for detecting this
situation and dealing with it. All methods share the same basic mode of
operation: If a path is found to be \(dqshaky\(dq or \(dqflipping\(dq,
and appears to be in healthy status, it is not reinstated (put back to use)
-immediately. Instead, it is watched for some time, and only reinstated
-if the healthy state appears to be stable. The logic of determining
-\(dqshaky\(dq condition, as well as the logic when to reinstate,
-differs between the three methods.
+immediately. Instead, it is placed in the \(dqdelayed\(dq state and watched
+for some time, and only reinstated if the healthy state appears to be stable.
+If the \fImarginal_pathgroups\fR option is set, the path will reinstated
+immediately, but placed in a special pathgroup for marginal paths. Marginal
+pathgroups will not be used until all other pathgroups have been tried. At the
+time when the path would normally be reinstated, it will be returned to its
+normal pathgroup. The logic of determining \(dqshaky\(dq condition, as well as
+the logic when to reinstate, differs between the three methods.
.TP 8
.B \(dqdelay_checks\(dq failure tracking
This method is \fBdeprecated\fR and mapped to the \(dqsan_path_err\(dq method.
diff --git a/multipathd/multipathd.8 b/multipathd/multipathd.8
index edac7a92..048a838d 100644
--- a/multipathd/multipathd.8
+++ b/multipathd/multipathd.8
@@ -277,6 +277,25 @@ Remove the persistent reservation key associated with $map from the
\fIreservation_key\fR is set to \fBfile\fR in \fI/etc/multipath.conf\fR.
.
.TP
+.B path $path setmarginal
+move $path to a marginal pathgroup. The path will remain in the marginal
+path group until \fIunsetmarginal\fR is called. This command will only
+work if \fImarginal_pathgroups\fR is enabled and there is no Shaky paths
+detection method configured (see the multipath.conf man page for details).
+.
+.TP
+.B path $path unsetmarginal
+return marginal path $path to its normal pathgroup. This command will only
+work if \fImarginal_pathgroups\fR is enabled and there is no Shaky paths
+detection method configured (see the multipath.conf man page for details).
+.
+.TP
+.B map $map unsetmarginal
+return all marginal paths in $map to their normal pathgroups. This command
+will only work if \fImarginal_pathgroups\fR is enabled and there is no Shaky
+paths detection method configured (see the multipath.conf man page for details).
+.
+.TP
.B quit|exit
End interactive session.
.
--
2.17.2
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [PATCH 16/16] multipath: update man pages
2019-08-02 16:33 ` [PATCH 16/16] multipath: update man pages Benjamin Marzinski
@ 2019-08-14 21:21 ` Martin Wilck
2019-08-16 20:54 ` Benjamin Marzinski
0 siblings, 1 reply; 41+ messages in thread
From: Martin Wilck @ 2019-08-14 21:21 UTC (permalink / raw)
To: bmarzins, christophe.varoqui; +Cc: dm-devel, mkumar
On Fri, 2019-08-02 at 11:33 -0500, Benjamin Marzinski wrote:
> Add documentation for the marginal_pathgroups option and the
> (un)setmarginal commands.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> multipath/multipath.conf.5 | 34 ++++++++++++++++++++++++++++++----
> multipathd/multipathd.8 | 19 +++++++++++++++++++
> 2 files changed, 49 insertions(+), 4 deletions(-)
>
> --- a/multipathd/multipathd.8
> +++ b/multipathd/multipathd.8
> @@ -277,6 +277,25 @@ Remove the persistent reservation key associated
> with $map from the
> \fIreservation_key\fR is set to \fBfile\fR in
> \fI/etc/multipath.conf\fR.
> .
> .TP
> +.B path $path setmarginal
> +move $path to a marginal pathgroup. The path will remain in the
> marginal
> +path group until \fIunsetmarginal\fR is called. This command will
> only
> +work if \fImarginal_pathgroups\fR is enabled and there is no Shaky
> paths
> +detection method configured (see the multipath.conf man page for
> details).
This is counter-intuitive. It's unclear to me why we need these
commands at all. By nature of "shaky" paths, it doesn't make a lot of
sense to make these settings (only) manually. I'd like it better if the
cli commands somehow hooked into the different "shaky" algorithms. E.g.
for the san_path_err_ algorithm, setting a path to marginal would mean
artificially setting its failure count to a value above the configured
threshold. That way, the manual settings could work togehter with the
automatic detection methods, and could be used for overriding them
in special cases.
Martin
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 16/16] multipath: update man pages
2019-08-14 21:21 ` Martin Wilck
@ 2019-08-16 20:54 ` Benjamin Marzinski
0 siblings, 0 replies; 41+ messages in thread
From: Benjamin Marzinski @ 2019-08-16 20:54 UTC (permalink / raw)
To: Martin Wilck; +Cc: dm-devel, mkumar
On Wed, Aug 14, 2019 at 09:21:32PM +0000, Martin Wilck wrote:
> On Fri, 2019-08-02 at 11:33 -0500, Benjamin Marzinski wrote:
> > Add documentation for the marginal_pathgroups option and the
> > (un)setmarginal commands.
> >
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> > multipath/multipath.conf.5 | 34 ++++++++++++++++++++++++++++++----
> > multipathd/multipathd.8 | 19 +++++++++++++++++++
> > 2 files changed, 49 insertions(+), 4 deletions(-)
> >
> > --- a/multipathd/multipathd.8
> > +++ b/multipathd/multipathd.8
> > @@ -277,6 +277,25 @@ Remove the persistent reservation key associated
> > with $map from the
> > \fIreservation_key\fR is set to \fBfile\fR in
> > \fI/etc/multipath.conf\fR.
> > .
> > .TP
> > +.B path $path setmarginal
> > +move $path to a marginal pathgroup. The path will remain in the
> > marginal
> > +path group until \fIunsetmarginal\fR is called. This command will
> > only
> > +work if \fImarginal_pathgroups\fR is enabled and there is no Shaky
> > paths
> > +detection method configured (see the multipath.conf man page for
> > details).
>
> This is counter-intuitive. It's unclear to me why we need these
> commands at all. By nature of "shaky" paths, it doesn't make a lot of
> sense to make these settings (only) manually. I'd like it better if the
> cli commands somehow hooked into the different "shaky" algorithms. E.g.
> for the san_path_err_ algorithm, setting a path to marginal would mean
> artificially setting its failure count to a value above the configured
> threshold. That way, the manual settings could work togehter with the
> automatic detection methods, and could be used for overriding them
> in special cases.
I get that it's weird, but like I mentioned in PATCH 00/16, The manual
control is for Broadcom's Fiber Channel Transport Daemon, since it
doesn't use the multipathd marginal path detectors, and thus will not
automatically reinstate marginal paths when all other paths have failed.
The way it works right now, the daemon is supposed to have control over
a path's marginal state, without users setting up a different marginal
path algorithm. I do agree that it would make sense to have a
discussion about automatically restoring paths that have been externally
marked as marginal.
-Ben
> Martin
>
^ permalink raw reply [flat|nested] 41+ messages in thread