| Message ID | 20241205035638.1218953-3-bmarzins@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Benjamin Marzinski |
| Headers | show |
| Series | handle maps that need reloading | expand |
On Wed, 2024-12-04 at 22:56 -0500, Benjamin Marzinski wrote: > update_pathvec_from_dm() may set mpp->need_reload if it finds > inconsistent settings. In this case, the map should be reloaded, but > so > far we don't do this reliably. Add a call to reload_and_sync_map() > when updating the maps at the end of a loop in checkerloop(). > > Suggested-by: Martin Wilck <mwilck@suse.com> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > --- > multipathd/main.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/multipathd/main.c b/multipathd/main.c > index fcab1ed3..c1ae44f2 100644 > --- a/multipathd/main.c > +++ b/multipathd/main.c > @@ -3038,7 +3038,10 @@ checkerloop (void *ap) > > start_time.tv_sec); > if (checker_state == CHECKER_FINISHED) { > vector_foreach_slot(vecs->mpvec, > mpp, i) { > - if (update_mpp_prio(vecs, > mpp) == 2) { > + if (update_mpp_prio(vecs, > mpp) == 2 || > + (mpp->need_reload && > + mpp->synced_count > 0 > && > + > reload_and_sync_map(mpp, vecs) == 2)) { > /* multipath device > deleted */ > i--; > The general logic is ok, but as noted on https://github.com/opensvc/multipath-tools/issues/105), I believe that do_sync_mpp() is the more natural point in the code to attempt the reload, but I think it doesn't hurt to do both. What I don't like so much about this patch is that from this point in the code, we can call reload_and_sync_map() directly, or indirectly through update_mpp_prio(). I find that confusing. I think we should change update_mpp_prio() to just set the need_reload() flag, and then simply look at the flag in checkerloop(). The "minor" actions in update_mpp_prio, such as switching path group, can still be done directly from there. I can send a patch on top of yours if that's ok with you. I have some questions. I've spent some time today pulling my hair over the many functions in the multipath-tools code that are related to syncing states. I'm kind of wound up in my own thinking, perhaps you can help me break out of the loop :-) 1) I observe that update_map() and reload_and_sync_map() are very similar. They both set up the map from multipathd's internal state, reload it, and then update multipathd's state. The differences are as follows: - update_map() calls adopt_paths() and verify_paths() before setup_map(), while reload_map() only calls update_mpp_paths(). - It looks like an oversight to me that we don't call update_mpp_paths() in update_map(), what do you think? - What about verify_paths(), why do we call it only in the update_map() code path? - update_map() retries, while reload_and_sync_map() does not. - update_map() calls the persistent-reservation related code. These differences are few enough I think update_map() could actually call reload_and_sync_map() as a helper. 2) update_map() is called from missing_uev_wait_tick() and ghost_delay_tick(), while deferred_failback_tick() and the update_mpp_prio() code path above "only" call reload_and_sync_map(). Is this intentional? If yes, I am missing the rationale (I believe that just calling reload_and_sync_map() from all the _tick() functions would be sufficient, but I may be overlooking something). I'd love it if we could integrate the various _tick() calls with the rest of the map-reload logic. Ideally the checkerloop and the tick() functions would just set the need_reload flag, and then we'd do the reload once at the end of the loop. Regards, Martin
On Wed, 2024-12-04 at 22:56 -0500, Benjamin Marzinski wrote: > update_pathvec_from_dm() may set mpp->need_reload if it finds > inconsistent settings. In this case, the map should be reloaded, but > so > far we don't do this reliably. Add a call to reload_and_sync_map() > when updating the maps at the end of a loop in checkerloop(). > > Suggested-by: Martin Wilck <mwilck@suse.com> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> Reviewed-by: Martin Wilck <mwilck@suse.com> (I'll send a couple of patches on top of yours)
diff --git a/multipathd/main.c b/multipathd/main.c index fcab1ed3..c1ae44f2 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -3038,7 +3038,10 @@ checkerloop (void *ap) start_time.tv_sec); if (checker_state == CHECKER_FINISHED) { vector_foreach_slot(vecs->mpvec, mpp, i) { - if (update_mpp_prio(vecs, mpp) == 2) { + if (update_mpp_prio(vecs, mpp) == 2 || + (mpp->need_reload && + mpp->synced_count > 0 && + reload_and_sync_map(mpp, vecs) == 2)) { /* multipath device deleted */ i--; }
update_pathvec_from_dm() may set mpp->need_reload if it finds inconsistent settings. In this case, the map should be reloaded, but so far we don't do this reliably. Add a call to reload_and_sync_map() when updating the maps at the end of a loop in checkerloop(). Suggested-by: Martin Wilck <mwilck@suse.com> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> --- multipathd/main.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)