On Sat, 2024-07-13 at 02:04 -0400, Benjamin Marzinski wrote:
> In ev_remove_path() it was possible to exit after calling setup_map()
> without resyncing the mpp state with the kernel. This meant that the
> mpp state in multipathd might not match with the kernel state at all.
>
> It's safe to exit before calling setup_map() if either wait_for_udev
> or need_do_map is set. In both cases, setup_map() will later be
> called,
> either by a uevent or by the calling function.
>
> Once setup_map() has been called, setup_multipath() and
> sync_map_state()
> are now always called, to make sure the mpp matches the kernel state.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@xxxxxxxxxx>
> ---
> multipathd/main.c | 40 +++++++++++++++++++---------------------
> 1 file changed, 19 insertions(+), 21 deletions(-)
>
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 179fec24..3c84c2a0 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -1395,6 +1395,8 @@ ev_remove_path (struct path *pp, struct vectors
> * vecs, int need_do_map)
> * avoid referring to the map of an orphaned path
> */
> if ((mpp = pp->mpp)) {
> + char devt[BLK_DEV_SIZE];
> +
> /*
> * Mark the path as removed. In case of success, we
> * will delete it for good. Otherwise, it will be
> deleted
> @@ -1428,12 +1430,6 @@ ev_remove_path (struct path *pp, struct
> vectors * vecs, int need_do_map)
> flush_map_nopaths(mpp, vecs))
> goto out;
>
> - if (setup_map(mpp, ¶ms, vecs)) {
> - condlog(0, "%s: failed to setup map for"
> - " removal of path %s", mpp->alias,
> pp->dev);
> - goto fail;
> - }
> -
> if (mpp->wait_for_udev) {
> mpp->wait_for_udev = 2;
> retval = REMOVE_PATH_DELAY;
> @@ -1444,33 +1440,35 @@ ev_remove_path (struct path *pp, struct
> vectors * vecs, int need_do_map)
> retval = REMOVE_PATH_DELAY;
> goto out;
> }
> +
> + if (setup_map(mpp, ¶ms, vecs)) {
> + condlog(0, "%s: failed to setup map for"
> + " removal of path %s", mpp->alias,
> pp->dev);
> + goto fail;
> + }
> /*
> * reload the map
> */
> mpp->action = ACT_RELOAD;
> + strlcpy(devt, pp->dev_t, sizeof(devt));
Move this statement further down, directly before the call to
setup_multipath(), perhaps? It's only for the condlog anyway.
Except for that, LGTM.
Martin
> if (domap(mpp, params, 1) == DOMAP_FAIL) {
> condlog(0, "%s: failed in domap for "
> "removal of path %s",
> mpp->alias, pp->dev);
> retval = REMOVE_PATH_FAILURE;
> - } else {
> - /*
> - * update our state from kernel
> - */
> - char devt[BLK_DEV_SIZE];
> -
> - strlcpy(devt, pp->dev_t, sizeof(devt));
> -
> - /* setup_multipath will free the path
> - * regardless of whether it succeeds or
> - * fails */
> - if (setup_multipath(vecs, mpp))
> - return REMOVE_PATH_MAP_ERROR;
> - sync_map_state(mpp);
> + }
> + /*
> + * update mpp state from kernel even if domap
> failed.
> + * If the path was removed from the mpp,
> setup_multipath will
> + * free the path regardless of whether it succeeds
> or fails
> + */
> + if (setup_multipath(vecs, mpp))
> + return REMOVE_PATH_MAP_ERROR;
> + sync_map_state(mpp);
>
> + if (retval == REMOVE_PATH_SUCCESS)
> condlog(2, "%s: path removed from map %s",
> devt, mpp->alias);
> - }
> } else {
> /* mpp == NULL */
> if ((i = find_slot(vecs->pathvec, (void *)pp)) != -
> 1)
