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