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	2009-09-25 18:30:27

Modified files:
	.              : WHATS_NEW WHATS_NEW_DM 
	libdm          : libdm-deptree.c 

Log message:
	pre-release

Patches:
http://sourceware.org/cgi-bin/cvsweb.cgi/LVM2/WHATS_NEW.diff?cvsroot=lvm2&r1=1.1270&r2=1.1271
http://sourceware.org/cgi-bin/cvsweb.cgi/LVM2/WHATS_NEW_DM.diff?cvsroot=lvm2&r1=1.301&r2=1.302
http://sourceware.org/cgi-bin/cvsweb.cgi/LVM2/libdm/libdm-deptree.c.diff?cvsroot=lvm2&r1=1.57&r2=1.58

--- LVM2/WHATS_NEW	2009/09/17 10:37:23	1.1270
+++ LVM2/WHATS_NEW	2009/09/25 18:30:26	1.1271
@@ -1,6 +1,5 @@
-Version 2.02.53 - 
+Version 2.02.53 - 25th September 2009
 =====================================
-  Fix dmeventd _temporary_log_fn parameters. (2.02.50)
   Enable dmeventd monitoring section of config file by default.
   Update lvm2 monitoring script to lvm2_monitoring_init_red_hat.in.
   Fix lvm2app test to run under test/api subdirectory only when configured.
--- LVM2/WHATS_NEW_DM	2009/09/25 18:08:04	1.301
+++ LVM2/WHATS_NEW_DM	2009/09/25 18:30:26	1.302
@@ -1,6 +1,10 @@
-Version 1.02.38 - 
+Version 1.02.38 - 25th September 2009
 =====================================
-  Handle any path supplied to dm_task_set_name() by looking up in /dev/mapper.
+  Handle any path supplied to dm_task_set_name by looking up in /dev/mapper.
+  Add several examples to 12-dm-permissions.rules.
+  Add splitname and --yes to dmsetup man page.
+  Fix _mirror_emit_segment_line return code.
+  Fix dmeventd _temporary_log_fn parameters. (2.02.50)
 
 Version 1.02.37 - 15th September 2009
 =====================================
--- LVM2/libdm/libdm-deptree.c	2009/09/22 16:26:59	1.57
+++ LVM2/libdm/libdm-deptree.c	2009/09/25 18:30:27	1.58
@@ -1285,14 +1285,12 @@
 }
 
 /*
- * mirror_emit_segment_line
- *
  * Returns: 1 on success, 0 on failure
  */
-static int mirror_emit_segment_line(struct dm_task *dmt, uint32_t major,
-				    uint32_t minor, struct load_segment *seg,
-				    uint64_t *seg_start, char *params,
-				    size_t paramsize)
+static int _mirror_emit_segment_line(struct dm_task *dmt, uint32_t major,
+				     uint32_t minor, struct load_segment *seg,
+				     uint64_t *seg_start, char *params,
+				     size_t paramsize)
 {
 	int r;
 	int block_on_error = 0;
@@ -1421,8 +1419,8 @@
 		break;
 	case SEG_MIRRORED:
 		/* Mirrors are pretty complicated - now in separate function */
-		r = mirror_emit_segment_line(dmt, major, minor, seg, seg_start,
-					     params, paramsize);
+		r = _mirror_emit_segment_line(dmt, major, minor, seg, seg_start,
+					      params, paramsize);
 		if (!r)
 			return_0;
 		break;

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.

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

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

[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;
 }