* LVM2 ./WHATS_NEW ./WHATS_NEW_DM libdm/libdm-de ... @ 2010-04-07 20:04 agk 2010-04-07 22:31 ` Petr Rockai 2010-04-19 22:38 ` Mike Snitzer 0 siblings, 2 replies; 13+ messages in thread From: agk @ 2010-04-07 20:04 UTC (permalink / raw) To: lvm-devel 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; } ^ permalink raw reply [flat|nested] 13+ messages in thread
* LVM2 ./WHATS_NEW ./WHATS_NEW_DM libdm/libdm-de ... 2010-04-07 20:04 LVM2 ./WHATS_NEW ./WHATS_NEW_DM libdm/libdm-de agk @ 2010-04-07 22:31 ` Petr Rockai 2010-04-07 23:00 ` Alasdair G Kergon 2010-04-19 22:38 ` Mike Snitzer 1 sibling, 1 reply; 13+ messages in thread From: Petr Rockai @ 2010-04-07 22:31 UTC (permalink / raw) To: lvm-devel 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* LVM2 ./WHATS_NEW ./WHATS_NEW_DM libdm/libdm-de ... 2010-04-07 22:31 ` Petr Rockai @ 2010-04-07 23:00 ` Alasdair G Kergon 0 siblings, 0 replies; 13+ messages in thread From: Alasdair G Kergon @ 2010-04-07 23:00 UTC (permalink / raw) To: lvm-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* LVM2 ./WHATS_NEW ./WHATS_NEW_DM libdm/libdm-de ... 2010-04-07 20:04 LVM2 ./WHATS_NEW ./WHATS_NEW_DM libdm/libdm-de agk 2010-04-07 22:31 ` Petr Rockai @ 2010-04-19 22:38 ` Mike Snitzer 2010-04-19 23:25 ` Alasdair G Kergon 1 sibling, 1 reply; 13+ messages in thread From: Mike Snitzer @ 2010-04-19 22:38 UTC (permalink / raw) To: lvm-devel 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* LVM2 ./WHATS_NEW ./WHATS_NEW_DM libdm/libdm-de ... 2010-04-19 22:38 ` Mike Snitzer @ 2010-04-19 23:25 ` Alasdair G Kergon 2010-04-21 6:35 ` Mike Snitzer 0 siblings, 1 reply; 13+ messages in thread From: Alasdair G Kergon @ 2010-04-19 23:25 UTC (permalink / raw) To: lvm-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* LVM2 ./WHATS_NEW ./WHATS_NEW_DM libdm/libdm-de ... 2010-04-19 23:25 ` Alasdair G Kergon @ 2010-04-21 6:35 ` Mike Snitzer 2010-04-21 17:59 ` [PATCH] conditionally avoid preloading the origin in vg_remove_snapshot Mike Snitzer 2010-04-21 19:38 ` Allowing an actively merging snapshot to be removed? Mike Snitzer 0 siblings, 2 replies; 13+ messages in thread From: Mike Snitzer @ 2010-04-21 6:35 UTC (permalink / raw) To: lvm-devel 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] conditionally avoid preloading the origin in vg_remove_snapshot 2010-04-21 6:35 ` Mike Snitzer @ 2010-04-21 17:59 ` Mike Snitzer 2010-04-23 0:11 ` Alasdair G Kergon 2010-04-21 19:38 ` Allowing an actively merging snapshot to be removed? Mike Snitzer 1 sibling, 1 reply; 13+ messages in thread From: Mike Snitzer @ 2010-04-21 17:59 UTC (permalink / raw) To: lvm-devel 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH] conditionally avoid preloading the origin in vg_remove_snapshot 2010-04-21 17:59 ` [PATCH] conditionally avoid preloading the origin in vg_remove_snapshot Mike Snitzer @ 2010-04-23 0:11 ` Alasdair G Kergon 0 siblings, 0 replies; 13+ messages in thread From: Alasdair G Kergon @ 2010-04-23 0:11 UTC (permalink / raw) To: lvm-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Allowing an actively merging snapshot to be removed? 2010-04-21 6:35 ` Mike Snitzer 2010-04-21 17:59 ` [PATCH] conditionally avoid preloading the origin in vg_remove_snapshot Mike Snitzer @ 2010-04-21 19:38 ` Mike Snitzer 2010-04-22 0:01 ` Mikulas Patocka 1 sibling, 1 reply; 13+ messages in thread From: Mike Snitzer @ 2010-04-21 19:38 UTC (permalink / raw) To: lvm-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Allowing an actively merging snapshot to be removed? 2010-04-21 19:38 ` Allowing an actively merging snapshot to be removed? Mike Snitzer @ 2010-04-22 0:01 ` Mikulas Patocka 2010-04-22 23:36 ` Alasdair G Kergon 0 siblings, 1 reply; 13+ messages in thread From: Mikulas Patocka @ 2010-04-22 0:01 UTC (permalink / raw) To: lvm-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Allowing an actively merging snapshot to be removed? 2010-04-22 0:01 ` Mikulas Patocka @ 2010-04-22 23:36 ` Alasdair G Kergon 2010-04-23 14:48 ` Mike Snitzer 0 siblings, 1 reply; 13+ messages in thread From: Alasdair G Kergon @ 2010-04-22 23:36 UTC (permalink / raw) To: lvm-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Allowing an actively merging snapshot to be removed? 2010-04-22 23:36 ` Alasdair G Kergon @ 2010-04-23 14:48 ` Mike Snitzer 0 siblings, 0 replies; 13+ messages in thread From: Mike Snitzer @ 2010-04-23 14:48 UTC (permalink / raw) To: lvm-devel 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; } ^ permalink raw reply [flat|nested] 13+ messages in thread
* LVM2 ./WHATS_NEW ./WHATS_NEW_DM libdm/libdm-de ... @ 2009-09-25 18:30 agk 0 siblings, 0 replies; 13+ messages in thread From: agk @ 2009-09-25 18:30 UTC (permalink / raw) To: lvm-devel 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; ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2010-04-23 14:48 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2010-04-07 20:04 LVM2 ./WHATS_NEW ./WHATS_NEW_DM libdm/libdm-de agk 2010-04-07 22:31 ` Petr Rockai 2010-04-07 23:00 ` Alasdair G Kergon 2010-04-19 22:38 ` Mike Snitzer 2010-04-19 23:25 ` Alasdair G Kergon 2010-04-21 6:35 ` Mike Snitzer 2010-04-21 17:59 ` [PATCH] conditionally avoid preloading the origin in vg_remove_snapshot Mike Snitzer 2010-04-23 0:11 ` Alasdair G Kergon 2010-04-21 19:38 ` Allowing an actively merging snapshot to be removed? Mike Snitzer 2010-04-22 0:01 ` Mikulas Patocka 2010-04-22 23:36 ` Alasdair G Kergon 2010-04-23 14:48 ` Mike Snitzer -- strict thread matches above, loose matches on Subject: below -- 2009-09-25 18:30 LVM2 ./WHATS_NEW ./WHATS_NEW_DM libdm/libdm-de agk
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.