Re: [Qemu-devel] [PATCH 13/16] block: Implement bdrv_append() without bdrv_swap()

[Max Reitz]

On 17.09.2015 15:48, Kevin Wolf wrote:
> Remember all parent nodes and just change the pointers there instead of
> swapping the contents of the BlockDriverState.
> 
> Handling of snapshot=on must be moved further down in bdrv_open()
> because *pbs (which is the bs pointer in the BlockBackend) must already
> be set before bdrv_append() is called. Otherwise bdrv_append() changes
> the BB's pointer to the temporary snapshot, but bdrv_open() overwrites
> it with the read-only original image.
> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  block.c | 109 
> +++++++++++++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 81 insertions(+), 28 deletions(-)
> 
> diff --git a/block.c b/block.c
> index c196f83..98fc17c 100644
> --- a/block.c
> +++ b/block.c
> @@ -1516,15 +1516,6 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
> const char *filename,
>  
>      bdrv_refresh_filename(bs);
>  
> -    /* For snapshot=on, create a temporary qcow2 overlay. bs points to the
> -     * temporary snapshot afterwards. */
> -    if (snapshot_flags) {
> -        ret = bdrv_append_temp_snapshot(bs, snapshot_flags, &local_err);
> -        if (local_err) {
> -            goto close_and_fail;
> -        }
> -    }
> -
>      /* Check if any unknown options were used */
>      if (options && (qdict_size(options) != 0)) {
>          const QDictEntry *entry = qdict_first(options);
> @@ -1556,6 +1547,16 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
> const char *filename,
>  
>      QDECREF(options);
>      *pbs = bs;
> +
> +    /* For snapshot=on, create a temporary qcow2 overlay. bs points to the
> +     * temporary snapshot afterwards. */
> +    if (snapshot_flags) {
> +        ret = bdrv_append_temp_snapshot(bs, snapshot_flags, &local_err);
> +        if (local_err) {
> +            goto close_and_fail;
> +        }
> +    }
> +
>      return 0;
>  
>  fail:
> @@ -1999,20 +2000,6 @@ static void bdrv_move_feature_fields(BlockDriverState 
> *bs_dest,
>  
>      bs_dest->enable_write_cache = bs_src->enable_write_cache;
>  
> -    /* i/o throttled req */
> -    bs_dest->throttle_state     = bs_src->throttle_state,
> -    bs_dest->io_limits_enabled  = bs_src->io_limits_enabled;
> -    bs_dest->pending_reqs[0]    = bs_src->pending_reqs[0];
> -    bs_dest->pending_reqs[1]    = bs_src->pending_reqs[1];
> -    bs_dest->throttled_reqs[0]  = bs_src->throttled_reqs[0];
> -    bs_dest->throttled_reqs[1]  = bs_src->throttled_reqs[1];
> -    memcpy(&bs_dest->round_robin,
> -           &bs_src->round_robin,
> -           sizeof(bs_dest->round_robin));
> -    memcpy(&bs_dest->throttle_timers,
> -           &bs_src->throttle_timers,
> -           sizeof(ThrottleTimers));
> -
>      /* r/w error */
>      bs_dest->on_read_error      = bs_src->on_read_error;
>      bs_dest->on_write_error     = bs_src->on_write_error;
> @@ -2026,10 +2013,25 @@ static void bdrv_move_feature_fields(BlockDriverState 
> *bs_dest,
>  }
>  
>  /* Fields that only need to be swapped if the contents of BDSes is swapped
> - * rather than pointers being changed in the parents. */
> + * rather than pointers being changed in the parents, and throttling fields
> + * because only bdrv_swap() messes with internals of throttling. */
>  static void bdrv_move_reference_fields(BlockDriverState *bs_dest,
>                                         BlockDriverState *bs_src)
>  {
> +    /* i/o throttled req */
> +    bs_dest->throttle_state     = bs_src->throttle_state,
> +    bs_dest->io_limits_enabled  = bs_src->io_limits_enabled;
> +    bs_dest->pending_reqs[0]    = bs_src->pending_reqs[0];
> +    bs_dest->pending_reqs[1]    = bs_src->pending_reqs[1];
> +    bs_dest->throttled_reqs[0]  = bs_src->throttled_reqs[0];
> +    bs_dest->throttled_reqs[1]  = bs_src->throttled_reqs[1];
> +    memcpy(&bs_dest->round_robin,
> +           &bs_src->round_robin,
> +           sizeof(bs_dest->round_robin));
> +    memcpy(&bs_dest->throttle_timers,
> +           &bs_src->throttle_timers,
> +           sizeof(ThrottleTimers));
> +
>      /* reference count */
>      bs_dest->refcnt             = bs_src->refcnt;
>  
> @@ -2155,6 +2157,42 @@ void bdrv_swap(BlockDriverState *bs_new, 
> BlockDriverState *bs_old)
>      bdrv_rebind(bs_old);
>  }
>  
> +static void change_parent_backing_link(BlockDriverState *from,
> +                                       BlockDriverState *to)
> +{
> +    BdrvChild *c, *next;
> +
> +    QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) {
> +        assert(c->role != &child_backing);
> +        c->bs = to;
> +        QLIST_REMOVE(c, next_parent);
> +        QLIST_INSERT_HEAD(&to->parents, c, next_parent);

This drops a reference from the parent BDS to @from, and adds a new one
from the parent BDS to @to. However, this is not reflected here.

> +    }
> +    if (from->blk) {
> +        blk_set_bs(from->blk, to);
> +        if (!to->device_list.tqe_prev) {
> +            QTAILQ_INSERT_BEFORE(from, to, device_list);
> +        }
> +        QTAILQ_REMOVE(&bdrv_states, from, device_list);
> +    }
> +}
> +
> +static void swap_feature_fields(BlockDriverState *bs_top,
> +                                BlockDriverState *bs_new)
> +{
> +    BlockDriverState tmp;
> +
> +    bdrv_move_feature_fields(&tmp, bs_top);
> +    bdrv_move_feature_fields(bs_top, bs_new);
> +    bdrv_move_feature_fields(bs_new, &tmp);
> +
> +    assert(!bs_new->io_limits_enabled);
> +    if (bs_top->io_limits_enabled) {
> +        bdrv_io_limits_enable(bs_new, throttle_group_get_name(bs_top));
> +        bdrv_io_limits_disable(bs_top);
> +    }
> +}
> +
>  /*
>   * Add new bs contents at the top of an image chain while the chain is
>   * live, while keeping required fields on the top layer.
> @@ -2165,14 +2203,29 @@ void bdrv_swap(BlockDriverState *bs_new, 
> BlockDriverState *bs_old)
>   * bs_new must not be attached to a BlockBackend.
>   *
>   * This function does not create any image files.
> + *
> + * bdrv_append() takes ownership of a bs_new reference and unrefs it because
> + * that's what the callers commonly need. bs_new will be referenced by the 
> old
> + * parents of bs_top after bdrv_append() returns. If the caller needs to 
> keep a
> + * reference of its own, it must call bdrv_ref().
>   */
>  void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
>  {
> -    bdrv_swap(bs_new, bs_top);
> +    assert(!bdrv_requests_pending(bs_top));
> +    assert(!bdrv_requests_pending(bs_new));
> +
> +    bdrv_ref(bs_top);
> +    change_parent_backing_link(bs_top, bs_new);
> +
> +    /* Some fields always stay on top of the backing file chain */
> +    swap_feature_fields(bs_top, bs_new);
> +
> +    bdrv_set_backing_hd(bs_new, bs_top);
> +    bdrv_unref(bs_top);
>  
> -    /* The contents of 'tmp' will become bs_top, as we are
> -     * swapping bs_new and bs_top contents. */
> -    bdrv_set_backing_hd(bs_top, bs_new);
> +    /* bs_new is now referenced by its new parents, we don't need the
> +     * additional reference any more. */
> +    bdrv_unref(bs_new);
>  }

Before, all pointers to @bs_new were moved to @bs_top. Now, they stay at
@bs_new. I suppose we are assuming there are no pointers to @bs_new,
should we assert that, and/or point it out in the documentation?

Max

>  
>  static void bdrv_delete(BlockDriverState *bs)
>

signature.asc
Description: OpenPGP digital signature