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@xxxxxxxxxx>
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@xxxxxxxx>
> ---
> 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@xxxxxxxx>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
