On Wed, May 31, 2023 at 03:19:55PM +0000, Martin Wilck wrote:
> On Fri, 2023-05-19 at 18:02 -0500, Benjamin Marzinski wrote:
> > When we group paths by prio and the priority changes, paths can end
> > up
> > temporarily in the wrong path groups. This usually happens when some
> > paths are down, so their priority can't be updated. To avoid this for
> > ALUA paths, group them by their target port group instead. The path
> > groups chosen by this policy won't always match with those chosen by
> > group_by_prio, since it is possible for multiple ALUA target port
> > groups to have the same priority.
> >
> > Signed-off-by: Benjamin Marzinski <bmarzins@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?
We can certainly log a message here, but like you said in your reply to
my cover letter, it does appear possible the a path can change the
Target Port Group it belongs to. In this case, we should update the
tpg_id if we notice the change, so the path gets placed in correct
group. With my second patchset, which makes multipath do more to update
the priorities of all the paths in non-group-by-prio setups, this should
mostly keep the paths in the correct pathgroup, if they change TPG.
Obviously the paths that are down won't get updated, but that's no worse
than we have now, and changing TPGs is a corner case.
>
> > condlog(3, "%s: reported target port group is %i", pp->dev,
> > tpg);
> > rc = get_asymmetric_access_state(pp, tpg, timeout);
> > if (rc < 0) {
> > diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
> > index a25cc921..841fa247 100644
> > --- a/libmultipath/propsel.c
> > +++ b/libmultipath/propsel.c
> > @@ -35,7 +35,8 @@ pgpolicyfn *pgpolicies[] = {
> > one_group,
> > group_by_serial,
> > group_by_prio,
> > - group_by_node_name
> > + group_by_node_name,
> > + group_by_tpg,
> > };
> >
> > #define do_set(var, src, dest,
> > msg) \
> > @@ -249,10 +250,26 @@ out:
> > return 0;
> > }
> >
> > +static bool
> > +verify_alua_prio(struct multipath *mp)
> > +{
> > + int i;
> > + struct path *pp;
> > +
> > + vector_foreach_slot(mp->paths, pp, i) {
> > + const char *name = prio_name(&pp->prio);
> > + if (strncmp(name, PRIO_ALUA, PRIO_NAME_LEN) &&
> > + strncmp(name, PRIO_SYSFS, PRIO_NAME_LEN))
> > + return false;
> > + }
> > + return true;
> > +}
> > +
> > int select_pgpolicy(struct config *conf, struct multipath * mp)
> > {
> > const char *origin;
> > char buff[POLICY_NAME_SIZE];
> > + int log_prio = 3;
> >
> > if (conf->pgpolicy_flag > 0) {
> > mp->pgpolicy = conf->pgpolicy_flag;
> > @@ -265,9 +282,15 @@ int select_pgpolicy(struct config *conf, struct
> > multipath * mp)
> > mp_set_conf(pgpolicy);
> > mp_set_default(pgpolicy, DEFAULT_PGPOLICY);
> > out:
> > + if (mp->pgpolicy == GROUP_BY_TPG && !verify_alua_prio(mp)) {
> > + mp->pgpolicy = DEFAULT_PGPOLICY;
> > + origin = "(setting: emergency fallback - not all
> > paths use alua prio)";
> > + log_prio = 1;
> > + }
>
> I don't understand this logic. Why don't you simply check pp->tpgs?
The call to verify_alua_prio is because the prio is set per path, and it
is possible that not all paths have the ALUA prioritizer. Regardless of
how that occured (and I admit that it's really only possible due to a
failure while trying to get the tpg), we can't really group the paths by
tpg if not all the paths have it set.
As for why verfy_alua_prio() doesn't check pp->tpgs, there are multiple
cases where a path that is configured with alua can fail detect_prio()
with what appears to be a temporary error. In that case the tpgs will
remain TPGS_UNDEF. So if pp->tgpgs == UNDEF, the code would still need
to check the prio name to see if the path is configured with ALUA. On
the other hand, if we check the prio name first, then that's the
definitive answer, and it doesn't matter if p->tpgs wasn't set yet.
> > mp->pgpolicyfn = pgpolicies[mp->pgpolicy];
> > get_pgpolicy_name(buff, POLICY_NAME_SIZE, mp->pgpolicy);
> > - condlog(3, "%s: path_grouping_policy = %s %s", mp->alias,
> > buff, origin);
> > + condlog(log_prio, "%s: path_grouping_policy = %s %s", mp-
> > >alias, buff,
> > + origin);
> > return 0;
> > }
> >
> > diff --git a/libmultipath/structs.c b/libmultipath/structs.c
> > index 87e84d5d..39504dca 100644
> > --- a/libmultipath/structs.c
> > +++ b/libmultipath/structs.c
> > @@ -125,6 +125,7 @@ alloc_path (void)
> > pp->sg_id.proto_id = PROTOCOL_UNSET;
> > pp->fd = -1;
> > pp->tpgs = TPGS_UNDEF;
> > + pp->tpg_id = GROUP_ID_UNDEF;
> > pp->priority = PRIO_UNDEF;
> > pp->checkint = CHECKINT_UNDEF;
> > checker_clear(&pp->checker);
> > diff --git a/libmultipath/structs.h b/libmultipath/structs.h
> > index a1208751..0eea19b4 100644
> > --- a/libmultipath/structs.h
> > +++ b/libmultipath/structs.h
> > @@ -317,6 +317,8 @@ struct hd_geometry {
> > };
> > #endif
> >
> > +#define GROUP_ID_UNDEF -1
> > +
> > struct path {
> > char dev[FILE_NAME_SIZE];
> > char dev_t[BLK_DEV_SIZE];
> > @@ -372,6 +374,7 @@ struct path {
> > /* configlet pointers */
> > vector hwe;
> > struct gen_path generic_path;
> > + int tpg_id;
> > };
> >
> > typedef int (pgpolicyfn) (struct multipath *, vector);
> > diff --git a/multipath/main.c b/multipath/main.c
> > index 90f940f1..b78f3162 100644
> > --- a/multipath/main.c
> > +++ b/multipath/main.c
> > @@ -157,6 +157,7 @@ usage (char * progname)
> > " . group_by_serial one priority group
> > per serial\n"
> > " . group_by_prio one priority group
> > per priority lvl\n"
> > " . group_by_node_name one priority group
> > per target node\n"
> > + " . group_by_tpg one priority group
> > per ALUA target port group\n"
> > " -v lvl verbosity level:\n"
> > " . 0 no output\n"
> > " . 1 print created devmap names only\n"
> > diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
> > index b4dccd1b..b65a543d 100644
> > --- a/multipath/multipath.conf.5
> > +++ b/multipath/multipath.conf.5
> > @@ -233,6 +233,10 @@ per-multipath option in the configuration file.
> > One priority group per target node name. Target node names are
> > fetched
> > in \fI/sys/class/fc_transport/target*/node_name\fR.
> > .TP
> > +.I group_by_tpg
> > +One priority group per ALUA target port group. In order to use this
> > policy,
> > +all paths in the multipath device must have \fIprio\fR set to
> > \fBalua\fR.
> > +.TP
> > The default is: \fBfailover\fR
> > .RE
> > .
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
