[lvm-devel] [PATCH] conditionally avoid preloading the origin in vg_remove_snapshot
Mike Snitzer
snitzer at redhat.com
Wed Apr 21 17:59:00 UTC 2010
On Wed, Apr 21 2010 at 2:35am -0400,
Mike Snitzer <snitzer at redhat.com> wrote:
> On Mon, Apr 19 2010 at 7:25pm -0400,
> Alasdair G Kergon <agk at 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 at 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 at 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
More information about the lvm-devel
mailing list