Re: [PATCH v5 1/1] multipathd: don't leave maps read-only after write protect is cleared

[Benjamin Marzinski <bmarzins@redhat.com>]

On Tue, Oct 27, 2026 at 05:24:37PM -0700, Brian Bunker wrote:
> ev_add_path() sets mpp->force_readonly when a newly added path is
> read-only, but never clears it. If a map is created or reloaded while
> its storage is write protected, the flag stays set. When write protect
> is cleared, the reload that uev_update_path() does for the DISK_RO=0
> uevent uses the stale flag and loads the map read-only, even though all
> of its paths are read-write. uev_update_path() clears the flag after
> that reload, so the map only recovers if another DISK_RO=0 uevent
> reloads it again. If none comes, the map stays read-only, and since
> mpp->dmi.read_only is then 1, needs_ro_update() also ignores later
> DISK_RO=1 uevents for it.
> 
> Fix this by setting force_readonly before each load and clearing it
> once the load has succeeded:
> 
> - uev_update_path() takes the path's ro state from sysfs instead of
>   from the uevent's DISK_RO. The uevent only says that something
>   changed. The kernel updates the ro attribute before it sends the
>   uevent, so sysfs is current when the uevent is handled, while the
>   uevent itself may have been queued behind later changes. If sysfs
>   can't be read, nothing is done.
> 
> - needs_ro_update() now sets force_readonly whenever it returns true:
>   to 1 when the path is read-only, and to 0 when all of the map's paths
>   are writable. uev_update_path() no longer sets or clears it itself.
>   When needs_ro_update() returns false, it clears the flag, unless the
>   map is read-write while one of its paths is still read-only. In that
>   case an earlier read-only reload failed, and the flag stays set so
>   that the next reload tries again. If the map has no dm info,
>   needs_ro_update() now fetches it, and returns false if that fails,
>   instead of assuming a reload is needed.
> 
> - ev_add_path() sets it from all of the map's paths rather than only
>   the path being added, and only when it is about to load the map
>   (need_do_map). mpp->paths is up to date there after adopt_paths(),
>   while mpp->pg doesn't contain the new paths yet.
> 
> - update_map(), which does reloads that were delayed while the map was
>   waiting for udev (UDEV_WAIT_RELOAD), recomputes it from the map's
>   paths before setting up the map.
> 
> - dm_addmap_create() clears it as soon as it has read it. A failed
>   create leaves no map behind, and the next create computes the flag
>   again.
> 
> - dm_addmap_reload() clears it once the reload and resume have
>   succeeded. If the reload fails, the map keeps its old table, and the
>   flag stays set so that the next reload tries again.
> 

I feel really bad after all this discussion, but there are still issues
with needs_ro_update(). But hopefully my suggestion will make this all
much simpler.

> Signed-off-by: Brian Bunker <brian@purestorage.com>
> ---
>  libmultipath/devmapper.c |  18 ++++-
>  multipathd/main.c        | 144 ++++++++++++++++++++++++++-------------
>  2 files changed, 111 insertions(+), 51 deletions(-)
> 
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index e3870f59..34e0f640 100644
> --- a/libmultipath/devmapper.c
> +++ b/libmultipath/devmapper.c
> @@ -539,10 +539,15 @@ static uint16_t build_udev_flags(const struct multipath *mpp, int reload)
>  
>  int dm_addmap_create (struct multipath *mpp, char *params)
>  {
> -	int ro;
> +	int ro = mpp->force_readonly ? 1 : 0;
>  	uint16_t udev_flags = build_udev_flags(mpp, 0);
>  
> -	for (ro = mpp->force_readonly ? 1 : 0; ro <= 1; ro++) {
> +	/*
> +	 * force_readonly only applies to this load. Clear it, so that it
> +	 * can't affect later loads, even if this one fails.
> +	 */
> +	mpp->force_readonly = 0;
> +	for (; ro <= 1; ro++) {
>  		int err;
>  
>  		if (dm_addmap(DM_DEVICE_CREATE, TGT_MPATH, mpp, params, ro,
> @@ -599,8 +604,15 @@ int dm_addmap_reload(struct multipath *mpp, char *params, int flush)
>  		r = dm_simplecmd(DM_DEVICE_RESUME, mpp->alias,
>  				 DMFL_NEED_SYNC | (flush ? 0 : DMFL_NO_FLUSH),
>  				 udev_flags);
> -	if (r)
> +	if (r) {
> +		/*
> +		 * force_readonly only applies to this reload. If the reload
> +		 * fails, the map keeps its old table, and the flag stays set
> +		 * so that the next reload tries again.
> +		 */
> +		mpp->force_readonly = 0;
>  		return r;
> +	}
>  
>  	/* If the resume failed, dm will leave the device suspended, and
>  	 * drop the new table, so doing a second resume will try using
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 9bce4d6b..1100584f 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -709,6 +709,55 @@ void cleanup_reset_vec(struct vector_s **v)
>  	vector_reset(*v);
>  }
>  
> +static int
> +sysfs_get_ro (struct path *pp)
> +{
> +	int ro;
> +	char buff[3]; /* Either "0\n\0" or "1\n\0" */
> +
> +	if (!pp->udev)
> +		return -1;
> +
> +	if (!sysfs_attr_get_value_ok(pp->udev, "ro", buff, sizeof(buff))) {
> +		condlog(3, "%s: Cannot read ro attribute in sysfs", pp->dev);
> +		return -1;
> +	}
> +
> +	if (sscanf(buff, "%d\n", &ro) != 1 || ro < 0 || ro > 1) {
> +		condlog(3, "%s: Cannot parse ro attribute", pp->dev);
> +		return -1;
> +	}
> +
> +	return ro;
> +}
> +
> +/* A map must be loaded read-only if any of its paths is read-only. */
> +static bool
> +has_ro_path(vector paths)
> +{
> +	struct path *pp;
> +	unsigned int i;
> +
> +	vector_foreach_slot (paths, pp, i) {
> +		if (sysfs_get_ro(pp) == 1)
> +			return true;
> +	}
> +	return false;
> +}
> +
> +static bool
> +map_has_ro_path(struct multipath *mpp)
> +{
> +	struct pathgroup *pgp;
> +	unsigned int i;
> +
> +	vector_foreach_slot (mpp->pg, pgp, i) {
> +		if (has_ro_path(pgp->paths))
> +			return true;
> +	}
> +	return false;
> +}
> +
>  static int
>  update_map (struct multipath *mpp, struct vectors *vecs, int new_map)
>  {
> @@ -732,6 +781,14 @@ retry:
>  
>  	mpp->action = ACT_RELOAD;
>  
> +	/*
> +	 * This may be a delayed reload, and the paths' write protect
> +	 * state may have changed since force_readonly was last set.
> +	 * Recompute it from the current paths. mpp->paths is up to date
> +	 * after adopt_paths(), while mpp->pg isn't until setup_map().
> +	 */
> +	mpp->force_readonly = has_ro_path(mpp->paths);
> +
>  	if (setup_map(mpp, &params, vecs)) {
>  		condlog(0, "%s: failed to setup new map in update", mpp->alias);
>  		retries = -1;
> @@ -1270,28 +1327,6 @@ out:
>  	return ret;
>  }
>  
> -static int
> -sysfs_get_ro (struct path *pp)
> -{
> -	int ro;
> -	char buff[3]; /* Either "0\n\0" or "1\n\0" */
> -
> -	if (!pp->udev)
> -		return -1;
> -
> -	if (!sysfs_attr_get_value_ok(pp->udev, "ro", buff, sizeof(buff))) {
> -		condlog(3, "%s: Cannot read ro attribute in sysfs", pp->dev);
> -		return -1;
> -	}
> -
> -	if (sscanf(buff, "%d\n", &ro) != 1 || ro < 0 || ro > 1) {
> -		condlog(3, "%s: Cannot parse ro attribute", pp->dev);
> -		return -1;
> -	}
> -
> -	return ro;
> -}
> -
>  /*
>   * returns:
>   * 0: added
> @@ -1305,7 +1340,6 @@ ev_add_path (struct path * pp, struct vectors * vecs, int need_do_map)
>  	int retries = 3;
>  	int start_waiter = 0;
>  	int ret;
> -	int ro;
>  	unsigned char prflag = PR_UNSET;
>  
>  	/*
> @@ -1370,14 +1404,16 @@ rescan:
>  			goto fail; /* leave path added to pathvec */
>  	}
>  
> -	/* ro check - if new path is ro, force map to be ro as well */
> -	ro = sysfs_get_ro(pp);
> -	if (ro == 1)
> -		mpp->force_readonly = 1;
> -
>  	if (!need_do_map)
>  		return 0;
>  
> +	/*
> +	 * Load the map read-only if any of its paths is read-only, not
> +	 * just the one being added. mpp->paths is up to date after
> +	 * adopt_paths(), while mpp->pg doesn't have the new paths yet.
> +	 */
> +	mpp->force_readonly = has_ro_path(mpp->paths);
> +
>  	if (!dm_map_present(mpp->alias)) {
>  		mpp->action = ACT_CREATE;
>  		start_waiter = 1;
> @@ -1596,24 +1632,34 @@ finish_path_init(struct path *pp, struct vectors * vecs)
>  static bool
>  needs_ro_update(struct multipath *mpp, int ro)
>  {
> -	struct pathgroup * pgp;
> -	struct path * pp;
> -	unsigned int i, j;
> -
>  	if (!mpp || ro < 0)
>  		return false;
> -	if (!has_dm_info(mpp))
> -		return true;
> -	if (mpp->dmi.read_only == ro)
> +	if (!has_dm_info(mpp) &&
> +	    dm_get_info(mpp->alias, &mpp->dmi) != DMP_OK) {
> +		condlog(2, "%s: failed to get dm info", mpp->alias);
>  		return false;
> -	if (ro == 1)
> -		return true;
> -	vector_foreach_slot (mpp->pg, pgp, i) {
> -		vector_foreach_slot (pgp->paths, pp, j) {
> -			if (sysfs_get_ro(pp) == 1)
> -				return false;
> -		}
>  	}
> +	if (mpp->dmi.read_only == ro) {
> +		/*
> +		 * No reload is needed. A failed read-only reload leaves
> +		 * force_readonly set while the map is still read-write. Keep
> +		 * it only while one of the map's paths is still read-only,
> +		 * so that the next reload tries again.
> +		 */
> +		if (mpp->force_readonly && (ro == 1 || !map_has_ro_path(mpp)))

This clears force_readonly if either the updated path is read-only or no
paths are read-only. That doesn't make sense, and it doesn't match the
comment.

But there's a bigger issue where this doesn't make sense. if ro == 0 and
map_has_ro_path(mpp) == 1, that means the map must be read-write (since
it matches the path in this block), but we do have a read-only path. In
that case, we should actually be setting mpp->force_readonly and
returning true.

I think we're making this too complicated here. Reading sysfs isn't that
expensive. Perhaps its better to just bite the bullet and stop trying so
hard to avoid reading all the paths ro states in needs_ro_update(). The
idea is that we skip the sysfs_get_ro() (or the old uevent_get_disk_ro)
call in uev_update_path(). Instead we just call needs_ro_update(), which
no longer takes a ro value, and looks something like

static bool
needs_ro_update(struct multipath *mpp)
{
	if (!mpp)
		return false;
	if (!has_dm_info(mpp) &&
	    dm_get_info(mpp->alias, &mpp->dmi) != DMP_OK) {
		condlog(2, "%s: failed to get dm info", mpp->alias);
		return false;
	}
	mpp->force_readonly = map_has_ro_path(mpp);

	/*
	 * The map is read-only and has to stay so. dm has to reopen its
	 * paths for any read-write reload anyway, so the flag isn't needed
	 */
	if (mpp->force_readonly && mpp->dmi.read_only) {
		mpp->force_readonly = 0;
		return false;
	}
	/* The current map state doesn't match the paths state. Update it */
	return (mpp->force_readonly != mpp->dmi.read_only);
}

Does this look reasonable?

-Ben

> +			mpp->force_readonly = 0;
> +		return false;
> +	}
> +	if (ro == 0 && map_has_ro_path(mpp)) {
> +		/*
> +		 * The map is read-only and has to stay so. dm has to reopen
> +		 * its paths for any read-write reload anyway, so the flag
> +		 * isn't needed.
> +		 */
> +		mpp->force_readonly = 0;
> +		return false;
> +	}
> +	mpp->force_readonly = ro;
>  	return true;
>  }
>  
> @@ -1725,22 +1771,24 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
>  			pthread_cleanup_pop(1);
>  		}
>  
> -		ro = uevent_get_disk_ro(uev);
> +		/*
> +		 * The uevent only tells us that something changed. Take the
> +		 * path's ro state from sysfs, which the kernel updates before
> +		 * sending the uevent. If sysfs can't be read, don't act on
> +		 * the uevent's DISK_RO either.
> +		 */
> +		ro = sysfs_get_ro(pp);
>  		if (needs_ro_update(mpp, ro)) {
>  			condlog(2, "%s: update path write_protect to '%d' (uevent)", uev->kernel, ro);
>  
>  			if (mpp->wait_for_udev != UDEV_WAIT_DONE)
>  				mpp->wait_for_udev = UDEV_WAIT_RELOAD;
>  			else {
> -				if (ro == 1)
> -					pp->mpp->force_readonly = 1;
>  				retval = reload_and_sync_map(mpp, vecs);
>  				if (retval == 2)
>  					condlog(2, "%s: map removed during reload", pp->dev);
> -				else {
> -					pp->mpp->force_readonly = 0;
> +				else
>  					condlog(2, "%s: map %s reloaded (retval %d)", uev->kernel, mpp->alias, retval);
> -				}
>  			}
>  		}
>  		if (auto_resize != AUTO_RESIZE_NEVER && mpp &&
> -- 
> 2.56.0