Re: [Qemu-devel] [PATCH] block: simplify bdrv_drop_intermediate

[Jeff Cody <jcody@redhat.com>]

On Sat, Oct 12, 2013 at 02:05:45PM +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>
> ---
>  block.c        | 100 ++++++++++++++++++---------------------------------------
>  block/commit.c |   1 +
>  2 files changed, 32 insertions(+), 69 deletions(-)
> 
> diff --git a/block.c b/block.c
> index fd05a80..b9e073f 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,55 @@ 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);
> -
>      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;
> -        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);

bdrv_find_overlay() can return NULL, this needs to be checked for
error.

> +        ret = bdrv_change_backing_file(overlay, base->filename,
> +                                       base->drv ?
> +                                            base->drv->format_name : "");

Using 'base' here makes the assumption that 'base' is definitely in
the  BDS chain, starting from overlay.  This was previously explicitly
verified by walking down through chain (this function made sure that
all BDS's specified - active, top, base - were all in the same chain, in
the correct order).

I guess a case could be made that we don't need to do that check, as
the current usage of this function verifies (in qmp_block_commit())
that base is indeed part of top.  

If we are going to go with that assumption, we should explicitly
mention that base needs to be verified to be in the chain prior to
calling this function, otherwise we will assert().

> +        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);

So in the case I mentioned above, we will assert() here, but have
already modified the image file on disk described by new_top_bs, which
will now most likely have an invalid backing file.  

Although this seems to be an extremely unlikely (impossible?)
occurrence in the current usage, I'm not sure the benefit of changing
the function to remove this check - you could leave those checks in
place and keep the active layer support in this patch.

>      }
> -    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
>