On Wed, Dec 17, 2025 at 10:20:55PM +0100, Martin Wilck wrote: > Modifying the pathvec in a deep call stack is unexpected and > error-prone. Free paths in the checkerloop instead, after > having synced all maps.
I'm not fond of the fact that this leaves paths around, when callers expect that the path will be removed. For instance, right now cli_add_path() and uev_add_path() can call ev_remove_path to handle INIT_REMOVED paths, and expect that success means the path is removed. They can end up storing duplicate paths with this code. What do you think about having calls to ev_remove_path() actually remove the path if multipath device successfully orphans it. All the callers of that function already expect that it likely will remove the path, and that means that whenever multipathd is explicitly told to remove a path, it will, if it can. If it can't, then the path was always going to get removed later, and delaying that later incidental remove till the next path check shouldn't make much difference. -Ben > Suggested-by: Benjamin Marzinski <[email protected]> > Signed-off-by: Martin Wilck <[email protected]> > --- > libmultipath/libmultipath.version | 1 + > libmultipath/structs_vec.c | 7 +++---- > libmultipath/structs_vec.h | 1 + > multipathd/main.c | 1 + > 4 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/libmultipath/libmultipath.version > b/libmultipath/libmultipath.version > index 89ae2a3..e5b7b83 100644 > --- a/libmultipath/libmultipath.version > +++ b/libmultipath/libmultipath.version > @@ -58,6 +58,7 @@ global: > change_foreign; > check_alias_settings; > check_daemon; > + check_removed_paths; > checker_clear_message; > checker_disable; > checker_enable; > diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c > index f651b29..3a9ab58 100644 > --- a/libmultipath/structs_vec.c > +++ b/libmultipath/structs_vec.c > @@ -573,16 +573,16 @@ static struct path *find_devt_in_pathgroups(const > struct multipath *mpp, > return NULL; > } > > -static void check_removed_paths(const struct multipath *mpp, vector pathvec) > +void check_removed_paths(vector pathvec) > { > struct path *pp; > int i; > > vector_foreach_slot(pathvec, pp, i) { > - if (pp->mpp == mpp && > + if (pp->mpp && > (pp->initialized == INIT_REMOVED || > pp->initialized == INIT_PARTIAL) && > - !find_devt_in_pathgroups(mpp, pp->dev_t)) { > + !find_devt_in_pathgroups(pp->mpp, pp->dev_t)) { > condlog(2, "%s: %s: freeing path in %s state", > __func__, pp->dev, > pp->initialized == INIT_REMOVED ? > @@ -615,7 +615,6 @@ void sync_paths(struct multipath *mpp, vector pathvec) > orphan_path(pp, "path removed externally"); > } > } > - check_removed_paths(mpp, pathvec); > update_mpp_paths(mpp, pathvec); > vector_foreach_slot (mpp->paths, pp, i) > pp->mpp = mpp; > diff --git a/libmultipath/structs_vec.h b/libmultipath/structs_vec.h > index 1188ada..cb02d75 100644 > --- a/libmultipath/structs_vec.h > +++ b/libmultipath/structs_vec.h > @@ -18,6 +18,7 @@ int adopt_paths (vector pathvec, struct multipath *mpp, > void orphan_path (struct path * pp, const char *reason); > void set_path_removed(struct path *pp); > > +void check_removed_paths(vector pathvec); > int verify_paths(struct multipath *mpp); > int update_mpp_paths(struct multipath * mpp, vector pathvec); > int update_multipath_strings (struct multipath *mpp, vector pathvec); > diff --git a/multipathd/main.c b/multipathd/main.c > index d3bf4d0..49a1f25 100644 > --- a/multipathd/main.c > +++ b/multipathd/main.c > @@ -3127,6 +3127,7 @@ static void checker_finished(struct vectors *vecs, > unsigned int ticks) > } > if (uev_timed_out && !need_to_delay_reconfig(vecs)) > unblock_reconfigure(); > + check_removed_paths(vecs->pathvec); > partial_retrigger_tick(vecs->pathvec); > } > > -- > 2.52.0
