[dm-devel] [PATCH 3/5] multipathd: refresh all priorities if one has changed

[Martin Wilck]

On Wed, 2023-05-24 at 18:21 -0500, Benjamin Marzinski wrote:
> For multipath devices with path group policies other than
> group_by_prio,
> multipathd wasn't updating all the paths' priorities when calling
> need_switch_pathgroup(), even in cases where it likely was necessary.
> When a path just becomes usable again, all paths' priorities get
> updated
> by update_prio().  But if the priority changes on a path that is
> already
> up, the other paths' priorities only get updated if the multipath
> device
> uses the group_by_prio path_grouping_policy, otherwise
> need_switch_pathgroup() is called with refresh set to 0. But if the
> priority of the checked path has changed, then likely so have the
> priorities of other paths. Since the pathgroup's priority is the
> average
> of its paths' priorities, changing the priority of just one path may
> not
> change the average enough to reorder the pathgroups.
> 
> Instead, set refresh in need_switch_pathgroup() if the priorty has
> changed to something other than PRIO_UNDEF (which usually means an
> error
> has occured) and the priorities of the other paths haven't already
> been
> updated by update_prio().
> 
> Signed-off-by: Benjamin Marzinski <bmarzins at redhat.com>
> ---
>  multipathd/main.c | 35 +++++++++++++++++++++--------------
>  1 file changed, 21 insertions(+), 14 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index bdeffe76..e7c272ad 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2575,20 +2575,27 @@ check_path (struct vectors * vecs, struct
> path * pp, unsigned int ticks)
>  
>         if (marginal_changed)
>                 reload_and_sync_map(pp->mpp, vecs, 1);
> -       else if (update_prio(pp, new_path_up) &&
> -           (pp->mpp->pgpolicyfn == (pgpolicyfn *)group_by_prio) &&
> -            pp->mpp->pgfailback == -FAILBACK_IMMEDIATE) {
> -               condlog(2, "%s: path priorities changed. reloading",
> -                       pp->mpp->alias);
> -               reload_and_sync_map(pp->mpp, vecs, !new_path_up);
> -       } else if (need_switch_pathgroup(pp->mpp, 0)) {
> -               if (pp->mpp->pgfailback > 0 &&
> -                   (new_path_up || pp->mpp->failback_tick <= 0))
> -                       pp->mpp->failback_tick =
> -                               pp->mpp->pgfailback + 1;
> -               else if (pp->mpp->pgfailback == -FAILBACK_IMMEDIATE
> ||
> -                        (chkr_new_path_up &&
> followover_should_failback(pp)))
> -                       switch_pathgroup(pp->mpp);
> +       else {
> +               int prio_changed = update_prio(pp, new_path_up);
> +               bool need_refresh = (!new_path_up && prio_changed &&
> +                                    pp->priority != PRIO_UNDEF);
> +

I have always found it confusing that we recalculate the priorities in
two functions (update_prio() and need_switch_pathgroup()), passing
boolean flags back and forth. IMO we should move this logic to
update_prio(), so that we don't need to refresh any priorities in
need_switch_pathgroup() any more. after determining the prio of the
"primary" path device, update_prio() has all the information
it needs to figure out whether priorities of other paths must be
refreshed.

That would even make the code easier to understand, IMO.

Regards
Martin



> +               if (prio_changed &&
> +                   pp->mpp->pgpolicyfn == (pgpolicyfn
> *)group_by_prio &&
> +                   pp->mpp->pgfailback == -FAILBACK_IMMEDIATE) {
> +                       condlog(2, "%s: path priorities changed.
> reloading",
> +                               pp->mpp->alias);
> +                       reload_and_sync_map(pp->mpp, vecs,
> !new_path_up);
> +               } else if (need_switch_pathgroup(pp->mpp,
> need_refresh)) {
> +                       if (pp->mpp->pgfailback > 0 &&
> +                           (new_path_up || pp->mpp->failback_tick <=
> 0))
> +                               pp->mpp->failback_tick =
> +                                       pp->mpp->pgfailback + 1;
> +                       else if (pp->mpp->pgfailback == -
> FAILBACK_IMMEDIATE ||
> +                                (chkr_new_path_up &&
> +                                 followover_should_failback(pp)))
> +                               switch_pathgroup(pp->mpp);
> +               }
>         }
>         return 1;
>  }