Re: [PATCH 03/21] multipathd: free paths in checker_finished()

[Benjamin Marzinski <bmarzins@redhat.com>]

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 <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  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