Re: [Qemu-devel] [PATCH v7 04/10] block: support dropping active in bdrv_drop_intermediate

[Kevin Wolf <kwolf@redhat.com>]

Am 12.12.2013 um 09:23 hat Fam Zheng geschrieben:
> Dropping intermediate could be useful both for commit and stream, and
> BDS refcnt plus bdrv_swap could do most of the job nicely. It also need
> some improvements in preparation for op blockers.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block.c        | 152 +++++++++++++++++++++++++++------------------------------
>  block/commit.c |   1 +
>  2 files changed, 74 insertions(+), 79 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 41562fd..681d3be 100644
> --- a/block.c
> +++ b/block.c
> @@ -2163,114 +2163,108 @@ BlockDriverState *bdrv_find_overlay(BlockDriverState *active,
>      return overlay;
>  }
>  
> -typedef struct BlkIntermediateStates {
> -    BlockDriverState *bs;
> -    QSIMPLEQ_ENTRY(BlkIntermediateStates) entry;
> -} BlkIntermediateStates;
> -
> +static void bdrv_set_backing_hd(BlockDriverState *bs,
> +                                BlockDriverState *new_backing)
> +{
> +    if (bs->backing_hd) {
> +        bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker);
> +    }
> +    bs->backing_hd = new_backing;
> +    if (new_backing) {
> +        bdrv_op_block_all(bs->backing_hd, bs->backing_blocker);

What about unblocking commit, like you did in patch 3?

Should bdrv_open_backing_file() be using this function?

> +    }
> +}
>  
>  /*
> - * Drops images above 'base' up to and including 'top', and sets the image
> - * above 'top' to have base as its backing file.
> + * 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.
>   *
> - * Requires that the overlay to 'top' is opened r/w, so that the backing file
> - * information in 'bs' can be properly updated.
> + * 1) This will convert the following chain:
> + * ... <- base <- ... <- top <- overlay <-... <- active
>   *
> - * E.g., this will convert the following chain:
> - * bottom <- base <- intermediate <- top <- active
> + * to
> + *
> + * ... <- base <- overlay <- active
> + *
> + * 2) It is allowed for bottom==base, in which case it converts:
> + *
> + * ... <- base <- ... <- top <- overlay <- ... <- active
>   *
>   * to
>   *
> - * bottom <- base <- active
> + * base <- overlay <- active
>   *
> - * It is allowed for bottom==base, in which case it converts:
> + * 2) It also allows active==top, in which case it converts:
>   *
> - * base <- intermediate <- top <- active
> + * ... <- base <- ... <- top (active)
>   *
>   * to
>   *
> - * base <- active
> + * base == active == top, i.e. only base and lower remains: *top == *base when
> + * return.
> + *
> + * 3) If base==NULL, it will drop all the BDS below overlay and set its
> + * backing_hd to NULL. I.e.:
>   *
> - * Error conditions:
> - *  if active == top, that is considered an error
> + * base(NULL) <- ... <- overlay <- ... <- active
>   *
> + * to
> + *
> + * overlay <- ... <- active
>   */
>  int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
>                             BlockDriverState *base)
>  {
> -    BlockDriverState *intermediate;
> -    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;
> -    }
> +    BlockDriverState *drop_start, *overlay;
> +    int ret = -EINVAL;
>  
> -    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 */
> +    if (!top->drv || (base && !base->drv)) {
>          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) {
> +    if (top == 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;
> +    } else if (top == active) {
> +        assert(base);
> +        drop_start = active->backing_hd;
> +        bdrv_swap(active, base);
> +        base->backing_hd = NULL;
> +        bdrv_unref(drop_start);
> +        ret = 0;
> +    } else {
> +        /* 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;
> +        }
> +        if (base) {
> +            ret = bdrv_change_backing_file(overlay, base->filename,
> +                                           base->drv->format_name);
> +        } else {
> +            ret = bdrv_change_backing_file(overlay, "", "");

This should be NULL, NULL instead of empty strings.

Kevin