On Fri, 2023-05-19 at 18:02 -0500, Benjamin Marzinski wrote:
> When we group paths by prio and the priority changes, paths can end
> up
> temporarily in the wrong path groups. This usually happens when some
> paths are down, so their priority can't be updated. To avoid this for
> ALUA paths, group them by their target port group instead. The path
> groups chosen by this policy won't always match with those chosen by
> group_by_prio, since it is possible for multiple ALUA target port
> groups to have the same priority.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@xxxxxxxxxx>
> ---
> libmultipath/discovery.c | 1 +
> libmultipath/pgpolicies.c | 19 +++++++++++++++++++
> libmultipath/pgpolicies.h | 4 +++-
> libmultipath/prioritizers/alua.c | 1 +
> libmultipath/propsel.c | 27 +++++++++++++++++++++++++--
> libmultipath/structs.c | 1 +
> libmultipath/structs.h | 3 +++
> multipath/main.c | 1 +
> multipath/multipath.conf.5 | 4 ++++
> 9 files changed, 58 insertions(+), 3 deletions(-)
>
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 6865cd92..2dcafe5d 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -1051,6 +1051,7 @@ detect_alua(struct path * pp)
> return;
> }
> pp->tpgs = tpgs;
> + pp->tpg_id = ret;
> }
>
> int path_get_tpgs(struct path *pp)
> diff --git a/libmultipath/pgpolicies.c b/libmultipath/pgpolicies.c
> index 10b44d37..e14da8cc 100644
> --- a/libmultipath/pgpolicies.c
> +++ b/libmultipath/pgpolicies.c
> @@ -25,6 +25,8 @@ int get_pgpolicy_id(char * str)
> return GROUP_BY_PRIO;
> if (0 == strncmp(str, "group_by_node_name", 18))
> return GROUP_BY_NODE_NAME;
> + if (0 == strncmp(str, "group_by_tpg", 12))
> + return GROUP_BY_TPG;
>
> return IOPOLICY_UNDEF;
> }
> @@ -49,6 +51,9 @@ int get_pgpolicy_name(char * buff, int len, int id)
> case GROUP_BY_NODE_NAME:
> s = "group_by_node_name";
> break;
> + case GROUP_BY_TPG:
> + s = "group_by_tpg";
> + break;
> default:
> s = "undefined";
> break;
> @@ -191,6 +196,12 @@ prios_match(struct path *pp1, struct path *pp2)
> return (pp1->priority == pp2->priority);
> }
>
> +bool
> +tpg_match(struct path *pp1, struct path *pp2)
> +{
> + return (pp1->tpg_id == pp2->tpg_id);
> +}
> +
> int group_by_match(struct multipath * mp, vector paths,
> bool (*path_match_fn)(struct path *, struct path
> *))
> {
> @@ -279,6 +290,14 @@ int group_by_prio(struct multipath *mp, vector
> paths)
> return group_by_match(mp, paths, prios_match);
> }
>
> +/*
> + * One path group per alua target port group present in the path
> vector
> + */
> +int group_by_tpg(struct multipath *mp, vector paths)
> +{
> + return group_by_match(mp, paths, tpg_match);
> +}
> +
> int one_path_per_group(struct multipath *mp, vector paths)
> {
> int i;
> diff --git a/libmultipath/pgpolicies.h b/libmultipath/pgpolicies.h
> index 15927610..d3ab2f35 100644
> --- a/libmultipath/pgpolicies.h
> +++ b/libmultipath/pgpolicies.h
> @@ -16,7 +16,8 @@ enum iopolicies {
> MULTIBUS,
> GROUP_BY_SERIAL,
> GROUP_BY_PRIO,
> - GROUP_BY_NODE_NAME
> + GROUP_BY_NODE_NAME,
> + GROUP_BY_TPG,
> };
>
> int get_pgpolicy_id(char *);
> @@ -30,5 +31,6 @@ int one_group(struct multipath *, vector);
> int group_by_serial(struct multipath *, vector);
> int group_by_prio(struct multipath *, vector);
> int group_by_node_name(struct multipath *, vector);
> +int group_by_tpg(struct multipath *, vector);
>
> #endif
> diff --git a/libmultipath/prioritizers/alua.c
> b/libmultipath/prioritizers/alua.c
> index 0ab06e2b..4888a974 100644
> --- a/libmultipath/prioritizers/alua.c
> +++ b/libmultipath/prioritizers/alua.c
> @@ -65,6 +65,7 @@ get_alua_info(struct path * pp, unsigned int
> timeout)
> return -ALUA_PRIO_NOT_SUPPORTED;
> return -ALUA_PRIO_RTPG_FAILED;
> }
> + pp->tpg_id = tpg;
In view of the discussion from the cover letter:
tpg_id should have already been set by detect_alua(). I'm not sure if
it's good to overwrite it silently here. Shouldn't we rather check
if the value is unchanged, or refrain from setting it more than once?
> condlog(3, "%s: reported target port group is %i", pp->dev,
> tpg);
> rc = get_asymmetric_access_state(pp, tpg, timeout);
> if (rc < 0) {
> diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
> index a25cc921..841fa247 100644
> --- a/libmultipath/propsel.c
> +++ b/libmultipath/propsel.c
> @@ -35,7 +35,8 @@ pgpolicyfn *pgpolicies[] = {
> one_group,
> group_by_serial,
> group_by_prio,
> - group_by_node_name
> + group_by_node_name,
> + group_by_tpg,
> };
>
> #define do_set(var, src, dest,
> msg) \
> @@ -249,10 +250,26 @@ out:
> return 0;
> }
>
> +static bool
> +verify_alua_prio(struct multipath *mp)
> +{
> + int i;
> + struct path *pp;
> +
> + vector_foreach_slot(mp->paths, pp, i) {
> + const char *name = prio_name(&pp->prio);
> + if (strncmp(name, PRIO_ALUA, PRIO_NAME_LEN) &&
> + strncmp(name, PRIO_SYSFS, PRIO_NAME_LEN))
> + return false;
> + }
> + return true;
> +}
> +
> int select_pgpolicy(struct config *conf, struct multipath * mp)
> {
> const char *origin;
> char buff[POLICY_NAME_SIZE];
> + int log_prio = 3;
>
> if (conf->pgpolicy_flag > 0) {
> mp->pgpolicy = conf->pgpolicy_flag;
> @@ -265,9 +282,15 @@ int select_pgpolicy(struct config *conf, struct
> multipath * mp)
> mp_set_conf(pgpolicy);
> mp_set_default(pgpolicy, DEFAULT_PGPOLICY);
> out:
> + if (mp->pgpolicy == GROUP_BY_TPG && !verify_alua_prio(mp)) {
> + mp->pgpolicy = DEFAULT_PGPOLICY;
> + origin = "(setting: emergency fallback - not all
> paths use alua prio)";
> + log_prio = 1;
> + }
I don't understand this logic. Why don't you simply check pp->tpgs?
> mp->pgpolicyfn = pgpolicies[mp->pgpolicy];
> get_pgpolicy_name(buff, POLICY_NAME_SIZE, mp->pgpolicy);
> - condlog(3, "%s: path_grouping_policy = %s %s", mp->alias,
> buff, origin);
> + condlog(log_prio, "%s: path_grouping_policy = %s %s", mp-
> >alias, buff,
> + origin);
> return 0;
> }
>
> diff --git a/libmultipath/structs.c b/libmultipath/structs.c
> index 87e84d5d..39504dca 100644
> --- a/libmultipath/structs.c
> +++ b/libmultipath/structs.c
> @@ -125,6 +125,7 @@ alloc_path (void)
> pp->sg_id.proto_id = PROTOCOL_UNSET;
> pp->fd = -1;
> pp->tpgs = TPGS_UNDEF;
> + pp->tpg_id = GROUP_ID_UNDEF;
> pp->priority = PRIO_UNDEF;
> pp->checkint = CHECKINT_UNDEF;
> checker_clear(&pp->checker);
> diff --git a/libmultipath/structs.h b/libmultipath/structs.h
> index a1208751..0eea19b4 100644
> --- a/libmultipath/structs.h
> +++ b/libmultipath/structs.h
> @@ -317,6 +317,8 @@ struct hd_geometry {
> };
> #endif
>
> +#define GROUP_ID_UNDEF -1
> +
> struct path {
> char dev[FILE_NAME_SIZE];
> char dev_t[BLK_DEV_SIZE];
> @@ -372,6 +374,7 @@ struct path {
> /* configlet pointers */
> vector hwe;
> struct gen_path generic_path;
> + int tpg_id;
> };
>
> typedef int (pgpolicyfn) (struct multipath *, vector);
> diff --git a/multipath/main.c b/multipath/main.c
> index 90f940f1..b78f3162 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -157,6 +157,7 @@ usage (char * progname)
> " . group_by_serial one priority group
> per serial\n"
> " . group_by_prio one priority group
> per priority lvl\n"
> " . group_by_node_name one priority group
> per target node\n"
> + " . group_by_tpg one priority group
> per ALUA target port group\n"
> " -v lvl verbosity level:\n"
> " . 0 no output\n"
> " . 1 print created devmap names only\n"
> diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
> index b4dccd1b..b65a543d 100644
> --- a/multipath/multipath.conf.5
> +++ b/multipath/multipath.conf.5
> @@ -233,6 +233,10 @@ per-multipath option in the configuration file.
> One priority group per target node name. Target node names are
> fetched
> in \fI/sys/class/fc_transport/target*/node_name\fR.
> .TP
> +.I group_by_tpg
> +One priority group per ALUA target port group. In order to use this
> policy,
> +all paths in the multipath device must have \fIprio\fR set to
> \fBalua\fR.
> +.TP
> The default is: \fBfailover\fR
> .RE
> .
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
