[dm-devel] [PATCH 4/5] multipathd: reload map if the path groups are out of order

[Martin Wilck]

On Wed, 2023-05-24 at 18:21 -0500, Benjamin Marzinski wrote:
> need_switch_pathgroup() only checks if the currently used pathgroup
> is
> not the highest priority pathgroup. If it isn't, all multipathd does
> is
> instruct the kernel to switch to the correct pathgroup.  However, the
> kernel treats the pathgroups as if they were ordered by priority.
> When
> the kernel runs out of paths to use in the currently selected
> pathgroup,
> it will start checking the pathgroups in order until it finds one
> with
> usable paths.
> 
> need_switch_pathgroup() should also check if the pathgroups are out
> of
> order, and if so, multipathd should reload the map to reorder them
> correctly.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins at redhat.com>
> ---
>  libmultipath/libmultipath.version |  5 ++++
>  libmultipath/switchgroup.c        | 27 ++++++++++++++++++++++
>  libmultipath/switchgroup.h        |  1 +
>  multipathd/main.c                 | 38 +++++++++++++++++++++--------
> --
>  4 files changed, 59 insertions(+), 12 deletions(-)
> 
> diff --git a/libmultipath/libmultipath.version
> b/libmultipath/libmultipath.version
> index 8f72c452..38074699 100644
> --- a/libmultipath/libmultipath.version
> +++ b/libmultipath/libmultipath.version
> @@ -237,3 +237,8 @@ global:
>  local:
>         *;
>  };
> +
> +LIBMULTIPATH_19.1.0 {
> +global:
> +       path_groups_in_order;
> +} LIBMULTIPATH_19.0.0;
> diff --git a/libmultipath/switchgroup.c b/libmultipath/switchgroup.c
> index b1e1f39b..b1180839 100644
> --- a/libmultipath/switchgroup.c
> +++ b/libmultipath/switchgroup.c
> @@ -7,6 +7,33 @@
>  #include "structs.h"
>  #include "switchgroup.h"
>  
> +bool path_groups_in_order(struct multipath *mpp)
> +{
> +       int i;
> +       struct pathgroup *pgp;
> +       bool seen_marginal_pg = false;
> +       int last_prio = INT_MAX;
> +
> +       if (VECTOR_SIZE(mpp->pg) < 2)
> +               return true;
> +
> +       vector_foreach_slot(mpp->pg, pgp, i) {
> +               /* skip pgs with PRIO_UNDEF, since this is likely
> temporary */
> +               if (!pgp->paths || pgp->priority == PRIO_UNDEF)
> +                       continue;
> +               if (pgp->marginal && !seen_marginal_pg) {
> +                       last_prio = INT_MAX;
> +                       continue;
> +               }
> +               if (seen_marginal_pg && !pgp->marginal)
> +                       return false;
> +               if (pgp->priority > last_prio)
> +                       return false;
> +               last_prio = pgp->priority;
> +       }
> +       return true;
> +}
> +

I stared at this code again while we were discussing, and I figured I
don't quite understand it. First of all, you never update
seen_marginal_pg. I suppose you want to set it in the if (pgp->marginal
&& !seen_marginal_pg) code block. But then, why set last_prio =
INT_MAX? If any non-marginal GP would follow, you would return false
anyway.

Martin