Abort if _finish_pvmove suspend_lvs fails instead of cleaning up incompletely.

Change suspend_lvs to call vg_revert internally.
Change vg_revert to void and remove superfluous calls after failed vg_commit.

WHATS_NEW

@@ -1,5 +1,8 @@
 Version 2.02.89 - 
 ==================================
+  Abort if _finish_pvmove suspend_lvs fails instead of cleaning up incompletely.
+  Change suspend_lvs to call vg_revert internally.
+  Change vg_revert to void and remove superfluous calls after failed vg_commit.
   Use execvp for CLVMD restart to preserve environment settings.
   Restart CLVMD with same cluster manager.
   Fix log_error() usage in raid and unknown segtype initialisation.

lib/locking/locking.c

@@ -493,8 +493,13 @@ int resume_lvs(struct cmd_context *cmd, struct dm_list *lvs)
 	return r;
 }
 
-/* Lock a list of LVs */
+/*
-int suspend_lvs(struct cmd_context *cmd, struct dm_list *lvs)
+ * Lock a list of LVs.
+ * On failure to lock any LV, calls vg_revert() if vg_to_revert is set and 
+ * then unlocks any LVs on the list already successfully locked.
+ */
+int suspend_lvs(struct cmd_context *cmd, struct dm_list *lvs,
+		struct volume_group *vg_to_revert)
 {
 	struct dm_list *lvh;
 	struct lv_list *lvl;
@@ -502,6 +507,8 @@ int suspend_lvs(struct cmd_context *cmd, struct dm_list *lvs)
 	dm_list_iterate_items(lvl, lvs) {
 		if (!suspend_lv(cmd, lvl->lv)) {
 			log_error("Failed to suspend %s", lvl->lv->name);
+			if (vg_to_revert)
+				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))

lib/locking/locking.h

@@ -187,7 +187,9 @@ int sync_local_dev_names(struct cmd_context* cmd);
 int sync_dev_names(struct cmd_context* cmd);
 
 /* Process list of LVs */
-int suspend_lvs(struct cmd_context *cmd, struct dm_list *lvs);
+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 activate_lvs(struct cmd_context *cmd, struct dm_list *lvs, unsigned exclusive);
 

lib/metadata/lv_manip.c

@@ -2793,11 +2793,8 @@ int lv_rename(struct cmd_context *cmd, struct logical_volume *lv,
 	if (!vg_write(vg))
 		return 0;
 
-
+	if (!suspend_lvs(cmd, &lvs_changed, vg))
-	if (!suspend_lvs(cmd, &lvs_changed)) {
-		vg_revert(vg);
 		goto_out;
-	}
 
 	if (!(r = vg_commit(vg)))
 		stack;

lib/metadata/metadata-exported.h

@@ -400,7 +400,7 @@ void pvcreate_params_set_defaults(struct pvcreate_params *pp);
 */
 int vg_write(struct volume_group *vg);
 int vg_commit(struct volume_group *vg);
-int vg_revert(struct volume_group *vg);
+void vg_revert(struct volume_group *vg);
 struct volume_group *vg_read_internal(struct cmd_context *cmd, const char *vg_name,
 				      const char *vgid, int warnings, int *consistent);
 struct physical_volume *pv_read(struct cmd_context *cmd, const char *pv_name,

lib/metadata/metadata.c

@@ -2685,7 +2685,7 @@ int vg_commit(struct volume_group *vg)
 }
 
 /* Don't commit any pending changes */
-int vg_revert(struct volume_group *vg)
+void vg_revert(struct volume_group *vg)
 {
 	struct metadata_area *mda;
 
@@ -2701,8 +2701,6 @@ int vg_revert(struct volume_group *vg)
 			  "after reverted update for VG %s.", vg->name);
 
 	remote_revert_cached_metadata(vg);
-
-	return 1;
 }
 
 /* Make orphan PVs look like a VG */

tools/pvmove.c

@@ -307,17 +307,21 @@ static int _detach_pvmove_mirror(struct cmd_context *cmd,
 
 static int _suspend_lvs(struct cmd_context *cmd, unsigned first_time,
 			struct logical_volume *lv_mirr,
-			struct dm_list *lvs_changed)
+			struct dm_list *lvs_changed,
+			struct volume_group *vg_to_revert)
 {
 	/*
 	 * Suspend lvs_changed the first time.
 	 * Suspend mirrors on subsequent calls.
 	 */
 	if (first_time) {
-		if (!suspend_lvs(cmd, lvs_changed))
+		if (!suspend_lvs(cmd, lvs_changed, vg_to_revert))
 			return_0;
-	} else if (!suspend_lv(cmd, lv_mirr))
+	} else if (!suspend_lv(cmd, lv_mirr)) {
+		if (vg_to_revert)
+			vg_revert(vg_to_revert);
 		return_0;
+	}
 
 	return 1;
 }
@@ -364,16 +368,14 @@ static int _update_metadata(struct cmd_context *cmd, struct volume_group *vg,
 		return 0;
 	}
 
-	if (!_suspend_lvs(cmd, first_time, lv_mirr, lvs_changed)) {
+	if (!_suspend_lvs(cmd, first_time, lv_mirr, lvs_changed, vg)) {
-		/* FIXME vg_revert must be moved *before* any LV resumes */
+		log_error("ABORTING: Volume group metadata update failed.");
-		vg_revert(vg);
+		goto out;
-		goto_out;
 	}
 
 	/* Commit on-disk metadata */
 	if (!vg_commit(vg)) {
 		log_error("ABORTING: Volume group metadata update failed.");
-		vg_revert(vg);
 		goto out;
 	}
 
@@ -555,16 +557,15 @@ 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)) {
+	if (!suspend_lvs(cmd, lvs_changed, vg)) {
-		log_error("Locking LVs to remove temporary mirror failed");
+		log_error("ABORTING: Locking LVs to remove temporary mirror failed");
-		r = 0;
+		return 0;
 	}
 
 	/* Store metadata without dependencies on mirror segments */
 	if (!vg_commit(vg)) {
 		log_error("ABORTING: Failed to write new data locations "
 			  "to disk.");
-		vg_revert(vg);
 		if (!resume_lv(cmd, lv_mirr))
 			stack;
 		if (!resume_lvs(cmd, lvs_changed))