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

[Benjamin Marzinski <bmarzins@redhat.com>]

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 <brian@purestorage.com>
> 
> 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 <mwilck@suse.com>