[dm-devel] Re: dm snapshot: allow live exception store handover between tables

[Mike Snitzer]

On Fri, Nov 06 2009 at  1:46pm -0500,
Mike Snitzer <snitzer at redhat.com> wrote:

> Alasdair,
> 
> Here is the incremental patch that illustrates the relevant changes that
> I layered ontop of the current patch you have in your tree.
> 
> . changed snapshot_ctr() to avoid read_metadata entirely for
>   snapshot-merge (unnecessary if handover will occur)
>   - adjusted a few other things in snapshot_ctr to accomodate early
>     return in the case of handover
> 
> . new snapshot's ti->split_io is now reset in handover_exceptions() to
>   match the chunk_size of the store that was just handed over
> 
> . register_snapshot() for snapshot-merge snapshot is now done after
>   handover_exceptions rather than in snapshot_ctr()
>   - not done in handover_exceptions() to avoid lock order issues between
>     s->lock and _origins_lock
> 
> . remove support for old->resume handover
>   - now that snapshot_ctr no longer does read_metadata() this is no
>     longer needed to support 'lvchange --refresh'

OK, actually in practice 'lvchange --refresh' is:

old->suspend
new->ctr
old->resume
 - device-mapper: snapshots: Unable to handover exceptions to another snapshot on resume.
new->resume
 - snapshot_resume: handing over exceptions

So it looks like we really do need to allow snapshot_resume to trigger
handover if the old is resumed before the new. (snapshot-merge is always
activated last by lvm because it acts as the origin).

I added this before, and  mistakenly thought it wasn't now:
http://patchwork.kernel.org/patch/57787/

Without it the old snapshot would be active with the exceptions that get
handed over to the snapshot-merge.  Quite bad.

Here is the incremental patch to add it back:

diff --git a/drivers/md/dm-snap.c b/drivers/md/dm-snap.c
index e5acff4..1ba1861 100644
--- a/drivers/md/dm-snap.c
+++ b/drivers/md/dm-snap.c
@@ -82,9 +82,10 @@ struct dm_snapshot {
 	 *
 	 * 'is_handover_destination' is set in snapshot_ctr if an existing
 	 * snapshot has the same cow device. The handover is performed,
-	 * and 'is_handover_destination' is cleared, when either of the
+	 * and 'is_handover_destination' is cleared, when one of the
 	 * following occurs:
 	 * - src (old) snapshot, that is handing over, is destructed
+	 * - src (old) snapshot, that is handing over, is resumed
 	 * - dest (new) snapshot, that is accepting the handover, is resumed
 	 */
 	int is_handover_destination;
@@ -1379,23 +1380,28 @@ static void snapshot_presuspend(struct dm_target *ti)
 static void snapshot_resume(struct dm_target *ti)
 {
 	struct dm_snapshot *s = ti->private;
-	struct dm_snapshot *old_snap, *new_snap = NULL;
+	struct dm_snapshot *lock_snap, *old_snap, *new_snap = NULL;
 
 	down_write(&s->lock);
 	if (s->handover_snap) {
-		if (!s->is_handover_destination) {
-			DMERR("Unable to handover exceptions to "
-			      "another snapshot on resume.");
-			goto out;
-		}
-		/* Get exception store from another snapshot */
+		/*
+		 * Initially assumes this snapshot will get
+		 * exception store from another snapshot
+		 */
 		old_snap = s->handover_snap;
 		new_snap = s;
+		lock_snap = old_snap;
 
-		down_write_nested(&old_snap->lock,
+		if (!s->is_handover_destination) {
+			/* Handover exceptions to another snapshot */
+			old_snap = s;
+			new_snap = s->handover_snap;
+			lock_snap = new_snap;
+		}
+		down_write_nested(&lock_snap->lock,
 				  SINGLE_DEPTH_NESTING);
 		handover_exceptions(old_snap, new_snap);
-		up_write(&old_snap->lock);
+		up_write(&lock_snap->lock);
 	}
 	/* An incomplete exception handover is not allowed */
 	BUG_ON(s->handover_snap);


I'll post v2 of the overall handover patch; it will include this patch.

Mike