Re: [PATCH 1/5] libmultipath: add group_by_tpg path_grouping_policy

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [DM Crypt]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Packaging]     [Fedora SELinux]     [Yosemite Discussion]     [KDE Users]     [Fedora Docs]

  Powered by Linux