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

[Brian Bunker <brian@purestorage.com>]

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