| Message ID | 20240713060506.2015463-18-bmarzins@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | path checker refactor and misc fixes | expand |
On Sat, 2024-07-13 at 02:05 -0400, Benjamin Marzinski wrote: > If multipathd noticed that the WWID didn't match the device when > adding > a partial path, but removing it failed, multipathd wasn't disabling > the > path. Instead of calling handle_path_wwid_change(), which doesn't > make > much sense when multipathd is expecting a uevent, just manually > disable > the path if ev_remove_path() fails. > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > --- > multipathd/cli_handlers.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c > index 0643be15..5d8ba3a6 100644 > --- a/multipathd/cli_handlers.c > +++ b/multipathd/cli_handlers.c > @@ -540,7 +540,11 @@ add_partial_path(struct path *pp, struct vectors > *vecs) > if (strlen(wwid) && strncmp(wwid, pp->wwid, WWID_SIZE) != 0) > { > condlog(0, "%s: path wwid changed from '%s' to '%s'. > removing", > pp->dev, wwid, pp->wwid); > - ev_remove_path(pp, vecs, 1); > + if (!(ev_remove_path(pp, vecs, 1) & > REMOVE_PATH_SUCCESS) && > + pp->mpp) { > + pp->dmstate = PSTATE_FAILED; > + dm_fail_path(pp->mpp->alias, pp->dev_t); > + } > udev_device_unref(udd); > return -1; > } Is this sufficient? Can we be certain that this path won't be reinstated by sync_map_state() later on? That's at least not immediately obvious. Perhaps we should orphan the path? Martin
On Mon, Jul 15, 2024 at 09:40:36PM +0200, Martin Wilck wrote: > On Sat, 2024-07-13 at 02:05 -0400, Benjamin Marzinski wrote: > > If multipathd noticed that the WWID didn't match the device when > > adding > > a partial path, but removing it failed, multipathd wasn't disabling > > the > > path. Instead of calling handle_path_wwid_change(), which doesn't > > make > > much sense when multipathd is expecting a uevent, just manually > > disable > > the path if ev_remove_path() fails. > > > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > > --- > > multipathd/cli_handlers.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c > > index 0643be15..5d8ba3a6 100644 > > --- a/multipathd/cli_handlers.c > > +++ b/multipathd/cli_handlers.c > > @@ -540,7 +540,11 @@ add_partial_path(struct path *pp, struct vectors > > *vecs) > > if (strlen(wwid) && strncmp(wwid, pp->wwid, WWID_SIZE) != 0) > > { > > condlog(0, "%s: path wwid changed from '%s' to '%s'. > > removing", > > pp->dev, wwid, pp->wwid); > > - ev_remove_path(pp, vecs, 1); > > + if (!(ev_remove_path(pp, vecs, 1) & > > REMOVE_PATH_SUCCESS) && > > + pp->mpp) { > > + pp->dmstate = PSTATE_FAILED; > > + dm_fail_path(pp->mpp->alias, pp->dev_t); > > + } > > udev_device_unref(udd); > > return -1; > > } > > > Is this sufficient? Can we be certain that this path won't be > reinstated by sync_map_state() later on? That's at least not > immediately obvious. Perhaps we should orphan the path? We can't orphan the path completely. That's why ev_remove_path() calls set_path_removed(), which keeps track of the fact it's still in the mpp, so that we can complete the removal once it is actually removed from the multipath device. Otherwise multipathd could just add it to another device, while it was still part of the old device. But you are correct about sync_map_state(). Good catch. We usually never need to worry about sync_map_state() restoring paths in INIT_REMOVED, since they really have been removed. In this case we do. We should probably be setting pp->state to PATH_UNCHECKED in orphan_path() anyway, which will keep sync_map_state() from restoring it. Since the path is in INIT_REMOVED, check_path will never update it's state. But sync_map_state() should also be ignoring paths in INIT_REMOVED. I vote we do both. -Ben > > Martin
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c index 0643be15..5d8ba3a6 100644 --- a/multipathd/cli_handlers.c +++ b/multipathd/cli_handlers.c @@ -540,7 +540,11 @@ add_partial_path(struct path *pp, struct vectors *vecs) if (strlen(wwid) && strncmp(wwid, pp->wwid, WWID_SIZE) != 0) { condlog(0, "%s: path wwid changed from '%s' to '%s'. removing", pp->dev, wwid, pp->wwid); - ev_remove_path(pp, vecs, 1); + if (!(ev_remove_path(pp, vecs, 1) & REMOVE_PATH_SUCCESS) && + pp->mpp) { + pp->dmstate = PSTATE_FAILED; + dm_fail_path(pp->mpp->alias, pp->dev_t); + } udev_device_unref(udd); return -1; }
If multipathd noticed that the WWID didn't match the device when adding a partial path, but removing it failed, multipathd wasn't disabling the path. Instead of calling handle_path_wwid_change(), which doesn't make much sense when multipathd is expecting a uevent, just manually disable the path if ev_remove_path() fails. Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> --- multipathd/cli_handlers.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)