> 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 a load succeeds:
>
> - needs_ro_update() now sets force_readonly whenever it returns true:
> to 1 for a DISK_RO=1 uevent, to 0 when all of the map's paths are
> writable, and from the paths' state when the map has no dm info.
> uev_update_path() no longer sets or clears it itself.
>
> - 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() clears it after a successful load, as it does for
> need_reload, so it can't outlive the load it was set for.
>
> Signed-off-by: Brian Bunker <[email protected]>
>
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index e3870f5..c2939dc 100644
> --- a/libmultipath/devmapper.c
> +++ b/libmultipath/devmapper.c
> @@ -520,8 +520,10 @@ dm_addmap (int task, const char *target, struct
> multipath *mpp,
> if (task == DM_DEVICE_CREATE)
> libmp_udev_wait(cookie);
>
> - if (r)
> + if (r) {
> mpp->need_reload = false;
> + mpp->force_readonly = 0;
> + }
> return r;
> }
>
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 9bce4d6..605e24a 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,35 @@ 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))
> + if (!has_dm_info(mpp)) {
This should never happen. But as we test the condition,
wonder why we don't just call dm_get_info(mpp, &mpp->dmi) to obtain
the missing information?
If that fails, I am not sure it makes sense to return true any more.
It would indicate a severe problem with the map at hand.
> + mpp->force_readonly = 0;
> + vector_foreach_slot (mpp->pg, pgp, i) {
if (ro == 1), you can skip this loop, as you already know that there's one
ro path.
> + if (has_ro_path(pgp->paths)) {
> + mpp->force_readonly = 1;
> + break;
> + }
> + }
> return true;
> + }
> if (mpp->dmi.read_only == ro)
> return false;
Should mpp->force_readonly be reset here? After all, there seems to be no
need for reloading. That would be a similar condition as what you fix
by resetting force_readonly below.
Actually, as we don't seem to trust our internal state anyway, we might
simply obtain the sysfs attribute for all paths, always, and compare it
to dmi.read_only. That might be somewhat slow-ish, though.
The alternative would be to improve our internal tracking, e.g. by saving
a "ro" flag for paths when we add them or receive an uevents for them.
That way we could save the effort to check sysfs repeatedly.
Thanks,
Martin
--
Martin Wilck <[email protected]>
