On Thu, 2024-01-18 at 09:16 +0100, Martin Wilck wrote:
> On Wed, 2024-01-17 at 14:51 -0800, Brian Bunker wrote:
> > When update_prio is called, there is a check to see if the local
> > path
> > has a priority change. Then all the remaining paths are simliarly
> > checked.
> >
> > Only the result of paths not matching the local path's priority are
> > considered in the calculation of 'changed'. In the case of failed
> > paths
> > becoming again available this can lead to multipath not reloading.
> >
> > The result will look like this:
> > 3624a93703c9f34490f6140a100011010 dm-2 PURE,FlashArray
> > size=3.0T features='1 retain_attached_hw_handler' hwhandler='1
> > alua'
> > wp=rw
> > > -+- policy='service-time 0' prio=50 status=active
> > > > - 1:0:0:1 sdb 8:16 active ready running
> > > > - 9:0:0:1 sdc 8:32 active ready running
> > > > - 11:0:0:1 sdd 8:48 active ready running
> > > `- 13:0:0:1 sdh 8:112 active ready running
> > `-+- policy='service-time 0' prio=50 status=enabled
> > |- 8:0:0:1 sde 8:64 active ready running
> > |- 10:0:0:1 sdf 8:80 active ready running
> > |- 12:0:0:1 sdg 8:96 active ready running
> > `- 14:0:0:1 sdi 8:128 active ready running
> >
> > Where the path's priorities are updated, but the path group is not
> > activated.
> >
> > Signed-off-by: Brian Bunker <brian@xxxxxxxxxxxxxxx>
> > Signed-off-by: Seamus Connor <sconnor@xxxxxxxxxxxxxxx>
> > Signed-off-by: Krisha Kant <krishna.kant@xxxxxxxxxxxxxxx>
> > ---
> > multipathd/main.c | 13 ++++++++-----
> > 1 file changed, 8 insertions(+), 5 deletions(-)
> >
>
> Thanks for the report and fix!
>
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index 230c9d10..62b87bf8 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -2127,7 +2127,7 @@ int update_prio(struct path *pp, int
> > refresh_all)
> > int oldpriority;
> > struct path *pp1;
> > struct pathgroup * pgp;
> > - int i, j, changed = 0;
> > + int i, j, local_changed = 0, other_changed = 0;
>
> Why use two variables here? AFAICS, you can implement the same logic
> with just the "changed" variable.
>
> Other than that, this looks good.
>
> When you submit a v2, please add bmarzins@xxxxxxxxxx and myself to
> cc,
> and add
>
> Fixes: 6ccd7b8 ("multipathd: only refresh priorities in
> update_prio()")
>
> Regards,
> Martin
>
> > struct config *conf;
> >
> > oldpriority = pp->priority;
> > @@ -2138,8 +2138,11 @@ int update_prio(struct path *pp, int
> > refresh_all)
> > pthread_cleanup_pop(1);
> > }
> >
> > - if (pp->priority == oldpriority && !refresh_all)
> > - return 0;
> > + if (pp->priority != oldpriority)
> > + local_changed = 1;
> > + else
> > + if (!refresh_all)
> > + return 0;
Just realized that this would unnecessarily run through the loop below
if (pp->priority != oldpriority && !refresh_all).
IMO the patch should simply look like this:
diff --git a/multipathd/main.c b/multipathd/main.c
index fbc3f8d..ca8e65c 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2115,8 +2115,10 @@ int update_prio(struct path *pp, int
refresh_all)
pthread_cleanup_pop(1);
}
- if (pp->priority == oldpriority && !refresh_all)
- return 0;
+ if (pp->priority != oldpriority)
+ changed = 1;
+ if (!refresh_all)
+ return changed;
vector_foreach_slot (pp->mpp->pg, pgp, i) {
vector_foreach_slot (pgp->paths, pp1, j) {
If you don't object, I'll apply it in this form, leaving you as author.
Martin
