diff mbox

Use Average path priority value for path switching

Message ID 1246583474.30568.2.camel@chandra-ubuntu (mailing list archive)
State Superseded, archived
Delegated to: christophe varoqui
Headers show

Commit Message

Chandra Seetharaman July 3, 2009, 1:11 a.m. UTC 
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.

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(-)



--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Comments

Hannes Reinecke July 3, 2009, 6:40 a.m. UTC | #1 
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
Chandra Seetharaman July 6, 2009, 6:17 p.m. UTC | #2 
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

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
diff mbox

Patch

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;
 	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;
+			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;
 	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;
+			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;