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

[lvm-devel] [PATCH] lvconvert --repair refactor



Hi,

the attached patch (no functional change intended, apart from a minor
bug fix) adds a new entry point in the mirror lvconvert code, for
removing missing mirror images and/or logs, without attempting any
further actions.

The minor bug fix relates to the warning messages issued at the end of
_lv_convert_mirror_repair, which sometimes fired even if they weren't
supposed to (and sometimes got the counts wrong, too).

The new entry point will be useful in vgreduce --removemissing --force
for treating mirrors, replacing a lot of custom code. Unfortunately, it
turns out that the code that lvconvert uses (_remove_mirror_images in
metadata/mirror.c) has some bugs on its own that need fixing before
vgreduce can be completely switched over.

Nevertheless, I'm going to post the draft patch for vgreduce and then
try to hunt down the _remove_mirror_images bugs.

Index: tools/lvconvert.c
===================================================================
RCS file: /cvs/lvm2/LVM2/tools/lvconvert.c,v
retrieving revision 1.157
diff -u -p -r1.157 lvconvert.c
--- tools/lvconvert.c	3 Feb 2011 01:24:47 -0000	1.157
+++ tools/lvconvert.c	16 Feb 2011 08:45:02 -0000
@@ -46,7 +46,6 @@ struct lvconvert_params {
 	int pv_count;
 	char **pvs;
 	struct dm_list *pvh;
-	struct dm_list *failed_pvs;
 
 	struct logical_volume *lv_to_poll;
 };
@@ -319,7 +318,6 @@ static int _read_params(struct lvconvert
 
 	lp->pv_count = argc;
 	lp->pvs = argv;
-	lp->failed_pvs = NULL;
 
 	return 1;
 }
@@ -568,6 +566,19 @@ static int _failed_mirrors_count(struct 
 	return ret;
 }
 
+static int _failed_logs_count(struct logical_volume *lv)
+{
+	struct logical_volume *log_lv = first_seg(lv)->log_lv;
+	if (log_lv && (log_lv->status & PARTIAL_LV)) {
+		if (log_lv->status & MIRRORED)
+			return _failed_mirrors_count(log_lv);
+		else
+			return 1;
+	}
+	return 0;
+}
+
+
 static struct dm_list *_failed_pv_list(struct volume_group *vg)
 {
 	struct dm_list *failed_pvs;
@@ -735,10 +746,6 @@ static int _lv_update_log_type(struct cm
 		return 1;
 
 	original_lv = _original_lv(lv);
-	region_size = adjusted_mirror_region_size(lv->vg->extent_size,
-						  lv->le_count,
-						  lp->region_size);
-
 	/* Remove an existing log completely */
 	if (!log_count) {
 		if (!remove_mirror_log(cmd, original_lv, operable_pvs,
@@ -752,6 +759,11 @@ static int _lv_update_log_type(struct cm
 
 	/* Adding redundancy to the log */
 	if (old_log_count < log_count) {
+
+		region_size = adjusted_mirror_region_size(lv->vg->extent_size,
+							  lv->le_count,
+							  lp->region_size);
+
 		if (!add_mirror_log(cmd, original_lv, log_count,
 				    region_size, operable_pvs, lp->alloc))
 			return_0;
@@ -1167,6 +1179,43 @@ out_skip_log_convert:
 	return 1;
 }
 
+static int _mirror_remove_missing(struct cmd_context *cmd,
+				  struct logical_volume *lv)
+{
+	struct dm_list *failed_pvs;
+	int log_count = _get_log_count(lv) - _failed_logs_count(lv);
+
+	if (!(failed_pvs = _failed_pv_list(lv->vg)))
+		return_0;
+
+	/* No point in keeping a log if the result is not a mirror */
+	if (_failed_mirrors_count(lv) + 1 >= lv_mirror_count(lv))
+		log_count = 0;
+
+	/*
+	 * We must adjust the log first, or the entire mirror
+	 * will get stuck during a suspend.
+	 */
+	if (!_lv_update_mirrored_log(lv, failed_pvs, log_count))
+		return 0;
+
+	if (_failed_mirrors_count(lv) > 0) {
+		if (!lv_remove_mirrors(cmd, lv, _failed_mirrors_count(lv),
+				       log_count ? 0U : 1U,
+				       _is_partial_lv, NULL, 0))
+			return 0;
+	}
+
+	if (!_lv_update_log_type(cmd, NULL, lv, failed_pvs,
+				 log_count))
+		return 0;
+
+	if (!_reload_lv(cmd, lv))
+		return 0;
+
+	return 1;
+}
+
 /*
  * _lvconvert_mirrors_repair
  *
@@ -1179,16 +1228,16 @@ out_skip_log_convert:
  */
 static int _lvconvert_mirrors_repair(struct cmd_context *cmd,
 				     struct logical_volume *lv,
-				     struct lvconvert_params *lp,
-				     uint32_t old_mimage_count,
-				     uint32_t old_log_count)
-{
-	int failed_log = 0;
-	int failed_mirrors = 0;
-	int replace_log = 0;
-	int replace_mirrors = 0;
-	uint32_t new_log_count, log_count;
-	struct logical_volume *log_lv;
+				     struct lvconvert_params *lp)
+{
+	int failed_logs = 0;
+	int failed_mimages = 0;
+	int replace_logs = 0;
+	int replace_mimages = 0;
+	uint32_t log_count;
+
+	uint32_t original_mimages = lv_mirror_count(lv);
+	uint32_t original_logs = _get_log_count(lv);
 
 	cmd->handles_missing_pvs = 1;
 	cmd->partial_activation = 1;
@@ -1201,93 +1250,43 @@ static int _lvconvert_mirrors_repair(str
 		return 1;
 	}
 
-	/*
-	 * Count the failed mimages - negative if 'lv' is not a mirror
-	 */
-	if ((failed_mirrors = _failed_mirrors_count(lv)) < 0)
-		return_0;
+	failed_mimages = _failed_mirrors_count(lv);
+	failed_logs = _failed_logs_count(lv);
 
-	lp->mirrors = old_mimage_count - failed_mirrors;
+	_mirror_remove_missing(cmd, lv);
 
-	if (lp->mirrors != old_mimage_count)
+	if (failed_mimages)
 		log_error("Mirror status: %d of %d images failed.",
-			  failed_mirrors, old_mimage_count);
+			  failed_mimages, original_mimages);
 
 	/*
 	 * Count the failed log devices
 	 */
-	new_log_count = old_log_count;
-	log_lv = first_seg(lv)->log_lv;
-	if (log_lv) {
-		new_log_count = lv_mirror_count(log_lv);
-		if (log_lv->status & PARTIAL_LV) {
-			failed_log = 1;
-			if (log_lv->status & MIRRORED)
-				new_log_count -= _failed_mirrors_count(log_lv);
-			else
-				new_log_count = 0;
-		}
-	}
-	if (old_log_count != new_log_count)
-		log_error("Mirror log status: %d of %d images failed%s",
-			  old_log_count - new_log_count, old_log_count,
-			  (!new_log_count) ? " - switching to core" : "");
+	if (failed_logs)
+		log_error("Mirror log status: %d of %d images failed.",
+			  failed_logs, original_logs);
 
 	/*
 	 * Find out our policies
 	 */
-	_lvconvert_mirrors_repair_ask(cmd, failed_log, failed_mirrors,
-				      &replace_log, &replace_mirrors);
-
-	/*
-	 * First phase - remove faulty devices
-	 */
-	if (!(lp->failed_pvs = _failed_pv_list(lv->vg)))
-		return_0;
-
-	log_count = new_log_count;
-
-	/*
-	 * We must adjust the log first, or the entire mirror
-	 * will get stuck during a suspend.
-	 */
-	if (!_lv_update_mirrored_log(lv, lp->failed_pvs, log_count))
-		return 0;
-
-	if (lp->mirrors == 1)
-		log_count = 0;
-
-	if (failed_mirrors) {
-		if (!lv_remove_mirrors(cmd, lv, failed_mirrors,
-				       log_count ? 0U : 1U,
-				       _is_partial_lv, NULL, 0))
-			return 0;
-	}
-
-	if (!_lv_update_log_type(cmd, lp, lv, lp->failed_pvs,
-				 log_count))
-		return 0;
-
-	if (!_reload_lv(cmd, lv))
-		return 0;
+	_lvconvert_mirrors_repair_ask(cmd, failed_logs, failed_mimages,
+				      &replace_logs, &replace_mimages);
 
 	/*
 	 * Second phase - replace faulty devices
 	 */
-
-	if (replace_mirrors)
-		lp->mirrors = old_mimage_count;
+	lp->mirrors = replace_mimages ? original_mimages : (original_mimages - failed_mimages);
 
 	/*
 	 * It does not make sense to replace the log if the volume is no longer
 	 * a mirror.
 	 */
-	if (!replace_mirrors && lp->mirrors == 1)
-		replace_log = 0;
+	if (lp->mirrors == 1)
+		replace_logs = 0;
 
-	log_count = replace_log ? old_log_count : new_log_count;
+	log_count = replace_logs ? original_logs : (original_logs - failed_logs);
 
-	while (replace_mirrors || replace_log) {
+	while (replace_mimages || replace_logs) {
 		log_warn("Trying to up-convert to %d images, %d logs.", lp->mirrors, log_count);
 		if (_lvconvert_mirrors_aux(cmd, lv, lp, NULL,
 					   lp->mirrors, log_count))
@@ -1302,12 +1301,12 @@ static int _lvconvert_mirrors_repair(str
 		}
 	}
 
-	if (replace_mirrors && lp->mirrors != old_mimage_count)
+	if (replace_mimages && lv_mirror_count(lv) != original_mimages)
 		log_warn("WARNING: Failed to replace %d of %d images in volume %s",
-			 old_mimage_count - lp->mirrors, old_mimage_count, lv->name);
-	if (replace_log && log_count != old_log_count)
+			 original_mimages - lv_mirror_count(lv), original_mimages, lv->name);
+	if (replace_logs && _get_log_count(lv) != original_logs)
 		log_warn("WARNING: Failed to replace %d of %d logs in volume %s",
-			 old_log_count - log_count, old_log_count, lv->name);
+			 original_logs - _get_log_count(lv), original_logs, lv->name);
 
 	/* if (!arg_count(cmd, use_policies_ARG) && (lp->mirrors != old_mimage_count
 						  || log_count != old_log_count))
@@ -1353,9 +1352,7 @@ static int _lvconvert_mirrors(struct cmd
 		return 1;
 
 	if (repair)
-		return _lvconvert_mirrors_repair(cmd, lv, lp,
-						 old_mimage_count,
-						 old_log_count);
+		return _lvconvert_mirrors_repair(cmd, lv, lp);
 
 	if (!_lvconvert_mirrors_aux(cmd, lv, lp, NULL,
 				    new_mimage_count, new_log_count))
@@ -1530,6 +1527,7 @@ static int _lvconvert_single(struct cmd_
 			     void *handle)
 {
 	struct lvconvert_params *lp = handle;
+	struct dm_list *failed_pvs;
 
 	if (lv->status & LOCKED) {
 		log_error("Cannot convert locked LV %s", lv->name);
@@ -1593,9 +1591,14 @@ static int _lvconvert_single(struct cmd_
 			return ECMD_FAILED;
 		}
 
+		if (!(failed_pvs = _failed_pv_list(lv->vg))) {
+			stack;
+			return ECMD_FAILED;
+		}
+
 		/* If repairing and using policies, remove missing PVs from VG */
 		if (arg_count(cmd, repair_ARG) && arg_count(cmd, use_policies_ARG))
-			_remove_missing_empty_pv(lv->vg, lp->failed_pvs);
+			_remove_missing_empty_pv(lv->vg, failed_pvs);
 	}
 
 	return ECMD_PROCESSED;
-- 
id' Ash = Ash; id' Dust = Dust; id' _ = undefined

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