[dm-devel] [PATCH 7/9] multipathd: ignore failed wwid recheck

[Martin Wilck]

On Fri, 2019-02-22 at 10:58 -0600, Benjamin Marzinski wrote:
> If disable_changed_wwids is set, when multipathd gets a change event
> on
> a path, it verifies that the wwid hasn't changed in
> uev_update_path().
> If get_uid() failed, uev_update_path treated this as a wwid change to
> 0.
> This could cause paths to suddenly be dropped due to an issue with
> getting the wwid.  Even if get_uid() failed because the path was
> down,
> it no change uevent happend when it later became active, multipathd
> would continue to ignore the path.
> 
> Instead, multipathd should neither set nor clear wwid_changed if
> get_uid() returned failure.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins at redhat.com>
> ---
>  multipathd/main.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)

Hm. Failure of get_uid() is a rather severe problem. With this change,
if get_uid() fails, pp->wwid won't be reset to the original value, and
dm_fail_path() won't be called. So the path will continue to be
considered "good", but it will have a zero WWID from this point on. Or
am I overlooking someting?

Maybe we should rather change the treatment of this case in
check_path(). In uev_update_path(), we could leave the WWID at 0 and
fail the path. Then in check_path(), when the path is up to be checked
next time, we'd detect the strlen(pp->wwid) == 0 case (indicating
previously failed get_uid()) and try to call get_uid() again to see if
this was just a temporary WWID lookup failure or if the WWID actually
changed (IOW: pp->wwid differs from pp->mpp->wwid). Would this make
sense?

Martin


> diff --git a/multipathd/main.c b/multipathd/main.c
> index 678ecf8..81ad6c0 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -1234,9 +1234,9 @@ uev_update_path (struct uevent *uev, struct
> vectors * vecs)
>  			goto out;
>  
>  		strcpy(wwid, pp->wwid);
> -		get_uid(pp, pp->state, uev->udev);
> +		rc = get_uid(pp, pp->state, uev->udev);
>  
> -		if (strncmp(wwid, pp->wwid, WWID_SIZE) != 0) {
> +		if (rc == 0 && strncmp(wwid, pp->wwid, WWID_SIZE) != 0)
> {
>  			condlog(0, "%s: path wwid changed from '%s' to
> '%s'. %s",
>  				uev->kernel, wwid, pp->wwid,
>  				(disable_changed_wwids ? "disallowing"
> :
> @@ -1252,7 +1252,8 @@ uev_update_path (struct uevent *uev, struct
> vectors * vecs)
>  				goto out;
>  			}
>  		} else {
> -			pp->wwid_changed = 0;
> +			if (rc == 0)
> +				pp->wwid_changed = 0;
>  			udev_device_unref(pp->udev);
>  			pp->udev = udev_device_ref(uev->udev);
>  			conf = get_multipath_config();