[dm-devel] [PATCH 5/7] multipathd: fully initialize paths added by update_pathvec_from_dm
Martin Wilck
martin.wilck at suse.com
Thu Nov 4 22:10:18 UTC 2021
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?
> 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 at redhat.com>
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 */
More information about the dm-devel
mailing list