Re: [PATCH 11/16] Fix max path checker timing

[Benjamin Marzinski <bmarzins@redhat.com>]

On Fri, May 03, 2013 at 08:59:11AM +0200, Hannes Reinecke wrote:
> On 05/02/2013 11:46 PM, Benjamin Marzinski wrote:
> > Due to some code being placed inside the wrong block, the number of
> > seconds to wait between path checks (pp->tick), was only getting set to
> > the path's individual check interval if that wasn't equal to the max
> > check interval.  Otherwise it was using the default for a failed path.
> > This patch makes sure that pp->ticks always always gets set correctly
> > for active paths.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  multipathd/main.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index 440d254..df1c5b9 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -1219,11 +1219,10 @@ check_path (struct vectors * vecs, struct path * pp)
> >  					pp->checkint = 2 * pp->checkint;
> >  				else
> >  					pp->checkint = conf->max_checkint;
> > -
> > -				pp->tick = pp->checkint;
> > -				condlog(4, "%s: delay next check %is",
> > -					pp->dev_t, pp->tick);
> >  			}
> > +			pp->tick = pp->checkint;
> > +			condlog(4, "%s: delay next check %is",
> > +				pp->dev_t, pp->tick);
> >  		}
> >  	}
> >  	else if (newstate == PATH_DOWN) {
> > 
> But then the message is wrong, isn't it?
> We should be printing the 'delay next check' message only
> if the check was actually delayed, ie inside the block.
> 
> So I think something like this should be more appropriate
> (minus line-breaks, of course):
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 6471d24..9901b02 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -1235,10 +1235,10 @@ check_path (struct vectors * vecs, struct
> path * pp)
>                                 else
>                                         pp->checkint =
> conf->max_checkint;
> 
> -                               pp->tick = pp->checkint;
>                                 condlog(4, "%s: delay next check %is",
>                                         pp->dev_t, pp->tick);

I think you mean pp->checkint here.  Otherwise the code will print
pp->tick before we set it. But otherwise, I'm fine with this.  I'll
respin the patch.

-Ben

>                         }
> +                       pp->tick = pp->checkint;
>                 }
>         }
>         else if (newstate == PATH_DOWN) {
> 
> 
> Cheers,
> 
> Hannes
> -- 
> Dr. Hannes Reinecke		      zSeries & Storage
> hare@suse.de			      +49 911 74053 688
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel