On Thu, 2026-10-22 at 20:24 -0400, Benjamin Marzinski wrote: > On Thu, Oct 22, 2026 at 10:17:39PM +0200, Martin Wilck 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, > > > even if > > > that load fails. Otherwise a failed load would leave it set, > > > and a > > > later reload for an unrelated reason, such as a path group > > > change, > > > would load the map read-only. > > > > > > Signed-off-by: Brian Bunker <[email protected]> > > > > Almost there. One remark below. > > > > > > > > diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c > > > index e3870f5..c994715 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,23 @@ 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); > > > > > > + /* As in dm_addmap_create(), force_readonly only applies > > > to this load */ > > > + mpp->force_readonly = 0; > > > + > > > > One minor issue here. If the reload below fails, the map is still > > read-write > > in the kernel. As we reset force_readonly, it will remain so, at > > least until > > we get another event with RO=1. > > > > If the RO reload fails with force_readonly=1, we could set > > force_readonly to 1 again > > after the failure. In needs_ro_update(), we'd then need to modify > > the fast > > return clause "if (mpp->dmi.read_only == ro)"; if both are 0 and > > force_readonly is > > still set, you'd need to check the paths and see if there's still a > > read-only path > > around. > > I can actually see both sides of this argument. If the reload fails, > and > the next reload doesn't check the paths again, we really don't know > what > the correct setting is. > > This may be overthinking things, but we could make > force_readonly a three state value, somthing like: > > MP_RELOAD_RO > MP_RELOAD_RW > MP_RELOAD_RECHECK > > If mpp->force_readonly was MP_RELOAD_RECHECK needs_ro_update() would > always return true, and dm_addmap_create() and dm_addmap_reload() > would > check the paths themselves. Again, because these could be called in > cases where we can't trust mpp->paths to be up to date, They should > probably loop through the mpp->pg. You can make it always correct to > use > mpp->pg in has_ro_path() by moving the calls to has_ro_path() to > after > setup_map() is called. If that succeeds, that pathgroups have to be > set up > correctly. > > If we don't do something complicated, it might be better to always > unset > mpp->force_readonly like it patch currently does. It's an unlikely > corner case and at worst you would get a readwrite mpath device where > writes failed, because the device really should be read-only.
I agree that it isn't a big issue if such writes fail. But if some paths are still read-write, writes might actually _succeed_ on them, Data corruption would be possible, as some writes would succeed an others wouldn't. Am I overlooking something, and this is actually impossible? I guess it's highly unlikely because in practice the ro/rw is toggled on the storage side, and happens per volume, i.e. for all paths simultaneously. But if that's true and writes-succeeding-although-they-shouldn't is not an issue, I wonder why we go through the hassle of setting the read- only attribute on our maps at all. We could just keep them read-write and trust that the writes (which shouldn't be attempted on read-only devices after all) will fail at lower layers. > That seems > better that making a read-only device that should be read-write. If this is consensus, Brian's v3 would actually be preferred to his v4. It isn't a religious matter to me, I agree that failing to reload read- only is a highly unlikely thing to happen. But I'd like to clarify the question above. I am not so enthusastic about the idea of the 3-valued force_readonly. If we want to do this right, we need to check on every reload. And IMO that means that we need to track the read-only state of the paths in multipathd. It's cheap to retrieve the "ro" attribute in sysfs_pathinfo() and track it. If it changes, we expect an uevent, where we check DISK_RO anyway. The logic could be similar as it is now, but we can check on each reload whether the map should be read-only, because the check would be just a cheap loop over the paths. And we wouldn't need force_readonly at all any more. 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)
