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

[Benjamin Marzinski <bmarzins@redhat.com>]

On Tue, Oct 20, 2026 at 01:48:39PM -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.
> 
> Add a helper, map_has_ro_path(), that checks whether any of a map's
> paths is read-only in sysfs, and use it to set force_readonly:
> 
> - In ev_add_path(), set force_readonly from all of the map's paths
>   rather than only the path being added, and clear it once the map has
>   been loaded, as uev_update_path() already does after its reload. With
>   need_do_map == 0 the flag is left set for the call that loads the map.
> 
> - In uev_update_path(), set force_readonly from the paths' current
>   state in sysfs instead of from the uevent. The kernel updates the ro
>   attribute before it sends the DISK_RO uevent, so sysfs is current
>   when the uevent is handled.
> 
> - In update_map(), which does reloads that were delayed while the map
>   was waiting for udev (UDEV_WAIT_RELOAD), recompute force_readonly
>   before setting up the map, since the paths' state may have changed
>   since it was last set.
> 
> Signed-off-by: Brian Bunker <brian@purestorage.com>
> ---
>  multipathd/main.c | 44 +++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 37 insertions(+), 7 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 9bce4d6b..68916256 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -709,6 +709,22 @@ void cleanup_reset_vec(struct vector_s **v)
>  	vector_reset(*v);
>  }
>  
> +static int sysfs_get_ro(struct path *pp);
> +
> +/* A map must be loaded read-only if any of its paths is read-only. */
> +static bool
> +map_has_ro_path(struct multipath *mpp)
> +{
> +	struct path *pp;
> +	unsigned int i;
> +

In multipathd, you can't trust mpp->paths to be up to date unless you
know that you've called update_mpp_paths() after acquiring the vecs
lock. I don't believe that's the case for uev_update_path(). Instead
this should use:

        vector_foreach_slot (mpp->pg, pgp, i) {
                vector_foreach_slot (pgp->paths, pp, j) {


> +	vector_foreach_slot (mpp->paths, pp, i) {
> +		if (sysfs_get_ro(pp) == 1)
> +			return true;
> +	}
> +	return false;
> +}
> +
>  static int
>  update_map (struct multipath *mpp, struct vectors *vecs, int new_map)
>  {
> @@ -732,6 +748,13 @@ 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->force_readonly = map_has_ro_path(mpp);
> +
>  	if (setup_map(mpp, &params, vecs)) {
>  		condlog(0, "%s: failed to setup new map in update", mpp->alias);
>  		retries = -1;
> @@ -1305,7 +1328,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,10 +1392,11 @@ 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;
> +	/*
> +	 * Load the map read-only if any of its paths is read-only,
> +	 * not just the one being added.
> +	 */
> +	mpp->force_readonly = map_has_ro_path(mpp);
>  

need_do_map is false when we are adding multiple paths at once, and only
want to reload once at the end (where we set it to true). Since we will
check all the paths ro state before doing the reload when need_do_map is
true, it seems safe to not check them all when need_do_map is false.
It's just extra work.


>  	if (!need_do_map)
>  		return 0;
> @@ -1400,6 +1423,8 @@ rescan:
>  		sleep(1);
>  		ret = domap(mpp, params, 1);
>  	}
> +	/* force_readonly only applies to this load, as in uev_update_path() */
> +	mpp->force_readonly = 0;

I think it may be better to remove this code from here and
uev_update_path(), and instead clear it in dm_addmap() on success, like
we do for mpp->nead_reload.

>  	if (ret == DOMAP_FAIL || ret == DOMAP_RETRY) {
>  		condlog(0, "%s: failed in domap for addition of new "
>  			"path %s", mpp->alias, pp->dev);
> @@ -1732,8 +1757,13 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
>  			if (mpp->wait_for_udev != UDEV_WAIT_DONE)
>  				mpp->wait_for_udev = UDEV_WAIT_RELOAD;
>  			else {
> -				if (ro == 1)
> -					pp->mpp->force_readonly = 1;
> +				/*
> +				 * force_readonly may be stale from ev_add_path().
> +				 * Set it from the paths' current state in sysfs,
> +				 * which the kernel updates before sending the
> +				 * uevent, rather than from the uevent itself.
> +				 */

Instead of calling this here, it might be better to set mpp->force_readonly
in needs_ro_update(), when it returns true. There are three cases in
that function which return true:

for "if (!has_dm_info(mpp))", it would have to call map_has_ro_path() to set
mpp->force_readonly correctly.

For "if (ro == 1)", it should unconditionally set mpp->force_readonly

for the final return, it could either leave it unset and trust that it
wasn't already set, or it could unconditionally clear it to be extra safe.

-Ben

> +				pp->mpp->force_readonly = map_has_ro_path(mpp);
>  				retval = reload_and_sync_map(mpp, vecs);
>  				if (retval == 2)
>  					condlog(2, "%s: map removed during reload", pp->dev);
> -- 
> 2.56.0