[Qemu-devel] [PATCH v2] block: support dropping active in bdrv_drop_intermediate

[Qemu-devel] [PATCH v2] block: support dropping active in bdrv_drop_intermediate

[Fam Zheng]

There is only one failure point: bdrv_change_backing_file in this
function, so we can drop the qlist and try to change the backing file
before deleting anything.

This way bdrv_drop_intermediate is simplified while keeping the
operation transactional. A bonus is dropping an active BDS is supported
too by swapping the base and top. Although no caller uses this yet, the
comment is updated to reflect the change.

Signed-off-by: Fam Zheng <famz@redhat.com>

---
v2: check for active, top and base being in a backing chain. (Jeff)

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c        | 103 ++++++++++++++++++++-------------------------------------
 block/commit.c |   1 +
 2 files changed, 37 insertions(+), 67 deletions(-)

diff --git a/block.c b/block.c
index fd05a80..9ead554 100644
--- a/block.c
+++ b/block.c
@@ -2130,18 +2130,11 @@ BlockDriverState *bdrv_find_overlay(BlockDriverState *active,
     return overlay;
 }
 
-typedef struct BlkIntermediateStates {
-    BlockDriverState *bs;
-    QSIMPLEQ_ENTRY(BlkIntermediateStates) entry;
-} BlkIntermediateStates;
-
-
 /*
- * Drops images above 'base' up to and including 'top', and sets the image
- * above 'top' to have base as its backing file.
- *
- * Requires that the overlay to 'top' is opened r/w, so that the backing file
- * information in 'bs' can be properly updated.
+ * Drops images above 'base' up to and including 'top', and sets new 'base'
+ * as backing_hd of top_overlay (the image orignally has 'top' as backing
+ * file). top_overlay may be NULL if 'top' is active, no such update needed.
+ * Requires that the top_overlay to 'top' is opened r/w.
  *
  * E.g., this will convert the following chain:
  * bottom <- base <- intermediate <- top <- active
@@ -2158,86 +2151,62 @@ typedef struct BlkIntermediateStates {
  *
  * base <- active
  *
- * Error conditions:
- *  if active == top, that is considered an error
+ * It also allows active==top, in which case it converts:
+ *
+ * base <- intermediate <- active (also top)
+ *
+ * to
+ *
+ * base == active == top, i.e. only base remains: *top == *base when return.
  *
  */
 int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
                            BlockDriverState *base)
 {
-    BlockDriverState *intermediate;
+    BlockDriverState *pbs;
+    BlockDriverState *overlay = NULL;
     BlockDriverState *base_bs = NULL;
-    BlockDriverState *new_top_bs = NULL;
-    BlkIntermediateStates *intermediate_state, *next;
-    int ret = -EIO;
-
-    QSIMPLEQ_HEAD(states_to_delete, BlkIntermediateStates) states_to_delete;
-    QSIMPLEQ_INIT(&states_to_delete);
+    int ret = -EINVAL;
 
     if (!top->drv || !base->drv) {
         goto exit;
     }
 
-    new_top_bs = bdrv_find_overlay(active, top);
-
-    if (new_top_bs == NULL) {
-        /* we could not find the image above 'top', this is an error */
-        goto exit;
-    }
-
-    /* special case of new_top_bs->backing_hd already pointing to base - nothing
-     * to do, no intermediate images */
-    if (new_top_bs->backing_hd == base) {
-        ret = 0;
+    if (!bdrv_find_overlay(active, base)) {
         goto exit;
     }
 
-    intermediate = 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;
+    if (active != top) {
+        /* If there's an overlay, its backing_hd points to top's BDS now,
+         * the top image is dropped but this BDS structure is kept and swapped
+         * with base, this way we keep the pointers valid after dropping top */
+        overlay = bdrv_find_overlay(active, top);
+        if (!overlay) {
+            goto exit;
+        }
+        ret = bdrv_change_backing_file(overlay, base->filename,
+                                       base->drv ?
+                                            base->drv->format_name : "");
+        if (ret) {
+            goto exit;
         }
-        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;
+    for (pbs = top->backing_hd; pbs != base; pbs = base_bs) {
+        assert(pbs);
+        base_bs = pbs->backing_hd;
+        pbs->backing_hd = NULL;
+        bdrv_unref(pbs);
     }
-    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_unref(intermediate_state->bs);
-    }
-    ret = 0;
+    bdrv_swap(base, top);
 
+    base->backing_hd = NULL;
+    bdrv_unref(base);
 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)
 {
diff --git a/block/commit.c b/block/commit.c
index d4090cb..4d8cd05 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -142,6 +142,7 @@ wait:
     if (!block_job_is_cancelled(&s->common) && sector_num == end) {
         /* success */
         ret = bdrv_drop_intermediate(active, top, base);
+        base = top;
     }
 
 exit_free_buf:
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2] block: support dropping active in bdrv_drop_intermediate

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1640 bytes --]

On 10/15/2013 01:25 AM, Fam Zheng wrote:
> There is only one failure point: bdrv_change_backing_file in this
> function, so we can drop the qlist and try to change the backing file
> before deleting anything.
> 
> This way bdrv_drop_intermediate is simplified while keeping the
> operation transactional. A bonus is dropping an active BDS is supported
> too by swapping the base and top. Although no caller uses this yet, the
> comment is updated to reflect the change.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> 
> ---
> v2: check for active, top and base being in a backing chain. (Jeff)
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block.c        | 103 ++++++++++++++++++++-------------------------------------
>  block/commit.c |   1 +
>  2 files changed, 37 insertions(+), 67 deletions(-)
> 

>  
>      if (!top->drv || !base->drv) {
>          goto exit;
>      }

So base->drv is non-NULL if we get here...

> +    if (active != top) {
> +        /* If there's an overlay, its backing_hd points to top's BDS now,
> +         * the top image is dropped but this BDS structure is kept and swapped
> +         * with base, this way we keep the pointers valid after dropping top */
> +        overlay = bdrv_find_overlay(active, top);
> +        if (!overlay) {
> +            goto exit;
> +        }
> +        ret = bdrv_change_backing_file(overlay, base->filename,
> +                                       base->drv ?

...yet you are checking it for NULL here.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH v2] block: support dropping active in bdrv_drop_intermediate

[Jeff Cody]

On Tue, Oct 15, 2013 at 03:25:00PM +0800, Fam Zheng wrote:
> There is only one failure point: bdrv_change_backing_file in this
> function, so we can drop the qlist and try to change the backing file
> before deleting anything.
> 
> This way bdrv_drop_intermediate is simplified while keeping the
> operation transactional. A bonus is dropping an active BDS is supported
> too by swapping the base and top. Although no caller uses this yet, the
> comment is updated to reflect the change.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> 
> ---
> v2: check for active, top and base being in a backing chain. (Jeff)

This does check for that, but it doesn't catch all errors.

It will verify:

[base] -> [active]

And verifies:

[top] -> [active]   (when active is != top)

However, it does not verify that the following is true:

[base] -> [top]

(e.g., it will pass on [top] -> [base] -> [active])

Rather than add another call to bdrv_find_overlay to verify the last
case, would just adding the bdrv_swap() and a check for active ==
top to the existing function do what you need for the active layer
support?


> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block.c        | 103 ++++++++++++++++++++-------------------------------------
>  block/commit.c |   1 +
>  2 files changed, 37 insertions(+), 67 deletions(-)
> 
> diff --git a/block.c b/block.c
> index fd05a80..9ead554 100644
> --- a/block.c
> +++ b/block.c
> @@ -2130,18 +2130,11 @@ BlockDriverState *bdrv_find_overlay(BlockDriverState *active,
>      return overlay;
>  }
>  
> -typedef struct BlkIntermediateStates {
> -    BlockDriverState *bs;
> -    QSIMPLEQ_ENTRY(BlkIntermediateStates) entry;
> -} BlkIntermediateStates;
> -
> -
>  /*
> - * Drops images above 'base' up to and including 'top', and sets the image
> - * above 'top' to have base as its backing file.
> - *
> - * Requires that the overlay to 'top' is opened r/w, so that the backing file
> - * information in 'bs' can be properly updated.
> + * Drops images above 'base' up to and including 'top', and sets new 'base'
> + * as backing_hd of top_overlay (the image orignally has 'top' as backing
> + * file). top_overlay may be NULL if 'top' is active, no such update needed.
> + * Requires that the top_overlay to 'top' is opened r/w.
>   *
>   * E.g., this will convert the following chain:
>   * bottom <- base <- intermediate <- top <- active
> @@ -2158,86 +2151,62 @@ typedef struct BlkIntermediateStates {
>   *
>   * base <- active
>   *
> - * Error conditions:
> - *  if active == top, that is considered an error
> + * It also allows active==top, in which case it converts:
> + *
> + * base <- intermediate <- active (also top)
> + *
> + * to
> + *
> + * base == active == top, i.e. only base remains: *top == *base when return.
>   *
>   */
>  int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
>                             BlockDriverState *base)
>  {
> -    BlockDriverState *intermediate;
> +    BlockDriverState *pbs;
> +    BlockDriverState *overlay = NULL;
>      BlockDriverState *base_bs = NULL;
> -    BlockDriverState *new_top_bs = NULL;
> -    BlkIntermediateStates *intermediate_state, *next;
> -    int ret = -EIO;
> -
> -    QSIMPLEQ_HEAD(states_to_delete, BlkIntermediateStates) states_to_delete;
> -    QSIMPLEQ_INIT(&states_to_delete);
> +    int ret = -EINVAL;
>  
>      if (!top->drv || !base->drv) {
>          goto exit;
>      }
>  
> -    new_top_bs = bdrv_find_overlay(active, top);
> -
> -    if (new_top_bs == NULL) {
> -        /* we could not find the image above 'top', this is an error */
> -        goto exit;
> -    }
> -
> -    /* special case of new_top_bs->backing_hd already pointing to base - nothing
> -     * to do, no intermediate images */
> -    if (new_top_bs->backing_hd == base) {
> -        ret = 0;
> +    if (!bdrv_find_overlay(active, base)) {
>          goto exit;
>      }
>  
> -    intermediate = 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;
> +    if (active != top) {
> +        /* If there's an overlay, its backing_hd points to top's BDS now,
> +         * the top image is dropped but this BDS structure is kept and swapped
> +         * with base, this way we keep the pointers valid after dropping top */
> +        overlay = bdrv_find_overlay(active, top);
> +        if (!overlay) {
> +            goto exit;
> +        }
> +        ret = bdrv_change_backing_file(overlay, base->filename,
> +                                       base->drv ?
> +                                            base->drv->format_name : "");
> +        if (ret) {
> +            goto exit;
>          }
> -        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;
> +    for (pbs = top->backing_hd; pbs != base; pbs = base_bs) {
> +        assert(pbs);
> +        base_bs = pbs->backing_hd;
> +        pbs->backing_hd = NULL;
> +        bdrv_unref(pbs);
>      }
> -    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_unref(intermediate_state->bs);
> -    }
> -    ret = 0;
> +    bdrv_swap(base, top);
>  
> +    base->backing_hd = NULL;
> +    bdrv_unref(base);
>  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)
>  {
> diff --git a/block/commit.c b/block/commit.c
> index d4090cb..4d8cd05 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -142,6 +142,7 @@ wait:
>      if (!block_job_is_cancelled(&s->common) && sector_num == end) {
>          /* success */
>          ret = bdrv_drop_intermediate(active, top, base);
> +        base = top;
>      }
>  
>  exit_free_buf:
> -- 
> 1.8.3.1
> 
>