[PATCH] Use Average path priority value for path switching

[PATCH] Use Average path priority value for path switching

[Chandra Seetharaman]

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

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;

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

[Hannes Reinecke]

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)

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

[Chandra Seetharaman]

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

[RESEND] [PATCH] Use Average path priority value for path switching

[Chandra Seetharaman]

Hi Christophe,

I submitted this patch on Jul 2
(http://marc.info/?l=dm-devel&m=124658334721911&w=2). Resending it.

Only change is a field name from up_paths to enabled_paths.

Hi Hannes,

Need an ACK from you :-).

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 
the path group to decide on which path group to 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 enabled_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->enabled_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->enabled_paths++;
+		}
 	}
 	pgp->priority = priority;
 }
@@ -29,8 +32,9 @@ extern int
 select_path_group (struct multipath * mpp)
 {
 	int i;
-	int highest = 0;
+	int max_priority = 0, avg_priority;
 	int bestpg = 1;
+	int max_enabled_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->enabled_paths) {
+			avg_priority = pgp->priority / pgp->enabled_paths;
+			if (avg_priority > max_priority) {
+				max_priority = avg_priority;
+				max_enabled_paths = pgp->enabled_paths;
+				bestpg = i + 1;
+			} else if (avg_priority == max_priority) {
+				if (pgp->enabled_paths > max_enabled_paths) {
+					max_enabled_paths = pgp->enabled_paths;
+					bestpg = i + 1;
+				}
+			}
 		}
 	}
 	return bestpg;

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

[Hannes Reinecke]

Chandra Seetharaman wrote:
> Hi Christophe,
> 
> I submitted this patch on Jul 2
> (http://marc.info/?l=dm-devel&m=124658334721911&w=2). Resending it.
> 
> Only change is a field name from up_paths to enabled_paths.
> 
> Hi Hannes,
> 
> Need an ACK from you :-).
> 
> 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 
> the path group to decide on which path group to switch over.
> 
> Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com>
Acked-by: Hannes Reinecke <hare@suse.de>

Thanks for this, 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)