| Message ID | 20240117225110.50072-1-brian@purestorage.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | multipathd: the local path change is not considered | expand |
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@purestorage.com> > Signed-off-by: Seamus Connor <sconnor@purestorage.com> > Signed-off-by: Krisha Kant <krishna.kant@purestorage.com> > --- > 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@redhat.com 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; > > vector_foreach_slot (pp->mpp->pg, pgp, i) { > vector_foreach_slot (pgp->paths, pp1, j) { > @@ -2151,10 +2154,10 @@ int update_prio(struct path *pp, int > refresh_all) > pathinfo(pp1, conf, DI_PRIO); > pthread_cleanup_pop(1); > if (pp1->priority != oldpriority) > - changed = 1; > + other_changed = 1; > } > } > - return changed; > + return (local_changed || other_changed); > } > > static int reload_map(struct vectors *vecs, struct multipath *mpp,
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@purestorage.com> > > Signed-off-by: Seamus Connor <sconnor@purestorage.com> > > Signed-off-by: Krisha Kant <krishna.kant@purestorage.com> > > --- > > 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@redhat.com 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
On Thu, 2024-01-18 at 09:30 +0100, Martin Wilck wrote: > > > > > 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). Sorry, this was nonsense. The commit msg of 6ccd7b8 explains that we must run the loop if "changed" is true, even if refresh_all is not set. Martin
On Thu, Jan 18, 2024 at 09:16:35AM +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@purestorage.com> > > Signed-off-by: Seamus Connor <sconnor@purestorage.com> > > Signed-off-by: Krisha Kant <krishna.kant@purestorage.com> > > --- > > 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@redhat.com and myself to cc, > and add > > Fixes: 6ccd7b8 ("multipathd: only refresh priorities in update_prio()") > > Regards, > Martin ACK for the code with using "changed" for both the local and other changes. Thanks for catching this. -Ben > > 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; > > > > vector_foreach_slot (pp->mpp->pg, pgp, i) { > > vector_foreach_slot (pgp->paths, pp1, j) { > > @@ -2151,10 +2154,10 @@ int update_prio(struct path *pp, int > > refresh_all) > > pathinfo(pp1, conf, DI_PRIO); > > pthread_cleanup_pop(1); > > if (pp1->priority != oldpriority) > > - changed = 1; > > + other_changed = 1; > > } > > } > > - return changed; > > + return (local_changed || other_changed); > > } > > > > static int reload_map(struct vectors *vecs, struct multipath *mpp, >
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; 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; vector_foreach_slot (pp->mpp->pg, pgp, i) { vector_foreach_slot (pgp->paths, pp1, j) { @@ -2151,10 +2154,10 @@ int update_prio(struct path *pp, int refresh_all) pathinfo(pp1, conf, DI_PRIO); pthread_cleanup_pop(1); if (pp1->priority != oldpriority) - changed = 1; + other_changed = 1; } } - return changed; + return (local_changed || other_changed); } static int reload_map(struct vectors *vecs, struct multipath *mpp,