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

[lvm-devel] [PATCH] Remove uninitialized cow if snapshot creation fails



Similar to the mirror log cleanup, snapshot cow also should be
removed if the snapshot creation fails.

# dmsetup create --notable testvg-lvol1-cow
# lvcreate -s -l1 -n lvol1 testvg/lvol0
  /dev/testvg/lvol1: not found: device not cleared
  Aborting. Failed to wipe snapshot exception store. Remove new LV and retry.
# lvs
  LV    VG     Attr   LSize Origin Snap%  Move Log Copy% 
  lvol0 testvg -wi-a- 4.00M
  lvol1 testvg -wi--- 4.00M

Attached patch removes the incomplete COW LV if the snapshot
creation fails.

A test script is also attached.

Thanks,
-- 
Jun'ichi Nomura, NEC Corporation of America
According to the error messages, there are some failure cases where
the halfway-created LV should be removed.
  - On snapshot creation, the cow is first activated and zero-filled.
    If either activation or zero-fill fails, the cow isn't properly
    initialized and should be removed.

Also, the comment in the case of '--zero n' for non-snapshot LV isn't right.
 	log_error("WARNING: \"%s\" not zeroed", lv->name);
	/* FIXME Remove the failed lv we just added */
This is a case for successful execution of 'lvcreate --zero n'
and the LV should not be removed.

Failure of zeroing might have to be handled in the same manner as
the cow. But this patch doesn't change it.

Index: LVM2.work/tools/lvcreate.c
===================================================================
--- LVM2.work.orig/tools/lvcreate.c
+++ LVM2.work/tools/lvcreate.c
@@ -517,7 +517,7 @@ static int _lvcreate(struct cmd_context 
 	struct logical_volume *lv, *org = NULL, *log_lv = NULL;
 	struct list *pvh, tags;
 	const char *tag = NULL;
-	int consistent = 1, origin_active = 0;
+	int consistent = 1, origin_active = 0, committed = 0;
 	struct alloc_handle *ah = NULL;
 	char lv_name_buf[128];
 	const char *lv_name;
@@ -822,11 +822,13 @@ static int _lvcreate(struct cmd_context 
 		return 0;
 	}
 
+	committed = 1;
+
 	if (lp->snapshot) {
 		if (!activate_lv_excl(cmd, lv)) {
 			log_error("Aborting. Failed to activate snapshot "
-				  "exception store. Remove new LV and retry.");
-			return 0;
+				  "exception store.");
+			goto error;
 		}
 	} else if (!activate_lv(cmd, lv)) {
 		log_error("Failed to activate new LV.");
@@ -835,15 +837,12 @@ static int _lvcreate(struct cmd_context 
 
 	if ((lp->zero || lp->snapshot) && activation()) {
 		if (!set_lv(cmd, lv, UINT64_C(0), 0) && lp->snapshot) {
-			/* FIXME Remove the failed lv we just added */
 			log_error("Aborting. Failed to wipe snapshot "
-				  "exception store. Remove new LV and retry.");
-			return 0;
+				  "exception store.");
+			goto error;
 		}
-	} else {
+	} else
 		log_error("WARNING: \"%s\" not zeroed", lv->name);
-		/* FIXME Remove the failed lv we just added */
-	}
 
 	if (lp->snapshot) {
 		/* Reset permission after zeroing */
@@ -896,6 +895,10 @@ static int _lvcreate(struct cmd_context 
 	return 1;
 
 error:
+ 	/* If metadata has already been committed, we have to remove the LV. */
+	if (committed && !lv_remove_single(cmd, lv, DONT_PROMPT_OVERRIDE))
+		stack;
+
 	if (ah)
 		alloc_destroy(ah);
 	return 0;

Attachment: cleanup-lvcreate-snapshot.sh
Description: Bourne shell script


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