[dm-devel] [PATCH 63/74] multipathd: deal with INIT_REMOVED during path addition

[Benjamin Marzinski]

On Thu, Jul 09, 2020 at 12:51:34PM +0200, mwilck at suse.com wrote:
> From: Martin Wilck <mwilck at suse.com>
> 
> With the introduction of INIT_REMOVED, we have to deal with the situation
> when a path is re-added in this state. This enables us to detect the
> situation where a path is added while still part of a map after a failed
> removal, which we couldn't before. Dealing with this correctly requires
> some additional logic. There's a good case (re-added path is still mapped
> by a map with matching WWID) and a bad case (non-matching WWID).
> 
> The logic is very similar in uev_add_path() and cli_add_path().
> 
> Signed-off-by: Martin Wilck <mwilck at suse.com>
> ---
>  multipathd/cli_handlers.c | 49 +++++++++++++++++++++++++++++++++--
>  multipathd/main.c         | 54 ++++++++++++++++++++++++++++++++++++---
>  2 files changed, 97 insertions(+), 6 deletions(-)
> 
> diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> index 782bb00..679fd57 100644
> --- a/multipathd/cli_handlers.c
> +++ b/multipathd/cli_handlers.c
> @@ -713,11 +713,56 @@ cli_add_path (void * v, char ** reply, int * len, void * data)
>  		goto blacklisted;
>  
>  	pp = find_path_by_dev(vecs->pathvec, param);
> -	if (pp) {
> +	if (pp && pp->initialized != INIT_REMOVED) {
>  		condlog(2, "%s: path already in pathvec", param);
>  		if (pp->mpp)
>  			return 0;
> -	} else {
> +	} else if (pp) {
> +		/* Trying to add a path in INIT_REMOVED state */
> +		struct multipath *prev_mpp;
> +
> +		prev_mpp = pp->mpp;

O.k. I might be missing something here. How is it possible for a
path to be in the INIT_REMOVED state, and not have pp->mpp set?
Is this just a safety check?  Is so, should we print a message,
since this is an unexpected state.

> +		pp->mpp = NULL;
> +		pp->initialized = INIT_NEW;
> +		pp->wwid[0] = '\0';
> +		conf = get_multipath_config();
> +		pthread_cleanup_push(put_multipath_config, conf);
> +		r = pathinfo(pp, conf, DI_ALL | DI_BLACKLIST);
> +		pthread_cleanup_pop(1);
> +
> +		if (prev_mpp) {
> +			/* Similar logic as in uev_add_path() */
> +			pp->mpp = prev_mpp;
> +			if (r == PATHINFO_OK &&
> +			    !strncmp(prev_mpp->wwid, pp->wwid, WWID_SIZE)) {
> +				condlog(2, "%s: path re-added to %s", pp->dev,
> +					pp->mpp->alias);

Should multipath be reinstating the path with dm here? I realize that it
will get resinstated when it's checked next. At the very least,
multipathd should be making sure it get checked the next time it's
examined in checkerloop.

> +				return 0;
> +			} else if (!ev_remove_path(pp, vecs, true))
> +				/* Path removed in ev_remove_path() */
> +				pp = NULL;
> +			else {
> +				/* Init state is now INIT_REMOVED again */
> +				pp->dmstate = PSTATE_FAILED;
> +				dm_fail_path(pp->mpp->alias, pp->dev_t);
> +				condlog(1, "%s: failed to re-add path still mapped in %s",
> +					pp->dev, pp->mpp->alias);
> +				return 1;
> +			}
> +		} else {
> +			switch (r) {
> +			case PATHINFO_SKIPPED:
> +				goto blacklisted;
> +			case PATHINFO_OK:
> +				break;
> +			default:
> +				condlog(0, "%s: failed to get pathinfo", param);
> +				return 1;
> +			}
> +		}
> +	}
> +
> +	if (!pp) {
>  		struct udev_device *udevice;
>  
>  		udevice = udev_device_new_from_subsystem_sysname(udev,
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 545eb6d..7b2d320 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -842,9 +842,21 @@ uev_add_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
>  	pp = find_path_by_dev(vecs->pathvec, uev->kernel);
>  	if (pp) {
>  		int r;
> +		struct multipath *prev_mpp = NULL;
> +		bool was_removed = pp->initialized == INIT_REMOVED;

was_removed is only used once, so it's not necessary.

> +
> +		if (was_removed) {
> +			condlog(3, "%s: re-adding removed path", pp->dev);
> +			pp->initialized = INIT_NEW;
> +			prev_mpp = pp->mpp;
> +			pp->mpp = NULL;
> +			/* make sure get_uid() is called */
> +			pp->wwid[0] = '\0';
> +		} else
> +			condlog(3,
> +				"%s: spurious uevent, path already in pathvec",
> +				uev->kernel);
>  
> -		condlog(3, "%s: spurious uevent, path already in pathvec",
> -			uev->kernel);
>  		if (!pp->mpp && !strlen(pp->wwid)) {
>  			condlog(3, "%s: reinitialize path", uev->kernel);
>  			udev_device_unref(pp->udev);
> @@ -854,9 +866,43 @@ uev_add_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
>  			r = pathinfo(pp, conf,
>  				     DI_ALL | DI_BLACKLIST);
>  			pthread_cleanup_pop(1);
> -			if (r == PATHINFO_OK)
> +			if (r == PATHINFO_OK && !prev_mpp)
>  				ret = ev_add_path(pp, vecs, need_do_map);
> -			else if (r == PATHINFO_SKIPPED) {
> +			else if (r == PATHINFO_OK &&

again, should we reinstate this path?

> +				 !strncmp(pp->wwid, prev_mpp->wwid, WWID_SIZE)) {
> +				/*
> +				 * Path was unsuccessfully removed, but now
> +				 * re-added, and still belongs to the right map
> +				 * - all fine.
> +				 */
> +				pp->mpp = prev_mpp;
> +				ret = 0;
> +			} else if (prev_mpp) {
> +				/*
> +				 * Bad: re-added path still hangs in wrong map
> +				 * Make another attempt to remove the path
> +				 */
> +				pp->mpp = prev_mpp;
> +				ret = ev_remove_path(pp, vecs, true);
> +				if (r == PATHINFO_OK && !ret)
> +					/*
> +					 * Path successfully freed, move on to
> +					 * "new path" code path below
> +					 */
> +					pp = NULL;
> +				else {
> +					/*
> +					 * Failure in ev_remove_path will keep
> +					 * path in pathvec in INIT_REMOVED state
> +					 * Fail the path to make sure it isn't
> +					 * used any more.
> +					 */
> +					pp->dmstate = PSTATE_FAILED;
> +					dm_fail_path(pp->mpp->alias, pp->dev_t);
> +					condlog(1, "%s: failed to re-add path still mapped in %s",
> +						pp->dev, pp->mpp->alias);
> +				}
> +			} else if (r == PATHINFO_SKIPPED) {
>  				condlog(3, "%s: remove blacklisted path",
>  					uev->kernel);
>  				i = find_slot(vecs->pathvec, (void *)pp);
> -- 
> 2.26.2