[PATCH] Fix handling of snapshots with virtual origin.

[PATCH] Fix handling of snapshots with virtual origin.

[Milan Broz]

Fix handling of snapshots with virtual origin.

 * validate code must count virtual origin into "invisible"
 volume count otherwise the test fails

 * when creating new LV snapshot with virtual origin, the virtual origin
 must be explicitly activated otherwise COW mapping table remains linear
 (but metadata are written correctly so later vgchange fixes it)
 Fix it by calling activate_lv on newly created origin.

 * when removing snapshot with virtual origin, code should not ask
 confirmation question for virtual origin
 (but it prints informational message that origin was removed too).

 Also skip virtual origin when iterating volumes in VG.
 (All operations are handled on snapshot above.)

Signed-off-by: Milan Broz <mbroz@redhat.com>
---
 lib/metadata/metadata.c  |    6 +++++-
 test/t-lvcreate-usage.sh |    6 ++++++
 tools/lvcreate.c         |   16 +++++++++-------
 tools/lvremove.c         |    9 ++++++---
 tools/toollib.c          |    3 +++
 5 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/lib/metadata/metadata.c b/lib/metadata/metadata.c
index f16cd55..f4ea237 100644
--- a/lib/metadata/metadata.c
+++ b/lib/metadata/metadata.c
@@ -1492,7 +1492,11 @@ int vg_validate(struct volume_group *vg)
 			continue;
 
 		/* snapshots */
-		if (lv_is_cow(lvl->lv) || lv_is_origin(lvl->lv))
+		if (lv_is_cow(lvl->lv))
+			continue;
+
+		/* virtual origin is invisible always */
+		if (lv_is_origin(lvl->lv) && !lv_is_virtual_origin(lvl->lv))
 			continue;
 
 		/* count other non-snapshot invisible volumes */
diff --git a/test/t-lvcreate-usage.sh b/test/t-lvcreate-usage.sh
index 43cf716..d6708ca 100755
--- a/test/t-lvcreate-usage.sh
+++ b/test/t-lvcreate-usage.sh
@@ -116,3 +116,9 @@ lvcreate -L 32M -n $lv --regionsize 4M -m 1 $vg
 check_lv_field_ $vg/$lv regionsize "4.00M"
 lvremove -ff $vg
 
+# snapshot with virtual origin works
+lvcreate -s --virtualoriginsize 64M -L 32M -n $lv1 $vg
+lvcreate -s --virtualoriginsize 64M -L 32M -n $lv2 $vg
+lvchange -a n $vg/$lv1
+lvremove $vg/$lv1
+lvremove -ff $vg
diff --git a/tools/lvcreate.c b/tools/lvcreate.c
index 67d065c..8d4ec23 100644
--- a/tools/lvcreate.c
+++ b/tools/lvcreate.c
@@ -913,13 +913,15 @@ static int _lvcreate(struct cmd_context *cmd, struct volume_group *vg,
 			return 0;
 		}
 
-		if (lp->voriginsize &&
-		    !(org = _create_virtual_origin(cmd, vg, lv->name,
-						   lp->permission,
-						   lp->voriginextents))) {
-			log_error("Couldn't create virtual origin for LV %s",
-				  lv->name);
-			goto deactivate_and_revert_new_lv;
+		if (lp->voriginsize) {
+			org = _create_virtual_origin(cmd, vg, lv->name,
+						     lp->permission,
+						     lp->voriginextents);
+			if (!org || !activate_lv(cmd, org)) {
+				log_error("Couldn't create virtual origin "
+					  "for LV %s", lv->name);
+				goto deactivate_and_revert_new_lv;
+			}
 		}
 
 		/* cow LV remains active and becomes snapshot LV */
diff --git a/tools/lvremove.c b/tools/lvremove.c
index 712f175..56b304e 100644
--- a/tools/lvremove.c
+++ b/tools/lvremove.c
@@ -18,17 +18,20 @@
 static int lvremove_single(struct cmd_context *cmd, struct logical_volume *lv,
 			   void *handle __attribute((unused)))
 {
-	struct logical_volume *origin;
+	struct logical_volume *virtual_origin = NULL;
 
 	/*
 	 * If this is a sparse device, remove its origin too.
 	 */
-        if (lv_is_cow(lv) && lv_is_virtual_origin(origin = origin_from_cow(lv)))
-                lv = origin;
+        if (lv_is_cow(lv) && lv_is_virtual_origin(origin_from_cow(lv)))
+                virtual_origin = origin_from_cow(lv);
 
 	if (!lv_remove_with_dependencies(cmd, lv, arg_count(cmd, force_ARG)))
 		return ECMD_FAILED;
 
+	if (virtual_origin && !lv_remove_single(cmd, virtual_origin, DONT_PROMPT))
+		return ECMD_FAILED;
+
 	return ECMD_PROCESSED;
 }
 
diff --git a/tools/toollib.c b/tools/toollib.c
index 7f9eef1..fed2406 100644
--- a/tools/toollib.c
+++ b/tools/toollib.c
@@ -122,6 +122,9 @@ int process_each_lv_in_vg(struct cmd_context *cmd,
 		if (lvl->lv->status & SNAPSHOT)
 			continue;
 
+		if (lv_is_virtual_origin(lvl->lv))
+			continue;
+
 		/* Should we process this LV? */
 		if (process_all)
 			process_lv = 1;

Re: [PATCH] Fix handling of snapshots with virtual origin.

[Mikulas Patocka]

It fixes the problem, but here is another one:

[gerlinda:~]# rmmod dm_zero
[gerlinda:~]# lvcreate -n vo -s --virtualoriginsize 64M -L 24M vg1
  Can't expand LV vo_vorigin: zero target support missing from kernel?
  Couldn't create virtual origin for LV vo
[gerlinda:~]# lvs
  LV      VG   Attr   LSize  Origin Snap%  Move Log         Copy%  Convert
  bla     vg1  -wi--- 24.00M
  er      vg1  vwi-a- 16.00M
  hid     vg1  owi--- 20.00M
  hid_sn  vg1  swi--- 20.00M hid
  hid_sn2 vg1  swi--- 24.00M hid
  mirror  vg1  mwi--- 30.00M                    mirror_mlog
[gerlinda:~]# lvcreate -n vo -s --virtualoriginsize 64M -L 24M vg1
  Internal error: Duplicate LV name vo_vorigin detected in vg1.
  Couldn't create virtual origin for LV vo
  Internal error: Duplicate LV name vo_vorigin detected in vg1.
  Manual intervention may be required to remove abandoned LV(s) before 
retrying.
[gerlinda:~]# lvs
  LV      VG   Attr   LSize  Origin Snap%  Move Log         Copy%  Convert
  bla     vg1  -wi--- 24.00M
  er      vg1  vwi-a- 16.00M
  hid     vg1  owi--- 20.00M
  hid_sn  vg1  swi--- 20.00M hid
  hid_sn2 vg1  swi--- 24.00M hid
  mirror  vg1  mwi--- 30.00M                    mirror_mlog
  vo      vg1  -wi--- 24.00M
[gerlinda:~]# lvremove vg1/vo
  Logical volume "vo" successfully removed

... after this, the vg contains junk invisible lv vo_vorigin in its 
metadata. It can be removed, but it can't be seen. (in fact, the junk lv 
is created on the first activation command if the module dm_zero is not 
loaded in the kernel)

Mikulas

Re: [PATCH] Fix handling of snapshots with virtual origin.

[Milan Broz]

Mikulas Patocka wrote:
> It fixes the problem, but here is another one:
> 
> [gerlinda:~]# rmmod dm_zero
> [gerlinda:~]# lvcreate -n vo -s --virtualoriginsize 64M -L 24M vg1
>   Can't expand LV vo_vorigin: zero target support missing from kernel?
>   Couldn't create virtual origin for LV vo

That's common problem for all target modules when missing, the automatic
module loading doesn't work and dm_zero is not loaded by default in RHEL btw.
(mirror and snapshpot is loaded in initrd).

> [gerlinda:~]# lvcreate -n vo -s --virtualoriginsize 64M -L 24M vg1
>   Internal error: Duplicate LV name vo_vorigin detected in vg1.
>   Couldn't create virtual origin for LV vo
>   Internal error: Duplicate LV name vo_vorigin detected in vg1.
>   Manual intervention may be required to remove abandoned LV(s) before 
> retrying.

hm, I'll check, probably another bug which was there since introduction of vorigin.

It is should fail more intelligently without residual LVs in metadata...

Milan

Re: [PATCH] Fix handling of snapshots with virtual origin.

[Mikulas Patocka]

On Wed, 27 May 2009, Milan Broz wrote:

> Mikulas Patocka wrote:
> > It fixes the problem, but here is another one:
> > 
> > [gerlinda:~]# rmmod dm_zero
> > [gerlinda:~]# lvcreate -n vo -s --virtualoriginsize 64M -L 24M vg1
> >   Can't expand LV vo_vorigin: zero target support missing from kernel?
> >   Couldn't create virtual origin for LV vo
> 
> That's common problem for all target modules when missing, the automatic
> module loading doesn't work and dm_zero is not loaded by default in RHEL btw.
> (mirror and snapshpot is loaded in initrd).

Creating snapshot without dm_snapshot.ko being loaded works in my system. 
It just loads the dm_snapshot module. dm_zero isn't automatically loaded.

This is a trace for attempting to create zero lvm (without any snapshots):

#lvmcmdline.c:986         Processing: lvcreate -vvvv --type zero -L 16M -n z2 vg1
#lvmcmdline.c:989         O_DIRECT will be used
#config/config.c:950       Setting global/locking_type to 1
#locking/locking.c:230       File-based locking selected.
#config/config.c:927       Setting global/locking_dir to /var/lock/lvm
#activate/activate.c:363       Getting target version for zero
#ioctl/libdm-iface.c:1672         dm version   OF   [16384]
#ioctl/libdm-iface.c:1672         dm versions   OF   [16384]
#lvcreate.c:445   zero: Required device-mapper target(s) not detected in your kernel
  Run `lvcreate --help' for more information.

Mikulas

[PATCH] More fixes for virtual origin

[Milan Broz]

More fixes for virtual origin (applied over patch above)

 * try to load dm_zero using modprobe if not present

 * remove virtual origin from metadata if activate fails

 * fail if residual vorigin exist and do not create LV with duplicate name

Signed-off-by: Milan Broz <mbroz@redhat.com>
---
 lib/zero/zero.c  |    2 +-
 tools/lvcreate.c |    8 ++++++++
 2 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/lib/zero/zero.c b/lib/zero/zero.c
index acf6453..7b3e62e 100644
--- a/lib/zero/zero.c
+++ b/lib/zero/zero.c
@@ -58,7 +58,7 @@ static int _zero_target_present(struct cmd_context *cmd,
 	static int _zero_present = 0;
 
 	if (!_zero_checked)
-		_zero_present = target_present(cmd, "zero", 0);
+		_zero_present = target_present(cmd, "zero", 1);
 
 	_zero_checked = 1;
 
diff --git a/tools/lvcreate.c b/tools/lvcreate.c
index 8d4ec23..cfdd589 100644
--- a/tools/lvcreate.c
+++ b/tools/lvcreate.c
@@ -576,6 +576,12 @@ static struct logical_volume *_create_virtual_origin(struct cmd_context *cmd,
 		return 0;
 	}
 
+	if (find_lv_in_vg(vg, vorigin_name)) {
+		log_error("Virtual origin LV %s already exists in "
+			  "volume group %s.", vorigin_name, vg->name);
+		return 0;
+	}
+
 	if (!(lv = lv_create_empty(vorigin_name, NULL, permission,
 				   ALLOC_INHERIT, vg)))
 		return_0;
@@ -920,6 +926,8 @@ static int _lvcreate(struct cmd_context *cmd, struct volume_group *vg,
 			if (!org || !activate_lv(cmd, org)) {
 				log_error("Couldn't create virtual origin "
 					  "for LV %s", lv->name);
+				if (org && !lv_remove(org))
+					stack;
 				goto deactivate_and_revert_new_lv;
 			}
 		}