LVM2 ./WHATS_NEW ./WHATS_NEW_DM libdm/libdm-de ...

LVM2 ./WHATS_NEW ./WHATS_NEW_DM libdm/libdm-de ...

[agk]

CVSROOT:	/cvs/lvm2
Module name:	LVM2
Changes by:	agk at sourceware.org	2010-04-07 20:04:42

Modified files:
	.              : WHATS_NEW WHATS_NEW_DM 
	libdm          : libdm-deptree.c 
	lib/activate   : dev_manager.c 

Log message:
	Fix incorrect removal of symlinks after LV deactivation fails.

Patches:
http://sourceware.org/cgi-bin/cvsweb.cgi/LVM2/WHATS_NEW.diff?cvsroot=lvm2&r1=1.1504&r2=1.1505
http://sourceware.org/cgi-bin/cvsweb.cgi/LVM2/WHATS_NEW_DM.diff?cvsroot=lvm2&r1=1.353&r2=1.354
http://sourceware.org/cgi-bin/cvsweb.cgi/LVM2/libdm/libdm-deptree.c.diff?cvsroot=lvm2&r1=1.72&r2=1.73
http://sourceware.org/cgi-bin/cvsweb.cgi/LVM2/lib/activate/dev_manager.c.diff?cvsroot=lvm2&r1=1.183&r2=1.184

--- LVM2/WHATS_NEW	2010/04/06 17:36:41	1.1504
+++ LVM2/WHATS_NEW	2010/04/07 20:04:41	1.1505
@@ -1,5 +1,6 @@
 Version 2.02.63 -  
 ================================
+  Fix incorrect removal of symlinks after LV deactivation fails.
   Fix is_partitioned_dev not to attempt to reopen device.
   Fix another thread race in clvmd.
   Refactor management of vg->pvs list.
--- LVM2/WHATS_NEW_DM	2010/04/07 15:57:20	1.353
+++ LVM2/WHATS_NEW_DM	2010/04/07 20:04:42	1.354
@@ -1,6 +1,7 @@
 Version 1.02.46 - 
 ================================
-  Wipe memory buffers for dm-ioctl parameters before releaseing.
+  Change dm_tree_deactivate_children to fail if device is open.
+  Wipe memory buffers for dm-ioctl parameters before releasing.
   Strictly require libudev if udev_sync is used.
   Add support for ioctl's DM_UEVENT_GENERATED_FLAG.
 
--- LVM2/libdm/libdm-deptree.c	2010/03/25 18:22:04	1.72
+++ LVM2/libdm/libdm-deptree.c	2010/04/07 20:04:42	1.73
@@ -1047,9 +1047,14 @@
 
 		/* Refresh open_count */
 		if (!_info_by_dev(dinfo->major, dinfo->minor, 1, &info) ||
-		    !info.exists || info.open_count)
+		    !info.exists)
 			continue;
 
+		if (info.open_count) {
+			r = 0;
+			continue;
+		}
+
 		if (!_deactivate_node(name, info.major, info.minor,
 				      &child->dtree->cookie, child->udev_flags)) {
 			log_error("Unable to deactivate %s (%" PRIu32
--- LVM2/lib/activate/dev_manager.c	2010/02/24 20:00:56	1.183
+++ LVM2/lib/activate/dev_manager.c	2010/04/07 20:04:42	1.184
@@ -1523,8 +1523,6 @@
 
 	r = _tree_action(dm, lv, DEACTIVATE);
 
-	fs_del_lv(lv);
-
 	return r;
 }
 

LVM2 ./WHATS_NEW ./WHATS_NEW_DM libdm/libdm-de ...

[Petr Rockai]

Hi,

agk at sourceware.org writes:
> --- LVM2/libdm/libdm-deptree.c	2010/03/25 18:22:04	1.72
> +++ LVM2/libdm/libdm-deptree.c	2010/04/07 20:04:42	1.73
> @@ -1047,9 +1047,14 @@
>  
>  		/* Refresh open_count */
>  		if (!_info_by_dev(dinfo->major, dinfo->minor, 1, &info) ||
> -		    !info.exists || info.open_count)
> +		    !info.exists)
>  			continue;
>  
> +		if (info.open_count) {
> +			r = 0;
> +			continue;
> +		}
As far as I can tell, this r = 0 is new behaviour that breaks a number
of tests and may break user scripts. Is it necessary?

Yours,
   Petr.

LVM2 ./WHATS_NEW ./WHATS_NEW_DM libdm/libdm-de ...

[Alasdair G Kergon]

On Thu, Apr 08, 2010 at 12:31:44AM +0200, Peter Rockai wrote:
> As far as I can tell, this r = 0 is new behaviour that breaks a number
> of tests and may break user scripts. Is it necessary?
 
Well the tests weren't working for other reasons, but I want to see what
fails and why before deciding what to do about it.

The code was inconsistent - if the device being deleted is opened before
this if statement, the function would return success, but if the device
being deleted is opened just after it (but before the real deletion) it would
return failure.  (It shouldn't be open at all if this point is reached.)

Alasdair

LVM2 ./WHATS_NEW ./WHATS_NEW_DM libdm/libdm-de ...

[Mike Snitzer]

On Wed, Apr 07 2010 at  4:04pm -0400,
agk at sourceware.org <agk@sourceware.org> wrote:

> CVSROOT:	/cvs/lvm2
> Module name:	LVM2
> Changes by:	agk at sourceware.org	2010-04-07 20:04:42
> 
> Modified files:
> 	.              : WHATS_NEW WHATS_NEW_DM 
> 	libdm          : libdm-deptree.c 
> 	lib/activate   : dev_manager.c 
> 
> Log message:
> 	Fix incorrect removal of symlinks after LV deactivation fails.

<snip>

> --- LVM2/WHATS_NEW_DM	2010/04/07 15:57:20	1.353
> +++ LVM2/WHATS_NEW_DM	2010/04/07 20:04:42	1.354
> @@ -1,6 +1,7 @@
>  Version 1.02.46 - 
>  ================================
> -  Wipe memory buffers for dm-ioctl parameters before releaseing.
> +  Change dm_tree_deactivate_children to fail if device is open.
> +  Wipe memory buffers for dm-ioctl parameters before releasing.
>    Strictly require libudev if udev_sync is used.
>    Add support for ioctl's DM_UEVENT_GENERATED_FLAG.
>  
> --- LVM2/libdm/libdm-deptree.c	2010/03/25 18:22:04	1.72
> +++ LVM2/libdm/libdm-deptree.c	2010/04/07 20:04:42	1.73
> @@ -1047,9 +1047,14 @@
>  
>  		/* Refresh open_count */
>  		if (!_info_by_dev(dinfo->major, dinfo->minor, 1, &info) ||
> -		    !info.exists || info.open_count)
> +		    !info.exists)
>  			continue;
>  
> +		if (info.open_count) {
> +			r = 0;
> +			continue;
> +		}
> +
>  		if (!_deactivate_node(name, info.major, info.minor,
>  				      &child->dtree->cookie, child->udev_flags)) {
>  			log_error("Unable to deactivate %s (%" PRIu32

This change causes test/t-snapshot-merge.sh's 2nd snapshot-merge test
(onactivate merge negative testing) to fail.  Later lvm2 changes refined
the failure to occur IFF the device is a toplevel device.  In the case
of snapshot-merge it is failing with the following error (using the
latest lvm2):

+ lvremove -f LVMTEST5289vg/LV1
  Unable to deactivate open LVMTEST3125vg-LV1-real (253:6)
  Failed to resume LV1.

It would appear that the non-zero open_count associated with the -real
device is stale (during lvremove's dm_tree_deactivate_children).  If I
unmount the snapshot before doing the --refresh the lvremove works fine.

Somewhat more detailed log from failing test:
lvs -a
+ lvs -a
  LV       VG            Attr   LSize  Origin Snap%  Move Log Copy%  Convert
  LV1      LVMTEST5289vg owi-a- 48.00m                                      
  LV1_snap LVMTEST5289vg swi-a-  8.00m LV1     73.05                        
mkdir test_mnt
+ mkdir test_mnt
mount $(lvdev_ $vg $lv1) test_mnt
lvdev_ $vg $lv1)
lvdev_ $vg $lv1
++ lvdev_ LVMTEST5289vg LV1
++ echo /root/git/lvm2/test/LVMTEST5289.7xFSvbHNO8/dev/LVMTEST5289vg/LV1
+ mount /root/git/lvm2/test/LVMTEST5289.7xFSvbHNO8/dev/LVMTEST5289vg/LV1 test_mnt
lvconvert --merge $vg/$(snap_lv_name_ $lv1)
snap_lv_name_ $lv1)
snap_lv_name_ $lv1
++ snap_lv_name_ LV1
++ echo LV1_snap
+ lvconvert --merge LVMTEST5289vg/LV1_snap
  Can't merge over open origin volume
  WARNING: This metadata update is NOT backed up
  Merging of snapshot LV1_snap will start next activation.
lvchange --refresh $vg/$lv1
+ lvchange --refresh LVMTEST5289vg/LV1
umount test_mnt
+ umount test_mnt
rm -r test_mnt
+ rm -r test_mnt
# an active merge uses the "snapshot-merge" target
dmsetup table ${vg}-${lv1} | grep -q " snapshot-origin "
+ dmsetup table LVMTEST5289vg-LV1
+ grep -q ' snapshot-origin '
test $? = 0
+ test 0 = 0
lvremove -f $vg/$lv1
+ lvremove -f LVMTEST5289vg/LV1
  Unable to deactivate open LVMTEST5289vg-LV1-real (253:6)
  Failed to resume LV1.

I'll look closer at this "stale open_count" theory I raised above.

But in the near-term I have a "fix" for test/t-snapshot-merge.sh (works
with both LVM_TEST_LOCKING=1 and LVM_TEST_LOCKING=3).  I just start
merging the snapshot and then lvremove the merging origin and its
snapshot:

diff --git a/test/t-snapshot-merge.sh b/test/t-snapshot-merge.sh
index b7c0cb0..ff78f14 100755
--- a/test/t-snapshot-merge.sh
+++ b/test/t-snapshot-merge.sh
@@ -55,19 +55,24 @@ lvconvert --merge $vg/$(snap_lv_name_ $lv1)
 lvremove -f $vg/$lv1
 
 
-# "onactivate merge" test -- refresh LV while FS is still mounted;
-# verify snapshot-origin target is still being used
+# "onactivate merge" test
 setup_merge $vg $lv1
 lvs -a
 mkdir test_mnt
 mount $(lvdev_ $vg $lv1) test_mnt
 lvconvert --merge $vg/$(snap_lv_name_ $lv1)
+# -- refresh LV while FS is still mounted (merge must not start),
+#    verify 'snapshot-origin' target is still being used
 lvchange --refresh $vg/$lv1
 umount test_mnt
 rm -r test_mnt
-# an active merge uses the "snapshot-merge" target
 dmsetup table ${vg}-${lv1} | grep -q " snapshot-origin "
-test $? = 0
+# -- refresh LV to start merge (now that FS is unmounted),
+#    an active merge uses the 'snapshot-merge' target
+lvchange --refresh $vg/$lv1
+dmsetup table ${vg}-${lv1} | grep -q " snapshot-merge "
+# -- don't care if merge is still active; lvremove at this point
+#    may test stopping an active merge
 lvremove -f $vg/$lv1
 
 

LVM2 ./WHATS_NEW ./WHATS_NEW_DM libdm/libdm-de ...

[Alasdair G Kergon]

On Mon, Apr 19, 2010 at 06:38:20PM -0400, Mike Snitzer wrote:
> On Wed, Apr 07 2010 at  4:04pm -0400,
> agk at sourceware.org <agk@sourceware.org> wrote:
 
> >  		/* Refresh open_count */
> >  		if (!_info_by_dev(dinfo->major, dinfo->minor, 1, &info) ||
> > -		    !info.exists || info.open_count)
> > +		    !info.exists)
> >  			continue;
> >  
> > +		if (info.open_count) {

> It would appear that the non-zero open_count associated with the -real
> device is stale (during lvremove's dm_tree_deactivate_children).  

Shouldn't be - it follows a 'refresh'.

Alasdair

LVM2 ./WHATS_NEW ./WHATS_NEW_DM libdm/libdm-de ...

[Mike Snitzer]

On Mon, Apr 19 2010 at  7:25pm -0400,
Alasdair G Kergon <agk@redhat.com> wrote:

> On Mon, Apr 19, 2010 at 06:38:20PM -0400, Mike Snitzer wrote:
> > On Wed, Apr 07 2010 at  4:04pm -0400,
> > agk at sourceware.org <agk@sourceware.org> wrote:
>  
> > >  		/* Refresh open_count */
> > >  		if (!_info_by_dev(dinfo->major, dinfo->minor, 1, &info) ||
> > > -		    !info.exists || info.open_count)
> > > +		    !info.exists)
> > >  			continue;
> > >  
> > > +		if (info.open_count) {
> 
> > It would appear that the non-zero open_count associated with the -real
> > device is stale (during lvremove's dm_tree_deactivate_children).  
> 
> Shouldn't be - it follows a 'refresh'.

You're right, it's not stale at all.

So the 2 issues I've found:
1) removal of snapshot with pending onactivate merge doesn't cleanup
   snapshot like normal (normal being "case A" below) -- it does "case B"
2) completely tangential but: when a merge is active the merging
   snapshot LV is accessible (by user with lvremove); so removing the
   merging snapshot is currently allowed (stops merge, deletes
   snapshot); should it be allowed?

The following is more of a "note to self" that I've collected while
looking into this.. but feel free to review it ("case B" below
elaborates on why the t-snapshot-merge.sh test was failing).

"case B" preloads the origin when cleaning up for a merge (I believe
this explains why we attempt to cleanup -real early).  See commit
eaef92b02f968856 -- the vg_remove_snapshot changes in particular. 

I've yet to arrive at a fix for the attempt to cleanup -real too early
in case B (which triggers the _dm_tree_deactivate_children: r = 0); it
involves metadata/deptree associations not reflecting the kernel
(because of pending onactivate merge metadata) -- so vg_remove_snapshot
preloads origin.


A)
Here is a normal snapshot deactivate/remove:

# lvremove -f test/testlv1_snap
  _dm_tree_deactivate_children: deactivate test-testlv1_snap (253:3) level=0, open_count=0
  _dm_tree_deactivate_children: deactivate test-testlv1-real (253:4) level=1, open_count=1
  _dm_tree_deactivate_children: deactivate test-testlv1_snap-cow (253:5) level=1, open_count=0
  _dm_tree_deactivate_children: deactivate test-testlv1-real (253:4) level=0, open_count=0
  Logical volume "testlv1_snap" successfully removed

NOTE that the -real's level changes from 1 to 0 (this is because
snapshot-origin is still active).

Also NOTE that -real's open_count=1 above because test-testlv1 (still
using snapshot-origin) doesn't get reloaded to use the linear target
until the end (origin is _not_ preloaded by vg_remove_snapshot in this
case).


B)
Here is the sequence for the t-snapshot-merge.sh case that returns 0
from _dm_tree_deactivate_children (though I commented out the r = 0).
I refresh'd while the origin device was still mounted then lvremove; so
it has snapshot-merge metadata but snapshot is active in the kernel:

+ dmsetup table
LVMTEST9035vg-LV1: 0 98304 snapshot-origin 253:6
LVMTEST9035vg-LV1_snap: 0 98304 snapshot 253:6 253:7 P 8
LVMTEST9035vg-LV1_snap-cow: 0 16384 linear 253:3 98688
LVMTEST9035vg-LV1-real: 0 98304 linear 253:3 384

NOTE: first thing the following lvremove does is reload
LVMTEST9035vg-LV1 to use linear target, prior to reload
LVMTEST9035vg-LV1's only dep was -real.  But because of
snapshot-merge metadata LVMTEST9035vg-LV1-real has level=0 (because of
preload).

+ lvremove -f LVMTEST9035vg/LV1_snap
  _dm_tree_deactivate_children: deactivate LVMTEST9035vg-LV1-real (253:6) level=0, open_count=1
  Unable to deactivate open LVMTEST9035vg-LV1-real (253:6)
  _dm_tree_deactivate_children: deactivate LVMTEST9035vg-LV1_snap (253:5) level=0, open_count=0
  _dm_tree_deactivate_children: deactivate LVMTEST9035vg-LV1-real (253:6) level=1, open_count=0
  _dm_tree_deactivate_children: deactivate LVMTEST9035vg-LV1_snap-cow (253:7) level=1, open_count=0
  Logical volume "LV1_snap" successfully removed

NOTE that the -real's level changes from 0 to 1.

[PATCH] conditionally avoid preloading the origin in vg_remove_snapshot

[Mike Snitzer]

On Wed, Apr 21 2010 at  2:35am -0400,
Mike Snitzer <snitzer@redhat.com> wrote:

> On Mon, Apr 19 2010 at  7:25pm -0400,
> Alasdair G Kergon <agk@redhat.com> wrote:
> 
> > On Mon, Apr 19, 2010 at 06:38:20PM -0400, Mike Snitzer wrote:
> > > On Wed, Apr 07 2010 at  4:04pm -0400,
> > > agk at sourceware.org <agk@sourceware.org> wrote:
> >  
> > > >  		/* Refresh open_count */
> > > >  		if (!_info_by_dev(dinfo->major, dinfo->minor, 1, &info) ||
> > > > -		    !info.exists || info.open_count)
> > > > +		    !info.exists)
> > > >  			continue;
> > > >  
> > > > +		if (info.open_count) {
> > 
> > > It would appear that the non-zero open_count associated with the -real
> > > device is stale (during lvremove's dm_tree_deactivate_children).  
> > 
> > Shouldn't be - it follows a 'refresh'.
> 
> You're right, it's not stale at all.
> 
> So the 2 issues I've found:
> 1) removal of snapshot with pending onactivate merge doesn't cleanup
>    snapshot like normal (normal being "case A" below) -- it does "case B"
> 2) completely tangential but: when a merge is active the merging
>    snapshot LV is accessible (by user with lvremove); so removing the
>    merging snapshot is currently allowed (stops merge, deletes
>    snapshot); should it be allowed?
> 
> The following is more of a "note to self" that I've collected while
> looking into this.. but feel free to review it ("case B" below
> elaborates on why the t-snapshot-merge.sh test was failing).
> 
> "case B" preloads the origin when cleaning up for a merge (I believe
> this explains why we attempt to cleanup -real early).  See commit
> eaef92b02f968856 -- the vg_remove_snapshot changes in particular. 
> 
> I've yet to arrive at a fix for the attempt to cleanup -real too early
> in case B (which triggers the _dm_tree_deactivate_children: r = 0); it
> involves metadata/deptree associations not reflecting the kernel
> (because of pending onactivate merge metadata) -- so vg_remove_snapshot
> preloads origin.

Here is a fix:

Avoid preloading the origin, when removing a snapshot, if snapshot-merge
target is not active.

[This adds a 2nd consumer of lv_has_target_type().  I'm still not seeing
 an alternative way to test if snapshot-merge was performed.]

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 lib/activate/activate.h       |    3 +++
 lib/activate/dev_manager.c    |   10 ++++------
 lib/metadata/snapshot_manip.c |   21 +++++++++++++++------
 test/t-snapshot-merge.sh      |   17 +++++++++++++++++
 4 files changed, 39 insertions(+), 12 deletions(-)

diff --git a/lib/activate/activate.h b/lib/activate/activate.h
index 32d9a5c..019f44c 100644
--- a/lib/activate/activate.h
+++ b/lib/activate/activate.h
@@ -95,6 +95,9 @@ int lvs_in_vg_opened(const struct volume_group *vg);
 
 int lv_is_active(struct logical_volume *lv);
 
+int lv_has_target_type(struct dm_pool *mem, struct logical_volume *lv,
+		       const char *layer, const char *target_type);
+
 int monitor_dev_for_events(struct cmd_context *cmd,
 			    struct logical_volume *lv, int do_reg);
 
diff --git a/lib/activate/dev_manager.c b/lib/activate/dev_manager.c
index 66cc94c..cc4e046 100644
--- a/lib/activate/dev_manager.c
+++ b/lib/activate/dev_manager.c
@@ -329,10 +329,8 @@ static int _status(const char *name, const char *uuid,
 	return 0;
 }
 
-static int _lv_has_target_type(struct dev_manager *dm,
-			       struct logical_volume *lv,
-			       const char *layer,
-			       const char *target_type)
+int lv_has_target_type(struct dm_pool *mem, struct logical_volume *lv,
+		       const char *layer, const char *target_type)
 {
 	int r = 0;
 	char *dlid;
@@ -343,7 +341,7 @@ static int _lv_has_target_type(struct dev_manager *dm,
 	char *type = NULL;
 	char *params = NULL;
 
-	if (!(dlid = build_dm_uuid(dm->mem, lv->lvid.s, layer)))
+	if (!(dlid = build_dm_uuid(mem, lv->lvid.s, layer)))
 		return_0;
 
 	if (!(dmt = _setup_task(NULL, dlid, 0,
@@ -1183,7 +1181,7 @@ static int _add_new_lv_to_dtree(struct dev_manager *dm, struct dm_tree *dtree,
 		    ((dinfo = _cached_info(dm->mem, find_merging_cow(lv)->cow,
 					   dtree)) && dinfo->open_count)) {
 			/* FIXME Is there anything simpler to check for instead? */
-			if (!_lv_has_target_type(dm, lv, NULL, "snapshot-merge"))
+			if (!lv_has_target_type(dm->mem, lv, NULL, "snapshot-merge"))	
 				clear_snapshot_merge(lv);
 		}
 	}
diff --git a/lib/metadata/snapshot_manip.c b/lib/metadata/snapshot_manip.c
index f7218c2..cb5df6b 100644
--- a/lib/metadata/snapshot_manip.c
+++ b/lib/metadata/snapshot_manip.c
@@ -18,6 +18,7 @@
 #include "locking.h"
 #include "toolcontext.h"
 #include "lv_alloc.h"
+#include "activate.h"
 
 int lv_is_origin(const struct logical_volume *lv)
 {
@@ -176,16 +177,24 @@ int vg_remove_snapshot(struct logical_volume *cow)
 
 	dm_list_del(&cow->snapshot->origin_list);
 	origin->origin_count--;
+
 	if (find_merging_cow(origin) == find_cow(cow)) {
 		clear_snapshot_merge(origin);
 		/*
-		 * preload origin to:
-		 * - allow proper release of -cow
-		 * - avoid allocations with other devices suspended
-		 *   when transitioning from "snapshot-merge" to
-		 *   "snapshot-origin after a merge completes.
+		 * preload origin IFF "snapshot-merge" target is active
+		 * - IMPORTANT: avoids preload if onactivate merge is pending
 		 */
-		preload_origin = 1;
+		if (lv_has_target_type(origin->vg->cmd->mem, origin, NULL,
+				       "snapshot-merge")) {
+			/*
+			 * preload origin to:
+			 * - allow proper release of -cow
+			 * - avoid allocations with other devices suspended
+			 *   when transitioning from "snapshot-merge" to
+			 *   "snapshot-origin after a merge completes.
+			 */
+			preload_origin = 1;
+		}
 	}
 
 	if (!lv_remove(cow->snapshot->lv)) {
diff --git a/test/t-snapshot-merge.sh b/test/t-snapshot-merge.sh
index ff78f14..d7239b9 100755
--- a/test/t-snapshot-merge.sh
+++ b/test/t-snapshot-merge.sh
@@ -76,6 +76,23 @@ dmsetup table ${vg}-${lv1} | grep -q " snapshot-merge "
 lvremove -f $vg/$lv1
 
 
+# "onactivate merge" test
+# -- deactivate/remove after disallowed merge attempt, tests
+#    to make sure preload of origin's metadata is _not_ performed
+setup_merge $vg $lv1
+lvs -a
+mkdir test_mnt
+mount $(lvdev_ $vg $lv1) test_mnt
+lvconvert --merge $vg/$(snap_lv_name_ $lv1)
+# -- refresh LV while FS is still mounted (merge must not start),
+#    verify 'snapshot-origin' target is still being used
+lvchange --refresh $vg/$lv1
+umount test_mnt
+rm -r test_mnt
+dmsetup table ${vg}-${lv1} | grep -q " snapshot-origin "
+lvremove -f $vg/$lv1
+
+
 # test multiple snapshot merge; tests copy out that is driven by merge
 setup_merge $vg $lv1 1
 lvs -a

[PATCH] conditionally avoid preloading the origin in vg_remove_snapshot

[Alasdair G Kergon]

On Wed, Apr 21, 2010 at 01:59:00PM -0400, Mike Snitzer wrote:
> Avoid preloading the origin, when removing a snapshot, if snapshot-merge
> target is not active.
> [This adds a 2nd consumer of lv_has_target_type().  I'm still not seeing
>  an alternative way to test if snapshot-merge was performed.]
 
If it works, exporting that is fine for now.

Alasdair

Allowing an actively merging snapshot to be removed?

[Mike Snitzer]

On Wed, Apr 21 2010 at  2:35am -0400,
Mike Snitzer <snitzer@redhat.com> wrote:

>    when a merge is active the merging
>    snapshot LV is accessible (by user with lvremove); so removing the
>    merging snapshot is currently allowed (stops merge, deletes
>    snapshot); should it be allowed?

Hi Mikulas,

What are your thoughts on this corner case?

thanks,
Mike

Allowing an actively merging snapshot to be removed?

[Mikulas Patocka]

On Wed, 21 Apr 2010, Mike Snitzer wrote:

> On Wed, Apr 21 2010 at  2:35am -0400,
> Mike Snitzer <snitzer@redhat.com> wrote:
> 
> >    when a merge is active the merging
> >    snapshot LV is accessible (by user with lvremove); so removing the
> >    merging snapshot is currently allowed (stops merge, deletes
> >    snapshot); should it be allowed?
> 
> Hi Mikulas,
> 
> What are your thoughts on this corner case?
> 
> thanks,
> Mike

I think the merging snapshot LV shouldn't be accessible (I initially coded 
it so that it is renamed and disappears completely). There is nothing that 
the admin can do with the merging snapshot, so you don't have to show it 
in "lvs" output.

You can't allow the admin to delete the merging snapshot. This would cause 
data loss. The origin contains transient data during merge process 
(partially the old and partially the new volume), if you delete the 
merging snapshot and directly access the origin, the result is a mess.

You can assume that the admin doesn't know the exact logic of merging 
(i.e. he may not be aware that deleting the merging snapshot destroys all 
the data on the origin instantly), so you must definitely prevent the 
tools from doing this.

You should allow the admin to delete the origin (and delete the merging 
snapshot with it). You can also allow deleting of other non-merging 
snapshots, while the merge is in progress.

Mikulas

Allowing an actively merging snapshot to be removed?

[Alasdair G Kergon]

On Wed, Apr 21, 2010 at 08:01:32PM -0400, Mikulas Patocka wrote:
> I think the merging snapshot LV shouldn't be accessible 

Ack.  We are past the user-generated 'commit' point at which it goes.

Alasdair

Allowing an actively merging snapshot to be removed?

[Mike Snitzer]

On Thu, Apr 22 2010 at  7:36pm -0400,
Alasdair G Kergon <agk@redhat.com> wrote:

> On Wed, Apr 21, 2010 at 08:01:32PM -0400, Mikulas Patocka wrote:
> > I think the merging snapshot LV shouldn't be accessible 
> 
> Ack.  We are past the user-generated 'commit' point at which it goes.

The following patch is one way to address this bug.  Given the current
merge support in lvm2 this is the most straight-forward fix.  I've
tested this to work well.  Does the following seem reasonable?


Disallow the direct removal of a merging snapshot.

Allow lv_remove_with_dependencies() to know the top-level LV that was
requested to be removed (before it recurses and loses context).

A merging snapshot cannot be removed directly but the associated origin
can be.  Disallow removal of a merging snapshot unless the associated
origin is also being removed.

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 lib/metadata/lv_manip.c          |   17 +++++++++++++----
 lib/metadata/metadata-exported.h |    2 +-
 lib/metadata/metadata.c          |    2 +-
 test/t-snapshot-merge.sh         |   12 +++++++++---
 tools/lvremove.c                 |    2 +-
 5 files changed, 25 insertions(+), 10 deletions(-)

Index: lvm2/lib/metadata/lv_manip.c
===================================================================
--- lvm2.orig/lib/metadata/lv_manip.c
+++ lvm2/lib/metadata/lv_manip.c
@@ -2306,16 +2306,25 @@ int lv_remove_single(struct cmd_context 
  * remove LVs with its dependencies - LV leaf nodes should be removed first
  */
 int lv_remove_with_dependencies(struct cmd_context *cmd, struct logical_volume *lv,
-				const force_t force)
+				const force_t force, unsigned level)
 {
 	struct dm_list *snh, *snht;
 
-        if (lv_is_origin(lv)) {
+	if (lv_is_cow(lv)) {
+		/* A merging snapshot cannot be removed directly */
+		if (lv_is_merging_cow(lv) && !level) {
+			log_error("Can't remove merging snapshot logical volume \"%s\"",
+				  lv->name);
+			return 0;
+		}
+	}
+
+	if (lv_is_origin(lv)) {
 		/* remove snapshot LVs first */
 		dm_list_iterate_safe(snh, snht, &lv->snapshot_segs) {
 			if (!lv_remove_with_dependencies(cmd, dm_list_struct_base(snh, struct lv_segment,
-									       origin_list)->cow,
-							 force))
+										  origin_list)->cow,
+							 force, level + 1))
 				return 0;
 		}
 	}
Index: lvm2/test/t-snapshot-merge.sh
===================================================================
--- lvm2.orig/test/t-snapshot-merge.sh
+++ lvm2/test/t-snapshot-merge.sh
@@ -44,17 +44,22 @@ setup_merge() {
 aux prepare_vg 1 100
 
 
-# full merge of a single LV
+# test full merge of a single LV
 setup_merge $vg $lv1
-
 # now that snapshot LV is created: test if snapshot-merge target is available
 $(dmsetup targets | grep -q snapshot-merge) || exit 200
-
 lvs -a
 lvconvert --merge $vg/$(snap_lv_name_ $lv1)
 lvremove -f $vg/$lv1
 
 
+# test that an actively merging snapshot may not be removed
+setup_merge $vg $lv1
+lvconvert -i+100 --merge --background $vg/$(snap_lv_name_ $lv1)
+not lvremove -f $vg/$(snap_lv_name_ $lv1)
+lvremove -f $vg/$lv1
+
+
 # "onactivate merge" test
 setup_merge $vg $lv1
 lvs -a
@@ -99,6 +104,7 @@ lvs -a
 lvconvert --merge $vg/$(snap_lv_name_ $lv1)
 lvremove -f $vg/$lv1
 
+
 # test merging multiple snapshots that share the same tag
 setup_merge $vg $lv1
 setup_merge $vg $lv2
Index: lvm2/lib/metadata/metadata-exported.h
===================================================================
--- lvm2.orig/lib/metadata/metadata-exported.h
+++ lvm2/lib/metadata/metadata-exported.h
@@ -534,7 +534,7 @@ int lv_remove_single(struct cmd_context 
 		     force_t force);
 
 int lv_remove_with_dependencies(struct cmd_context *cmd, struct logical_volume *lv,
-				force_t force);
+				force_t force, unsigned level);
 
 int lv_rename(struct cmd_context *cmd, struct logical_volume *lv,
 	      const char *new_name);
Index: lvm2/lib/metadata/metadata.c
===================================================================
--- lvm2.orig/lib/metadata/metadata.c
+++ lvm2/lib/metadata/metadata.c
@@ -484,7 +484,7 @@ int remove_lvs_in_vg(struct cmd_context 
 
 	while ((lst = dm_list_first(&vg->lvs))) {
 		lvl = dm_list_item(lst, struct lv_list);
-		if (!lv_remove_with_dependencies(cmd, lvl->lv, force))
+		if (!lv_remove_with_dependencies(cmd, lvl->lv, force, 0))
 		    return 0;
 	}
 
Index: lvm2/tools/lvremove.c
===================================================================
--- lvm2.orig/tools/lvremove.c
+++ lvm2/tools/lvremove.c
@@ -26,7 +26,7 @@ static int lvremove_single(struct cmd_co
         if (lv_is_cow(lv) && lv_is_virtual_origin(origin = origin_from_cow(lv)))
                 lv = origin;
 
-	if (!lv_remove_with_dependencies(cmd, lv, arg_count(cmd, force_ARG))) {
+	if (!lv_remove_with_dependencies(cmd, lv, arg_count(cmd, force_ARG), 0)) {
 		stack;
 		return ECMD_FAILED;
 	}