From 10d0d9c7c468abf1144586bf50a118eb94d0c7b6 Mon Sep 17 00:00:00 2001 From: Alasdair Kergon Date: Tue, 27 Sep 2011 22:43:40 +0000 Subject: [PATCH] Introduce revert_lv for better pvmove cleanup. (One further fix needed to remove the stray pvmove LVs left behind.) --- WHATS_NEW | 1 + daemons/clvmd/lvm-functions.c | 5 +++-- lib/activate/activate.c | 15 +++++++++------ lib/activate/activate.h | 3 ++- lib/activate/dev_manager.c | 2 +- lib/locking/cluster_locking.c | 3 +++ lib/locking/file_locking.c | 5 +++-- lib/locking/locking.c | 16 +++++++++++++++- lib/locking/locking.h | 4 ++++ lib/locking/no_locking.c | 2 +- libdm/libdm-deptree.c | 2 ++ tools/pvmove.c | 20 +++++++++++++++----- 12 files changed, 59 insertions(+), 19 deletions(-) diff --git a/WHATS_NEW b/WHATS_NEW index e2eab96e8..93c2512b7 100644 --- a/WHATS_NEW +++ b/WHATS_NEW @@ -1,5 +1,6 @@ Version 2.02.89 - ================================== + Introduce revert_lv for better pvmove cleanup. Replace incomplete pvmove activation failure recovery code with a message. Abort if _finish_pvmove suspend_lvs fails instead of cleaning up incompletely. Change suspend_lvs to call vg_revert internally. diff --git a/daemons/clvmd/lvm-functions.c b/daemons/clvmd/lvm-functions.c index f16d006c9..aa3067091 100644 --- a/daemons/clvmd/lvm-functions.c +++ b/daemons/clvmd/lvm-functions.c @@ -406,7 +406,7 @@ error: /* Resume the LV if it was active */ static int do_resume_lv(char *resource, unsigned char lock_flags) { - int oldmode, origin_only, exclusive; + int oldmode, origin_only, exclusive, revert; /* Is it open ? */ oldmode = get_current_lock(resource); @@ -416,8 +416,9 @@ static int do_resume_lv(char *resource, unsigned char lock_flags) } origin_only = (lock_flags & LCK_ORIGIN_ONLY_MODE) ? 1 : 0; exclusive = (oldmode == LCK_EXCL) ? 1 : 0; + revert = (lock_flags & LCK_REVERT_MODE) ? 1 : 0; - if (!lv_resume_if_active(cmd, resource, origin_only, exclusive)) + if (!lv_resume_if_active(cmd, resource, origin_only, exclusive, revert)) return EIO; return 0; diff --git a/lib/activate/activate.c b/lib/activate/activate.c index 4ae419264..51862ac3d 100644 --- a/lib/activate/activate.c +++ b/lib/activate/activate.c @@ -190,7 +190,7 @@ int lv_resume(struct cmd_context *cmd, const char *lvid_s, unsigned origin_only) return 1; } int lv_resume_if_active(struct cmd_context *cmd, const char *lvid_s, - unsigned origin_only, unsigned exclusive) + unsigned origin_only, unsigned exclusive, unsigned revert) { return 1; } @@ -1356,14 +1356,16 @@ static int _lv_resume(struct cmd_context *cmd, const char *lvid_s, laopts->origin_only = 0; if (test_mode()) { - _skip("Resuming %s%s.", lv->name, laopts->origin_only ? " without snapshots" : ""); + _skip("Resuming %s%s%s.", lv->name, laopts->origin_only ? " without snapshots" : "", + laopts->revert ? " (reverting)" : ""); r = 1; goto out; } - log_debug("Resuming LV %s/%s%s%s.", lv->vg->name, lv->name, + log_debug("Resuming LV %s/%s%s%s%s.", lv->vg->name, lv->name, error_if_not_active ? "" : " if active", - laopts->origin_only ? " without snapshots" : ""); + laopts->origin_only ? " without snapshots" : "", + laopts->revert ? " (reverting)" : ""); if (!lv_info(cmd, lv, laopts->origin_only, &info, 0, 0)) goto_out; @@ -1395,7 +1397,7 @@ out: /* Returns success if the device is not active */ int lv_resume_if_active(struct cmd_context *cmd, const char *lvid_s, - unsigned origin_only, unsigned exclusive) + unsigned origin_only, unsigned exclusive, unsigned revert) { struct lv_activate_opts laopts = { .origin_only = origin_only, @@ -1404,7 +1406,8 @@ int lv_resume_if_active(struct cmd_context *cmd, const char *lvid_s, * non-clustered target should be used. This only happens * if exclusive is set. */ - .exclusive = exclusive + .exclusive = exclusive, + .revert = revert }; return _lv_resume(cmd, lvid_s, &laopts, 0); diff --git a/lib/activate/activate.h b/lib/activate/activate.h index 5b90420f8..3b3d738a8 100644 --- a/lib/activate/activate.h +++ b/lib/activate/activate.h @@ -34,6 +34,7 @@ struct lv_activate_opts { int exclusive; int origin_only; int no_merging; + unsigned revert; }; /* target attribute flags */ @@ -63,7 +64,7 @@ void activation_exit(void); int lv_suspend_if_active(struct cmd_context *cmd, const char *lvid_s, unsigned origin_only); int lv_resume(struct cmd_context *cmd, const char *lvid_s, unsigned origin_only); int lv_resume_if_active(struct cmd_context *cmd, const char *lvid_s, - unsigned origin_only, unsigned exclusive); + unsigned origin_only, unsigned exclusive, unsigned revert); int lv_activate(struct cmd_context *cmd, const char *lvid_s, int exclusive); int lv_activate_with_filter(struct cmd_context *cmd, const char *lvid_s, int exclusive); diff --git a/lib/activate/dev_manager.c b/lib/activate/dev_manager.c index 0a3c29c14..b364d9197 100644 --- a/lib/activate/dev_manager.c +++ b/lib/activate/dev_manager.c @@ -1590,7 +1590,7 @@ static int _add_new_lv_to_dtree(struct dev_manager *dm, struct dm_tree *dtree, layer ? UINT32_C(0) : (uint32_t) lv->major, layer ? UINT32_C(0) : (uint32_t) lv->minor, _read_only_lv(lv), - (lv->vg->status & PRECOMMITTED) ? 1 : 0, + ((lv->vg->status & PRECOMMITTED) | laopts->revert) ? 1 : 0, lvlayer, _get_udev_flags(dm, lv, layer)))) return_0; diff --git a/lib/locking/cluster_locking.c b/lib/locking/cluster_locking.c index 4401bc7b7..bd5fe4399 100644 --- a/lib/locking/cluster_locking.c +++ b/lib/locking/cluster_locking.c @@ -327,6 +327,9 @@ static int _lock_for_cluster(struct cmd_context *cmd, unsigned char clvmd_cmd, if (flags & LCK_ORIGIN_ONLY) args[1] |= LCK_ORIGIN_ONLY_MODE; + if (flags & LCK_REVERT) + args[1] |= LCK_REVERT_MODE; + if (mirror_in_sync()) args[1] |= LCK_MIRROR_NOSYNC_MODE; diff --git a/lib/locking/file_locking.c b/lib/locking/file_locking.c index 93c1e1bd7..468c8b500 100644 --- a/lib/locking/file_locking.c +++ b/lib/locking/file_locking.c @@ -257,6 +257,7 @@ static int _file_lock_resource(struct cmd_context *cmd, const char *resource, { char lockfile[PATH_MAX]; unsigned origin_only = (flags & LCK_ORIGIN_ONLY) ? 1 : 0; + unsigned revert = (flags & LCK_REVERT) ? 1 : 0; switch (flags & LCK_SCOPE_MASK) { case LCK_VG: @@ -292,8 +293,8 @@ static int _file_lock_resource(struct cmd_context *cmd, const char *resource, case LCK_LV: switch (flags & LCK_TYPE_MASK) { case LCK_UNLOCK: - log_very_verbose("Unlocking LV %s%s", resource, origin_only ? " without snapshots" : ""); - if (!lv_resume_if_active(cmd, resource, origin_only, 0)) + log_very_verbose("Unlocking LV %s%s%s", resource, origin_only ? " without snapshots" : "", revert ? " (reverting)" : ""); + if (!lv_resume_if_active(cmd, resource, origin_only, 0, revert)) return 0; break; case LCK_NULL: diff --git a/lib/locking/locking.c b/lib/locking/locking.c index 17fa3a8c6..7dfd5fca7 100644 --- a/lib/locking/locking.c +++ b/lib/locking/locking.c @@ -493,6 +493,20 @@ int resume_lvs(struct cmd_context *cmd, struct dm_list *lvs) return r; } +/* Unlock and revert list of LVs */ +int revert_lvs(struct cmd_context *cmd, struct dm_list *lvs) +{ + struct lv_list *lvl; + int r = 1; + + dm_list_iterate_items(lvl, lvs) + if (!revert_lv(cmd, lvl->lv)) { + r = 0; + stack; + } + + return r; +} /* * Lock a list of LVs. * On failure to lock any LV, calls vg_revert() if vg_to_revert is set and @@ -511,7 +525,7 @@ int suspend_lvs(struct cmd_context *cmd, struct dm_list *lvs, vg_revert(vg_to_revert); dm_list_uniterate(lvh, lvs, &lvl->list) { lvl = dm_list_item(lvh, struct lv_list); - if (!resume_lv(cmd, lvl->lv)) + if (!revert_lv(cmd, lvl->lv)) stack; } diff --git a/lib/locking/locking.h b/lib/locking/locking.h index 69b31ee09..1924440e5 100644 --- a/lib/locking/locking.h +++ b/lib/locking/locking.h @@ -97,6 +97,7 @@ int check_lvm1_vg_inactive(struct cmd_context *cmd, const char *vgname); #define LCK_CLUSTER_VG 0x00000080U /* VG is clustered */ #define LCK_CACHE 0x00000100U /* Operation on cache only using P_ lock */ #define LCK_ORIGIN_ONLY 0x00000200U /* Operation should bypass any snapshots */ +#define LCK_REVERT 0x00000400U /* Revert any incomplete change */ /* * Additional lock bits for cluster communication via args[1] @@ -107,6 +108,7 @@ int check_lvm1_vg_inactive(struct cmd_context *cmd, const char *vgname); #define LCK_CONVERT 0x08 /* Convert existing lock */ #define LCK_ORIGIN_ONLY_MODE 0x20 /* Same as above */ #define LCK_TEST_MODE 0x10 /* Test mode: No activation */ +#define LCK_REVERT_MODE 0x40 /* Remove inactive tables */ /* * Special cases of VG locks. @@ -164,6 +166,7 @@ int check_lvm1_vg_inactive(struct cmd_context *cmd, const char *vgname); #define resume_lv(cmd, lv) lock_lv_vol(cmd, lv, LCK_LV_RESUME) #define resume_lv_origin(cmd, lv) lock_lv_vol(cmd, lv, LCK_LV_RESUME | LCK_ORIGIN_ONLY) +#define revert_lv(cmd, lv) lock_lv_vol(cmd, lv, LCK_LV_RESUME | LCK_REVERT) #define suspend_lv(cmd, lv) lock_lv_vol(cmd, lv, LCK_LV_SUSPEND | LCK_HOLD) #define suspend_lv_origin(cmd, lv) lock_lv_vol(cmd, lv, LCK_LV_SUSPEND | LCK_HOLD | LCK_ORIGIN_ONLY) #define deactivate_lv(cmd, lv) lock_lv_vol(cmd, lv, LCK_LV_DEACTIVATE) @@ -191,6 +194,7 @@ struct volume_group; int suspend_lvs(struct cmd_context *cmd, struct dm_list *lvs, struct volume_group *vg_to_revert); int resume_lvs(struct cmd_context *cmd, struct dm_list *lvs); +int revert_lvs(struct cmd_context *cmd, struct dm_list *lvs); int activate_lvs(struct cmd_context *cmd, struct dm_list *lvs, unsigned exclusive); /* Interrupt handling */ diff --git a/lib/locking/no_locking.c b/lib/locking/no_locking.c index 212eb4dcf..b8ee61375 100644 --- a/lib/locking/no_locking.c +++ b/lib/locking/no_locking.c @@ -46,7 +46,7 @@ static int _no_lock_resource(struct cmd_context *cmd, const char *resource, case LCK_NULL: return lv_deactivate(cmd, resource); case LCK_UNLOCK: - return lv_resume_if_active(cmd, resource, (flags & LCK_ORIGIN_ONLY) ? 1: 0, 0); + return lv_resume_if_active(cmd, resource, (flags & LCK_ORIGIN_ONLY) ? 1: 0, 0, (flags & LCK_REVERT) ? 1 : 0); case LCK_READ: return lv_activate_with_filter(cmd, resource, 0); case LCK_WRITE: diff --git a/libdm/libdm-deptree.c b/libdm/libdm-deptree.c index c38f15c87..5cf98bca3 100644 --- a/libdm/libdm-deptree.c +++ b/libdm/libdm-deptree.c @@ -610,6 +610,8 @@ static int _node_clear_table(struct dm_tree_node *dnode) if (!info->exists || !info->inactive_table) return 1; +// FIXME Get inactive deps. If any dev referenced has 1 opener and no live table, remove it after the clear. + log_verbose("Clearing inactive table %s (%" PRIu32 ":%" PRIu32 ")", name, info->major, info->minor); diff --git a/tools/pvmove.c b/tools/pvmove.c index e603086f6..4a67e481e 100644 --- a/tools/pvmove.c +++ b/tools/pvmove.c @@ -370,13 +370,19 @@ static int _update_metadata(struct cmd_context *cmd, struct volume_group *vg, if (!_suspend_lvs(cmd, first_time, lv_mirr, lvs_changed, vg)) { log_error("ABORTING: Volume group metadata update failed."); - goto out; + if (!first_time && !revert_lv(cmd, lv_mirr)) + stack; + return 0; } /* Commit on-disk metadata */ if (!vg_commit(vg)) { log_error("ABORTING: Volume group metadata update failed."); - goto out; + if (!_resume_lvs(cmd, first_time, lv_mirr, lvs_changed)) + stack; + if (!first_time && !revert_lv(cmd, lv_mirr)) + stack; + return 0; } /* Activate the temporary mirror LV */ @@ -403,7 +409,9 @@ out: if (!_resume_lvs(cmd, first_time, lv_mirr, lvs_changed)) r = 0; - backup(vg); + if (r) + backup(vg); + return r; } @@ -546,6 +554,8 @@ static int _finish_pvmove(struct cmd_context *cmd, struct volume_group *vg, /* Suspend LVs changed (implicitly suspends lv_mirr) */ if (!suspend_lvs(cmd, lvs_changed, vg)) { log_error("ABORTING: Locking LVs to remove temporary mirror failed"); + if (!revert_lv(cmd, lv_mirr)) + stack; return 0; } @@ -553,9 +563,9 @@ static int _finish_pvmove(struct cmd_context *cmd, struct volume_group *vg, if (!vg_commit(vg)) { log_error("ABORTING: Failed to write new data locations " "to disk."); - if (!resume_lv(cmd, lv_mirr)) + if (!revert_lv(cmd, lv_mirr)) stack; - if (!resume_lvs(cmd, lvs_changed)) + if (!revert_lvs(cmd, lvs_changed)) stack; return 0; }