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

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

[Brian Bunker]

A multipath map can stay read-only after write protect is cleared on
the storage, even though all of its paths are read-write. It happens
when the map's paths are removed and re-added while write protect is
set:

1. Write protect is set. The map is reloaded read-only.
   uev_update_path() sets force_readonly for that reload and clears it
   afterwards.

2. All of the map's paths are removed, and new paths are added while
   write protect is still set. ev_add_path() sees a read-only path and
   sets force_readonly = 1. Nothing ever clears it.

3. Write protect is cleared on the new paths, one after another. Each
   path's DISK_RO=0 uevent is ignored while any path of the map is
   still read-only.

4. The last path's uevent reloads the map. The reload sees the stale
   force_readonly = 1 and loads the map read-only, logging "reloaded
   (retval 0)".

force_readonly only works in one direction: ev_add_path() can set it
but never clears it. If the new paths had been added already writable,
ev_add_path() wouldn't have set the flag and the map wouldn't have
stuck. The paths have to be added while write protected and then
cleared in place.

After step 4 nothing reloads the map again. A rescan doesn't change the
paths' ro state, so it generates no DISK_RO uevent. And since
multipathd now sees the map as read-only, needs_ro_update() also
ignores the next DISK_RO=1 uevent for it.

If two paths' uevents happen to be handled after all paths are
writable, the map does recover: uev_update_path() clears
force_readonly after its reload, so the second reload is read-write.
That makes the problem look intermittent.

Example
-------

We hit this on RHEL 9 device-mapper-multipath 0.8.7, whose
ev_add_path() and uev_update_path() handle force_readonly the same way
as current master. The host's iSCSI sessions were torn down and
re-established while the array was write protecting the volume. From
the messages log, trimmed:

  05:48:27  ...114d8: reload [0 51200 multipath 0 1 alua 1 1
            service-time 0 6 1 135:272 1 66:624 1 69:720 1 129:560 1
            132:656 1 135:752 1]
  05:48:27  sdsd: map ...114d8 reloaded (retval 0)
  05:51:04  kernel: sd 24:0:0:48: [sdaer] Write Protect is on
  05:51:05  ...114d8: remaining active paths: 0
  05:51:07  kernel: sd 29:0:0:48: [sdaoi] Write Protect is on
            (and four more new paths)
  06:09:01  kernel: sd 24:0:0:48: [sdaer] Write Protect is off
  06:09:07  kernel: sd 29:0:0:48: [sdaoi] Write Protect is off
  06:09:10  sdaoi: update path write_protect to '0' (uevent)
  06:09:10  ...114d8: reload [0 51200 multipath 0 1 alua 1 1
            service-time 0 6 1 67:880 1 70:992 1 130:848 1 133:960 1
            65:1072 1 67:1056 1]
  06:09:10  sdaoi: map ...114d8 reloaded (retval 0)

The state afterwards, as recorded by the test:

  dm-106: ro
  sdaer: rw  sdagu: rw  sdaix: rw  sdala: rw  sdand: rw  sdaoi: rw

The patch
---------

The patch adds map_has_ro_path(), which checks whether any of a map's
paths is read-only in sysfs, and uses it wherever force_readonly is
set. ev_add_path() now computes the flag from all of the map's paths
and clears it once the map is loaded, as uev_update_path() already
does after its reload. uev_update_path() and update_map() recompute it
before reloading. With the patch, the 06:09:10 reload would have found
no read-only path and loaded the map read-write.

Open points for review:

- uev_update_path() now takes the read-only state from sysfs rather
  than from the uevent's DISK_RO value. That relies on set_disk_ro()
  updating the disk's read-only state before sending the uevent, which
  current kernels do. Older kernels (e.g. v4.18) sent the uevent first.

- force_readonly is a one-shot hint that several callers have to set
  and clear correctly. A follow-up could remove it and have
  dm_addmap_create() and dm_addmap_reload() call map_has_ro_path()
  themselves. That would move sysfs_get_ro() into libmultipath and
  change the multipath command's behavior too, so it isn't part of
  this fix.

Brian Bunker (1):
  multipathd: don't leave maps read-only after write protect is cleared

 multipathd/main.c | 44 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 37 insertions(+), 7 deletions(-)

-- 
2.56.0

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

[Brian Bunker]

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.

Add a helper, map_has_ro_path(), that checks whether any of a map's
paths is read-only in sysfs, and use it to set force_readonly:

- In ev_add_path(), set force_readonly from all of the map's paths
  rather than only the path being added, and clear it once the map has
  been loaded, as uev_update_path() already does after its reload. With
  need_do_map == 0 the flag is left set for the call that loads the map.

- In uev_update_path(), set force_readonly from the paths' current
  state in sysfs instead of from the uevent. The kernel updates the ro
  attribute before it sends the DISK_RO uevent, so sysfs is current
  when the uevent is handled.

- In update_map(), which does reloads that were delayed while the map
  was waiting for udev (UDEV_WAIT_RELOAD), recompute force_readonly
  before setting up the map, since the paths' state may have changed
  since it was last set.

Signed-off-by: Brian Bunker <brian@purestorage.com>
---
 multipathd/main.c | 44 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 37 insertions(+), 7 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 9bce4d6b..68916256 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -709,6 +709,22 @@ void cleanup_reset_vec(struct vector_s **v)
 	vector_reset(*v);
 }
 
+static int sysfs_get_ro(struct path *pp);
+
+/* A map must be loaded read-only if any of its paths is read-only. */
+static bool
+map_has_ro_path(struct multipath *mpp)
+{
+	struct path *pp;
+	unsigned int i;
+
+	vector_foreach_slot (mpp->paths, pp, i) {
+		if (sysfs_get_ro(pp) == 1)
+			return true;
+	}
+	return false;
+}
+
 static int
 update_map (struct multipath *mpp, struct vectors *vecs, int new_map)
 {
@@ -732,6 +748,13 @@ retry:
 
 	mpp->action = ACT_RELOAD;
 
+	/*
+	 * This may be a delayed reload, and the paths' write protect
+	 * state may have changed since force_readonly was last set.
+	 * Recompute it from the current paths.
+	 */
+	mpp->force_readonly = map_has_ro_path(mpp);
+
 	if (setup_map(mpp, &params, vecs)) {
 		condlog(0, "%s: failed to setup new map in update", mpp->alias);
 		retries = -1;
@@ -1305,7 +1328,6 @@ ev_add_path (struct path * pp, struct vectors * vecs, int need_do_map)
 	int retries = 3;
 	int start_waiter = 0;
 	int ret;
-	int ro;
 	unsigned char prflag = PR_UNSET;
 
 	/*
@@ -1370,10 +1392,11 @@ rescan:
 			goto fail; /* leave path added to pathvec */
 	}
 
-	/* ro check - if new path is ro, force map to be ro as well */
-	ro = sysfs_get_ro(pp);
-	if (ro == 1)
-		mpp->force_readonly = 1;
+	/*
+	 * Load the map read-only if any of its paths is read-only,
+	 * not just the one being added.
+	 */
+	mpp->force_readonly = map_has_ro_path(mpp);
 
 	if (!need_do_map)
 		return 0;
@@ -1400,6 +1423,8 @@ rescan:
 		sleep(1);
 		ret = domap(mpp, params, 1);
 	}
+	/* force_readonly only applies to this load, as in uev_update_path() */
+	mpp->force_readonly = 0;
 	if (ret == DOMAP_FAIL || ret == DOMAP_RETRY) {
 		condlog(0, "%s: failed in domap for addition of new "
 			"path %s", mpp->alias, pp->dev);
@@ -1732,8 +1757,13 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
 			if (mpp->wait_for_udev != UDEV_WAIT_DONE)
 				mpp->wait_for_udev = UDEV_WAIT_RELOAD;
 			else {
-				if (ro == 1)
-					pp->mpp->force_readonly = 1;
+				/*
+				 * force_readonly may be stale from ev_add_path().
+				 * Set it from the paths' current state in sysfs,
+				 * which the kernel updates before sending the
+				 * uevent, rather than from the uevent itself.
+				 */
+				pp->mpp->force_readonly = map_has_ro_path(mpp);
 				retval = reload_and_sync_map(mpp, vecs);
 				if (retval == 2)
 					condlog(2, "%s: map removed during reload", pp->dev);
-- 
2.56.0