Re: [PATCH 5/7] multipathd: fully initialize paths added by update_pathvec_from_dm

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@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





[Index of Archives]     [DM Crypt]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Packaging]     [Fedora SELinux]     [Yosemite Discussion]     [KDE Users]     [Fedora Docs]

  Powered by Linux