On Wed, 2018-02-28 at 17:40 -0600, Benjamin Marzinski wrote:
> On Tue, Feb 20, 2018 at 02:26:43PM +0100, Martin Wilck wrote:
> > Updating the prio values for printing makes no sense. The user
> > wants to see
> > the prio values multipath is actually using for path group
> > selection, and
> > updating the values here means actually lying to the user if the
> > prio values
> > have changed, but multipathd hasn't updated them internally.
> >
> > If we really don't update the pathgroup prios when we need to, this
> > should be
> > fixed elsewhere. The current wrong output would just hide that if
> > it occured.
> >
> > Moreover, correctness forbids changing properties so deeply in a
> > code path
> > that's supposed to print them only. Finally, this piece of code
> > prevents the
> > print.c code to be converted to proper "const" usage.
>
> Well, it is true that we've only been updating the path group
> priority
> when we've needed it, and we've only need it to be uptodate when we
> are
> picking a new pathgroup, or are printing it out. When failback is set
> to
> "manual", we rarely are picking a new pathgroup, so we rarely update
> the
> pathgroup prio.
>
> [...]
>
> I'd be fine with simply updating the path group priority whever we
> change a
> path's priority, if we aren't updating it when printing it. The
> bigger
> work of actually making sure that the path group order it the table
> is always uptodate needs to happen, but it doesn't need to happen in
> this patchset.
I just reviewed the code flow in check_path(), and here's what I see:
1. calls update_prio(pp, new_path_up)
-> updates path's prio, or all paths' prios if the path was
reinstated
Now it calls either
2a. update_path_groups() (for group_by_prio and failback immediate)
(-> reload_map() -> setup_map() -> select_path_group()
-> path_group_prio_update()), or
2b. need_switch_pathgroup() (otherwise)
So all we need to make sure is that need_switch_pathgroup() calls
select_path_group(). It does that already, except for the "failback
manual" case.
So all we need is IMO the attached patch. Tell me what you think.
[All of the above is only called if (!mpp->wait_for_udev), but if
wait_for_udev is set, either when the event arrives or the wait times
out, we'll call reconfigure() which makes sure all priorities are
correct].
Best,
Martin
--
Dr. Martin Wilck <mwilck@xxxxxxxx>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
From 149f4458d138d504ee5947aaa6e38d134b21368a Mon Sep 17 00:00:00 2001
From: Martin Wilck <mwilck@xxxxxxxx>
Date: Fri, 2 Mar 2018 14:51:52 +0100
Subject: [PATCH] multipathd: update path group prio in check_path
The previous patch "libmultipath: don't update path groups when printing"
removed the call to path_group_prio_update() in the printing code path.
To compensate for that, recalculate path group prio also when it's not
strictly necessary (i.e. if failback "manual" is set).
Signed-off-by: Martin Wilck <mwilck@xxxxxxxx>
---
multipathd/main.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/multipathd/main.c b/multipathd/main.c
index e1d98861a841..61739ac6ea59 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -252,8 +252,9 @@ need_switch_pathgroup (struct multipath * mpp, int refresh)
struct path * pp;
unsigned int i, j;
struct config *conf;
+ int bestpg;
- if (!mpp || mpp->pgfailback == -FAILBACK_MANUAL)
+ if (!mpp)
return 0;
/*
@@ -272,8 +273,11 @@ need_switch_pathgroup (struct multipath * mpp, int refresh)
if (!mpp->pg || VECTOR_SIZE(mpp->paths) == 0)
return 0;
- mpp->bestpg = select_path_group(mpp);
+ bestpg = select_path_group(mpp);
+ if (mpp->pgfailback == -FAILBACK_MANUAL)
+ return 0;
+ mpp->bestpg = bestpg;
if (mpp->bestpg != mpp->nextpg)
return 1;
--
2.16.1
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
