On Mon, Jul 15, 2024 at 06:32:22PM +0200, Martin Wilck wrote:
> 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.
>
Sure.
> 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)
