Re: [PATCH] multipathd: the local path change is not considered

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

 



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






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

  Powered by Linux