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

[Martin Wilck <martin.wilck@suse.com>]

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 <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. 

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 <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)