On Thu, Nov 04, 2021 at 10:10:18PM +0000, Martin Wilck wrote:
> On Wed, 2021-10-20 at 14:15 -0500, Benjamin Marzinski wrote:
> > When paths are added by update_pathvec_from_dm(), udev may not have
> > initialized them. This means that it's possible that they are
> > supposed
> > to be blacklisted by udev properties, but weren't. Also, in order to
> > avoid doing potentially stalling IO, update_pathvec_from_dm() doesn't
> > get all the path information in pathinfo(). These paths end up in the
> > unexpected state of INIT_MISSING_UDEV or INIT_NEW, but with their mpp
> > and wwid set.
> >
> > If udev has already initialized the path, but multipathed wasn't
> > monitoring it, the blacklist checks and wwid determination in
> > update_pathvec_from_dm() will work correctly, so paths added by it
> > are
> > safe, but not completely initialized. The most likely reason why this
> > would happen is if the path was manually removed from multipathd
> > monitoring with "multipathd del path".
>
> Not quite getting this - normally the path would be removed from maps,
> too. Are you referring to the REMOVE_PATH_DELAY case?
Yeah, but if the user then runs "multipath", it will add that no longer
tracked path back to the multipath device. Like I said in 0/7, I'm not
sure that how much we need to proatively try to initalize these paths,
or if we can just ignore them on the grounds that sometimes when people
shoot themselves in the foot, it hurts.
-Ben
>
>
> > The most common time when
> > uninitialized paths would in be part of multipath devices is during
> > boot, after the pivot root, but before the udev coldplug happens.
> > These
> > paths are not necessarily safe. It's possible that
> > /etc/multipath.conf
> > in the initramfs and regular filesystem differ, and they should now
> > be
> > either blacklisted by udev_property, or have a different wwid.
> > However
> > an "add" event should appear for them shortly.
> >
> > Multipath now has a new state to deal with these devices,
> > INIT_PARTIAL.
> > Devices in this state are treated mostly like INIT_OK devices, but
> > when "multipathd add path" is called or an add/change uevent happens
> > on these devices, multipathd will finish initializing them, and
> > remove
> > them if necessary.
> >
> > Signed-off-by: Benjamin Marzinski <bmarzins@xxxxxxxxxx>
>
> Nice. Somehow in my mind the issue always look much more complex.
> I like this, but I want to give it some testing before I finally ack
> it.
>
> Regards
> Martin
>
>
>
>
> > ---
> > libmultipath/structs.h | 6 +++++
> > libmultipath/structs_vec.c | 5 ++--
> > multipathd/cli_handlers.c | 4 ++++
> > multipathd/main.c | 48 ++++++++++++++++++++++++++++++++++--
> > --
> > multipathd/main.h | 1 +
> > 5 files changed, 58 insertions(+), 6 deletions(-)
> >
> > diff --git a/libmultipath/structs.h b/libmultipath/structs.h
> > index d0b266b7..69409fd4 100644
> > --- a/libmultipath/structs.h
> > +++ b/libmultipath/structs.h
> > @@ -200,6 +200,12 @@ enum initialized_states {
> > * mapped by some multipath map because of map reload
> > failure.
> > */
> > INIT_REMOVED,
> > + /*
> > + * INIT_PARTIAL: paths added by update_pathvec_from_dm() will
> > not
> > + * be fully initialized. This will be handled when an add or
> > + * change uevent is received.
> > + */
> > + INIT_PARTIAL,
> > };
> >
> > enum prkey_sources {
> > diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
> > index fb26437a..1de9175e 100644
> > --- a/libmultipath/structs_vec.c
> > +++ b/libmultipath/structs_vec.c
> > @@ -194,6 +194,7 @@ bool update_pathvec_from_dm(vector pathvec,
> > struct multipath *mpp,
> > }
> > condlog(2, "%s: adding new
> > path %s",
> > mpp->alias, pp->dev);
> > + pp->initialized =
> > INIT_PARTIAL;
> > store_path(pathvec, pp);
> > pp->tick = 1;
> > }
> > @@ -392,12 +393,12 @@ extract_hwe_from_path(struct multipath * mpp)
> > condlog(4, "%s: searching paths for valid hwe", mpp->alias);
> > /* doing this in two passes seems like paranoia to me */
> > vector_foreach_slot(mpp->paths, pp, i) {
> > - if (pp->state == PATH_UP &&
> > + if (pp->state == PATH_UP && pp->initialized !=
> > INIT_PARTIAL &&
> > pp->initialized != INIT_REMOVED && pp->hwe)
> > goto done;
> > }
> > vector_foreach_slot(mpp->paths, pp, i) {
> > - if (pp->state != PATH_UP &&
> > + if ((pp->state != PATH_UP || pp->initialized ==
> > INIT_PARTIAL) &&
> > pp->initialized != INIT_REMOVED && pp->hwe)
> > goto done;
> > }
> > diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> > index 58b9916c..8d37431e 100644
> > --- a/multipathd/cli_handlers.c
> > +++ b/multipathd/cli_handlers.c
> > @@ -526,6 +526,10 @@ cli_add_path (void *v, struct strbuf *reply,
> > void *data)
> > return 1;
> > }
> >
> > + if (pp->initialized == INIT_PARTIAL &&
> > + finish_path_init(pp, vecs) < 0)
> > + return 1;
> > +
> > if (pp->mpp)
> > return 0;
> > } else if (pp) {
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index cc4a4a5d..4b8e2cde 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -976,12 +976,19 @@ check_path_wwid_change(struct path *pp)
> > return false;
> > }
> >
> > +/*
> > + * uev_add_path can call uev_update_path, and uev_update_path can
> > call
> > + * uev_add_path
> > + */
> > +static int uev_update_path (struct uevent *uev, struct vectors *
> > vecs);
> > +
> > static int
> > uev_add_path (struct uevent *uev, struct vectors * vecs, int
> > need_do_map)
> > {
> > struct path *pp;
> > int ret = 0, i;
> > struct config *conf;
> > + bool partial_init = false;
> >
> > condlog(3, "%s: add path (uevent)", uev->kernel);
> > if (strstr(uev->kernel, "..") != NULL) {
> > @@ -1000,7 +1007,10 @@ uev_add_path (struct uevent *uev, struct
> > vectors * vecs, int need_do_map)
> > int r;
> > struct multipath *prev_mpp = NULL;
> >
> > - if (pp->initialized == INIT_REMOVED) {
> > + if (pp->initialized == INIT_PARTIAL) {
> > + partial_init = true;
> > + goto out;
> > + } else if (pp->initialized == INIT_REMOVED) {
> > condlog(3, "%s: re-adding removed path", pp-
> > >dev);
> > pp->initialized = INIT_NEW;
> > prev_mpp = pp->mpp;
> > @@ -1110,6 +1120,8 @@ uev_add_path (struct uevent *uev, struct
> > vectors * vecs, int need_do_map)
> > }
> > out:
> > lock_cleanup_pop(vecs->lock);
> > + if (partial_init)
> > + return uev_update_path(uev, vecs);
> > return ret;
> > }
> >
> > @@ -1405,6 +1417,25 @@ fail:
> > return REMOVE_PATH_MAP_ERROR;
> > }
> >
> > +int
> > +finish_path_init(struct path *pp, struct vectors * vecs)
> > +{
> > + int r;
> > + struct config *conf;
> > +
> > + conf = get_multipath_config();
> > + pthread_cleanup_push(put_multipath_config, conf);
> > + r = pathinfo(pp, conf, DI_ALL|DI_BLACKLIST);
> > + pthread_cleanup_pop(1);
> > +
> > + if (r == PATHINFO_OK)
> > + return 0;
> > +
> > + condlog(0, "%s: error fully initializing path, removing", pp-
> > >dev);
> > + ev_remove_path(pp, vecs, 1);
> > + return -1;
> > +}
> > +
> > static int
> > uev_update_path (struct uevent *uev, struct vectors * vecs)
> > {
> > @@ -1443,7 +1474,7 @@ uev_update_path (struct uevent *uev, struct
> > vectors * vecs)
> > }
> > /* Don't deal with other types of failed
> > initialization
> > * now. check_path will handle it */
> > - if (!strlen(pp->wwid))
> > + if (!strlen(pp->wwid) && pp->initialized !=
> > INIT_PARTIAL)
> > goto out;
> >
> > strcpy(wwid, pp->wwid);
> > @@ -1451,12 +1482,20 @@ uev_update_path (struct uevent *uev, struct
> > vectors * vecs)
> >
> > if (rc != 0)
> > strcpy(pp->wwid, wwid);
> > - else if (strncmp(wwid, pp->wwid, WWID_SIZE) != 0) {
> > + else if (strlen(wwid) &&
> > + strncmp(wwid, pp->wwid, WWID_SIZE) != 0) {
> > condlog(0, "%s: path wwid changed from '%s'
> > to '%s'",
> > uev->kernel, wwid, pp->wwid);
> > ev_remove_path(pp, vecs, 1);
> > needs_reinit = 1;
> > goto out;
> > + } else if (pp->initialized == INIT_PARTIAL) {
> > + udev_device_unref(pp->udev);
> > + pp->udev = udev_device_ref(uev->udev);
> > + if (finish_path_init(pp, vecs) < 0) {
> > + retval = 1;
> > + goto out;
> > + }
> > } else {
> > udev_device_unref(pp->udev);
> > pp->udev = udev_device_ref(uev->udev);
> > @@ -1507,6 +1546,7 @@ out:
> >
> > condlog(0, "%s: spurious uevent, path not found",
> > uev->kernel);
> > }
> > + /* pp->initalized must not be INIT_PARTIAL if needs_reinit is
> > set */
>
> Did you mean "cannot" here? At least for the "wwid changed" case this
> is subtle, as it's set to INIT_REMOVED in ev_remove_path().
>
> > if (needs_reinit)
> > retval = uev_add_path(uev, vecs, 1);
> > return retval;
> > @@ -2116,7 +2156,7 @@ check_path (struct vectors * vecs, struct path
> > * pp, unsigned int ticks)
> > int marginal_pathgroups, marginal_changed = 0;
> > int ret;
> >
> > - if (((pp->initialized == INIT_OK ||
> > + if (((pp->initialized == INIT_OK || pp->initialized ==
> > INIT_PARTIAL ||
> > pp->initialized == INIT_REQUESTED_UDEV) && !pp->mpp) ||
> > pp->initialized == INIT_REMOVED)
> > return 0;
> > diff --git a/multipathd/main.h b/multipathd/main.h
> > index c8a1ce92..4acd1b8c 100644
> > --- a/multipathd/main.h
> > +++ b/multipathd/main.h
> > @@ -66,4 +66,5 @@ int reload_and_sync_map(struct multipath *mpp,
> > struct vectors *vecs,
> >
> > void handle_path_wwid_change(struct path *pp, struct vectors *vecs);
> > bool check_path_wwid_change(struct path *pp);
> > +int finish_path_init(struct path *pp, struct vectors * vecs);
> > #endif /* MAIN_H */
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
