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

[Benjamin Marzinski <bmarzins@redhat.com>]

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)