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

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

@@ -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.

@@ -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.
 

@@ -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

@@ -1523,8 +1523,6 @@
 
 	r = _tree_action(dm, lv, DEACTIVATE);
 
-	fs_del_lv(lv);
-
 	return r;
 }
 

Hi,

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

<snip>

# 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.

index b7c0cb0..ff78f14 100755
@@ -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
 
 

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

Alasdair

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.

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.]

---
 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(-)

index 32d9a5c..019f44c 100644
@@ -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);
 

index 66cc94c..cc4e046 100644
@@ -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);
 		}
 	}

index f7218c2..cb5df6b 100644
@@ -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)) {

index ff78f14..d7239b9 100755
@@ -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

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

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

Alasdair

If it works, exporting that is fine for now.

Alasdair

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.

---
 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
===================================================================
@@ -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
===================================================================
@@ -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
===================================================================
@@ -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
===================================================================
@@ -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
===================================================================
@@ -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;
 	}