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. That seems better that making a read-only device that should be read-write. Thoughts? -Ben > > Martin > > -- > Martin Wilck <[email protected]>
