Re: [PATCH 17/22] multipathd: handle changed wwid when adding partial path

[Benjamin Marzinski <bmarzins@redhat.com>]

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