[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

[lvm-devel] [PATCH] (3/3) new vgreduce --removemissing semantics



Hi,

after a discussion with Alasdair, we have concluded that instead of using
"--consolidate" (which is not quite clear) for the new, safer semantics, we
will hijack existing --removemissing switch. To get the old (LV-removing)
behaviour, --force needs to be specified as well: the current --removemissing
handler will advise users about that (together with list of LVs that will need
to be removed or downconverted).

Note this breaks dmeventd as it is, but we likely want to use lvconvert
--repair there nevertheless and not vgreduce.

                       
Fri Jul 11 14:49:59 CEST 2008  me mornfall net
  tagged hotspare: vgreduce --removemissing 1
Fri Jul 11 14:49:00 CEST 2008  me mornfall net
  tagged hotspare: handles_missing_pvs 1
Fri Jul 11 13:06:37 CEST 2008  me mornfall net
  * Use the "repairing" alias for arg_count(cmd, removemissing...).
Fri Jul 11 12:58:37 CEST 2008  me mornfall net
  * Fix vgreduce help string.
Fri Jul 11 12:53:20 CEST 2008  me mornfall net
  * If there are PARTIAL_LVs, we *will* end up writing out metadata with MISSING_PVs in them.
Fri Jul 11 12:53:02 CEST 2008  me mornfall net
  * Only print "wrote out consistent vg ..." when we actually fixed it.
Fri Jul 11 12:47:43 CEST 2008  me mornfall net
  * The _consolidate_vg call needs consistent = 1 to be passed to vg_read.
Fri Jul 11 12:47:11 CEST 2008  me mornfall net
  * Improve log_warn formatting.
Fri Jul 11 12:46:55 CEST 2008  me mornfall net
  * Fix typo in warning.
Fri Jul 11 12:29:47 CEST 2008  me mornfall net
  * Drop unused variables.
Fri Jul 11 12:27:46 CEST 2008  me mornfall net
  * Drop --consolidate, use --removemissing, --removemissing --force.
  
  Now vgreduce --removemissing will warn about partial LVs and only remove
  *empty* missing PVs. The old behaviour can be retained through --removemissing
  --force for now (ie. remove/downconvert LVs and then remove PVs).
Thu May  8 11:31:58 CEST 2008  me mornfall net
  * Fix flag validation for --consolidate.
Thu May  8 11:24:17 CEST 2008  me mornfall net
  * Implement --consolidate, use it from --removemissing to share code.
Thu May  8 11:19:47 CEST 2008  me mornfall net
  * Share code with --removemissing, stub out functionality (for --consolidate).
Thu May  8 11:16:11 CEST 2008  me mornfall net
  * Flag validation in vgreduce for --consolidate.
Thu May  8 11:13:17 CEST 2008  me mornfall net
  * Add a --consolidate argument for vgreduce.
Sun Apr 20 10:17:05 CEST 2008  me mornfall net
  * Check for MISSING_PV in vgreduce --removemissing.
Fri Feb 15 09:49:42 CET 2008  me mornfall net
  * Fix vgreduce --removemissing for consistent partial VGs.
diff -rN -p -u old-vgreduce-removemissing/man/vgreduce.8 new-vgreduce-removemissing/man/vgreduce.8
--- old-vgreduce-removemissing/man/vgreduce.8	2008-07-11 15:21:54.856004930 +0200
+++ new-vgreduce-removemissing/man/vgreduce.8	2008-07-11 15:21:55.028007891 +0200
@@ -18,11 +18,13 @@ See \fBlvm\fP for common options.
 Removes all empty physical volumes if none are given on command line.
 .TP
 .I \-\-removemissing
-Removes all missing physical volumes from the volume group and makes 
-the volume group consistent again.  
+Removes all missing physical volumes from the volume group, if there are no
+logical volumes allocated on those. This resumes normal operation of the volume
+group (new logical volumes may again be created, changed and so on).
 
-It's a good idea to run this option with --test first to find out what it 
-would remove before running it for real.  
+If this is not possible (there are logical volumes referencing the missing
+physical volumes) and you cannot or do not want to remove them manually, you
+can run this option with --force to have vgreduce remove any partial LVs.
 
 Any logical volumes and dependent snapshots that were partly on the 
 missing disks get removed completely. This includes those parts 
diff -rN -p -u old-vgreduce-removemissing/tools/commands.h new-vgreduce-removemissing/tools/commands.h
--- old-vgreduce-removemissing/tools/commands.h	2008-07-11 15:21:54.980008483 +0200
+++ new-vgreduce-removemissing/tools/commands.h	2008-07-11 15:21:55.024008691 +0200
@@ -850,13 +850,15 @@ xx(vgreduce,
    "\t[-h|--help]\n"
    "\t[--mirrorsonly]\n"
    "\t[--removemissing]\n"
+   "\t[-f|--force]\n"
    "\t[-t|--test]\n"
    "\t[-v|--verbose]\n"
    "\t[--version]" "\n"
    "\tVolumeGroupName\n"
    "\t[PhysicalVolumePath...]\n",
 
-   all_ARG, autobackup_ARG, mirrorsonly_ARG, removemissing_ARG, test_ARG)
+   all_ARG, autobackup_ARG, mirrorsonly_ARG, removemissing_ARG,
+   force_ARG, test_ARG)
 
 xx(vgremove,
    "Remove volume group(s)",
diff -rN -p -u old-vgreduce-removemissing/tools/vgreduce.c new-vgreduce-removemissing/tools/vgreduce.c
--- old-vgreduce-removemissing/tools/vgreduce.c	2008-07-11 15:21:54.980008483 +0200
+++ new-vgreduce-removemissing/tools/vgreduce.c	2008-07-11 15:21:55.020009561 +0200
@@ -16,7 +16,7 @@
 #include "tools.h"
 #include "lv_alloc.h"
 
-static int _remove_pv(struct volume_group *vg, struct pv_list *pvl)
+static int _remove_pv(struct volume_group *vg, struct pv_list *pvl, int silent)
 {
 	char uuid[64] __attribute((aligned(8)));
 
@@ -31,8 +31,9 @@ static int _remove_pv(struct volume_grou
 	log_verbose("Removing PV with UUID %s from VG %s", uuid, vg->name);
 
 	if (pvl->pv->pe_alloc_count) {
-		log_error("LVs still present on PV with UUID %s: Can't remove "
-			  "from VG %s", uuid, vg->name);
+		if (!silent)
+			log_error("LVs still present on PV with UUID %s: "
+				  "Can't remove from VG %s", uuid, vg->name);
 		return 0;
 	}
 
@@ -130,11 +131,41 @@ static int _remove_lv(struct cmd_context
 	return 1;
 }
 
+static int _consolidate_vg(struct cmd_context *cmd, struct volume_group *vg)
+{
+	struct pv_list *pvl;
+	struct lv_list *lvl;
+	int err = 0;
+
+	list_iterate_items(lvl, &vg->lvs) {
+		if (lvl->lv->status & PARTIAL_LV) {
+			log_warn("WARNING: Partial LV %s needs to be repaired "
+				 "or removed. ", lvl->lv->name);
+			err = 1;
+		}
+	}
+
+	if (err) {
+		cmd->handles_missing_pvs = 1;
+		log_warn("WARNING: There are still partial LVs in the VG. To "
+			 "unconditionally remove them,");
+		log_warn("         use vgreduce --removemissing --force.");
+		log_warn("Proceeding to remove empty missing PVs.");
+	}
+
+	list_iterate_items(pvl, &vg->pvs) {
+		if (pvl->pv->dev && !(pvl->pv->status & MISSING_PV))
+			continue;
+		if (!err && !_remove_pv(vg, pvl, err))
+			return_0;
+	}
+
+	return !err;
+}
+
 static int _make_vg_consistent(struct cmd_context *cmd, struct volume_group *vg)
 {
-	struct list *pvh, *pvht;
 	struct list *lvh, *lvht;
-	struct pv_list *pvl;
 	struct lv_list *lvl, *lvl2, *lvlt;
 	struct logical_volume *lv;
 	struct physical_volume *pv;
@@ -183,20 +214,8 @@ static int _make_vg_consistent(struct cm
 		return 0;
 	}
 
-	/* Remove missing PVs */
-	list_iterate_safe(pvh, pvht, &vg->pvs) {
-		pvl = list_item(pvh, struct pv_list);
-		if (pvl->pv->dev)
-			continue;
-		if (!_remove_pv(vg, pvl))
-			return_0;
-	}
-
-	/* VG is now consistent */
-	vg->status &= ~PARTIAL_VG;
-	vg->status |= LVM_WRITE;
-
-	init_partial(0);
+	if (!_consolidate_vg(cmd, vg))
+		return_0;
 
 	/* FIXME Recovery.  For now people must clean up by hand. */
 
@@ -441,26 +460,26 @@ int vgreduce(struct cmd_context *cmd, in
 	char *vg_name;
 	int ret = 1;
 	int consistent = 1;
+	int fixed = 1;
+	int repairing = arg_count(cmd, removemissing_ARG);
 
-	if (!argc && !arg_count(cmd, removemissing_ARG)) {
+	if (!argc && !repairing) {
 		log_error("Please give volume group name and "
 			  "physical volume paths");
 		return EINVALID_CMD_LINE;
 	}
-
-	if (!argc && arg_count(cmd, removemissing_ARG)) {
+	
+	if (!argc && repairing) {
 		log_error("Please give volume group name");
 		return EINVALID_CMD_LINE;
 	}
 
-	if (arg_count(cmd, mirrorsonly_ARG) &&
-	    !arg_count(cmd, removemissing_ARG)) {
+	if (arg_count(cmd, mirrorsonly_ARG) && !repairing) {
 		log_error("--mirrorsonly requires --removemissing");
 		return EINVALID_CMD_LINE;
 	}
 
-	if (argc == 1 && !arg_count(cmd, all_ARG)
-	    && !arg_count(cmd, removemissing_ARG)) {
+	if (argc == 1 && !arg_count(cmd, all_ARG) && !repairing) {
 		log_error("Please enter physical volume paths or option -a");
 		return EINVALID_CMD_LINE;
 	}
@@ -471,7 +490,7 @@ int vgreduce(struct cmd_context *cmd, in
 		return EINVALID_CMD_LINE;
 	}
 
-	if (argc > 1 && arg_count(cmd, removemissing_ARG)) {
+	if (argc > 1 && repairing) {
 		log_error("Please only specify the volume group");
 		return EINVALID_CMD_LINE;
 	}
@@ -492,8 +511,8 @@ int vgreduce(struct cmd_context *cmd, in
 		return ECMD_FAILED;
 	}
 
-	if ((!(vg = vg_read(cmd, vg_name, NULL, &consistent)) || !consistent) &&
-	    !arg_count(cmd, removemissing_ARG)) {
+	if ((!(vg = vg_read(cmd, vg_name, NULL, &consistent)) || !consistent)
+	    && !repairing) {
 		log_error("Volume group \"%s\" doesn't exist", vg_name);
 		unlock_vg(cmd, vg_name);
 		return ECMD_FAILED;
@@ -504,8 +523,8 @@ int vgreduce(struct cmd_context *cmd, in
 		return ECMD_FAILED;
 	}
 
-	if (arg_count(cmd, removemissing_ARG)) {
-		if (vg && consistent) {
+	if (repairing) {
+		if (vg && consistent && !vg_missing_pv_count(vg)) {
 			log_error("Volume group \"%s\" is already consistent",
 				  vg_name);
 			unlock_vg(cmd, vg_name);
@@ -513,7 +532,7 @@ int vgreduce(struct cmd_context *cmd, in
 		}
 
 		init_partial(1);
-		consistent = 0;
+		consistent = !arg_count(cmd, force_ARG);
 		if (!(vg = vg_read(cmd, vg_name, NULL, &consistent))) {
 			log_error("Volume group \"%s\" not found", vg_name);
 			unlock_vg(cmd, vg_name);
@@ -529,10 +548,14 @@ int vgreduce(struct cmd_context *cmd, in
 			return ECMD_FAILED;
 		}
 
-		if (!_make_vg_consistent(cmd, vg)) {
-			init_partial(0);
-			unlock_vg(cmd, vg_name);
-			return ECMD_FAILED;
+		if (arg_count(cmd, force_ARG)) {
+			if (!_make_vg_consistent(cmd, vg)) {
+				init_partial(0);
+				unlock_vg(cmd, vg_name);
+				return ECMD_FAILED;
+			}
+		} else {
+			fixed = _consolidate_vg(cmd, vg);
 		}
 
 		if (!vg_write(vg) || !vg_commit(vg)) {
@@ -544,7 +567,9 @@ int vgreduce(struct cmd_context *cmd, in
 
 		backup(vg);
 
-		log_print("Wrote out consistent volume group %s", vg_name);
+		if (fixed)
+			log_print("Wrote out consistent volume group %s",
+				  vg_name);
 
 	} else {
 		if (!vg_check_status(vg, EXPORTED_VG | LVM_WRITE | RESIZEABLE_VG)) {

Yours,
   Petr.

PS: About the remaining 2 patches in the set: partial activation did not change
yet (it'll come in separately with a name generation fix). The lvconvert
--repair patch essentially did not change.

-- 
Peter Rockai | me()mornfall!net | prockai()redhat!com
 http://blog.mornfall.net | http://web.mornfall.net

"In My Egotistical Opinion, most people's C programs should be
 indented six feet downward and covered with dirt."
     -- Blair P. Houghton on the subject of C program indentation

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]