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() no longer takes the path's ro state 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. It just calls needs_ro_update(), which reads the ro state of all of the map's paths from sysfs. - needs_ro_update() no longer takes an ro value. It sets force_readonly from the ro state of the map's paths, and returns true if that differs from the map's current state. If the map is already read-only and has to stay so, it clears the flag and returns false, since dm has to reopen the paths for any read-write reload anyway. If the map has no dm info, it fetches it, and returns false if that fails, instead of assuming a reload is needed. Since the flag is recomputed on every call, a failed read-only reload is retried on the next uevent without any special handling. - 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 <[email protected]> --- libmultipath/devmapper.c | 18 ++++- multipathd/main.c | 144 ++++++++++++++++++++++++--------------- 2 files changed, 105 insertions(+), 57 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..f3b0cb5f 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, ¶ms, 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; @@ -1594,27 +1630,27 @@ finish_path_init(struct path *pp, struct vectors * vecs) } static bool -needs_ro_update(struct multipath *mpp, int ro) +needs_ro_update(struct multipath *mpp) { - struct pathgroup * pgp; - struct path * pp; - unsigned int i, j; - - if (!mpp || ro < 0) + if (!mpp) 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; - } } - return true; + mpp->force_readonly = 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. + */ + if (mpp->force_readonly && mpp->dmi.read_only) { + mpp->force_readonly = 0; + return false; + } + /* The current map state doesn't match the paths' state. Update it. */ + return (mpp->force_readonly != mpp->dmi.read_only); } int resize_map(struct multipath *mpp, unsigned long long size, @@ -1652,7 +1688,7 @@ out: static int uev_update_path (struct uevent *uev, struct vectors * vecs) { - int ro, retval = 0, rc; + int retval = 0, rc; struct path * pp; struct config *conf; int needs_reinit = 0; @@ -1725,22 +1761,22 @@ uev_update_path (struct uevent *uev, struct vectors * vecs) pthread_cleanup_pop(1); } - ro = uevent_get_disk_ro(uev); - if (needs_ro_update(mpp, ro)) { - condlog(2, "%s: update path write_protect to '%d' (uevent)", uev->kernel, ro); + /* + * The uevent only tells us that something changed. Check the + * paths' ro state in sysfs, which the kernel updates before + * sending the uevent, instead of trusting the uevent's DISK_RO. + */ + if (needs_ro_update(mpp)) { + condlog(2, "%s: update path write_protect to '%d' (uevent)", uev->kernel, mpp->force_readonly); 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
