[dm-devel] [PATCH 20/57] multipathd: Do not update the paths vec when removing paths

[Benjamin Marzinski]

On Wed, Apr 27, 2016 at 01:10:21PM +0200, Hannes Reinecke wrote:
> When we remove a path it's totally pointless to add it to
> the path list first; it'll be removed on the next step anyway.
> And we should be cleaning up the comments while we're at it.

This one causes problems. The easiest way to see that is to run
something like

# multipathd reload map <map> ; multipathd del path <path-in-that-map>

This really messes things up. The reason is that at the start of
ev_remove_path, there is no guarantee that any of the paths will be in
mpp->paths.  This is because when multipath runs the pgpolicyfn in
setup_map(), all of policy functions free mpp->paths once they have set
up the path groups.  I assume that this was done so that there is no
chance that the list of paths in mpp->paths will get out of sync with
the list of paths in the pathgroups.

I can see why it someone might want to only keep mpp->pg as the
definitive list of paths, and to use update_mpp_paths() to regenerate
mpp->paths when necessary. But that's not what multipathd does. Instead,
mpp->paths is almost always regenerated by calling setup_multipath()
later in the same function that called setup_map(). However not every
function will always do this.  ev_remove_path doesn't do this if domap()
fails, and reload_map() never calls setup_multipath(). coalesce_paths()
doesn't call setup_multipath() itself, but some if it's callers do. Even
if mpp->paths isn't restored right away, it will be when check_path
calls update_multipath_strings().

So, if you call "cli reload map" and then call "cli del path" before the
checker function restores mpp->paths, and multipath doesn't call
update_mpp_pths() in ev_remove_path, you get into problems.

The question is, what's the right thing to do?

Option 1 is to never delete mpp->paths in the first place. Then we can
probably do away with some more of the update_mpp_paths() calls. We just
need to make certain that whenever we update mpp->pg, we are always
either getting the paths from mpp->paths, or we call update_mpp_paths()
afterwards to sync them.

Option 2 is to say that we will alway regenerate mpp->paths whenever we
need it. In that case, we should probably be freeing it after we're done
with it.

I don't really care either way, as long as we're consistent. Otherwise
we'll get into bizzare situations like the one above.

-Ben

> 
> Signed-off-by: Hannes Reinecke <hare at suse.de>
> ---
>  multipathd/main.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 59f0c68..61b82f6 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -695,14 +695,8 @@ ev_remove_path (struct path *pp, struct vectors * vecs)
>  	 */
>  	if ((mpp = pp->mpp)) {
>  		/*
> -		 * transform the mp->pg vector of vectors of paths
> -		 * into a mp->params string to feed the device-mapper
> +		 * Remove path from paths list
>  		 */
> -		if (update_mpp_paths(mpp, vecs->pathvec)) {
> -			condlog(0, "%s: failed to update paths",
> -				mpp->alias);
> -			goto fail;
> -		}
>  		if ((i = find_slot(mpp->paths, (void *)pp)) != -1)
>  			vector_del_slot(mpp->paths, i);
>  
> @@ -735,6 +729,10 @@ ev_remove_path (struct path *pp, struct vectors * vecs)
>  			 */
>  		}
>  
> +		/*
> +		 * transform the mp->pg vector of vectors of paths
> +		 * into a mp->params string to feed the device-mapper
> +		 */
>  		if (setup_map(mpp, params, PARAMS_SIZE)) {
>  			condlog(0, "%s: failed to setup map for"
>  				" removal of path %s", mpp->alias, pp->dev);
> @@ -1016,6 +1014,7 @@ uxlsnrloop (void * ap)
>  
>  	umask(077);
>  	uxsock_listen(&uxsock_trigger, ap);
> +	condlog(1, "terminate uxsock listener");
>  
>  	return NULL;
>  }
> -- 
> 2.6.6