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

[Hannes Reinecke <hare@suse.de>]

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)