* [PATCH v4 0/1] multipathd: don't leave maps read-only after write protect is cleared
@ 2026-10-22 23:48 Brian Bunker
2026-10-22 23:48 ` [PATCH v4 1/1] " Brian Bunker
0 siblings, 1 reply; 3+ messages in thread
From: Brian Bunker @ 2026-10-22 23:48 UTC (permalink / raw)
To: dm-devel; +Cc: Benjamin Marzinski, Martin Wilck, Christophe Varoqui,
Brian Bunker
This is v4. It follows Martin's review of v3: if a reload fails,
dm_addmap_reload() now sets force_readonly back, so that a map that was
supposed to become read-only is retried on the next reload, and
needs_ro_update() clears the flag again once none of the map's paths is
read-only. The changes are listed at the end.
Ben, your Reviewed-by from v2 is still dropped, since needs_ro_update()
has changed since then. 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. If a reload fails, dm_addmap_reload()
sets it back. 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 v3:
- If a reload fails, dm_addmap_reload() sets force_readonly back to
the value it had on entry. The map still has its old table, so a map
that was supposed to become read-only is tried again on the next
reload. This covers both a failed load and a failed resume.
- In needs_ro_update(), the early return for dmi.read_only == ro now
clears force_readonly if the map and the uevent are both read-write
and none of the map's paths is read-only, so the restored flag can't
make a later, unrelated reload load the map read-only.
- Move the walk over the map's path groups into map_has_ro_path(),
since needs_ro_update() now needs it in two places.
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 | 26 ++++++--
multipathd/main.c | 128 +++++++++++++++++++++++++--------------
2 files changed, 103 insertions(+), 51 deletions(-)
--
2.56.0
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH v4 1/1] multipathd: don't leave maps read-only after write protect is cleared
2026-10-22 23:48 [PATCH v4 0/1] multipathd: don't leave maps read-only after write protect is cleared Brian Bunker
@ 2026-10-22 23:48 ` Brian Bunker
2026-10-27 4:17 ` Benjamin Marzinski
0 siblings, 1 reply; 3+ messages in thread
From: Brian Bunker @ 2026-10-22 23:48 UTC (permalink / raw)
To: dm-devel; +Cc: Benjamin Marzinski, Martin Wilck, Christophe Varoqui,
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.
- If a reload fails, dm_addmap_reload() sets it back, because the map
still has its old table. If the map was supposed to become read-only,
the next reload then tries again. needs_ro_update() clears the flag
again when a DISK_RO=0 uevent arrives for a read-write map and none
of the map's paths is read-only any more, so that a later reload for
an unrelated reason, such as a path group change, doesn't load the
map read-only.
Signed-off-by: Brian Bunker <brian@purestorage.com>
---
libmultipath/devmapper.c | 26 ++++++--
multipathd/main.c | 128 +++++++++++++++++++++++++--------------
2 files changed, 103 insertions(+), 51 deletions(-)
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index e3870f59..324a92cc 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,26 @@ 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);
+ /*
+ * force_readonly only applies to this load. Clear it, so that it
+ * can't affect later loads, unless this one fails (see below).
+ */
+ 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);
@@ -609,6 +621,12 @@ int dm_addmap_reload(struct multipath *mpp, char *params, int flush)
dm_simplecmd(DM_DEVICE_RESUME, mpp->alias,
DMFL_NEED_SYNC | (flush ? 0 : DMFL_NO_FLUSH),
udev_flags);
+ /*
+ * The map still has its old table. If it was supposed to become
+ * read-only, keep force_readonly set, so that the next reload
+ * tries again.
+ */
+ mpp->force_readonly = force_ro;
return 0;
}
diff --git a/multipathd/main.c b/multipathd/main.c
index 9bce4d6b..ddef64fa 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -709,6 +709,55 @@ void cleanup_reset_vec(struct vector_s **v)
vector_reset(*v);
}
+static int
+sysfs_get_ro (struct path *pp)
+{
+ int ro;
+ char buff[3]; /* Either "0\n\0" or "1\n\0" */
+
+ if (!pp->udev)
+ return -1;
+
+ if (!sysfs_attr_get_value_ok(pp->udev, "ro", buff, sizeof(buff))) {
+ condlog(3, "%s: Cannot read ro attribute in sysfs", pp->dev);
+ return -1;
+ }
+
+ if (sscanf(buff, "%d\n", &ro) != 1 || ro < 0 || ro > 1) {
+ condlog(3, "%s: Cannot parse ro attribute", pp->dev);
+ return -1;
+ }
+
+ return ro;
+}
+
+/* A map must be loaded read-only if any of its paths is read-only. */
+static bool
+has_ro_path(vector paths)
+{
+ struct path *pp;
+ unsigned int i;
+
+ vector_foreach_slot (paths, pp, i) {
+ if (sysfs_get_ro(pp) == 1)
+ return true;
+ }
+ return false;
+}
+
+static bool
+map_has_ro_path(struct multipath *mpp)
+{
+ struct pathgroup *pgp;
+ unsigned int i;
+
+ vector_foreach_slot (mpp->pg, pgp, i) {
+ if (has_ro_path(pgp->paths))
+ return true;
+ }
+ return false;
+}
+
static int
update_map (struct multipath *mpp, struct vectors *vecs, int new_map)
{
@@ -732,6 +781,14 @@ retry:
mpp->action = ACT_RELOAD;
+ /*
+ * This may be a delayed reload, and the paths' write protect
+ * state may have changed since force_readonly was last set.
+ * Recompute it from the current paths. mpp->paths is up to date
+ * after adopt_paths(), while mpp->pg isn't until setup_map().
+ */
+ mpp->force_readonly = has_ro_path(mpp->paths);
+
if (setup_map(mpp, ¶ms, vecs)) {
condlog(0, "%s: failed to setup new map in update", mpp->alias);
retries = -1;
@@ -1270,28 +1327,6 @@ out:
return ret;
}
-static int
-sysfs_get_ro (struct path *pp)
-{
- int ro;
- char buff[3]; /* Either "0\n\0" or "1\n\0" */
-
- if (!pp->udev)
- return -1;
-
- if (!sysfs_attr_get_value_ok(pp->udev, "ro", buff, sizeof(buff))) {
- condlog(3, "%s: Cannot read ro attribute in sysfs", pp->dev);
- return -1;
- }
-
- if (sscanf(buff, "%d\n", &ro) != 1 || ro < 0 || ro > 1) {
- condlog(3, "%s: Cannot parse ro attribute", pp->dev);
- return -1;
- }
-
- return ro;
-}
-
/*
* returns:
* 0: added
@@ -1305,7 +1340,6 @@ ev_add_path (struct path * pp, struct vectors * vecs, int need_do_map)
int retries = 3;
int start_waiter = 0;
int ret;
- int ro;
unsigned char prflag = PR_UNSET;
/*
@@ -1370,14 +1404,16 @@ rescan:
goto fail; /* leave path added to pathvec */
}
- /* ro check - if new path is ro, force map to be ro as well */
- ro = sysfs_get_ro(pp);
- if (ro == 1)
- mpp->force_readonly = 1;
-
if (!need_do_map)
return 0;
+ /*
+ * Load the map read-only if any of its paths is read-only, not
+ * just the one being added. mpp->paths is up to date after
+ * adopt_paths(), while mpp->pg doesn't have the new paths yet.
+ */
+ mpp->force_readonly = has_ro_path(mpp->paths);
+
if (!dm_map_present(mpp->alias)) {
mpp->action = ACT_CREATE;
start_waiter = 1;
@@ -1596,24 +1632,26 @@ finish_path_init(struct path *pp, struct vectors * vecs)
static bool
needs_ro_update(struct multipath *mpp, int ro)
{
- struct pathgroup * pgp;
- struct path * pp;
- unsigned int i, j;
-
if (!mpp || ro < 0)
return false;
- if (!has_dm_info(mpp))
- return true;
- if (mpp->dmi.read_only == ro)
+ if (!has_dm_info(mpp) &&
+ dm_get_info(mpp->alias, &mpp->dmi) != DMP_OK) {
+ condlog(2, "%s: failed to get dm info", mpp->alias);
return false;
- if (ro == 1)
- return true;
- vector_foreach_slot (mpp->pg, pgp, i) {
- vector_foreach_slot (pgp->paths, pp, j) {
- if (sysfs_get_ro(pp) == 1)
- return false;
- }
}
+ if (mpp->dmi.read_only == ro) {
+ /*
+ * A failed read-only reload leaves force_readonly set while
+ * the map is still read-write. Clear it once no path is
+ * read-only any more.
+ */
+ if (ro == 0 && mpp->force_readonly && !map_has_ro_path(mpp))
+ mpp->force_readonly = 0;
+ return false;
+ }
+ if (ro == 0 && map_has_ro_path(mpp))
+ return false;
+ mpp->force_readonly = ro;
return true;
}
@@ -1732,15 +1770,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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v4 1/1] multipathd: don't leave maps read-only after write protect is cleared
2026-10-22 23:48 ` [PATCH v4 1/1] " Brian Bunker
@ 2026-10-27 4:17 ` Benjamin Marzinski
0 siblings, 0 replies; 3+ messages in thread
From: Benjamin Marzinski @ 2026-10-27 4:17 UTC (permalink / raw)
To: Brian Bunker; +Cc: dm-devel, Martin Wilck, Christophe Varoqui
On Thu, Oct 22, 2026 at 04:48:53PM -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 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.
>
> - If a reload fails, dm_addmap_reload() sets it back, because the map
> still has its old table. If the map was supposed to become read-only,
> the next reload then tries again. needs_ro_update() clears the flag
> again when a DISK_RO=0 uevent arrives for a read-write map and none
> of the map's paths is read-only any more, so that a later reload for
> an unrelated reason, such as a path group change, doesn't load the
> map read-only.
>
> Signed-off-by: Brian Bunker <brian@purestorage.com>
> ---
> libmultipath/devmapper.c | 26 ++++++--
> multipathd/main.c | 128 +++++++++++++++++++++++++--------------
> 2 files changed, 103 insertions(+), 51 deletions(-)
>
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index e3870f59..324a92cc 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,26 @@ 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);
>
> + /*
> + * force_readonly only applies to this load. Clear it, so that it
> + * can't affect later loads, unless this one fails (see below).
> + */
> + 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);
> @@ -609,6 +621,12 @@ int dm_addmap_reload(struct multipath *mpp, char *params, int flush)
> dm_simplecmd(DM_DEVICE_RESUME, mpp->alias,
> DMFL_NEED_SYNC | (flush ? 0 : DMFL_NO_FLUSH),
> udev_flags);
> + /*
> + * The map still has its old table. If it was supposed to become
> + * read-only, keep force_readonly set, so that the next reload
> + * tries again.
> + */
> + mpp->force_readonly = force_ro;
> return 0;
Unsetting and then resetting force_readonly seems like more work than
necessary. It seems simpler to just clear mpp->force_readonly if we
successfully reload the table and resume the map.
> }
>
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 9bce4d6b..ddef64fa 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -709,6 +709,55 @@ void cleanup_reset_vec(struct vector_s **v)
> vector_reset(*v);
> }
>
> +static int
> +sysfs_get_ro (struct path *pp)
> +{
> + int ro;
> + char buff[3]; /* Either "0\n\0" or "1\n\0" */
> +
> + if (!pp->udev)
> + return -1;
> +
> + if (!sysfs_attr_get_value_ok(pp->udev, "ro", buff, sizeof(buff))) {
> + condlog(3, "%s: Cannot read ro attribute in sysfs", pp->dev);
> + return -1;
> + }
> +
> + if (sscanf(buff, "%d\n", &ro) != 1 || ro < 0 || ro > 1) {
> + condlog(3, "%s: Cannot parse ro attribute", pp->dev);
> + return -1;
> + }
> +
> + return ro;
> +}
> +
> +/* A map must be loaded read-only if any of its paths is read-only. */
> +static bool
> +has_ro_path(vector paths)
> +{
> + struct path *pp;
> + unsigned int i;
> +
> + vector_foreach_slot (paths, pp, i) {
> + if (sysfs_get_ro(pp) == 1)
> + return true;
> + }
> + return false;
> +}
> +
> +static bool
> +map_has_ro_path(struct multipath *mpp)
> +{
> + struct pathgroup *pgp;
> + unsigned int i;
> +
> + vector_foreach_slot (mpp->pg, pgp, i) {
> + if (has_ro_path(pgp->paths))
> + return true;
> + }
> + return false;
> +}
> +
> static int
> update_map (struct multipath *mpp, struct vectors *vecs, int new_map)
> {
> @@ -732,6 +781,14 @@ retry:
>
> mpp->action = ACT_RELOAD;
>
> + /*
> + * This may be a delayed reload, and the paths' write protect
> + * state may have changed since force_readonly was last set.
> + * Recompute it from the current paths. mpp->paths is up to date
> + * after adopt_paths(), while mpp->pg isn't until setup_map().
> + */
> + mpp->force_readonly = has_ro_path(mpp->paths);
> +
> if (setup_map(mpp, ¶ms, vecs)) {
> condlog(0, "%s: failed to setup new map in update", mpp->alias);
> retries = -1;
> @@ -1270,28 +1327,6 @@ out:
> return ret;
> }
>
> -static int
> -sysfs_get_ro (struct path *pp)
> -{
> - int ro;
> - char buff[3]; /* Either "0\n\0" or "1\n\0" */
> -
> - if (!pp->udev)
> - return -1;
> -
> - if (!sysfs_attr_get_value_ok(pp->udev, "ro", buff, sizeof(buff))) {
> - condlog(3, "%s: Cannot read ro attribute in sysfs", pp->dev);
> - return -1;
> - }
> -
> - if (sscanf(buff, "%d\n", &ro) != 1 || ro < 0 || ro > 1) {
> - condlog(3, "%s: Cannot parse ro attribute", pp->dev);
> - return -1;
> - }
> -
> - return ro;
> -}
> -
> /*
> * returns:
> * 0: added
> @@ -1305,7 +1340,6 @@ ev_add_path (struct path * pp, struct vectors * vecs, int need_do_map)
> int retries = 3;
> int start_waiter = 0;
> int ret;
> - int ro;
> unsigned char prflag = PR_UNSET;
>
> /*
> @@ -1370,14 +1404,16 @@ rescan:
> goto fail; /* leave path added to pathvec */
> }
>
> - /* ro check - if new path is ro, force map to be ro as well */
> - ro = sysfs_get_ro(pp);
> - if (ro == 1)
> - mpp->force_readonly = 1;
> -
> if (!need_do_map)
> return 0;
>
> + /*
> + * Load the map read-only if any of its paths is read-only, not
> + * just the one being added. mpp->paths is up to date after
> + * adopt_paths(), while mpp->pg doesn't have the new paths yet.
> + */
> + mpp->force_readonly = has_ro_path(mpp->paths);
> +
> if (!dm_map_present(mpp->alias)) {
> mpp->action = ACT_CREATE;
> start_waiter = 1;
> @@ -1596,24 +1632,26 @@ finish_path_init(struct path *pp, struct vectors * vecs)
> static bool
> needs_ro_update(struct multipath *mpp, int ro)
> {
> - struct pathgroup * pgp;
> - struct path * pp;
> - unsigned int i, j;
> -
> if (!mpp || ro < 0)
> return false;
I said we should clear mpp->force_reload when we return false, but we
obviously can't do that when mpp is NULL. Perhaps if ro is -1 in
uev_update_path(), we should check sysfs instead. Something like:
if (ro < 0)
ro = (sysfs_get_ro == 1);
That way we'd always call needs_ro_update() with a valid value for
ro.
> - if (!has_dm_info(mpp))
> - return true;
> - if (mpp->dmi.read_only == ro)
> + if (!has_dm_info(mpp) &&
> + dm_get_info(mpp->alias, &mpp->dmi) != DMP_OK) {
> + condlog(2, "%s: failed to get dm info", mpp->alias);
> return false;
Not sure it matters what we do here. I supposed that we could check all
the paths and set force_readonly if one of them is read-only.
> - if (ro == 1)
> - return true;
> - vector_foreach_slot (mpp->pg, pgp, i) {
> - vector_foreach_slot (pgp->paths, pp, j) {
> - if (sysfs_get_ro(pp) == 1)
> - return false;
> - }
> }
> + if (mpp->dmi.read_only == ro) {
> + /*
> + * A failed read-only reload leaves force_readonly set while
> + * the map is still read-write. Clear it once no path is
> + * read-only any more.
> + */
> + if (ro == 0 && mpp->force_readonly && !map_has_ro_path(mpp))
> + mpp->force_readonly = 0;
This is correct.
> + return false;
> + }
> + if (ro == 0 && map_has_ro_path(mpp))
If ro is 0 and not equal to mpp->dmi.read_only, then that means the
map is already read-only. We don't need force_readonly here, since
dm_addmap_reload() won't be able to reload the map read-write unless it
can actually re-open the devices read-write (the path devices in the
existing table must be opened read-only since the multipath device is
read-only, so dm can't simply reuse them for the new table that it's
loading).
-Ben
> + return false;
> + mpp->force_readonly = ro;
> return true;
> }
>
> @@ -1732,15 +1770,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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-10-27 4:17 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-22 23:48 [PATCH v4 0/1] multipathd: don't leave maps read-only after write protect is cleared Brian Bunker
2026-10-22 23:48 ` [PATCH v4 1/1] " Brian Bunker
2026-10-27 4:17 ` Benjamin Marzinski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.