> 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]>
Almost there. One remark below.
>
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index e3870f5..c994715 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;
> +
One minor issue here. If the reload below fails, the map is still read-write
in the kernel. As we reset force_readonly, it will remain so, at least until
we get another event with RO=1.
If the RO reload fails with force_readonly=1, we could set force_readonly to 1
again
after the failure. In needs_ro_update(), we'd then need to modify the fast
return clause "if (mpp->dmi.read_only == ro)"; if both are 0 and force_readonly
is
still set, you'd need to check the paths and see if there's still a read-only
path
around.
Martin
--
Martin Wilck <[email protected]>
