| Message ID | 1537571127-10143-20-git-send-email-bmarzins@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | Misc Multipath patches | expand |
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@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@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)
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@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. I'm fine with defaulting disable_changed_wwids to 1. -Ben > > Reviewed-by: Martin Wilck <mwilck@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@suse.com>, 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@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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)
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@redhat.com> --- multipathd/main.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-)