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 <[email protected]> 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)
