Re: [PATCH 13/22] multipathd: resync map after setup_map in ev_remove_path

[Benjamin Marzinski <bmarzins@redhat.com>]

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@redhat.com>
> > ---
> >  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, &params, 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, &params, 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)