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

[Brian Bunker <brian@purestorage.com>]

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
once the load has succeeded:

- uev_update_path() takes the path's ro state from sysfs instead of
  from the uevent's DISK_RO. The uevent only says that something
  changed. The kernel updates the ro attribute before it sends the
  uevent, so sysfs is current when the uevent is handled, while the
  uevent itself may have been queued behind later changes. If sysfs
  can't be read, nothing is done.

- needs_ro_update() now sets force_readonly whenever it returns true:
  to 1 when the path is read-only, and to 0 when all of the map's paths
  are writable. uev_update_path() no longer sets or clears it itself.
  When needs_ro_update() returns false, it clears the flag, unless the
  map is read-write while one of its paths is still read-only. In that
  case an earlier read-only reload failed, and the flag stays set so
  that the next reload tries again. 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() clears it as soon as it has read it. A failed
  create leaves no map behind, and the next create computes the flag
  again.

- dm_addmap_reload() clears it once the reload and resume have
  succeeded. If the reload fails, the map keeps its old table, and the
  flag stays set so that the next reload tries again.

Signed-off-by: Brian Bunker <brian@purestorage.com>
---
 libmultipath/devmapper.c |  18 ++++-
 multipathd/main.c        | 144 ++++++++++++++++++++++++++-------------
 2 files changed, 111 insertions(+), 51 deletions(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index e3870f59..34e0f640 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,
@@ -599,8 +604,15 @@ int dm_addmap_reload(struct multipath *mpp, char *params, int flush)
 		r = dm_simplecmd(DM_DEVICE_RESUME, mpp->alias,
 				 DMFL_NEED_SYNC | (flush ? 0 : DMFL_NO_FLUSH),
 				 udev_flags);
-	if (r)
+	if (r) {
+		/*
+		 * force_readonly only applies to this reload. If the reload
+		 * fails, the map keeps its old table, and the flag stays set
+		 * so that the next reload tries again.
+		 */
+		mpp->force_readonly = 0;
 		return r;
+	}
 
 	/* If the resume failed, dm will leave the device suspended, and
 	 * drop the new table, so doing a second resume will try using
diff --git a/multipathd/main.c b/multipathd/main.c
index 9bce4d6b..1100584f 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -709,6 +709,55 @@ void cleanup_reset_vec(struct vector_s **v)
 	vector_reset(*v);
 }
 
+static int
+sysfs_get_ro (struct path *pp)
+{
+	int ro;
+	char buff[3]; /* Either "0\n\0" or "1\n\0" */
+
+	if (!pp->udev)
+		return -1;
+
+	if (!sysfs_attr_get_value_ok(pp->udev, "ro", buff, sizeof(buff))) {
+		condlog(3, "%s: Cannot read ro attribute in sysfs", pp->dev);
+		return -1;
+	}
+
+	if (sscanf(buff, "%d\n", &ro) != 1 || ro < 0 || ro > 1) {
+		condlog(3, "%s: Cannot parse ro attribute", pp->dev);
+		return -1;
+	}
+
+	return ro;
+}
+
+/* A map must be loaded read-only if any of its paths is read-only. */
+static bool
+has_ro_path(vector paths)
+{
+	struct path *pp;
+	unsigned int i;
+
+	vector_foreach_slot (paths, pp, i) {
+		if (sysfs_get_ro(pp) == 1)
+			return true;
+	}
+	return false;
+}
+
+static bool
+map_has_ro_path(struct multipath *mpp)
+{
+	struct pathgroup *pgp;
+	unsigned int i;
+
+	vector_foreach_slot (mpp->pg, pgp, i) {
+		if (has_ro_path(pgp->paths))
+			return true;
+	}
+	return false;
+}
+
 static int
 update_map (struct multipath *mpp, struct vectors *vecs, int new_map)
 {
@@ -732,6 +781,14 @@ 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->paths is up to date
+	 * after adopt_paths(), while mpp->pg isn't until setup_map().
+	 */
+	mpp->force_readonly = has_ro_path(mpp->paths);
+
 	if (setup_map(mpp, &params, vecs)) {
 		condlog(0, "%s: failed to setup new map in update", mpp->alias);
 		retries = -1;
@@ -1270,28 +1327,6 @@ out:
 	return ret;
 }
 
-static int
-sysfs_get_ro (struct path *pp)
-{
-	int ro;
-	char buff[3]; /* Either "0\n\0" or "1\n\0" */
-
-	if (!pp->udev)
-		return -1;
-
-	if (!sysfs_attr_get_value_ok(pp->udev, "ro", buff, sizeof(buff))) {
-		condlog(3, "%s: Cannot read ro attribute in sysfs", pp->dev);
-		return -1;
-	}
-
-	if (sscanf(buff, "%d\n", &ro) != 1 || ro < 0 || ro > 1) {
-		condlog(3, "%s: Cannot parse ro attribute", pp->dev);
-		return -1;
-	}
-
-	return ro;
-}
-
 /*
  * returns:
  * 0: added
@@ -1305,7 +1340,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,14 +1404,16 @@ 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;
-
 	if (!need_do_map)
 		return 0;
 
+	/*
+	 * Load the map read-only if any of its paths is read-only, not
+	 * just the one being added. mpp->paths is up to date after
+	 * adopt_paths(), while mpp->pg doesn't have the new paths yet.
+	 */
+	mpp->force_readonly = has_ro_path(mpp->paths);
+
 	if (!dm_map_present(mpp->alias)) {
 		mpp->action = ACT_CREATE;
 		start_waiter = 1;
@@ -1596,24 +1632,34 @@ finish_path_init(struct path *pp, struct vectors * vecs)
 static bool
 needs_ro_update(struct multipath *mpp, int ro)
 {
-	struct pathgroup * pgp;
-	struct path * pp;
-	unsigned int i, j;
-
 	if (!mpp || ro < 0)
 		return false;
-	if (!has_dm_info(mpp))
-		return true;
-	if (mpp->dmi.read_only == ro)
+	if (!has_dm_info(mpp) &&
+	    dm_get_info(mpp->alias, &mpp->dmi) != DMP_OK) {
+		condlog(2, "%s: failed to get dm info", mpp->alias);
 		return false;
-	if (ro == 1)
-		return true;
-	vector_foreach_slot (mpp->pg, pgp, i) {
-		vector_foreach_slot (pgp->paths, pp, j) {
-			if (sysfs_get_ro(pp) == 1)
-				return false;
-		}
 	}
+	if (mpp->dmi.read_only == ro) {
+		/*
+		 * No reload is needed. A failed read-only reload leaves
+		 * force_readonly set while the map is still read-write. Keep
+		 * it only while one of the map's paths is still read-only,
+		 * so that the next reload tries again.
+		 */
+		if (mpp->force_readonly && (ro == 1 || !map_has_ro_path(mpp)))
+			mpp->force_readonly = 0;
+		return false;
+	}
+	if (ro == 0 && map_has_ro_path(mpp)) {
+		/*
+		 * The map is read-only and has to stay so. dm has to reopen
+		 * its paths for any read-write reload anyway, so the flag
+		 * isn't needed.
+		 */
+		mpp->force_readonly = 0;
+		return false;
+	}
+	mpp->force_readonly = ro;
 	return true;
 }
 
@@ -1725,22 +1771,24 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
 			pthread_cleanup_pop(1);
 		}
 
-		ro = uevent_get_disk_ro(uev);
+		/*
+		 * The uevent only tells us that something changed. Take the
+		 * path's ro state from sysfs, which the kernel updates before
+		 * sending the uevent. If sysfs can't be read, don't act on
+		 * the uevent's DISK_RO either.
+		 */
+		ro = sysfs_get_ro(pp);
 		if (needs_ro_update(mpp, ro)) {
 			condlog(2, "%s: update path write_protect to '%d' (uevent)", uev->kernel, ro);
 
 			if (mpp->wait_for_udev != UDEV_WAIT_DONE)
 				mpp->wait_for_udev = UDEV_WAIT_RELOAD;
 			else {
-				if (ro == 1)
-					pp->mpp->force_readonly = 1;
 				retval = reload_and_sync_map(mpp, vecs);
 				if (retval == 2)
 					condlog(2, "%s: map removed during reload", pp->dev);
-				else {
-					pp->mpp->force_readonly = 0;
+				else
 					condlog(2, "%s: map %s reloaded (retval %d)", uev->kernel, mpp->alias, retval);
-				}
 			}
 		}
 		if (auto_resize != AUTO_RESIZE_NEVER && mpp &&
-- 
2.56.0