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

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

[Brian Bunker]

A multipath map can stay read-only after write protect is cleared on
the storage, even though all of its paths are read-write. It happens
when the map's paths are removed and re-added while write protect is
set:

1. Write protect is set. The map is reloaded read-only.
   uev_update_path() sets force_readonly for that reload and clears it
   afterwards.

2. All of the map's paths are removed, and new paths are added while
   write protect is still set. ev_add_path() sees a read-only path and
   sets force_readonly = 1. Nothing ever clears it.

3. Write protect is cleared on the new paths, one after another. Each
   path's DISK_RO=0 uevent is ignored while any path of the map is
   still read-only.

4. The last path's uevent reloads the map. The reload sees the stale
   force_readonly = 1 and loads the map read-only, logging "reloaded
   (retval 0)".

force_readonly only works in one direction: ev_add_path() can set it
but never clears it. If the new paths had been added already writable,
ev_add_path() wouldn't have set the flag and the map wouldn't have
stuck. The paths have to be added while write protected and then
cleared in place.

After step 4 nothing reloads the map again. A rescan doesn't change the
paths' ro state, so it generates no DISK_RO uevent. And since
multipathd now sees the map as read-only, needs_ro_update() also
ignores the next DISK_RO=1 uevent for it.

If two paths' uevents happen to be handled after all paths are
writable, the map does recover: uev_update_path() clears
force_readonly after its reload, so the second reload is read-write.
That makes the problem look intermittent.

Example
-------

We hit this on RHEL 9 device-mapper-multipath 0.8.7, whose
ev_add_path() and uev_update_path() handle force_readonly the same way
as current master. The host's iSCSI sessions were torn down and
re-established while the array was write protecting the volume. From
the messages log, trimmed:

  05:48:27  ...114d8: reload [0 51200 multipath 0 1 alua 1 1
            service-time 0 6 1 135:272 1 66:624 1 69:720 1 129:560 1
            132:656 1 135:752 1]
  05:48:27  sdsd: map ...114d8 reloaded (retval 0)
  05:51:04  kernel: sd 24:0:0:48: [sdaer] Write Protect is on
  05:51:05  ...114d8: remaining active paths: 0
  05:51:07  kernel: sd 29:0:0:48: [sdaoi] Write Protect is on
            (and four more new paths)
  06:09:01  kernel: sd 24:0:0:48: [sdaer] Write Protect is off
  06:09:07  kernel: sd 29:0:0:48: [sdaoi] Write Protect is off
  06:09:10  sdaoi: update path write_protect to '0' (uevent)
  06:09:10  ...114d8: reload [0 51200 multipath 0 1 alua 1 1
            service-time 0 6 1 67:880 1 70:992 1 130:848 1 133:960 1
            65:1072 1 67:1056 1]
  06:09:10  sdaoi: map ...114d8 reloaded (retval 0)

The state afterwards, as recorded by the test:

  dm-106: ro
  sdaer: rw  sdagu: rw  sdaix: rw  sdala: rw  sdand: rw  sdaoi: rw

The patch
---------

The patch adds map_has_ro_path(), which checks whether any of a map's
paths is read-only in sysfs, and uses it wherever force_readonly is
set. ev_add_path() now computes the flag from all of the map's paths
and clears it once the map is loaded, as uev_update_path() already
does after its reload. uev_update_path() and update_map() recompute it
before reloading. With the patch, the 06:09:10 reload would have found
no read-only path and loaded the map read-write.

Open points for review:

- uev_update_path() now takes the read-only state from sysfs rather
  than from the uevent's DISK_RO value. That relies on set_disk_ro()
  updating the disk's read-only state before sending the uevent, which
  current kernels do. Older kernels (e.g. v4.18) sent the uevent first.

- force_readonly is a one-shot hint that several callers have to set
  and clear correctly. A follow-up could remove it and have
  dm_addmap_create() and dm_addmap_reload() call map_has_ro_path()
  themselves. That would move sysfs_get_ro() into libmultipath and
  change the multipath command's behavior too, so it isn't part of
  this fix.

Brian Bunker (1):
  multipathd: don't leave maps read-only after write protect is cleared

 multipathd/main.c | 44 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 37 insertions(+), 7 deletions(-)

-- 
2.56.0

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

[Brian Bunker]

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;
+
+	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);
 
 	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;
 	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.
+				 */
+				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

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

[Benjamin Marzinski]

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