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@xxxxxxxxxx> > --- > 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@xxxxxxx +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@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel
