[dm-devel] [PATCH v3 19/19] libmultipath: Fixup updating paths

[Martin Wilck]

On Fri, 2018-09-21 at 18:05 -0500, Benjamin Marzinski wrote:
> Commit 582c56cc broke some code paths in uev_update_path. First, it
> changed the handling of paths that were not fully initialized.
> uev_update_path was simply setting the wwids for all of these paths.
> Instead, it should ignore the ones that had not requested a new
> uevent.
> These paths are likely down, and are already getting handled by
> check_path, after it verifies that they have become active. Also,
> setting the wwid doesn't update all of the other information that
> may have been missed when the path was initially added.
> 
> Also, it wasn't possible for pp->wwid_changed to transition back to
> zero, unless the path's wwid was empty, in which case there was no
> reason to worry about the wwid change in the first place, since the
> path
> hadn't been fully initialized yet. So, even if a path's wwid changed
> and
> then changed back to the original value, the path still could not be
> used.
> 
> This patch fixes these issues, and also moves the check for paths
> that
> have requested a new uevent up in the functions. These paths will get
> fully reinitialized anyway, so there is no reason to do all the other
> work first.
> 
> Fixes: 582c56cc ("libmultipath: uev_update_path: always warn if WWID
> changed")
> 
> Signed-off-by: Benjamin Marzinski <bmarzins at redhat.com>

Thanks for fixing this. Looking at the cleaned-up logic after this
patch, I believe that we should hard-code "disable_changed_wwids" to 1.
It's crazy to realize that a path WWID has changed and simply go on as
if nothing had happened. We shouldn't allow this configuration any
more.

Reviewed-by: Martin Wilck <mwilck at suse.com>

> ---
>  multipathd/main.c | 24 ++++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 61ca455..d6d122a 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -1207,6 +1207,15 @@ uev_update_path (struct uevent *uev, struct
> vectors * vecs)
>  		struct multipath *mpp = pp->mpp;
>  		char wwid[WWID_SIZE];
>  
> +		if (pp->initialized == INIT_REQUESTED_UDEV) {
> +			needs_reinit = 1;
> +			goto out;
> +		}
> +		/* Don't deal with other types of failed initialization
> +		 * now. check_path will handle it */
> +		if (!strlen(pp->wwid))
> +			goto out;
> +
>  		strcpy(wwid, pp->wwid);
>  		get_uid(pp, pp->state, uev->udev);
>  
> @@ -1215,9 +1224,8 @@ uev_update_path (struct uevent *uev, struct
> vectors * vecs)
>  				uev->kernel, wwid, pp->wwid,
>  				(disable_changed_wwids ? "disallowing"
> :
>  				 "continuing"));
> -			if (disable_changed_wwids &&
> -			    (strlen(wwid) || pp->wwid_changed)) {
> -				strcpy(pp->wwid, wwid);
> +			strcpy(pp->wwid, wwid);
> +			if (disable_changed_wwids) {
>  				if (!pp->wwid_changed) {
>  					pp->wwid_changed = 1;
>  					pp->tick = 1;
> @@ -1225,11 +1233,9 @@ uev_update_path (struct uevent *uev, struct
> vectors * vecs)
>  						dm_fail_path(pp->mpp-
> >alias, pp->dev_t);
>  				}
>  				goto out;
> -			} else if (!disable_changed_wwids)
> -				strcpy(pp->wwid, wwid);
> -			else
> -				pp->wwid_changed = 0;
> +			}
>  		} else {
> +			pp->wwid_changed = 0;
>  			udev_device_unref(pp->udev);
>  			pp->udev = udev_device_ref(uev->udev);
>  			conf = get_multipath_config();
> @@ -1240,9 +1246,7 @@ uev_update_path (struct uevent *uev, struct
> vectors * vecs)
>  			pthread_cleanup_pop(1);
>  		}
>  
> -		if (pp->initialized == INIT_REQUESTED_UDEV)
> -			needs_reinit = 1;
> -		else if (mpp && ro >= 0) {
> +		if (mpp && ro >= 0) {
>  			condlog(2, "%s: update path write_protect to
> '%d' (uevent)", uev->kernel, ro);
>  
>  			if (mpp->wait_for_udev)

-- 
Dr. Martin Wilck <mwilck at suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)