* [PATCH v5 0/1] multipathd: don't leave maps read-only after write protect is cleared
@ 2026-10-28 0:24 Brian Bunker
2026-10-28 0:24 ` [PATCH v5 1/1] " Brian Bunker
0 siblings, 1 reply; 3+ messages in thread
From: Brian Bunker @ 2026-10-28 0:24 UTC (permalink / raw)
To: dm-devel
Cc: Benjamin Marzinski, Martin Wilck, Christophe Varoqui,
Uday Shankar, Brian Bunker
This is v5. It follows Ben's review of v4 and the discussion with
Martin on v3: dm_addmap_reload() now clears force_readonly only after a
successful reload, needs_ro_update() clears it on every false return
except when a read-only reload still has to be retried, and
uev_update_path() takes the path's ro state from sysfs instead of from
the uevent. The changes are listed at the end.
As agreed, tracking the ro state per path and dropping force_readonly
altogether, as Martin and Uday Shankar suggested, will be a separate
series on top of this fix.
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. dm_addmap_create() clears it as soon as it has read
it, and dm_addmap_reload() clears it once the reload has succeeded.
needs_ro_update() sets it for the reload done for a change uevent,
using the path's ro state in sysfs, 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 v4:
- dm_addmap_reload() no longer clears force_readonly up front and sets
it back on failure. It clears the flag only once the reload and
resume have succeeded, as Ben suggested. A failed reload leaves it
set, so the next reload tries read-only again.
- needs_ro_update() clears force_readonly on every false return,
except while the map is read-write and one of its paths is still
read-only, which means a read-only reload failed and must be
retried. This covers the two cases Martin listed (map and path both
read-only, and path read-write on a read-only map that still has a
read-only path). In the dmi.read_only == ro case, it only walks the
paths if the flag is actually set. The early returns for ro < 0 and
for a failed dm_get_info() leave the flag alone, since there is no
valid state to decide from there.
- uev_update_path() takes the path's ro state from sysfs instead of
from the uevent's DISK_RO. The uevent only says that something
changed. The kernel updates the ro attribute before it sends the
uevent, so sysfs is current when the uevent is handled, while the
uevent itself may have been queued behind later changes. If sysfs
can't be read, the uevent's DISK_RO isn't trusted either, and
nothing is done. This also gives needs_ro_update() a valid ro value
for every change uevent, as Ben asked.
Note that this changes behavior: any change uevent for a path in a
map, not only one with DISK_RO, can now trigger a read-only reload
if sysfs disagrees with the map. It also avoids the pointless
reload that 6e40bf92 describes when a rescan's DISK_RO=0 and
DISK_RO=1 uevents are handled after the rescan has finished, since
sysfs then shows read-only for both. The cost is one sysfs read per
change uevent, next to the pathinfo() call uev_update_path() already
makes.
- dm_addmap_create() is unchanged and keeps the read-only shortcut.
Ben, I read your "There's much to be gained by pulling out the
dm_addmap_create() code" as "not much", in agreement with Martin.
Please correct me if I got that wrong.
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 | 18 ++++-
multipathd/main.c | 144 ++++++++++++++++++++++++++-------------
2 files changed, 111 insertions(+), 51 deletions(-)
--
2.56.0
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH v5 1/1] multipathd: don't leave maps read-only after write protect is cleared
2026-10-28 0:24 [PATCH v5 0/1] multipathd: don't leave maps read-only after write protect is cleared Brian Bunker
@ 2026-10-28 0:24 ` Brian Bunker
2026-10-29 19:51 ` Benjamin Marzinski
0 siblings, 1 reply; 3+ messages in thread
From: Brian Bunker @ 2026-10-28 0:24 UTC (permalink / raw)
To: dm-devel
Cc: Benjamin Marzinski, Martin Wilck, Christophe Varoqui,
Uday Shankar, 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
once the load has succeeded:
- uev_update_path() takes the path's ro state from sysfs instead of
from the uevent's DISK_RO. The uevent only says that something
changed. The kernel updates the ro attribute before it sends the
uevent, so sysfs is current when the uevent is handled, while the
uevent itself may have been queued behind later changes. If sysfs
can't be read, nothing is done.
- needs_ro_update() now sets force_readonly whenever it returns true:
to 1 when the path is read-only, and to 0 when all of the map's paths
are writable. uev_update_path() no longer sets or clears it itself.
When needs_ro_update() returns false, it clears the flag, unless the
map is read-write while one of its paths is still read-only. In that
case an earlier read-only reload failed, and the flag stays set so
that the next reload tries again. If the map has no dm info,
needs_ro_update() now fetches it, and returns false if that fails,
instead of assuming a reload is needed.
- ev_add_path() sets it from all of the map's paths rather than only
the path being added, and only when it is about to load the map
(need_do_map). mpp->paths is up to date there after adopt_paths(),
while mpp->pg doesn't contain the new paths yet.
- update_map(), which does reloads that were delayed while the map was
waiting for udev (UDEV_WAIT_RELOAD), recomputes it from the map's
paths before setting up the map.
- dm_addmap_create() clears it as soon as it has read it. A failed
create leaves no map behind, and the next create computes the flag
again.
- dm_addmap_reload() clears it once the reload and resume have
succeeded. If the reload fails, the map keeps its old table, and the
flag stays set so that the next reload tries again.
Signed-off-by: Brian Bunker <brian@purestorage.com>
---
libmultipath/devmapper.c | 18 ++++-
multipathd/main.c | 144 ++++++++++++++++++++++++++-------------
2 files changed, 111 insertions(+), 51 deletions(-)
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index e3870f59..34e0f640 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -539,10 +539,15 @@ static uint16_t build_udev_flags(const struct multipath *mpp, int reload)
int dm_addmap_create (struct multipath *mpp, char *params)
{
- int ro;
+ int ro = mpp->force_readonly ? 1 : 0;
uint16_t udev_flags = build_udev_flags(mpp, 0);
- for (ro = mpp->force_readonly ? 1 : 0; ro <= 1; ro++) {
+ /*
+ * force_readonly only applies to this load. Clear it, so that it
+ * can't affect later loads, even if this one fails.
+ */
+ mpp->force_readonly = 0;
+ for (; ro <= 1; ro++) {
int err;
if (dm_addmap(DM_DEVICE_CREATE, TGT_MPATH, mpp, params, ro,
@@ -599,8 +604,15 @@ int dm_addmap_reload(struct multipath *mpp, char *params, int flush)
r = dm_simplecmd(DM_DEVICE_RESUME, mpp->alias,
DMFL_NEED_SYNC | (flush ? 0 : DMFL_NO_FLUSH),
udev_flags);
- if (r)
+ if (r) {
+ /*
+ * force_readonly only applies to this reload. If the reload
+ * fails, the map keeps its old table, and the flag stays set
+ * so that the next reload tries again.
+ */
+ mpp->force_readonly = 0;
return r;
+ }
/* If the resume failed, dm will leave the device suspended, and
* drop the new table, so doing a second resume will try using
diff --git a/multipathd/main.c b/multipathd/main.c
index 9bce4d6b..1100584f 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -709,6 +709,55 @@ void cleanup_reset_vec(struct vector_s **v)
vector_reset(*v);
}
+static int
+sysfs_get_ro (struct path *pp)
+{
+ int ro;
+ char buff[3]; /* Either "0\n\0" or "1\n\0" */
+
+ if (!pp->udev)
+ return -1;
+
+ if (!sysfs_attr_get_value_ok(pp->udev, "ro", buff, sizeof(buff))) {
+ condlog(3, "%s: Cannot read ro attribute in sysfs", pp->dev);
+ return -1;
+ }
+
+ if (sscanf(buff, "%d\n", &ro) != 1 || ro < 0 || ro > 1) {
+ condlog(3, "%s: Cannot parse ro attribute", pp->dev);
+ return -1;
+ }
+
+ return ro;
+}
+
+/* A map must be loaded read-only if any of its paths is read-only. */
+static bool
+has_ro_path(vector paths)
+{
+ struct path *pp;
+ unsigned int i;
+
+ vector_foreach_slot (paths, pp, i) {
+ if (sysfs_get_ro(pp) == 1)
+ return true;
+ }
+ return false;
+}
+
+static bool
+map_has_ro_path(struct multipath *mpp)
+{
+ struct pathgroup *pgp;
+ unsigned int i;
+
+ vector_foreach_slot (mpp->pg, pgp, i) {
+ if (has_ro_path(pgp->paths))
+ return true;
+ }
+ return false;
+}
+
static int
update_map (struct multipath *mpp, struct vectors *vecs, int new_map)
{
@@ -732,6 +781,14 @@ retry:
mpp->action = ACT_RELOAD;
+ /*
+ * This may be a delayed reload, and the paths' write protect
+ * state may have changed since force_readonly was last set.
+ * Recompute it from the current paths. mpp->paths is up to date
+ * after adopt_paths(), while mpp->pg isn't until setup_map().
+ */
+ mpp->force_readonly = has_ro_path(mpp->paths);
+
if (setup_map(mpp, ¶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,34 @@ finish_path_init(struct path *pp, struct vectors * vecs)
static bool
needs_ro_update(struct multipath *mpp, int ro)
{
- struct pathgroup * pgp;
- struct path * pp;
- unsigned int i, j;
-
if (!mpp || ro < 0)
return false;
- if (!has_dm_info(mpp))
- return true;
- if (mpp->dmi.read_only == ro)
+ if (!has_dm_info(mpp) &&
+ dm_get_info(mpp->alias, &mpp->dmi) != DMP_OK) {
+ condlog(2, "%s: failed to get dm info", mpp->alias);
return false;
- if (ro == 1)
- return true;
- vector_foreach_slot (mpp->pg, pgp, i) {
- vector_foreach_slot (pgp->paths, pp, j) {
- if (sysfs_get_ro(pp) == 1)
- return false;
- }
}
+ if (mpp->dmi.read_only == ro) {
+ /*
+ * No reload is needed. A failed read-only reload leaves
+ * force_readonly set while the map is still read-write. Keep
+ * it only while one of the map's paths is still read-only,
+ * so that the next reload tries again.
+ */
+ if (mpp->force_readonly && (ro == 1 || !map_has_ro_path(mpp)))
+ mpp->force_readonly = 0;
+ return false;
+ }
+ if (ro == 0 && map_has_ro_path(mpp)) {
+ /*
+ * The map is read-only and has to stay so. dm has to reopen
+ * its paths for any read-write reload anyway, so the flag
+ * isn't needed.
+ */
+ mpp->force_readonly = 0;
+ return false;
+ }
+ mpp->force_readonly = ro;
return true;
}
@@ -1725,22 +1771,24 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
pthread_cleanup_pop(1);
}
- ro = uevent_get_disk_ro(uev);
+ /*
+ * The uevent only tells us that something changed. Take the
+ * path's ro state from sysfs, which the kernel updates before
+ * sending the uevent. If sysfs can't be read, don't act on
+ * the uevent's DISK_RO either.
+ */
+ ro = sysfs_get_ro(pp);
if (needs_ro_update(mpp, ro)) {
condlog(2, "%s: update path write_protect to '%d' (uevent)", uev->kernel, ro);
if (mpp->wait_for_udev != UDEV_WAIT_DONE)
mpp->wait_for_udev = UDEV_WAIT_RELOAD;
else {
- if (ro == 1)
- pp->mpp->force_readonly = 1;
retval = reload_and_sync_map(mpp, vecs);
if (retval == 2)
condlog(2, "%s: map removed during reload", pp->dev);
- else {
- pp->mpp->force_readonly = 0;
+ else
condlog(2, "%s: map %s reloaded (retval %d)", uev->kernel, mpp->alias, retval);
- }
}
}
if (auto_resize != AUTO_RESIZE_NEVER && mpp &&
--
2.56.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v5 1/1] multipathd: don't leave maps read-only after write protect is cleared
2026-10-28 0:24 ` [PATCH v5 1/1] " Brian Bunker
@ 2026-10-29 19:51 ` Benjamin Marzinski
0 siblings, 0 replies; 3+ messages in thread
From: Benjamin Marzinski @ 2026-10-29 19:51 UTC (permalink / raw)
To: Brian Bunker; +Cc: dm-devel, Martin Wilck, Christophe Varoqui, Uday Shankar
On Tue, Oct 27, 2026 at 05:24:37PM -0700, Brian Bunker wrote:
> ev_add_path() sets mpp->force_readonly when a newly added path is
> read-only, but never clears it. If a map is created or reloaded while
> its storage is write protected, the flag stays set. When write protect
> is cleared, the reload that uev_update_path() does for the DISK_RO=0
> uevent uses the stale flag and loads the map read-only, even though all
> of its paths are read-write. uev_update_path() clears the flag after
> that reload, so the map only recovers if another DISK_RO=0 uevent
> reloads it again. If none comes, the map stays read-only, and since
> mpp->dmi.read_only is then 1, needs_ro_update() also ignores later
> DISK_RO=1 uevents for it.
>
> Fix this by setting force_readonly before each load and clearing it
> once the load has succeeded:
>
> - uev_update_path() takes the path's ro state from sysfs instead of
> from the uevent's DISK_RO. The uevent only says that something
> changed. The kernel updates the ro attribute before it sends the
> uevent, so sysfs is current when the uevent is handled, while the
> uevent itself may have been queued behind later changes. If sysfs
> can't be read, nothing is done.
>
> - needs_ro_update() now sets force_readonly whenever it returns true:
> to 1 when the path is read-only, and to 0 when all of the map's paths
> are writable. uev_update_path() no longer sets or clears it itself.
> When needs_ro_update() returns false, it clears the flag, unless the
> map is read-write while one of its paths is still read-only. In that
> case an earlier read-only reload failed, and the flag stays set so
> that the next reload tries again. If the map has no dm info,
> needs_ro_update() now fetches it, and returns false if that fails,
> instead of assuming a reload is needed.
>
> - ev_add_path() sets it from all of the map's paths rather than only
> the path being added, and only when it is about to load the map
> (need_do_map). mpp->paths is up to date there after adopt_paths(),
> while mpp->pg doesn't contain the new paths yet.
>
> - update_map(), which does reloads that were delayed while the map was
> waiting for udev (UDEV_WAIT_RELOAD), recomputes it from the map's
> paths before setting up the map.
>
> - dm_addmap_create() clears it as soon as it has read it. A failed
> create leaves no map behind, and the next create computes the flag
> again.
>
> - dm_addmap_reload() clears it once the reload and resume have
> succeeded. If the reload fails, the map keeps its old table, and the
> flag stays set so that the next reload tries again.
>
I feel really bad after all this discussion, but there are still issues
with needs_ro_update(). But hopefully my suggestion will make this all
much simpler.
> Signed-off-by: Brian Bunker <brian@purestorage.com>
> ---
> libmultipath/devmapper.c | 18 ++++-
> multipathd/main.c | 144 ++++++++++++++++++++++++++-------------
> 2 files changed, 111 insertions(+), 51 deletions(-)
>
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index e3870f59..34e0f640 100644
> --- a/libmultipath/devmapper.c
> +++ b/libmultipath/devmapper.c
> @@ -539,10 +539,15 @@ static uint16_t build_udev_flags(const struct multipath *mpp, int reload)
>
> int dm_addmap_create (struct multipath *mpp, char *params)
> {
> - int ro;
> + int ro = mpp->force_readonly ? 1 : 0;
> uint16_t udev_flags = build_udev_flags(mpp, 0);
>
> - for (ro = mpp->force_readonly ? 1 : 0; ro <= 1; ro++) {
> + /*
> + * force_readonly only applies to this load. Clear it, so that it
> + * can't affect later loads, even if this one fails.
> + */
> + mpp->force_readonly = 0;
> + for (; ro <= 1; ro++) {
> int err;
>
> if (dm_addmap(DM_DEVICE_CREATE, TGT_MPATH, mpp, params, ro,
> @@ -599,8 +604,15 @@ int dm_addmap_reload(struct multipath *mpp, char *params, int flush)
> r = dm_simplecmd(DM_DEVICE_RESUME, mpp->alias,
> DMFL_NEED_SYNC | (flush ? 0 : DMFL_NO_FLUSH),
> udev_flags);
> - if (r)
> + if (r) {
> + /*
> + * force_readonly only applies to this reload. If the reload
> + * fails, the map keeps its old table, and the flag stays set
> + * so that the next reload tries again.
> + */
> + mpp->force_readonly = 0;
> return r;
> + }
>
> /* If the resume failed, dm will leave the device suspended, and
> * drop the new table, so doing a second resume will try using
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 9bce4d6b..1100584f 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -709,6 +709,55 @@ void cleanup_reset_vec(struct vector_s **v)
> vector_reset(*v);
> }
>
> +static int
> +sysfs_get_ro (struct path *pp)
> +{
> + int ro;
> + char buff[3]; /* Either "0\n\0" or "1\n\0" */
> +
> + if (!pp->udev)
> + return -1;
> +
> + if (!sysfs_attr_get_value_ok(pp->udev, "ro", buff, sizeof(buff))) {
> + condlog(3, "%s: Cannot read ro attribute in sysfs", pp->dev);
> + return -1;
> + }
> +
> + if (sscanf(buff, "%d\n", &ro) != 1 || ro < 0 || ro > 1) {
> + condlog(3, "%s: Cannot parse ro attribute", pp->dev);
> + return -1;
> + }
> +
> + return ro;
> +}
> +
> +/* A map must be loaded read-only if any of its paths is read-only. */
> +static bool
> +has_ro_path(vector paths)
> +{
> + struct path *pp;
> + unsigned int i;
> +
> + vector_foreach_slot (paths, pp, i) {
> + if (sysfs_get_ro(pp) == 1)
> + return true;
> + }
> + return false;
> +}
> +
> +static bool
> +map_has_ro_path(struct multipath *mpp)
> +{
> + struct pathgroup *pgp;
> + unsigned int i;
> +
> + vector_foreach_slot (mpp->pg, pgp, i) {
> + if (has_ro_path(pgp->paths))
> + return true;
> + }
> + return false;
> +}
> +
> static int
> update_map (struct multipath *mpp, struct vectors *vecs, int new_map)
> {
> @@ -732,6 +781,14 @@ retry:
>
> mpp->action = ACT_RELOAD;
>
> + /*
> + * This may be a delayed reload, and the paths' write protect
> + * state may have changed since force_readonly was last set.
> + * Recompute it from the current paths. mpp->paths is up to date
> + * after adopt_paths(), while mpp->pg isn't until setup_map().
> + */
> + mpp->force_readonly = has_ro_path(mpp->paths);
> +
> if (setup_map(mpp, ¶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,34 @@ finish_path_init(struct path *pp, struct vectors * vecs)
> static bool
> needs_ro_update(struct multipath *mpp, int ro)
> {
> - struct pathgroup * pgp;
> - struct path * pp;
> - unsigned int i, j;
> -
> if (!mpp || ro < 0)
> return false;
> - if (!has_dm_info(mpp))
> - return true;
> - if (mpp->dmi.read_only == ro)
> + if (!has_dm_info(mpp) &&
> + dm_get_info(mpp->alias, &mpp->dmi) != DMP_OK) {
> + condlog(2, "%s: failed to get dm info", mpp->alias);
> return false;
> - if (ro == 1)
> - return true;
> - vector_foreach_slot (mpp->pg, pgp, i) {
> - vector_foreach_slot (pgp->paths, pp, j) {
> - if (sysfs_get_ro(pp) == 1)
> - return false;
> - }
> }
> + if (mpp->dmi.read_only == ro) {
> + /*
> + * No reload is needed. A failed read-only reload leaves
> + * force_readonly set while the map is still read-write. Keep
> + * it only while one of the map's paths is still read-only,
> + * so that the next reload tries again.
> + */
> + if (mpp->force_readonly && (ro == 1 || !map_has_ro_path(mpp)))
This clears force_readonly if either the updated path is read-only or no
paths are read-only. That doesn't make sense, and it doesn't match the
comment.
But there's a bigger issue where this doesn't make sense. if ro == 0 and
map_has_ro_path(mpp) == 1, that means the map must be read-write (since
it matches the path in this block), but we do have a read-only path. In
that case, we should actually be setting mpp->force_readonly and
returning true.
I think we're making this too complicated here. Reading sysfs isn't that
expensive. Perhaps its better to just bite the bullet and stop trying so
hard to avoid reading all the paths ro states in needs_ro_update(). The
idea is that we skip the sysfs_get_ro() (or the old uevent_get_disk_ro)
call in uev_update_path(). Instead we just call needs_ro_update(), which
no longer takes a ro value, and looks something like
static bool
needs_ro_update(struct multipath *mpp)
{
if (!mpp)
return false;
if (!has_dm_info(mpp) &&
dm_get_info(mpp->alias, &mpp->dmi) != DMP_OK) {
condlog(2, "%s: failed to get dm info", mpp->alias);
return false;
}
mpp->force_readonly = map_has_ro_path(mpp);
/*
* The map is read-only and has to stay so. dm has to reopen its
* paths for any read-write reload anyway, so the flag isn't needed
*/
if (mpp->force_readonly && mpp->dmi.read_only) {
mpp->force_readonly = 0;
return false;
}
/* The current map state doesn't match the paths state. Update it */
return (mpp->force_readonly != mpp->dmi.read_only);
}
Does this look reasonable?
-Ben
> + mpp->force_readonly = 0;
> + return false;
> + }
> + if (ro == 0 && map_has_ro_path(mpp)) {
> + /*
> + * The map is read-only and has to stay so. dm has to reopen
> + * its paths for any read-write reload anyway, so the flag
> + * isn't needed.
> + */
> + mpp->force_readonly = 0;
> + return false;
> + }
> + mpp->force_readonly = ro;
> return true;
> }
>
> @@ -1725,22 +1771,24 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
> pthread_cleanup_pop(1);
> }
>
> - ro = uevent_get_disk_ro(uev);
> + /*
> + * The uevent only tells us that something changed. Take the
> + * path's ro state from sysfs, which the kernel updates before
> + * sending the uevent. If sysfs can't be read, don't act on
> + * the uevent's DISK_RO either.
> + */
> + ro = sysfs_get_ro(pp);
> if (needs_ro_update(mpp, ro)) {
> condlog(2, "%s: update path write_protect to '%d' (uevent)", uev->kernel, ro);
>
> if (mpp->wait_for_udev != UDEV_WAIT_DONE)
> mpp->wait_for_udev = UDEV_WAIT_RELOAD;
> else {
> - if (ro == 1)
> - pp->mpp->force_readonly = 1;
> retval = reload_and_sync_map(mpp, vecs);
> if (retval == 2)
> condlog(2, "%s: map removed during reload", pp->dev);
> - else {
> - pp->mpp->force_readonly = 0;
> + else
> condlog(2, "%s: map %s reloaded (retval %d)", uev->kernel, mpp->alias, retval);
> - }
> }
> }
> if (auto_resize != AUTO_RESIZE_NEVER && mpp &&
> --
> 2.56.0
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-10-29 19:52 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-28 0:24 [PATCH v5 0/1] multipathd: don't leave maps read-only after write protect is cleared Brian Bunker
2026-10-28 0:24 ` [PATCH v5 1/1] " Brian Bunker
2026-10-29 19:51 ` 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.