Patchwork [RFC,1/4] block: add support functions for live commit, to find and delete images.

login
register
mail settings
Submitter Jeff Cody
Date July 31, 2012, 5:16 a.m.
Message ID <e9ac3c39bf16a9f6e287a75f8332598acef51e9f.1343710713.git.jcody@redhat.com>
Download mbox | patch
Permalink /patch/174141/
State New
Headers show

Comments

Jeff Cody - July 31, 2012, 5:16 a.m.
Add bdrv_find_image(), bdrv_find_base(), and bdrv_delete_intermediate().

bdrv_find_image():  given a filename and a BDS, find the image in the chain
                    that matches the passed filename.

bdrv_find_base():   given a BDS, find the base image (parent-most image)

bdrv_delete_intermediate():
                    Given 3 BDS (active, top, base), delete images above
                    base up to and including top, and set base to be the
                    parent of top's child node.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block.c |  136 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 block.h |    4 ++
 2 files changed, 139 insertions(+), 1 deletion(-)
Eric Blake - July 31, 2012, 5:34 p.m.
On 07/30/2012 11:16 PM, Jeff Cody wrote:
> Add bdrv_find_image(), bdrv_find_base(), and bdrv_delete_intermediate().
> 
> bdrv_find_image():  given a filename and a BDS, find the image in the chain
>                     that matches the passed filename.
> 
> bdrv_find_base():   given a BDS, find the base image (parent-most image)
> 
> bdrv_delete_intermediate():
>                     Given 3 BDS (active, top, base), delete images above
>                     base up to and including top, and set base to be the
>                     parent of top's child node.

A diagram, as was in your cover letter, would help (either here, or
better yet in the comments describing this function in place)...

> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block.c |  136 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  block.h |    4 ++
>  2 files changed, 139 insertions(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index 522acd1..a3c8d33 100644
> --- a/block.c
> +++ b/block.c
> @@ -1408,7 +1408,7 @@ int bdrv_commit(BlockDriverState *bs)
>  
>      if (!drv)
>          return -ENOMEDIUM;
> -    
> +
>      if (!bs->backing_hd) {
>          return -ENOTSUP;
>      }

Spurious whitespace cleanup, since nothing else in this hunk belongs to
you.  Is that trailing whitespace present upstream, or was it introduced
in one of the patches you based off of?

> @@ -1661,6 +1661,110 @@ int bdrv_change_backing_file(BlockDriverState *bs,
>      return ret;
>  }
>  
> +typedef struct BlkIntermediateStates {
> +    BlockDriverState *bs;
> +    QSIMPLEQ_ENTRY(BlkIntermediateStates) entry;
> +} BlkIntermediateStates;
> +
> +
> +/* deletes images above 'base' up to and including 'top', and sets the image
> + * above 'top' to have base as its backing file.
> + */
> +int bdrv_delete_intermediate(BlockDriverState *active, BlockDriverState *top,
> +                             BlockDriverState *base)

...that is, I think this would aid the reader:

Converts:

bottom <- base <- intermediate <- top <- active

to

bottom <- base <- active

where top==active is permitted

> @@ -3128,6 +3232,36 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
>      return NULL;
>  }
>  
> +BlockDriverState *bdrv_find_image(BlockDriverState *bs,
> +                                  const char *filename)
> +{
> +    if (!bs || !bs->drv) {
> +        return NULL;
> +    }
> +
> +    if (strcmp(bs->filename, filename) == 0) {
> +        return bs;
> +    } else {
> +        return bdrv_find_image(bs->backing_hd, filename);

Tail-recursive; are we worried enough about ever hitting stack overflow
due to a really deep change to convert this into a while loop recursion
instead?  [Probably not]

> +    }
> +}
> +
> +BlockDriverState *bdrv_find_base(BlockDriverState *bs)
> +{
> +    BlockDriverState *curr_bs = NULL;
> +
> +    if (!bs) {
> +        return NULL;
> +    }
> +
> +    curr_bs = bs;
> +
> +    while (curr_bs->backing_hd) {
> +        curr_bs = curr_bs->backing_hd;
> +    }

Then again, here you did while-loop recursion, so using the same style
in both functions might be worthwhile.
Jeff Cody - July 31, 2012, 5:52 p.m.
On 07/31/2012 01:34 PM, Eric Blake wrote:
> On 07/30/2012 11:16 PM, Jeff Cody wrote:
>> Add bdrv_find_image(), bdrv_find_base(), and bdrv_delete_intermediate().
>>
>> bdrv_find_image():  given a filename and a BDS, find the image in the chain
>>                     that matches the passed filename.
>>
>> bdrv_find_base():   given a BDS, find the base image (parent-most image)
>>
>> bdrv_delete_intermediate():
>>                     Given 3 BDS (active, top, base), delete images above
>>                     base up to and including top, and set base to be the
>>                     parent of top's child node.
> 
> A diagram, as was in your cover letter, would help (either here, or
> better yet in the comments describing this function in place)...
>

Or even better, both :)

I'll add that in for v1.


>>
>> Signed-off-by: Jeff Cody <jcody@redhat.com>
>> ---
>>  block.c |  136 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>  block.h |    4 ++
>>  2 files changed, 139 insertions(+), 1 deletion(-)
>>
>> diff --git a/block.c b/block.c
>> index 522acd1..a3c8d33 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1408,7 +1408,7 @@ int bdrv_commit(BlockDriverState *bs)
>>  
>>      if (!drv)
>>          return -ENOMEDIUM;
>> -    
>> +
>>      if (!bs->backing_hd) {
>>          return -ENOTSUP;
>>      }
> 
> Spurious whitespace cleanup, since nothing else in this hunk belongs to
> you.  Is that trailing whitespace present upstream, or was it introduced
> in one of the patches you based off of?

Likely a spurious cleanup - I had several trailing whitespaces in my
block.c changes, and scripts/checkpatch.pl warned me of those.  I
cleaned them up, and I must have cleaned up an extra one with my regex.


> 
>> @@ -1661,6 +1661,110 @@ int bdrv_change_backing_file(BlockDriverState *bs,
>>      return ret;
>>  }
>>  
>> +typedef struct BlkIntermediateStates {
>> +    BlockDriverState *bs;
>> +    QSIMPLEQ_ENTRY(BlkIntermediateStates) entry;
>> +} BlkIntermediateStates;
>> +
>> +
>> +/* deletes images above 'base' up to and including 'top', and sets the image
>> + * above 'top' to have base as its backing file.
>> + */
>> +int bdrv_delete_intermediate(BlockDriverState *active, BlockDriverState *top,
>> +                             BlockDriverState *base)
> 
> ...that is, I think this would aid the reader:
> 
> Converts:
> 
> bottom <- base <- intermediate <- top <- active
> 
> to
> 
> bottom <- base <- active
> 
> where top==active is permitted

I agree that is better.  And, for clarity, bottom==base is permitted as
well.

> 
>> @@ -3128,6 +3232,36 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
>>      return NULL;
>>  }
>>  
>> +BlockDriverState *bdrv_find_image(BlockDriverState *bs,
>> +                                  const char *filename)
>> +{
>> +    if (!bs || !bs->drv) {
>> +        return NULL;
>> +    }
>> +
>> +    if (strcmp(bs->filename, filename) == 0) {
>> +        return bs;
>> +    } else {
>> +        return bdrv_find_image(bs->backing_hd, filename);
> 
> Tail-recursive; are we worried enough about ever hitting stack overflow
> due to a really deep change to convert this into a while loop recursion
> instead?  [Probably not]

Not too worried about it, because the chain should not be *that* long,
and also the block layer handles the chain in a similar fashion other
places, so we'll likely blow up in those places first :)

That said, when doing some automated live snapshot testing with an
obscene number of snapshots, I did manage to blow the stack (IIRC) in
the recursive open.  That was with something like a chain of 1500
snapshots, which seems a bit excessive.

But, I agree with your point below:

> 
>> +    }
>> +}
>> +
>> +BlockDriverState *bdrv_find_base(BlockDriverState *bs)
>> +{
>> +    BlockDriverState *curr_bs = NULL;
>> +
>> +    if (!bs) {
>> +        return NULL;
>> +    }
>> +
>> +    curr_bs = bs;
>> +
>> +    while (curr_bs->backing_hd) {
>> +        curr_bs = curr_bs->backing_hd;
>> +    }
> 
> Then again, here you did while-loop recursion, so using the same style
> in both functions might be worthwhile.
> 

Yes - maybe that is a good reason to have bdrv_find_image() be a
while-loop (I based it off of bdrv_find_backing_image(), which
is why it was recursive).  In general, I find recursive functions make
my brain hurt,  so I tend to like while-loops better :)

Patch

diff --git a/block.c b/block.c
index 522acd1..a3c8d33 100644
--- a/block.c
+++ b/block.c
@@ -1408,7 +1408,7 @@  int bdrv_commit(BlockDriverState *bs)
 
     if (!drv)
         return -ENOMEDIUM;
-    
+
     if (!bs->backing_hd) {
         return -ENOTSUP;
     }
@@ -1661,6 +1661,110 @@  int bdrv_change_backing_file(BlockDriverState *bs,
     return ret;
 }
 
+typedef struct BlkIntermediateStates {
+    BlockDriverState *bs;
+    QSIMPLEQ_ENTRY(BlkIntermediateStates) entry;
+} BlkIntermediateStates;
+
+
+/* deletes images above 'base' up to and including 'top', and sets the image
+ * above 'top' to have base as its backing file.
+ */
+int bdrv_delete_intermediate(BlockDriverState *active, BlockDriverState *top,
+                             BlockDriverState *base)
+{
+    BlockDriverState *intermediate;
+    BlockDriverState *base_bs = NULL;
+    BlockDriverState *new_top_bs = NULL;
+    BlkIntermediateStates *intermediate_state, *next;
+    int ret = -1;
+
+    QSIMPLEQ_HEAD(states_to_delete, BlkIntermediateStates) states_to_delete;
+    QSIMPLEQ_INIT(&states_to_delete);
+
+    if (!top->drv || !base->drv) {
+        goto exit;
+    }
+
+    /* special case of top->backing_hd already pointing to base - nothing
+     * to do, no intermediate images
+     */
+    if (top->backing_hd == base) {
+        ret = 0;
+        goto exit;
+    }
+
+    /* if the active bs layer is the same as the new top, then there
+     * is no image above the top, so it is also the new_top (and we must
+     * not delete it below, either)
+     */
+    if (active == top) {
+        new_top_bs = active;
+    } else {
+        intermediate = active;
+        while (intermediate->backing_hd) {
+            if (intermediate->backing_hd == top) {
+                new_top_bs = intermediate;
+                break;
+            }
+            intermediate = intermediate->backing_hd;
+        }
+    }
+
+    if (new_top_bs == NULL) {
+        /* we could not find the image above 'top', this is an error */
+        goto exit;
+    }
+
+    /* if the active and top image passed in are the same, then we
+     * can't delete the active, so we start one below
+     */
+    intermediate = (active == top) ? active->backing_hd : top;
+
+    /* now we will go down through the list, and add each BDS we find
+     * into our deletion queue, until we hit the 'base'
+     */
+    while (intermediate) {
+        intermediate_state = g_malloc0(sizeof(BlkIntermediateStates));
+        intermediate_state->bs = intermediate;
+        QSIMPLEQ_INSERT_TAIL(&states_to_delete, intermediate_state, entry);
+
+        if (intermediate->backing_hd == base) {
+            base_bs = intermediate->backing_hd;
+            break;
+        }
+        intermediate = intermediate->backing_hd;
+    }
+    if (base_bs == NULL) {
+        /* something went wrong, we did not end at the base. safely
+         * unravel everything, and exit with error */
+        goto exit;
+    }
+
+    /* success - we can delete the intermediate states, and link top->base */
+    ret = bdrv_change_backing_file(new_top_bs, base_bs->filename,
+                                   base_bs->drv ? base_bs->drv->format_name : "");
+    if (ret) {
+        goto exit;
+    }
+    new_top_bs->backing_hd = base_bs;
+
+
+    QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry, next) {
+        /* so that bdrv_close() does not recursively close the chain */
+        intermediate_state->bs->backing_hd = NULL;
+        bdrv_delete(intermediate_state->bs);
+    }
+    ret = 0;
+
+exit:
+    QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry, next) {
+        g_free(intermediate_state);
+    }
+    return ret;
+}
+
+
 static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset,
                                    size_t size)
 {
@@ -3128,6 +3232,36 @@  BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
     return NULL;
 }
 
+BlockDriverState *bdrv_find_image(BlockDriverState *bs,
+                                  const char *filename)
+{
+    if (!bs || !bs->drv) {
+        return NULL;
+    }
+
+    if (strcmp(bs->filename, filename) == 0) {
+        return bs;
+    } else {
+        return bdrv_find_image(bs->backing_hd, filename);
+    }
+}
+
+BlockDriverState *bdrv_find_base(BlockDriverState *bs)
+{
+    BlockDriverState *curr_bs = NULL;
+
+    if (!bs) {
+        return NULL;
+    }
+
+    curr_bs = bs;
+
+    while (curr_bs->backing_hd) {
+        curr_bs = curr_bs->backing_hd;
+    }
+    return curr_bs;
+}
+
 #define NB_SUFFIXES 4
 
 char *get_human_readable_size(char *buf, int buf_size, int64_t size)
diff --git a/block.h b/block.h
index 25e0230..70fa53c 100644
--- a/block.h
+++ b/block.h
@@ -183,6 +183,10 @@  int bdrv_change_backing_file(BlockDriverState *bs,
     const char *backing_file, const char *backing_fmt);
 void bdrv_register(BlockDriver *bdrv);
 int bdrv_change_hostcache(BlockDriverState *bs, bool enable_host_cache);
+BlockDriverState *bdrv_find_image(BlockDriverState *bs, const char *filename);
+BlockDriverState *bdrv_find_base(BlockDriverState *bs);
+int bdrv_delete_intermediate(BlockDriverState *active, BlockDriverState *top,
+                             BlockDriverState *base);
 
 
 typedef struct BdrvCheckResult {