public inbox for lvm2-cvs@sourceware.org
help / color / mirror / Atom feed
From: jbrassow@sourceware.org
To: lvm-devel@redhat.com, lvm2-cvs@sourceware.org
Subject: LVM2 ./WHATS_NEW lib/metadata/mirror.c
Date: Thu, 01 Sep 2011 19:22:00 -0000	[thread overview]
Message-ID: <20110901192212.29121.qmail@sourceware.org> (raw)

CVSROOT:	/cvs/lvm2
Module name:	LVM2
Changes by:	jbrassow@sourceware.org	2011-09-01 19:22:11

Modified files:
	.              : WHATS_NEW 
	lib/metadata   : mirror.c 

Log message:
	Fix for bug 732142: Unsafe table load during mirror image split
	
	There was a bad sequence:
	*) Make changes to LV layout to split images (e.g. 4-way -> 2-way/2-way)
	1) vg_write, suspend_lv(original_mirror), vg_commit
	2) activate_lv(newly_split_lv)
	3) resume_lv(original_mirror)
	
	Step #2 is not allowed.  However, without it, the resume of the original
	mirror will also resume its former sub-LVs - making it impossible to
	activate the newly split LV due to the changes in layering, pointers, and
	names that had already been made.  Additionally, the resume or the original
	brings the sub-lv's online with names that differ from the metadata on disk -
	also a no-no.  Thus, the split must be done in stages such that the active LVs
	always reflect what is in the committed LVM metadata.
	
	First, alter the original mirror by releasing the images.  The images are made
	visible and independent as an intermediate stage.  (This way, we can have
	consistency between LVM metadata and active LVs.)  The second stage collects
	the recently split LVs, deactivates them, forms them into a mirror if necessary,
	and then activates them.  It is a bit of a circuitous method, but it is the only
	way to split a mirror from a mirror and obey these general rules:
	1) Never [de]activate sub-lvs when the top-level LV is suspended
	2) Avoid having active LVs that differ from the description in the LVM metadata
	
	Signed-off-by: Jonathan Brassow <jbrassow@redhat.com>

Patches:
http://sourceware.org/cgi-bin/cvsweb.cgi/LVM2/WHATS_NEW.diff?cvsroot=lvm2&r1=1.2088&r2=1.2089
http://sourceware.org/cgi-bin/cvsweb.cgi/LVM2/lib/metadata/mirror.c.diff?cvsroot=lvm2&r1=1.159&r2=1.160

--- LVM2/WHATS_NEW	2011/09/01 10:25:22	1.2088
+++ LVM2/WHATS_NEW	2011/09/01 19:22:11	1.2089
@@ -1,5 +1,6 @@
 Version 2.02.89 - 
 ==================================
+  Fix unsafe table load when splitting off smaller mirror from a larger one.
   Use size_t return type for text_vg_export_raw() and export_vg_to_buffer().
   Add configure --enable-lvmetad for building the (experimental) LVMetaD.
   Fix resource leak when strdup fails in _get_device_status() (2.02.85).
--- LVM2/lib/metadata/mirror.c	2011/08/30 14:55:17	1.159
+++ LVM2/lib/metadata/mirror.c	2011/09/01 19:22:11	1.160
@@ -585,7 +585,8 @@
 	struct logical_volume *detached_log_lv = NULL;
 	struct lv_segment *mirrored_seg = first_seg(lv);
 	struct dm_list split_images;
-	struct lv_list *lvl;
+	struct lv_list *lvl, *new_lvl;
+	struct cmd_context *cmd = lv->vg->cmd;
 
 	if (!(lv->status & MIRRORED)) {
 		log_error("Unable to split non-mirrored LV, %s",
@@ -594,7 +595,7 @@
 	}
 
 	if (!split_count) {
-		log_error("split_count is zero!");
+		log_error(INTERNAL_ERROR "split_count is zero!");
 		return 0;
 	}
 
@@ -614,6 +615,12 @@
 		return 0;
 	}
 
+	/*
+	 * Step 1:
+	 *   Remove the images from the mirror.
+	 *   Make them visible, independent LVs (don't change names yet).
+	 *   Track them in a list for later instantiation.
+	 */
 	dm_list_init(&split_images);
 	for (i = 0; i < split_count; i++) {
 		mirrored_seg->area_count--;
@@ -626,31 +633,114 @@
 
 		log_very_verbose("%s assigned to be split", sub_lv->name);
 
+		lvl = dm_pool_alloc(lv->vg->vgmem, sizeof(*lvl));
+		if (!lvl) {
+			log_error("lv_list alloc failed");
+			return 0;
+		}
+		lvl->lv = sub_lv;
+		dm_list_add(&split_images, &lvl->list);
+	}
+	sub_lv = NULL;
+
+	/*
+	 * If no more mirrors, remove mirror layer.
+	 * The sub_lv is removed entirely later - leaving
+	 * only the top-level (now linear) LV.
+	 */
+	if (mirrored_seg->area_count == 1) {
+		sub_lv = seg_lv(mirrored_seg, 0);
+		sub_lv->status &= ~MIRROR_IMAGE;
+		lv_set_visible(sub_lv);
+		detached_log_lv = detach_mirror_log(mirrored_seg);
+		if (!remove_layer_from_lv(lv, sub_lv))
+			return_0;
+		lv->status &= ~MIRRORED;
+		lv->status &= ~LV_NOTSYNCED;
+	}
+
+	if (!vg_write(mirrored_seg->lv->vg)) {
+		log_error("Intermediate VG metadata write failed.");
+		return 0;
+	}
+
+	/*
+	 * Suspend the mirror - this includes all the sub-LVs and
+	 *                      soon-to-be-split sub-LVs
+	 */
+	if (!suspend_lv(cmd, mirrored_seg->lv)) {
+		log_error("Failed to lock %s", mirrored_seg->lv->name);
+		vg_revert(mirrored_seg->lv->vg);
+		return 0;
+	}
+
+	if (!vg_commit(mirrored_seg->lv->vg)) {
+		resume_lv(cmd, mirrored_seg->lv);
+		return 0;
+	}
+
+	log_very_verbose("Updating \"%s\" in kernel", mirrored_seg->lv->name);
+
+	/*
+	 * Resume the mirror - this also activates the visible, independent
+	 *                     soon-to-be-split sub-LVs
+	 */
+	if (!resume_lv(cmd, mirrored_seg->lv)) {
+		log_error("Problem resuming %s", mirrored_seg->lv->name);
+		return 0;
+	}
+
+	/* Remove original mirror layer if it has been converted to linear */
+	if (sub_lv && !_delete_lv(lv, sub_lv))
+		return_0;
+
+	/* Remove the log if it has been converted to linear */
+	if (detached_log_lv && !_delete_lv(lv, detached_log_lv))
+		return_0;
+
+	/*
+	 * Step 2:
+	 *   The original mirror has been changed and we now have visible,
+	 *   independent, not-yet-renamed, active sub-LVs.  We must:
+	 *   - deactivate the split sub-LVs
+	 *   - rename them
+	 *   - form new mirror if necessary
+	 *   - commit VG changes
+	 *   - activate the new LV
+	 */
+	new_lv = NULL;
+	dm_list_iterate_items(lvl, &split_images) {
 		if (!new_lv) {
-			new_lv = sub_lv;
-			new_lv->name = dm_pool_strdup(lv->vg->cmd->mem,
-						      split_name);
-			if (!new_lv->name) {
-				log_error("Unable to rename newly split LV");
-				return 0;
-			}
-		} else {
-			lvl = dm_pool_alloc(lv->vg->cmd->mem, sizeof(*lvl));
-			if (!lvl) {
-				log_error("lv_list alloc failed");
-				return 0;
-			}
-			lvl->lv = sub_lv;
-			dm_list_add(&split_images, &lvl->list);
+			/* Grab 1st sub-LV for later */
+			new_lvl = lvl;
+			new_lv = lvl->lv;
+		}
+
+		sub_lv = lvl->lv;
+		if (!deactivate_lv(cmd, sub_lv)) {
+			log_error("Failed to deactivate former mirror image, %s",
+				  sub_lv->name);
+			return 0;
 		}
 	}
 
+	dm_list_del(&new_lvl->list);
+	new_lv->name = dm_pool_strdup(lv->vg->vgmem, split_name);
+	if (!new_lv->name) {
+		log_error("Unable to rename newly split LV");
+		return 0;
+	}
+
 	if (!dm_list_empty(&split_images)) {
 		size_t len = strlen(new_lv->name) + 32;
 		char *layer_name, format[len];
 
-		if (!insert_layer_for_lv(lv->vg->cmd, new_lv,
-					 0, "_mimage_%d")) {
+		/*
+		 * A number of images have been split and
+		 * a new mirror layer must be formed
+		 */
+
+		if (!insert_layer_for_lv(cmd, new_lv, 0, "_mimage_%d")) {
 			log_error("Failed to build new mirror, %s",
 				  new_lv->name);
 			return 0;
@@ -664,7 +754,7 @@
 			dm_snprintf(format, len, "%s_mimage_%%d",
 				    new_lv->name);
 
-			layer_name = dm_pool_alloc(lv->vg->cmd->mem, len);
+			layer_name = dm_pool_alloc(lv->vg->vgmem, len);
 			if (!layer_name) {
 				log_error("Unable to allocate memory");
 				return 0;
@@ -691,63 +781,17 @@
 		init_mirror_in_sync(1);
 	}
 
-	sub_lv = NULL;
-
-	/*
-	 * If no more mirrors, remove mirror layer.
-	 * The sub_lv is removed entirely later - leaving
-	 * only the top-level (now linear) LV.
-	 */
-	if (mirrored_seg->area_count == 1) {
-		sub_lv = seg_lv(mirrored_seg, 0);
-		sub_lv->status &= ~MIRROR_IMAGE;
-		lv_set_visible(sub_lv);
-		detached_log_lv = detach_mirror_log(mirrored_seg);
-		if (!remove_layer_from_lv(lv, sub_lv))
-			return_0;
-		lv->status &= ~MIRRORED;
-		lv->status &= ~LV_NOTSYNCED;
-	}
-
-	if (!vg_write(mirrored_seg->lv->vg)) {
-		log_error("Intermediate VG metadata write failed.");
-		return 0;
-	}
-
-	/*
-	 * Suspend the original device and all its sub devices
-	 */
-	if (!suspend_lv(mirrored_seg->lv->vg->cmd, mirrored_seg->lv)) {
-		log_error("Failed to lock %s", mirrored_seg->lv->name);
-		vg_revert(mirrored_seg->lv->vg);
-		return 0;
-	}
-
-	if (!vg_commit(mirrored_seg->lv->vg)) {
-		resume_lv(mirrored_seg->lv->vg->cmd, mirrored_seg->lv);
-		return 0;
-	}
+	if (!vg_write(mirrored_seg->lv->vg) ||
+	    !vg_commit(mirrored_seg->lv->vg))
+		return_0;
 
 	/* Bring newly split-off LV into existence */
-	if (!activate_lv(lv->vg->cmd, new_lv)) {
+	if (!activate_lv(cmd, new_lv)) {
 		log_error("Failed to activate newly split LV, %s",
 			  new_lv->name);
 		return 0;
 	}
 
-	/* Resume altered original LV */
-	log_very_verbose("Updating \"%s\" in kernel", mirrored_seg->lv->name);
-	if (!resume_lv(mirrored_seg->lv->vg->cmd, mirrored_seg->lv)) {
-		log_error("Problem reactivating %s", mirrored_seg->lv->name);
-		return 0;
-	}
-
-	if (sub_lv && !_delete_lv(lv, sub_lv))
-		return_0;
-
-	if (detached_log_lv && !_delete_lv(lv, detached_log_lv))
-		return_0;
-
 	log_very_verbose("%" PRIu32 " image(s) detached from %s",
 			 split_count, lv->name);
 


             reply	other threads:[~2011-09-01 19:22 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-09-01 19:22 jbrassow [this message]
  -- strict thread matches above, loose matches on Subject: below --
2012-03-23 16:28 mbroz
2012-02-01 13:50 zkabelac
2011-10-25 13:17 jbrassow
2011-10-06 14:49 jbrassow
2011-09-14  2:45 jbrassow
2011-09-13 21:13 jbrassow
2011-09-13 14:37 jbrassow
2011-09-13 13:59 jbrassow
2011-06-17 14:27 zkabelac
2010-08-16 18:02 jbrassow
2010-07-13 22:24 jbrassow
2010-07-13 21:48 jbrassow
2010-07-09 15:08 jbrassow
2010-06-28 14:19 jbrassow
2010-06-21 16:12 jbrassow
2010-04-27 15:27 jbrassow
2010-04-27 14:57 jbrassow
2009-12-09 19:53 mbroz
2009-12-09 18:09 mbroz
2009-04-10  9:53 mbroz
2008-10-17 10:50 agk
2008-09-18 19:09 agk
2008-06-13 12:15 meyering
2008-02-22 13:28 agk
2007-11-22 13:57 agk
2006-11-30 17:52 agk
2006-11-10 19:35 agk
2006-07-20 20:37 agk

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20110901192212.29121.qmail@sourceware.org \
    --to=jbrassow@sourceware.org \
    --cc=lvm-devel@redhat.com \
    --cc=lvm2-cvs@sourceware.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).