[dm-devel] Re: clustered snapshots

[Mike Snitzer]

On Mon, Oct 05 2009 at 12:33am -0400,
Mike Snitzer <snitzer at redhat.com> wrote:

> On Sun, Oct 04 2009 at 10:25pm -0400,
> Mikulas Patocka <mpatocka at redhat.com> wrote:
> 
> > Hi
> > 
> > I don't understand the purpose of this patch. I think it's useless.
> 
> You don't understand it, so you think it's useless.. nice to know where
> I'm starting.
> 
> > - during merge, nothing except the merge process reads the on-disk 
> > exceptions. So it doesn't matter when you clean them. You must clean them 
> > before dropping the interlock, but the order is not important.
> 
> Sure, I thought the same but quickly realized that in practice order
> does matter if you'd like to avoid failing the in-core exception cleanup
> (IFF we allow chunks to be inserted before existing chunks; aka
> "back-merges").
> 
> Your original approach to cleaning all on-disk exceptions then all
> in-core has no hope of working for "back-merges".  Your original
> snapshot-merge completely eliminated insertions before existing chunks.

<snip - my long-winded defensiveness>

> Lesson I learned is that the in-core dm_snap_exception's consecutive
> chunk accounting is _really_ subtle.  And while I agree there should be
> no relation between the proper cleanup of on-disk and in-core
> exceptions; doing the cleanup of each in lock-step appears to be the
> only way forward (that I found).

If I just judge the location of the chunk within the in-core exception
support for back-merges works just as well.  Here is a minimalist patch
the accomplishes the same:

diff --git a/drivers/md/dm-snap.c b/drivers/md/dm-snap.c
index bcfbe85..b271f56 100644
--- a/drivers/md/dm-snap.c
+++ b/drivers/md/dm-snap.c
@@ -790,17 +790,25 @@ static void merge_callback(int read_err, unsigned long write_err, void *context)
         * so that dm_consecutive_chunk_count_dec() accounting works
         */
        for (i = s->merge_write_interlock_n - 1; i >= 0; i--) {
-               e = lookup_exception(&s->complete,
-                                    s->merge_write_interlock + i);
+               chunk_t old_chunk = s->merge_write_interlock + i;
+               e = lookup_exception(&s->complete, old_chunk);
                if (!e) {
                        DMERR("exception for block %llu is on "
                              "disk but not in memory",
-                             (unsigned long long)
-                             s->merge_write_interlock + i);
+                             (unsigned long long)old_chunk);
                        up_write(&s->lock);
                        goto shut;
                }
                if (dm_consecutive_chunk_count(e)) {
+                       if (old_chunk == e->old_chunk) {
+                               e->old_chunk++;
+                               e->new_chunk++;
+                       } else if (old_chunk != e->old_chunk +
+                                  dm_consecutive_chunk_count(e)) {
+                               DMERR("merge from the middle of a chunk range");
+                               up_write(&s->lock);
+                               goto shut;
+                       }
                        dm_consecutive_chunk_count_dec(e);
                } else {
                        remove_exception(e);