Re: Re: [PATCH] Use Average path priority value for path switching

[Chandra Seetharaman <sekharan@us.ibm.com>]

Hi Hannes,

Thanks for your review comments. I also had similar line of thought when
I coded this up. But, concluded the way I did due to the following
reasoning.

Having a single priority instead of aggregates:
 1. If different paths in a path group have different priorities, then
    we would take only the last path's priority into account, which is
    not correct.
 2. Currently, multipath -ll displays the sum of priorities of the path
    group. If we change it that of a single path, it will confuse users,
    unnecessarily.

up_paths Vs active paths:
 - in the generic nomenclature we use "active path" to refer to the path
   that is currently being used for sending I/Os. But, here (when
   calculating priorities), we do consider paths that are not "active"
   also. So, if we use "active_paths" it will not be consistent with
   the generic usage.

Let me know what you think.

chandra
On Fri, 2009-07-03 at 08:40 +0200, Hannes Reinecke wrote:
> Hi Chandra,
> 
> Chandra Seetharaman wrote:
> > Hello,
> > 
> > Few weeks back I posted some issues w.r.t the way path priorities are
> > used during path switching.
> > 
> > Here is Hannes's latest response
> > (http://marc.info/?l=dm-devel&m=124573807907764&w=2) and this patch is
> > based on his suggestion.
> > 
> Very cool! Well done there. But a few comments I have, see inline.
> 
> > regards,
> > 
> > chandra
> > ----------------------------------------------------------------------- 
> > Failback happens only when the sum of priorities of all paths
> > (on the higher priority path group) is greater than the sum
> > of priorities of all paths on the lower priority path group.
> > 
> > This leads into problems when there are more than one paths
> > in each of the path groups, and the sum of all paths in the
> > lower priority path group is greater than that of path priority
> > of a single high priority path.
> > 
> > This patch fixes the problem by using average priority of a
> > path group in deciding path group switch over.
> > 
> > Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com>
> > ---
> >  libmultipath/structs.h     |    1 +
> >  libmultipath/switchgroup.c |   23 ++++++++++++++++++-----
> >  2 files changed, 19 insertions(+), 5 deletions(-)
> > 
> > Index: multipath-tools-mainline/libmultipath/structs.h
> > ===================================================================
> > --- multipath-tools-mainline.orig/libmultipath/structs.h
> > +++ multipath-tools-mainline/libmultipath/structs.h
> > @@ -202,6 +202,7 @@ struct pathgroup {
> >  	long id;
> >  	int status;
> >  	int priority;
> > +	int up_paths;
> 
> Maybe rename this to active_paths?
> 
> >  	vector paths;
> >  	char * selector;
> >  };
> > Index: multipath-tools-mainline/libmultipath/switchgroup.c
> > ===================================================================
> > --- multipath-tools-mainline.orig/libmultipath/switchgroup.c
> > +++ multipath-tools-mainline/libmultipath/switchgroup.c
> > @@ -14,13 +14,16 @@ path_group_prio_update (struct pathgroup
> >  	int priority = 0;
> >  	struct path * pp;
> >  
> > +	pgp->up_paths = 0;
> >  	if (!pgp->paths) {
> >  		pgp->priority = 0;
> >  		return;
> >  	}
> >  	vector_foreach_slot (pgp->paths, pp, i) {
> > -		if (pp->state != PATH_DOWN)
> > +		if (pp->state != PATH_DOWN) {
> >  			priority += pp->priority;
> Do _not_ aggregate the path state here; just do
> 
> priority = pp->priority
> 
> it'll save you the averaging out later on.
> 
> > +			pgp->up_paths++;
> > +		}
> >  	}
> >  	pgp->priority = priority;
> >  }
> > @@ -29,8 +32,9 @@ extern int
> >  select_path_group (struct multipath * mpp)
> >  {
> >  	int i;
> > -	int highest = 0;
> > +	int highest_avg = 0;
> >  	int bestpg = 1;
> > +	int avg_priority, highest_up_paths = 1;
> 
> Again, maybe use max_active_paths and max_priority
> 
> >  	struct pathgroup * pgp;
> >  
> >  	if (!mpp->pg)
> > @@ -41,9 +45,18 @@ select_path_group (struct multipath * mp
> >  			continue;
> >  
> >  		path_group_prio_update(pgp);
> > -		if (pgp->priority > highest) {
> > -			highest = pgp->priority;
> > -			bestpg = i + 1;
> > +		if (pgp->up_paths) {
> > +			avg_priority = pgp->priority / pgp->up_paths;
> You don't have to average here, if you don't aggregate the priority
> as mentioned above.
> 
> The test would then just be
> if (pgp->priority > max_priority) {
>     max_priority = pgp->priority;
>     max_active_paths = pgp->active_paths;
>     bestpg = i + i;
> } else if (pgp->priority == max_priority) {
>     if (pgp->active_paths > max_active_paths) {
>         max_active_paths = pgp->active_paths;
>         bestpg = i + 1;
>     }
> }
> 
> > +			if (avg_priority > highest_avg) {
> > +				highest_avg = avg_priority;
> > +				highest_up_paths = pgp->up_paths;
> > +				bestpg = i + 1;
> > +			} else if (avg_priority == highest_avg) {
> > +				if (pgp->up_paths > highest_up_paths) {
> > +					highest_up_paths = pgp->up_paths;
> > +					bestpg = i + 1;
> > +				}
> > +			}
> >  		}
> >  	}
> >  	return bestpg;
> > 
> > 
> 
> But apart from this: Yes, this is exactly how I
> think it should be done.
> 
> Great job, Chandra.
> 
> Cheers,
> 
> Hannes
> -- 
> Dr. Hannes Reinecke		      zSeries & Storage
> hare@suse.de			      +49 911 74053 688
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: Markus Rex, HRB 16746 (AG Nürnberg)
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel