On Tue, Oct 02, 2018 at 12:30:51AM +0200, Martin Wilck wrote:
> 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.
I'm fine with defaulting disable_changed_wwids to 1.
-Ben
>
> 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
