| Message ID | 20250117202738.126196-4-mwilck@suse.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Benjamin Marzinski |
| Headers | show |
| Series | multipathd: More map reload handling, and checkerloop work | expand |
On Fri, Jan 17, 2025 at 09:27:26PM +0100, Martin Wilck wrote: > Rather than calling sync_mpp early in the checkerloop and tracking > map synchronization with synced_count, call sync_mpp() in the CHECKER_FINISHED > state, if either at least one path of the map has been checked in the current > iteration, or the sync tick has expired. This avoids potentially deleting > paths from the pathvec through the do_sync_mpp() -> update_multipath_strings() > -> sync_paths -> check_removed_paths() call chain while we're iterating over > the pathvec. Also, the time gap between obtaining path states and syncing > the state with the kernel is smaller this way. > > Suggested-by: Benjamin Marzinski <bmarzins@redhat.com> > Signed-off-by: Martin Wilck <mwilck@suse.com> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com> > --- > libmultipath/structs.h | 2 +- > libmultipath/structs_vec.c | 1 - > multipathd/main.c | 26 +++++++++++--------------- > 3 files changed, 12 insertions(+), 17 deletions(-) > > diff --git a/libmultipath/structs.h b/libmultipath/structs.h > index 6a30c59..9d22bdd 100644 > --- a/libmultipath/structs.h > +++ b/libmultipath/structs.h > @@ -471,7 +471,7 @@ struct multipath { > int ghost_delay_tick; > int queue_mode; > unsigned int sync_tick; > - int synced_count; > + int checker_count; > enum prio_update_type prio_update; > uid_t uid; > gid_t gid; > diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c > index 7a4e3eb..6aa744d 100644 > --- a/libmultipath/structs_vec.c > +++ b/libmultipath/structs_vec.c > @@ -530,7 +530,6 @@ update_multipath_table (struct multipath *mpp, vector pathvec, int flags) > conf = get_multipath_config(); > mpp->sync_tick = conf->max_checkint; > put_multipath_config(conf); > - mpp->synced_count++; > > r = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_MPATH_ONLY, > (mapid_t) { .str = mpp->alias }, > diff --git a/multipathd/main.c b/multipathd/main.c > index 4a28fbb..e4e6bf7 100644 > --- a/multipathd/main.c > +++ b/multipathd/main.c > @@ -2470,7 +2470,7 @@ sync_mpp(struct vectors * vecs, struct multipath *mpp, unsigned int ticks) > if (mpp->sync_tick) > mpp->sync_tick -= (mpp->sync_tick > ticks) ? ticks : > mpp->sync_tick; > - if (mpp->sync_tick) > + if (mpp->sync_tick && !mpp->checker_count) > return; > > do_sync_mpp(vecs, mpp); > @@ -2513,12 +2513,6 @@ update_path_state (struct vectors * vecs, struct path * pp) > return handle_path_wwid_change(pp, vecs)? CHECK_PATH_REMOVED : > CHECK_PATH_SKIPPED; > } > - if (pp->mpp->synced_count == 0) { > - do_sync_mpp(vecs, pp->mpp); > - /* if update_multipath_strings orphaned the path, quit early */ > - if (!pp->mpp) > - return CHECK_PATH_SKIPPED; > - } > if ((newstate != PATH_UP && newstate != PATH_GHOST && > newstate != PATH_PENDING) && (pp->state == PATH_DELAYED)) { > /* If path state become failed again cancel path delay state */ > @@ -2918,9 +2912,11 @@ check_paths(struct vectors *vecs, unsigned int ticks) > vector_foreach_slot(vecs->pathvec, pp, i) { > if (pp->is_checked != CHECK_PATH_UNCHECKED) > continue; > - if (pp->mpp) > + if (pp->mpp) { > pp->is_checked = check_path(pp, ticks); > - else > + if (pp->is_checked == CHECK_PATH_STARTED) > + pp->mpp->checker_count++; > + } else > pp->is_checked = check_uninitialized_path(pp, ticks); > if (pp->is_checked == CHECK_PATH_STARTED && > checker_need_wait(&pp->checker)) > @@ -3014,12 +3010,10 @@ checkerloop (void *ap) > pthread_cleanup_push(cleanup_lock, &vecs->lock); > lock(&vecs->lock); > pthread_testcancel(); > - vector_foreach_slot(vecs->mpvec, mpp, i) > - mpp->synced_count = 0; > if (checker_state == CHECKER_STARTING) { > vector_foreach_slot(vecs->mpvec, mpp, i) { > - sync_mpp(vecs, mpp, ticks); > mpp->prio_update = PRIO_UPDATE_NONE; > + mpp->checker_count = 0; > } > vector_foreach_slot(vecs->pathvec, pp, i) > pp->is_checked = CHECK_PATH_UNCHECKED; > @@ -3032,11 +3026,13 @@ checkerloop (void *ap) > start_time.tv_sec); > if (checker_state == CHECKER_FINISHED) { > vector_foreach_slot(vecs->mpvec, mpp, i) { > - if ((update_mpp_prio(mpp) || > - (mpp->need_reload && mpp->synced_count > 0)) && > - reload_and_sync_map(mpp, vecs) == 2) > + sync_mpp(vecs, mpp, ticks); > + if ((update_mpp_prio(mpp) || mpp->need_reload) && > + reload_and_sync_map(mpp, vecs) == 2) { > /* multipath device deleted */ > i--; > + continue; > + } > } > } > lock_cleanup_pop(vecs->lock); > -- > 2.47.1
diff --git a/libmultipath/structs.h b/libmultipath/structs.h index 6a30c59..9d22bdd 100644 --- a/libmultipath/structs.h +++ b/libmultipath/structs.h @@ -471,7 +471,7 @@ struct multipath { int ghost_delay_tick; int queue_mode; unsigned int sync_tick; - int synced_count; + int checker_count; enum prio_update_type prio_update; uid_t uid; gid_t gid; diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c index 7a4e3eb..6aa744d 100644 --- a/libmultipath/structs_vec.c +++ b/libmultipath/structs_vec.c @@ -530,7 +530,6 @@ update_multipath_table (struct multipath *mpp, vector pathvec, int flags) conf = get_multipath_config(); mpp->sync_tick = conf->max_checkint; put_multipath_config(conf); - mpp->synced_count++; r = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_MPATH_ONLY, (mapid_t) { .str = mpp->alias }, diff --git a/multipathd/main.c b/multipathd/main.c index 4a28fbb..e4e6bf7 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -2470,7 +2470,7 @@ sync_mpp(struct vectors * vecs, struct multipath *mpp, unsigned int ticks) if (mpp->sync_tick) mpp->sync_tick -= (mpp->sync_tick > ticks) ? ticks : mpp->sync_tick; - if (mpp->sync_tick) + if (mpp->sync_tick && !mpp->checker_count) return; do_sync_mpp(vecs, mpp); @@ -2513,12 +2513,6 @@ update_path_state (struct vectors * vecs, struct path * pp) return handle_path_wwid_change(pp, vecs)? CHECK_PATH_REMOVED : CHECK_PATH_SKIPPED; } - if (pp->mpp->synced_count == 0) { - do_sync_mpp(vecs, pp->mpp); - /* if update_multipath_strings orphaned the path, quit early */ - if (!pp->mpp) - return CHECK_PATH_SKIPPED; - } if ((newstate != PATH_UP && newstate != PATH_GHOST && newstate != PATH_PENDING) && (pp->state == PATH_DELAYED)) { /* If path state become failed again cancel path delay state */ @@ -2918,9 +2912,11 @@ check_paths(struct vectors *vecs, unsigned int ticks) vector_foreach_slot(vecs->pathvec, pp, i) { if (pp->is_checked != CHECK_PATH_UNCHECKED) continue; - if (pp->mpp) + if (pp->mpp) { pp->is_checked = check_path(pp, ticks); - else + if (pp->is_checked == CHECK_PATH_STARTED) + pp->mpp->checker_count++; + } else pp->is_checked = check_uninitialized_path(pp, ticks); if (pp->is_checked == CHECK_PATH_STARTED && checker_need_wait(&pp->checker)) @@ -3014,12 +3010,10 @@ checkerloop (void *ap) pthread_cleanup_push(cleanup_lock, &vecs->lock); lock(&vecs->lock); pthread_testcancel(); - vector_foreach_slot(vecs->mpvec, mpp, i) - mpp->synced_count = 0; if (checker_state == CHECKER_STARTING) { vector_foreach_slot(vecs->mpvec, mpp, i) { - sync_mpp(vecs, mpp, ticks); mpp->prio_update = PRIO_UPDATE_NONE; + mpp->checker_count = 0; } vector_foreach_slot(vecs->pathvec, pp, i) pp->is_checked = CHECK_PATH_UNCHECKED; @@ -3032,11 +3026,13 @@ checkerloop (void *ap) start_time.tv_sec); if (checker_state == CHECKER_FINISHED) { vector_foreach_slot(vecs->mpvec, mpp, i) { - if ((update_mpp_prio(mpp) || - (mpp->need_reload && mpp->synced_count > 0)) && - reload_and_sync_map(mpp, vecs) == 2) + sync_mpp(vecs, mpp, ticks); + if ((update_mpp_prio(mpp) || mpp->need_reload) && + reload_and_sync_map(mpp, vecs) == 2) { /* multipath device deleted */ i--; + continue; + } } } lock_cleanup_pop(vecs->lock);
Rather than calling sync_mpp early in the checkerloop and tracking map synchronization with synced_count, call sync_mpp() in the CHECKER_FINISHED state, if either at least one path of the map has been checked in the current iteration, or the sync tick has expired. This avoids potentially deleting paths from the pathvec through the do_sync_mpp() -> update_multipath_strings() -> sync_paths -> check_removed_paths() call chain while we're iterating over the pathvec. Also, the time gap between obtaining path states and syncing the state with the kernel is smaller this way. Suggested-by: Benjamin Marzinski <bmarzins@redhat.com> Signed-off-by: Martin Wilck <mwilck@suse.com> --- libmultipath/structs.h | 2 +- libmultipath/structs_vec.c | 1 - multipathd/main.c | 26 +++++++++++--------------- 3 files changed, 12 insertions(+), 17 deletions(-)