[dm-devel] Re: [PATCH] Use Average path priority value for path switching
Hannes Reinecke
hare at suse.de
Fri Jul 3 06:40:12 UTC 2009
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 at 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 at suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)
More information about the dm-devel
mailing list