On Thu, Jul 09, 2020 at 12:51:34PM +0200, mwi...@suse.com wrote:
> From: Martin Wilck <mwi...@suse.com>
>
> With the introduction of INIT_REMOVED, we have to deal with the situation
> when a path is re-added in this state. This enables us to detect the
> situation where a path is added while still part of a map after a failed
> removal, which we couldn't before. Dealing with this correctly requires
> some additional logic. There's a good case (re-added path is still mapped
> by a map with matching WWID) and a bad case (non-matching WWID).
>
> The logic is very similar in uev_add_path() and cli_add_path().
>
> Signed-off-by: Martin Wilck <mwi...@suse.com>
> ---
> multipathd/cli_handlers.c | 49 +++++++++++++++++++++++++++++++++--
> multipathd/main.c | 54 ++++++++++++++++++++++++++++++++++++---
> 2 files changed, 97 insertions(+), 6 deletions(-)
>
> diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> index 782bb00..679fd57 100644
> --- a/multipathd/cli_handlers.c
> +++ b/multipathd/cli_handlers.c
> @@ -713,11 +713,56 @@ cli_add_path (void * v, char ** reply, int * len, void
> * data)
> goto blacklisted;
>
> pp = find_path_by_dev(vecs->pathvec, param);
> - if (pp) {
> + if (pp && pp->initialized != INIT_REMOVED) {
> condlog(2, "%s: path already in pathvec", param);
> if (pp->mpp)
> return 0;
> - } else {
> + } else if (pp) {
> + /* Trying to add a path in INIT_REMOVED state */
> + struct multipath *prev_mpp;
> +
> + prev_mpp = pp->mpp;
O.k. I might be missing something here. How is it possible for a
path to be in the INIT_REMOVED state, and not have pp->mpp set?
Is this just a safety check? Is so, should we print a message,
since this is an unexpected state.
> + pp->mpp = NULL;
> + pp->initialized = INIT_NEW;
> + pp->wwid[0] = '\0';
> + conf = get_multipath_config();
> + pthread_cleanup_push(put_multipath_config, conf);
> + r = pathinfo(pp, conf, DI_ALL | DI_BLACKLIST);
> + pthread_cleanup_pop(1);
> +
> + if (prev_mpp) {
> + /* Similar logic as in uev_add_path() */
> + pp->mpp = prev_mpp;
> + if (r == PATHINFO_OK &&
> + !strncmp(prev_mpp->wwid, pp->wwid, WWID_SIZE)) {
> + condlog(2, "%s: path re-added to %s", pp->dev,
> + pp->mpp->alias);
Should multipath be reinstating the path with dm here? I realize that it
will get resinstated when it's checked next. At the very least,
multipathd should be making sure it get checked the next time it's
examined in checkerloop.
> + return 0;
> + } else if (!ev_remove_path(pp, vecs, true))
> + /* Path removed in ev_remove_path() */
> + pp = NULL;
> + else {
> + /* Init state is now INIT_REMOVED again */
> + pp->dmstate = PSTATE_FAILED;
> + dm_fail_path(pp->mpp->alias, pp->dev_t);
> + condlog(1, "%s: failed to re-add path still
> mapped in %s",
> + pp->dev, pp->mpp->alias);
> + return 1;
> + }
> + } else {
> + switch (r) {
> + case PATHINFO_SKIPPED:
> + goto blacklisted;
> + case PATHINFO_OK:
> + break;
> + default:
> + condlog(0, "%s: failed to get pathinfo", param);
> + return 1;
> + }
> + }
> + }
> +
> + if (!pp) {
> struct udev_device *udevice;
>
> udevice = udev_device_new_from_subsystem_sysname(udev,
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 545eb6d..7b2d320 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -842,9 +842,21 @@ uev_add_path (struct uevent *uev, struct vectors * vecs,
> int need_do_map)
> pp = find_path_by_dev(vecs->pathvec, uev->kernel);
> if (pp) {
> int r;
> + struct multipath *prev_mpp = NULL;
> + bool was_removed = pp->initialized == INIT_REMOVED;
was_removed is only used once, so it's not necessary.
> +
> + if (was_removed) {
> + condlog(3, "%s: re-adding removed path", pp->dev);
> + pp->initialized = INIT_NEW;
> + prev_mpp = pp->mpp;
> + pp->mpp = NULL;
> + /* make sure get_uid() is called */
> + pp->wwid[0] = '\0';
> + } else
> + condlog(3,
> + "%s: spurious uevent, path already in pathvec",
> + uev->kernel);
>
> - condlog(3, "%s: spurious uevent, path already in pathvec",
> - uev->kernel);
> if (!pp->mpp && !strlen(pp->wwid)) {
> condlog(3, "%s: reinitialize path", uev->kernel);
> udev_device_unref(pp->udev);
> @@ -854,9 +866,43 @@ uev_add_path (struct uevent *uev, struct vectors * vecs,
> int need_do_map)
> r = pathinfo(pp, conf,
> DI_ALL | DI_BLACKLIST);
> pthread_cleanup_pop(1);
> - if (r == PATHINFO_OK)
> + if (r == PATHINFO_OK && !prev_mpp)
> ret = ev_add_path(pp, vecs, need_do_map);
> - else if (r == PATHINFO_SKIPPED) {
> + else if (r == PATHINFO_OK &&
again, should we reinstate this path?
> + !strncmp(pp->wwid, prev_mpp->wwid, WWID_SIZE))
> {
> + /*
> + * Path was unsuccessfully removed, but now
> + * re-added, and still belongs to the right map
> + * - all fine.
> + */
> + pp->mpp = prev_mpp;
> + ret = 0;
> + } else if (prev_mpp) {
> + /*
> + * Bad: re-added path still hangs in wrong map
> + * Make another attempt to remove the path
> + */
> + pp->mpp = prev_mpp;
> + ret = ev_remove_path(pp, vecs, true);
> + if (r == PATHINFO_OK && !ret)
> + /*
> + * Path successfully freed, move on to
> + * "new path" code path below
> + */
> + pp = NULL;
> + else {
> + /*
> + * Failure in ev_remove_path will keep
> + * path in pathvec in INIT_REMOVED state
> + * Fail the path to make sure it isn't
> + * used any more.
> + */
> + pp->dmstate = PSTATE_FAILED;
> + dm_fail_path(pp->mpp->alias, pp->dev_t);
> + condlog(1, "%s: failed to re-add path
> still mapped in %s",
> + pp->dev, pp->mpp->alias);
> + }
> + } else if (r == PATHINFO_SKIPPED) {
> condlog(3, "%s: remove blacklisted path",
> uev->kernel);
> i = find_slot(vecs->pathvec, (void *)pp);
> --
> 2.26.2
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
