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 <mwi...@suse.com>, 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 <mwi...@suse.com> 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 <mwi...@suse.com> --- 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 firstname.lastname@example.org https://www.redhat.com/mailman/listinfo/dm-devel