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

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

[Brian Bunker]

This is v2. It reworks how force_readonly is set and cleared,
following Ben's review of v1. The changes are listed at the end.

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 sets force_readonly right before each load from the state of
the map's paths, and dm_addmap() clears it when a load succeeds.
needs_ro_update() sets it for the reload done for a DISK_RO uevent,
and ev_add_path() and update_map() compute it from all of the map's
paths. With the patch, the 06:09:10 reload would have found no
read-only path and loaded the map read-write.

Changes since v1:

- Set force_readonly in needs_ro_update() when it returns true,
  instead of in uev_update_path(). The DISK_RO=1 case uses the uevent
  again, rather than rereading sysfs.

- Clear force_readonly in dm_addmap() on success, as for need_reload,
  instead of after the loads in ev_add_path() and uev_update_path().

- In ev_add_path(), only compute force_readonly when need_do_map is
  set.

- Walk mpp->pg rather than mpp->paths in needs_ro_update(), since
  mpp->paths may be stale there. ev_add_path() and update_map() still
  use mpp->paths: both call adopt_paths() first, and mpp->pg doesn't
  contain the new paths until setup_map().

- Move sysfs_get_ro() above has_ro_path() instead of adding a forward
  declaration. The function is unchanged.

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

 libmultipath/devmapper.c |   4 +-
 multipathd/main.c        | 113 +++++++++++++++++++++++++--------------
 2 files changed, 75 insertions(+), 42 deletions(-)

-- 
2.56.0

[PATCH v2 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.

Fix this by setting force_readonly before each load and clearing it
when a load succeeds:

- needs_ro_update() now sets force_readonly whenever it returns true:
  to 1 for a DISK_RO=1 uevent, to 0 when all of the map's paths are
  writable, and from the paths' state when the map has no dm info.
  uev_update_path() no longer sets or clears it itself.

- 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() clears it after a successful load, as it does for
  need_reload, so it can't outlive the load it was set for.

Signed-off-by: Brian Bunker <brian@purestorage.com>
---
 libmultipath/devmapper.c |   4 +-
 multipathd/main.c        | 113 +++++++++++++++++++++++++--------------
 2 files changed, 75 insertions(+), 42 deletions(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index e3870f59..c2939dca 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -520,8 +520,10 @@ dm_addmap (int task, const char *target, struct multipath *mpp,
 	if (task == DM_DEVICE_CREATE)
 		libmp_udev_wait(cookie);
 
-	if (r)
+	if (r) {
 		mpp->need_reload = false;
+		mpp->force_readonly = 0;
+	}
 	return r;
 }
 
diff --git a/multipathd/main.c b/multipathd/main.c
index 9bce4d6b..605e24ae 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -709,6 +709,42 @@ 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 int
 update_map (struct multipath *mpp, struct vectors *vecs, int new_map)
 {
@@ -732,6 +768,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 +1314,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 +1327,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 +1391,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;
@@ -1597,23 +1620,35 @@ static bool
 needs_ro_update(struct multipath *mpp, int ro)
 {
 	struct pathgroup * pgp;
-	struct path * pp;
-	unsigned int i, j;
+	unsigned int i;
 
 	if (!mpp || ro < 0)
 		return false;
-	if (!has_dm_info(mpp))
+	if (!has_dm_info(mpp)) {
+		mpp->force_readonly = 0;
+		vector_foreach_slot (mpp->pg, pgp, i) {
+			if (has_ro_path(pgp->paths)) {
+				mpp->force_readonly = 1;
+				break;
+			}
+		}
 		return true;
+	}
 	if (mpp->dmi.read_only == ro)
 		return false;
-	if (ro == 1)
+	if (ro == 1) {
+		mpp->force_readonly = 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 (has_ro_path(pgp->paths))
+			return false;
 	}
+	/*
+	 * Clear force_readonly in case it is still set from an earlier
+	 * failed load.
+	 */
+	mpp->force_readonly = 0;
 	return true;
 }
 
@@ -1732,15 +1767,11 @@ 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;
 				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

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

[Benjamin Marzinski]

On Wed, Oct 21, 2026 at 11:27:59AM -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
> when a load succeeds:
> 
> - needs_ro_update() now sets force_readonly whenever it returns true:
>   to 1 for a DISK_RO=1 uevent, to 0 when all of the map's paths are
>   writable, and from the paths' state when the map has no dm info.
>   uev_update_path() no longer sets or clears it itself.
> 
> - 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.

Oops. Good catch.

> 
> - 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() clears it after a successful load, as it does for
>   need_reload, so it can't outlive the load it was set for.
> 
> Signed-off-by: Brian Bunker <brian@purestorage.com>

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

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

[mwilck]

> 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
> when a load succeeds:
> 
> - needs_ro_update() now sets force_readonly whenever it returns true:
>   to 1 for a DISK_RO=1 uevent, to 0 when all of the map's paths are
>   writable, and from the paths' state when the map has no dm info.
>   uev_update_path() no longer sets or clears it itself.
> 
> - 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() clears it after a successful load, as it does for
>   need_reload, so it can't outlive the load it was set for.
> 
> Signed-off-by: Brian Bunker <brian@purestorage.com>
>
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index e3870f5..c2939dc 100644
> --- a/libmultipath/devmapper.c
> +++ b/libmultipath/devmapper.c
> @@ -520,8 +520,10 @@ dm_addmap (int task, const char *target, struct multipath *mpp,
>  	if (task == DM_DEVICE_CREATE)
>  		libmp_udev_wait(cookie);
>  
> -	if (r)
> +	if (r) {
>  		mpp->need_reload = false;
> +		mpp->force_readonly = 0;
> +	}
>  	return r;
>  }
>  
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 9bce4d6..605e24a 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -709,6 +709,42 @@ 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 int
>  update_map (struct multipath *mpp, struct vectors *vecs, int new_map)
>  {
> @@ -732,6 +768,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 +1314,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 +1327,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 +1391,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;
> @@ -1597,23 +1620,35 @@ static bool
>  needs_ro_update(struct multipath *mpp, int ro)
>  {
>  	struct pathgroup * pgp;
> -	struct path * pp;
> -	unsigned int i, j;
> +	unsigned int i;
>  
>  	if (!mpp || ro < 0)
>  		return false;
> -	if (!has_dm_info(mpp))
> +	if (!has_dm_info(mpp)) {

This should never happen. But as we test the condition,
wonder why we don't just call dm_get_info(mpp, &mpp->dmi) to obtain
the missing information?

If that fails, I am not sure it makes sense to return true any more.
It would indicate a severe problem with the map at hand.

> +		mpp->force_readonly = 0;
> +		vector_foreach_slot (mpp->pg, pgp, i) {

if (ro == 1), you can skip this loop, as you already know that there's one
ro path.

> +			if (has_ro_path(pgp->paths)) {
> +				mpp->force_readonly = 1;
> +				break;
> +			}
> +		}
>  		return true;
> +	}
>  	if (mpp->dmi.read_only == ro)
>  		return false;

Should mpp->force_readonly be reset here? After all, there seems to be no
need for reloading. That would be a similar condition as what you fix
by resetting force_readonly below.

Actually, as we don't seem to trust our internal state anyway, we might
simply obtain the sysfs attribute for all paths, always, and compare it
to dmi.read_only. That might be somewhat slow-ish, though.

The alternative would be to improve our internal tracking, e.g. by saving
a "ro" flag for paths when we add them or receive an uevents for them.
That way we could save the effort to check sysfs repeatedly.

Thanks,
Martin

-- 
Martin Wilck <mwilck@suse.com>

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

[Brian Bunker]

This is v3. It follows Martin's review of v2: needs_ro_update() now
fetches missing dm info instead of assuming a reload is needed, and
force_readonly is cleared when a load reads it rather than only when
the load succeeds. The changes are listed at the end.

Ben, I dropped your Reviewed-by because the needs_ro_update() logic
changed. The no-dm-info case reverses the "assume it needs to be
reloaded" behavior from 6e40bf92, so please check that you're OK
with it.

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 sets force_readonly right before each load from the state of
the map's paths, and dm_addmap_create() and dm_addmap_reload() clear it
as soon as they have read it. needs_ro_update() sets it for the reload
done for a DISK_RO uevent, and ev_add_path() and update_map() compute
it from all of the map's paths. With the patch, the 06:09:10 reload
would have found no read-only path and loaded the map read-write.

Changes since v2:

- In needs_ro_update(), fetch the dm info with dm_get_info() if the
  map has none, and return false if that fails, instead of assuming a
  reload is needed. This also removes the separate path walk for that
  case.

- Clear force_readonly in dm_addmap_create() and dm_addmap_reload() as
  soon as they have read it, instead of in dm_addmap() on success. A
  failed load can no longer leave the flag set for a later, unrelated
  reload, such as one for a path group change. needs_ro_update() no
  longer needs to clear the flag itself.

- I didn't add per-path tracking of the ro state. force_readonly is
  only computed from sysfs right before a load, which is rare, so the
  cost of reading sysfs there is small. Per-path tracking could be a
  follow-up if you want it.

Changes since v1:

- Set force_readonly in needs_ro_update() when it returns true,
  instead of in uev_update_path(). The DISK_RO=1 case uses the uevent
  again, rather than rereading sysfs.

- Clear force_readonly in dm_addmap() on success, as for need_reload,
  instead of after the loads in ev_add_path() and uev_update_path().

- In ev_add_path(), only compute force_readonly when need_do_map is
  set.

- Walk mpp->pg rather than mpp->paths in needs_ro_update(), since
  mpp->paths may be stale there. ev_add_path() and update_map() still
  use mpp->paths: both call adopt_paths() first, and mpp->pg doesn't
  contain the new paths until setup_map().

- Move sysfs_get_ro() above has_ro_path() instead of adding a forward
  declaration. The function is unchanged.

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

 libmultipath/devmapper.c |  17 +++++--
 multipathd/main.c        | 104 +++++++++++++++++++++++----------------
 2 files changed, 75 insertions(+), 46 deletions(-)

-- 
2.56.0

[PATCH v3 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.

Fix this by setting force_readonly before each load and clearing it
when the load reads it:

- needs_ro_update() now sets force_readonly whenever it returns true:
  to 1 for a DISK_RO=1 uevent, and to 0 when all of the map's paths
  are writable. uev_update_path() no longer sets or clears it itself.
  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() and dm_addmap_reload() clear it as soon as they
  have read it, so it only applies to the load it was set for, even if
  that load fails. Otherwise a failed load would leave it set, and a
  later reload for an unrelated reason, such as a path group change,
  would load the map read-only.

Signed-off-by: Brian Bunker <brian@purestorage.com>
---
 libmultipath/devmapper.c |  17 +++++--
 multipathd/main.c        | 104 +++++++++++++++++++++++----------------
 2 files changed, 75 insertions(+), 46 deletions(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index e3870f59..c994715e 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,
@@ -578,19 +583,23 @@ int dm_addmap_create (struct multipath *mpp, char *params)
 int dm_addmap_reload(struct multipath *mpp, char *params, int flush)
 {
 	int r = 0;
+	int force_ro = mpp->force_readonly;
 	uint16_t udev_flags = build_udev_flags(mpp, 1);
 
+	/* As in dm_addmap_create(), force_readonly only applies to this load */
+	mpp->force_readonly = 0;
+
 	/*
 	 * DM_DEVICE_RELOAD cannot wait on a cookie, as
 	 * the cookie will only ever be released after an
 	 * DM_DEVICE_RESUME. So call DM_DEVICE_RESUME
 	 * after each successful call to DM_DEVICE_RELOAD.
 	 */
-	if (!mpp->force_readonly)
+	if (!force_ro)
 		r = dm_addmap(DM_DEVICE_RELOAD, TGT_MPATH, mpp, params,
 			      ADDMAP_RW, 0);
 	if (!r) {
-		if (!mpp->force_readonly && errno != EROFS)
+		if (!force_ro && errno != EROFS)
 			return 0;
 		r = dm_addmap(DM_DEVICE_RELOAD, TGT_MPATH, mpp,
 			      params, ADDMAP_RO, 0);
diff --git a/multipathd/main.c b/multipathd/main.c
index 9bce4d6b..c5788ef9 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -709,6 +709,42 @@ 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 int
 update_map (struct multipath *mpp, struct vectors *vecs, int new_map)
 {
@@ -732,6 +768,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 +1314,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 +1327,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 +1391,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;
@@ -1597,23 +1620,24 @@ static bool
 needs_ro_update(struct multipath *mpp, int ro)
 {
 	struct pathgroup * pgp;
-	struct path * pp;
-	unsigned int i, j;
+	unsigned int i;
 
 	if (!mpp || ro < 0)
 		return false;
-	if (!has_dm_info(mpp))
-		return true;
+	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 (mpp->dmi.read_only == ro)
 		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)
+	if (ro == 0) {
+		vector_foreach_slot (mpp->pg, pgp, i) {
+			if (has_ro_path(pgp->paths))
 				return false;
 		}
 	}
+	mpp->force_readonly = ro;
 	return true;
 }
 
@@ -1732,15 +1756,11 @@ 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;
 				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

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

[Martin Wilck]

> 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
> when the load reads it:
> 
> - needs_ro_update() now sets force_readonly whenever it returns true:
>   to 1 for a DISK_RO=1 uevent, and to 0 when all of the map's paths
>   are writable. uev_update_path() no longer sets or clears it itself.
>   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() and dm_addmap_reload() clear it as soon as they
>   have read it, so it only applies to the load it was set for, even if
>   that load fails. Otherwise a failed load would leave it set, and a
>   later reload for an unrelated reason, such as a path group change,
>   would load the map read-only.
> 
> Signed-off-by: Brian Bunker <brian@purestorage.com>

Almost there. One remark below.

>
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index e3870f5..c994715 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,
> @@ -578,19 +583,23 @@ int dm_addmap_create (struct multipath *mpp, char *params)
>  int dm_addmap_reload(struct multipath *mpp, char *params, int flush)
>  {
>  	int r = 0;
> +	int force_ro = mpp->force_readonly;
>  	uint16_t udev_flags = build_udev_flags(mpp, 1);
>  
> +	/* As in dm_addmap_create(), force_readonly only applies to this load */
> +	mpp->force_readonly = 0;
> +

One minor issue here. If the reload below fails, the map is still read-write
in the kernel. As we reset force_readonly, it will remain so, at least until
we get another event with RO=1.

If the RO reload fails with force_readonly=1, we could set force_readonly to 1 again
after the failure. In needs_ro_update(), we'd then need to modify the fast
return clause "if (mpp->dmi.read_only == ro)"; if both are 0 and force_readonly is
still set, you'd need to check the paths and see if there's still a read-only path
around.

Martin

-- 
Martin Wilck <mwilck@suse.com>

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

[Benjamin Marzinski]

On Thu, Oct 22, 2026 at 10:17:39PM +0200, Martin Wilck 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
> > when the load reads it:
> > 
> > - needs_ro_update() now sets force_readonly whenever it returns true:
> >   to 1 for a DISK_RO=1 uevent, and to 0 when all of the map's paths
> >   are writable. uev_update_path() no longer sets or clears it itself.
> >   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() and dm_addmap_reload() clear it as soon as they
> >   have read it, so it only applies to the load it was set for, even if
> >   that load fails. Otherwise a failed load would leave it set, and a
> >   later reload for an unrelated reason, such as a path group change,
> >   would load the map read-only.
> > 
> > Signed-off-by: Brian Bunker <brian@purestorage.com>
> 
> Almost there. One remark below.
> 
> >
> > diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> > index e3870f5..c994715 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,
> > @@ -578,19 +583,23 @@ int dm_addmap_create (struct multipath *mpp, char *params)
> >  int dm_addmap_reload(struct multipath *mpp, char *params, int flush)
> >  {
> >  	int r = 0;
> > +	int force_ro = mpp->force_readonly;
> >  	uint16_t udev_flags = build_udev_flags(mpp, 1);
> >  
> > +	/* As in dm_addmap_create(), force_readonly only applies to this load */
> > +	mpp->force_readonly = 0;
> > +
> 
> One minor issue here. If the reload below fails, the map is still read-write
> in the kernel. As we reset force_readonly, it will remain so, at least until
> we get another event with RO=1.
> 
> If the RO reload fails with force_readonly=1, we could set force_readonly to 1 again
> after the failure. In needs_ro_update(), we'd then need to modify the fast
> return clause "if (mpp->dmi.read_only == ro)"; if both are 0 and force_readonly is
> still set, you'd need to check the paths and see if there's still a read-only path
> around.

I can actually see both sides of this argument. If the reload fails, and
the next reload doesn't check the paths again, we really don't know what
the correct setting is. This may be overthinking things, but we could make
force_readonly a three state value, somthing like:

MP_RELOAD_RO
MP_RELOAD_RW
MP_RELOAD_RECHECK

If mpp->force_readonly was MP_RELOAD_RECHECK needs_ro_update() would
always return true, and dm_addmap_create() and dm_addmap_reload() would
check the paths themselves. Again, because these could be called in
cases where we can't trust mpp->paths to be up to date, They should
probably loop through the mpp->pg. You can make it always correct to use
mpp->pg in has_ro_path() by moving the calls to has_ro_path() to after
setup_map() is called. If that succeeds, that pathgroups have to be set up
correctly.

If we don't do something complicated, it might be better to always unset
mpp->force_readonly like it patch currently does. It's an unlikely
corner case and at worst you would get a readwrite mpath device where
writes failed, because the device really should be read-only. That seems
better that making a read-only device that should be read-write.

Thoughts?

-Ben

> 
> Martin
> 
> -- 
> Martin Wilck <mwilck@suse.com>

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

[Martin Wilck]

On Thu, 2026-10-22 at 20:24 -0400, Benjamin Marzinski wrote:
> On Thu, Oct 22, 2026 at 10:17:39PM +0200, Martin Wilck 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
> > > when the load reads it:
> > > 
> > > - needs_ro_update() now sets force_readonly whenever it returns
> > > true:
> > >   to 1 for a DISK_RO=1 uevent, and to 0 when all of the map's
> > > paths
> > >   are writable. uev_update_path() no longer sets or clears it
> > > itself.
> > >   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() and dm_addmap_reload() clear it as soon as
> > > they
> > >   have read it, so it only applies to the load it was set for,
> > > even if
> > >   that load fails. Otherwise a failed load would leave it set,
> > > and a
> > >   later reload for an unrelated reason, such as a path group
> > > change,
> > >   would load the map read-only.
> > > 
> > > Signed-off-by: Brian Bunker <brian@purestorage.com>
> > 
> > Almost there. One remark below.
> > 
> > > 
> > > diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> > > index e3870f5..c994715 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,
> > > @@ -578,19 +583,23 @@ int dm_addmap_create (struct multipath
> > > *mpp, char *params)
> > >  int dm_addmap_reload(struct multipath *mpp, char *params, int
> > > flush)
> > >  {
> > >  	int r = 0;
> > > +	int force_ro = mpp->force_readonly;
> > >  	uint16_t udev_flags = build_udev_flags(mpp, 1);
> > >  
> > > +	/* As in dm_addmap_create(), force_readonly only applies
> > > to this load */
> > > +	mpp->force_readonly = 0;
> > > +
> > 
> > One minor issue here. If the reload below fails, the map is still
> > read-write
> > in the kernel. As we reset force_readonly, it will remain so, at
> > least until
> > we get another event with RO=1.
> > 
> > If the RO reload fails with force_readonly=1, we could set
> > force_readonly to 1 again
> > after the failure. In needs_ro_update(), we'd then need to modify
> > the fast
> > return clause "if (mpp->dmi.read_only == ro)"; if both are 0 and
> > force_readonly is
> > still set, you'd need to check the paths and see if there's still a
> > read-only path
> > around.
> 
> I can actually see both sides of this argument. If the reload fails,
> and
> the next reload doesn't check the paths again, we really don't know
> what
> the correct setting is.
> 
>  This may be overthinking things, but we could make
> force_readonly a three state value, somthing like:
> 
> MP_RELOAD_RO
> MP_RELOAD_RW
> MP_RELOAD_RECHECK
> 
> If mpp->force_readonly was MP_RELOAD_RECHECK needs_ro_update() would
> always return true, and dm_addmap_create() and dm_addmap_reload()
> would
> check the paths themselves. Again, because these could be called in
> cases where we can't trust mpp->paths to be up to date, They should
> probably loop through the mpp->pg. You can make it always correct to
> use
> mpp->pg in has_ro_path() by moving the calls to has_ro_path() to
> after
> setup_map() is called. If that succeeds, that pathgroups have to be
> set up
> correctly.
> 
> If we don't do something complicated, it might be better to always
> unset
> mpp->force_readonly like it patch currently does. It's an unlikely
> corner case and at worst you would get a readwrite mpath device where
> writes failed, because the device really should be read-only. 

I agree that it isn't a big issue if such writes fail. But if some
paths are still read-write, writes might actually _succeed_ on them,
Data corruption would be possible, as some writes would succeed an
others wouldn't.

Am I overlooking something, and this is actually impossible? I guess
it's highly unlikely because in practice the ro/rw is toggled on the
storage side, and happens per volume, i.e. for all paths
simultaneously.

But if that's true and writes-succeeding-although-they-shouldn't is not
an issue, I wonder why we go through the hassle of setting the read-
only attribute on our maps at all. We could just keep them read-write
and trust that the writes (which shouldn't be attempted on read-only
devices after all) will fail at lower layers.

> That seems
> better that making a read-only device that should be read-write.

If this is consensus, Brian's v3 would actually be preferred to his v4.
It isn't a religious matter to me, I agree that failing to reload read-
only is a highly unlikely thing to happen. But I'd like to clarify the
question above.

I am not so enthusastic about the idea of the 3-valued force_readonly.

If we want to do this right, we need to check on every reload. And IMO
that means that we need to track the read-only state of the paths in
multipathd. It's cheap to retrieve the "ro" attribute in
sysfs_pathinfo() and track it. If it changes, we expect an uevent,
where we check DISK_RO anyway. The logic could be similar as it is now,
but we can check on each reload whether the map should be read-only,
because the check would be just a cheap loop over the paths. And we
wouldn't need force_readonly at all any more.

Regards,
Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>
SUSE Software Solutions Germany GmbH, Frankenstr. 146, 90461 Nürnberg,
Germany
Geschäftsführer: Stefan Gaiser, Jochen Jaser, Abhinav Puri (HRB
36809,AG Nürnberg)

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

[Benjamin Marzinski]

On Fri, Oct 23, 2026 at 10:50:08PM +0200, Martin Wilck wrote:
> On Thu, 2026-10-22 at 20:24 -0400, Benjamin Marzinski wrote:
> > On Thu, Oct 22, 2026 at 10:17:39PM +0200, Martin Wilck 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
> > > > when the load reads it:
> > > > 
> > > > - needs_ro_update() now sets force_readonly whenever it returns
> > > > true:
> > > >   to 1 for a DISK_RO=1 uevent, and to 0 when all of the map's
> > > > paths
> > > >   are writable. uev_update_path() no longer sets or clears it
> > > > itself.
> > > >   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() and dm_addmap_reload() clear it as soon as
> > > > they
> > > >   have read it, so it only applies to the load it was set for,
> > > > even if
> > > >   that load fails. Otherwise a failed load would leave it set,
> > > > and a
> > > >   later reload for an unrelated reason, such as a path group
> > > > change,
> > > >   would load the map read-only.
> > > > 
> > > > Signed-off-by: Brian Bunker <brian@purestorage.com>
> > > 
> > > Almost there. One remark below.
> > > 
> > > > 
> > > > diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> > > > index e3870f5..c994715 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,
> > > > @@ -578,19 +583,23 @@ int dm_addmap_create (struct multipath
> > > > *mpp, char *params)
> > > >  int dm_addmap_reload(struct multipath *mpp, char *params, int
> > > > flush)
> > > >  {
> > > >  	int r = 0;
> > > > +	int force_ro = mpp->force_readonly;
> > > >  	uint16_t udev_flags = build_udev_flags(mpp, 1);
> > > >  
> > > > +	/* As in dm_addmap_create(), force_readonly only applies
> > > > to this load */
> > > > +	mpp->force_readonly = 0;
> > > > +
> > > 
> > > One minor issue here. If the reload below fails, the map is still
> > > read-write
> > > in the kernel. As we reset force_readonly, it will remain so, at
> > > least until
> > > we get another event with RO=1.
> > > 
> > > If the RO reload fails with force_readonly=1, we could set
> > > force_readonly to 1 again
> > > after the failure. In needs_ro_update(), we'd then need to modify
> > > the fast
> > > return clause "if (mpp->dmi.read_only == ro)"; if both are 0 and
> > > force_readonly is
> > > still set, you'd need to check the paths and see if there's still a
> > > read-only path
> > > around.
> > 
> > I can actually see both sides of this argument. If the reload fails,
> > and
> > the next reload doesn't check the paths again, we really don't know
> > what
> > the correct setting is.
> > 
> >  This may be overthinking things, but we could make
> > force_readonly a three state value, somthing like:
> > 
> > MP_RELOAD_RO
> > MP_RELOAD_RW
> > MP_RELOAD_RECHECK
> > 
> > If mpp->force_readonly was MP_RELOAD_RECHECK needs_ro_update() would
> > always return true, and dm_addmap_create() and dm_addmap_reload()
> > would
> > check the paths themselves. Again, because these could be called in
> > cases where we can't trust mpp->paths to be up to date, They should
> > probably loop through the mpp->pg. You can make it always correct to
> > use
> > mpp->pg in has_ro_path() by moving the calls to has_ro_path() to
> > after
> > setup_map() is called. If that succeeds, that pathgroups have to be
> > set up
> > correctly.
> > 
> > If we don't do something complicated, it might be better to always
> > unset
> > mpp->force_readonly like it patch currently does. It's an unlikely
> > corner case and at worst you would get a readwrite mpath device where
> > writes failed, because the device really should be read-only. 
> 
> I agree that it isn't a big issue if such writes fail. But if some
> paths are still read-write, writes might actually _succeed_ on them,
> Data corruption would be possible, as some writes would succeed an
> others wouldn't.
>
> Am I overlooking something, and this is actually impossible? I guess
> it's highly unlikely because in practice the ro/rw is toggled on the
> storage side, and happens per volume, i.e. for all paths
> simultaneously.

Yeah. I don't think there really is such a thing as only one path being
read-only. If you have a read/write dm device, all of the underlying
devices must be opened read/write. The table won't load if they can't be
opened with a matching mode. So the IO will go through to the scsi
device, and I think it will fail on any path.

> But if that's true and writes-succeeding-although-they-shouldn't is not
> an issue, I wonder why we go through the hassle of setting the read-
> only attribute on our maps at all. We could just keep them read-write
> and trust that the writes (which shouldn't be attempted on read-only
> devices after all) will fail at lower layers.

Originally, dm would open the underlying devices anew whenever you
loaded a new table. This caused problems with multipath, because
sometimes failed device aren't openable when you try to reload the
table. It used to cause multipath to be unable to reload the device
table when it included failed paths in certain states.

To fix that, if a device in a new inactive table already exists in the
existing active table, opened in the same mode as the new table wants, 
dm no longer opens the device again. It just shares the opened device
between the two tables.

However this caused a problem with multipath's ability to automatically
switch to read-only when a path switched to read-only. When dm still
opened the devices per table, the read-write load of the table would
fail, because it couldn't open all the path devices read-write. However
now that dm shares the path devices between tables, multipath devices
would never switch to read-only if they were previously loaded
read-write, as long as you weren't adding a new path device in the new
table. All the existing devices were already opened read-write, and the
first time multipath tries to reload the table it tries read-write.
Since it just shared the devices instead of opening them again, this
would succeed, regardless of whether or not the devices really were
read-write.

To solve this, whenever multipath detects that a device has switched to
read-only, it forces a read-only table reload, so that all the path
devices get re-opened read-only (since their requested mode doesn't
match their current mode). After that, the regular reload code will
work, since it will have to re-open all the paths read-write for the
read-write multipath table load-to succeed (again, since the requested
mode changed).

Actually, this makes me realize that there's no reason to care about
force_readonly in dm_addmap_create().

> > That seems
> > better that making a read-only device that should be read-write.

I do still waver a bit on whether or not we could leave force_readonly
set on failed reloads. Once a device turns read-only, it should stay
that way until we get another uevent, at which point we will call
needs_ro_update() again. Perhaps we just need to make sure that we clear
force_readonly when needs_ro_update() returns false. In that case, it
seems like the only way we would do the wrong thing is if we somehow
missed all the uevents for setting the path devices back to read-write.

Thoughts? I'm clearly overthinking this.

-Ben

> If this is consensus, Brian's v3 would actually be preferred to his v4.
> It isn't a religious matter to me, I agree that failing to reload read-
> only is a highly unlikely thing to happen. But I'd like to clarify the
> question above.
> 
> I am not so enthusastic about the idea of the 3-valued force_readonly.
> 
> If we want to do this right, we need to check on every reload. And IMO
> that means that we need to track the read-only state of the paths in
> multipathd. It's cheap to retrieve the "ro" attribute in
> sysfs_pathinfo() and track it. If it changes, we expect an uevent,
> where we check DISK_RO anyway. The logic could be similar as it is now,
> but we can check on each reload whether the map should be read-only,
> because the check would be just a cheap loop over the paths. And we
> wouldn't need force_readonly at all any more.
> 
> Regards,
> Martin
> 
> -- 
> Dr. Martin Wilck <mwilck@suse.com>
> SUSE Software Solutions Germany GmbH, Frankenstr. 146, 90461 Nürnberg,
> Germany
> Geschäftsführer: Stefan Gaiser, Jochen Jaser, Abhinav Puri (HRB
> 36809,AG Nürnberg)

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

[Martin Wilck]

On Fri, 2026-10-23 at 19:40 -0400, Benjamin Marzinski wrote:
> 
> I do still waver a bit on whether or not we could leave
> force_readonly
> set on failed reloads. Once a device turns read-only, it should stay
> that way until we get another uevent, at which point we will call
> needs_ro_update() again. Perhaps we just need to make sure that we
> clear
> force_readonly when needs_ro_update() returns false.

AFAICS, this is exactly what Brian's v4 patch does. No?

Martin


-- 
Dr. Martin Wilck <mwilck@suse.com>
SUSE Software Solutions Germany GmbH, Frankenstr. 146, 90461 Nürnberg,
Germany
Geschäftsführer: Stefan Gaiser, Jochen Jaser, Abhinav Puri (HRB
36809,AG Nürnberg)

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

[Brian Bunker]

On Mon, 2026-10-26 at 10:26 +0100, Martin Wilck wrote:
> On Fri, 2026-10-23 at 19:40 -0400, Benjamin Marzinski wrote:
> >
> > I do still waver a bit on whether or not we could leave
> > force_readonly
> > set on failed reloads. Once a device turns read-only, it should stay
> > that way until we get another uevent, at which point we will call
> > needs_ro_update() again. Perhaps we just need to make sure that we
> > clear
> > force_readonly when needs_ro_update() returns false.
>
> AFAICS, this is exactly what Brian's v4 patch does. No?

Mostly, yes. v4 sets force_readonly back in dm_addmap_reload() when a
reload fails, so the next reload tries read-only again.

needs_ro_update() doesn't clear it on every false return, though. It
clears it in the dmi.read_only == ro case only when both the map and
the uevent are read-write and none of the map's paths is read-only. If
a path is still read-only, the flag stays set, so a map whose read-only
reload failed keeps retrying instead of staying read-write.

About dm_addmap_create(): agreed that the flag isn't needed for
correctness there, since a new map opens all paths fresh and a
read-write create fails with EROFS anyway. It only saves that first
attempt. Create still clears the flag, so that a value set by
ev_add_path() can't carry over to a later reload. I can drop the
read-only shortcut in create if you'd prefer.

Uday Shankar, who did 430f8f89, also suggested dropping force_readonly
altogether, and tracking the ro state per path in sysfs_pathinfo()
sounds like the right direction to me. But it changes behavior: every
reload, not just the one done for a DISK_RO uevent, would follow the
paths' ro state, and so would "multipath -r", since dm_addmap_reload()
is in libmultipath. So I'd rather do it as a separate series on top of
this fix, and keep this one small enough to backport.

Brian

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

[Martin Wilck]

On Mon, 2026-10-26 at 11:04 -0700, Brian Bunker wrote:
> On Mon, 2026-10-26 at 10:26 +0100, Martin Wilck wrote:
> > On Fri, 2026-10-23 at 19:40 -0400, Benjamin Marzinski wrote:
> > > 
> > > I do still waver a bit on whether or not we could leave
> > > force_readonly
> > > set on failed reloads. Once a device turns read-only, it should
> > > stay
> > > that way until we get another uevent, at which point we will call
> > > needs_ro_update() again. Perhaps we just need to make sure that
> > > we
> > > clear
> > > force_readonly when needs_ro_update() returns false.
> > 
> > AFAICS, this is exactly what Brian's v4 patch does. No?
> 
> Mostly, yes. v4 sets force_readonly back in dm_addmap_reload() when a
> reload fails, so the next reload tries read-only again.
> 
> needs_ro_update() doesn't clear it on every false return, though. It
> clears it in the dmi.read_only == ro case only when both the map and
> the uevent are read-write and none of the map's paths is read-only.
> If
> a path is still read-only, the flag stays set, so a map whose read-
> only
> reload failed keeps retrying instead of staying read-write.

Which is correct. If the map is read-write but there are ro paths, we
need to retry.

There are two other cases where needs_ro_update() returns false:

1. ro == 1 && dmi.read_only == 1
   I don't see how force_readonly could be one in this case.
   But maybe we should just reset it anyway, just to be sure.
2. ro == 0 && dmi.read_only == 1 && map_has_ro_path()
   Same here, read_only is already set, no need to force it,
   but resetting force_read_only wouldn't hurt.

> About dm_addmap_create(): agreed that the flag isn't needed for
> correctness there, since a new map opens all paths fresh and a
> read-write create fails with EROFS anyway. It only saves that first
> attempt. Create still clears the flag, so that a value set by
> ev_add_path() can't carry over to a later reload. I can drop the
> read-only shortcut in create if you'd prefer.

I am uncertain about this. If we know in advance that trying to set up
the map read-write will fail, how much sense does it make to try? It's
just a minimal simplification of our code.

> Uday Shankar, who did 430f8f89, also suggested dropping
> force_readonly
> altogether, 
> and tracking the ro state per path in sysfs_pathinfo()
> sounds like the right direction to me. But it changes behavior: every
> reload, not just the one done for a DISK_RO uevent, would follow the
> paths' ro state, and so would "multipath -r", since
> dm_addmap_reload()
> is in libmultipath. So I'd rather do it as a separate series on top
> of
> this fix, and keep this one small enough to backport.

Sounds good to me, even if it may mean that this particular patch
will be short-lived.

Regards
Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>
SUSE Software Solutions Germany GmbH, Frankenstr. 146, 90461 Nürnberg,
Germany
Geschäftsführer: Stefan Gaiser, Jochen Jaser, Abhinav Puri (HRB
36809,AG Nürnberg)

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

[Benjamin Marzinski]

On Mon, Oct 26, 2026 at 11:23:09PM +0100, Martin Wilck wrote:
> On Mon, 2026-10-26 at 11:04 -0700, Brian Bunker wrote:
> > On Mon, 2026-10-26 at 10:26 +0100, Martin Wilck wrote:
> > > On Fri, 2026-10-23 at 19:40 -0400, Benjamin Marzinski wrote:
> > > > 
> > > > I do still waver a bit on whether or not we could leave
> > > > force_readonly
> > > > set on failed reloads. Once a device turns read-only, it should
> > > > stay
> > > > that way until we get another uevent, at which point we will call
> > > > needs_ro_update() again. Perhaps we just need to make sure that
> > > > we
> > > > clear
> > > > force_readonly when needs_ro_update() returns false.
> > > 
> > > AFAICS, this is exactly what Brian's v4 patch does. No?
> > 
> > Mostly, yes. v4 sets force_readonly back in dm_addmap_reload() when a
> > reload fails, so the next reload tries read-only again.
> > 
> > needs_ro_update() doesn't clear it on every false return, though. It
> > clears it in the dmi.read_only == ro case only when both the map and
> > the uevent are read-write and none of the map's paths is read-only.
> > If
> > a path is still read-only, the flag stays set, so a map whose read-
> > only
> > reload failed keeps retrying instead of staying read-write.
> 
> Which is correct. If the map is read-write but there are ro paths, we
> need to retry.
> 
> There are two other cases where needs_ro_update() returns false:
> 
> 1. ro == 1 && dmi.read_only == 1
>    I don't see how force_readonly could be one in this case.
>    But maybe we should just reset it anyway, just to be sure.
> 2. ro == 0 && dmi.read_only == 1 && map_has_ro_path()
>    Same here, read_only is already set, no need to force it,
>    but resetting force_read_only wouldn't hurt.
> 
> > About dm_addmap_create(): agreed that the flag isn't needed for
> > correctness there, since a new map opens all paths fresh and a
> > read-write create fails with EROFS anyway. It only saves that first
> > attempt. Create still clears the flag, so that a value set by
> > ev_add_path() can't carry over to a later reload. I can drop the
> > read-only shortcut in create if you'd prefer.
> 
> I am uncertain about this. If we know in advance that trying to set up
> the map read-write will fail, how much sense does it make to try? It's
> just a minimal simplification of our code.

Also, looking at it now, I'm kind of dubious about 430f8f89
("multipath-tools: update mpp->force_readonly in ev_add_path"), but it
does appear that there was an issue where multipath over NVMe should not
attempt to create the device read-write, if it had read-only paths. My
problem with the patch is that if that's true, I don't see why a regular
reload wouldn't just cause the device to switch to read-write again.
But I agree with Martin. There's much to be gained by pulling out the
dm_addmap_create() code.  
 
> > Uday Shankar, who did 430f8f89, also suggested dropping
> > force_readonly
> > altogether, 
> > and tracking the ro state per path in sysfs_pathinfo()
> > sounds like the right direction to me. But it changes behavior: every
> > reload, not just the one done for a DISK_RO uevent, would follow the
> > paths' ro state, and so would "multipath -r", since
> > dm_addmap_reload()
> > is in libmultipath. So I'd rather do it as a separate series on top
> > of
> > this fix, and keep this one small enough to backport.
> 
> Sounds good to me, even if it may mean that this particular patch
> will be short-lived.
> 
> Regards
> Martin
> 
> -- 
> Dr. Martin Wilck <mwilck@suse.com>
> SUSE Software Solutions Germany GmbH, Frankenstr. 146, 90461 Nürnberg,
> Germany
> Geschäftsführer: Stefan Gaiser, Jochen Jaser, Abhinav Puri (HRB
> 36809,AG Nürnberg)

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

[Martin Wilck]

On Fri, 2026-10-23 at 19:40 -0400, Benjamin Marzinski wrote:
> 
> To solve this, whenever multipath detects that a device has switched
> to
> read-only, it forces a read-only table reload, so that all the path
> devices get re-opened read-only (since their requested mode doesn't
> match their current mode).

I am sorry, I guess I am just looking in the wrong place. But the way I
read the code in dm_get_device() [1], the kernel will re-open a device
only if the mode is upgraded, i.e. switched from ro to rw.

[1] https://elixir.bootlin.com/linux/v7.3/source/drivers/md/dm-table.c#L398

I don't see how a switch from rw to ro would cause a device to be
reopened.

Thanks
Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>
SUSE Software Solutions Germany GmbH, Frankenstr. 146, 90461 Nürnberg,
Germany
Geschäftsführer: Stefan Gaiser, Jochen Jaser, Abhinav Puri (HRB
36809,AG Nürnberg)

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

[Benjamin Marzinski]

On Mon, Oct 26, 2026 at 11:20:35PM +0100, Martin Wilck wrote:
> On Fri, 2026-10-23 at 19:40 -0400, Benjamin Marzinski wrote:
> > 
> > To solve this, whenever multipath detects that a device has switched
> > to
> > read-only, it forces a read-only table reload, so that all the path
> > devices get re-opened read-only (since their requested mode doesn't
> > match their current mode).
> 
> I am sorry, I guess I am just looking in the wrong place. But the way I
> read the code in dm_get_device() [1], the kernel will re-open a device
> only if the mode is upgraded, i.e. switched from ro to rw.
> 
> [1] https://elixir.bootlin.com/linux/v7.3/source/drivers/md/dm-table.c#L398
> 
> I don't see how a switch from rw to ro would cause a device to be
> reopened.

That only happens if find_device() already found that device in THIS
table (it's called with t->devices). For instance, the dm-delay table
can list the same device three times. Instead of opening the device 3
times, you just open it once.  Additionally, if you have already opened
the device in the current table in a mode that includes the capabilites
of the mode you need in a future call to dm_get_device() it will just
use the existing open. If you need more capabilites, you will upgrade
the open.

What matters in the multipath case is if the device isn't found in the
existing table. In that case you call dm_get_table_device(), which
calls find_table_device(). This looks for a device in the
md->table_devices list, which includes devices from all tables. But in
this case, the mode must match exactly
https://elixir.bootlin.com/linux/v7.3/source/drivers/md/dm.c#L807

Thinking about it now, I'm actually not sure if that upgrading from
dm_get_device() is a good thing. If a target asks to open multiple
copies of the same device with different modes, and dm gives it one open
that has a union of all the modes, it seems like we've dropped some
safety guardrails. I'm not sure off the top of my head if there actually
is a target that does this.

-Ben

> 
> Thanks
> Martin
> 
> -- 
> Dr. Martin Wilck <mwilck@suse.com>
> SUSE Software Solutions Germany GmbH, Frankenstr. 146, 90461 Nürnberg,
> Germany
> Geschäftsführer: Stefan Gaiser, Jochen Jaser, Abhinav Puri (HRB
> 36809,AG Nürnberg)