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 <[email protected]> --- libmultipath/devmapper.c | 17 +++++-- multipathd/main.c | 104 +++++++++++++++++++++++---------------- 2 files changed, 75 insertions(+), 46 deletions(-) diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c index e3870f59..c994715e 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; + /* * DM_DEVICE_RELOAD cannot wait on a cookie, as * the cookie will only ever be released after an * DM_DEVICE_RESUME. So call DM_DEVICE_RESUME * after each successful call to DM_DEVICE_RELOAD. */ - if (!mpp->force_readonly) + if (!force_ro) r = dm_addmap(DM_DEVICE_RELOAD, TGT_MPATH, mpp, params, ADDMAP_RW, 0); if (!r) { - if (!mpp->force_readonly && errno != EROFS) + if (!force_ro && errno != EROFS) return 0; r = dm_addmap(DM_DEVICE_RELOAD, TGT_MPATH, mpp, params, ADDMAP_RO, 0); diff --git a/multipathd/main.c b/multipathd/main.c index 9bce4d6b..c5788ef9 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -709,6 +709,42 @@ 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 int update_map (struct multipath *mpp, struct vectors *vecs, int new_map) { @@ -732,6 +768,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, ¶ms, vecs)) { condlog(0, "%s: failed to setup new map in update", mpp->alias); retries = -1; @@ -1270,28 +1314,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 +1327,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 +1391,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; @@ -1597,23 +1620,24 @@ static bool needs_ro_update(struct multipath *mpp, int ro) { struct pathgroup * pgp; - struct path * pp; - unsigned int i, j; + unsigned int i; if (!mpp || ro < 0) return false; - if (!has_dm_info(mpp)) - return true; + 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 (mpp->dmi.read_only == ro) 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) + if (ro == 0) { + vector_foreach_slot (mpp->pg, pgp, i) { + if (has_ro_path(pgp->paths)) return false; } } + mpp->force_readonly = ro; return true; } @@ -1732,15 +1756,11 @@ 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; 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
