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

[Brian Bunker <brian@purestorage.com>]

This is v4. It follows Martin's review of v3: if a reload fails,
dm_addmap_reload() now sets force_readonly back, so that a map that was
supposed to become read-only is retried on the next reload, and
needs_ro_update() clears the flag again once none of the map's paths is
read-only. The changes are listed at the end.

Ben, your Reviewed-by from v2 is still dropped, since needs_ro_update()
has changed since then. The no-dm-info case reverses the "assume it
needs to be reloaded" behavior from 6e40bf92, so please check that
you're OK with it.

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 sets force_readonly right before each load from the state of
the map's paths, and dm_addmap_create() and dm_addmap_reload() clear it
as soon as they have read it. If a reload fails, dm_addmap_reload()
sets it back. needs_ro_update() sets it for the reload done for a
DISK_RO uevent, and ev_add_path() and update_map() compute it from all
of the map's paths. With the patch, the 06:09:10 reload would have
found no read-only path and loaded the map read-write.

Changes since v3:

- If a reload fails, dm_addmap_reload() sets force_readonly back to
  the value it had on entry. The map still has its old table, so a map
  that was supposed to become read-only is tried again on the next
  reload. This covers both a failed load and a failed resume.

- In needs_ro_update(), the early return for dmi.read_only == ro now
  clears force_readonly if the map and the uevent are both read-write
  and none of the map's paths is read-only, so the restored flag can't
  make a later, unrelated reload load the map read-only.

- Move the walk over the map's path groups into map_has_ro_path(),
  since needs_ro_update() now needs it in two places.

Changes since v2:

- In needs_ro_update(), fetch the dm info with dm_get_info() if the
  map has none, and return false if that fails, instead of assuming a
  reload is needed. This also removes the separate path walk for that
  case.

- Clear force_readonly in dm_addmap_create() and dm_addmap_reload() as
  soon as they have read it, instead of in dm_addmap() on success. A
  failed load can no longer leave the flag set for a later, unrelated
  reload, such as one for a path group change. needs_ro_update() no
  longer needs to clear the flag itself.

- I didn't add per-path tracking of the ro state. force_readonly is
  only computed from sysfs right before a load, which is rare, so the
  cost of reading sysfs there is small. Per-path tracking could be a
  follow-up if you want it.

Changes since v1:

- Set force_readonly in needs_ro_update() when it returns true,
  instead of in uev_update_path(). The DISK_RO=1 case uses the uevent
  again, rather than rereading sysfs.

- Clear force_readonly in dm_addmap() on success, as for need_reload,
  instead of after the loads in ev_add_path() and uev_update_path().

- In ev_add_path(), only compute force_readonly when need_do_map is
  set.

- Walk mpp->pg rather than mpp->paths in needs_ro_update(), since
  mpp->paths may be stale there. ev_add_path() and update_map() still
  use mpp->paths: both call adopt_paths() first, and mpp->pg doesn't
  contain the new paths until setup_map().

- Move sysfs_get_ro() above has_ro_path() instead of adding a forward
  declaration. The function is unchanged.

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

 libmultipath/devmapper.c |  26 ++++++--
 multipathd/main.c        | 128 +++++++++++++++++++++++++--------------
 2 files changed, 103 insertions(+), 51 deletions(-)

-- 
2.56.0