Re: [Qemu-devel] [PATCH 15/16] block: Add and use bdrv_replace_in_backing_chain()

[Max Reitz <mreitz@redhat.com>]

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

On 17.09.2015 15:48, Kevin Wolf wrote:
> This cleans up the mess we left behind in the mirror code after the
> previous patch. Instead of using bdrv_swap(), just change pointers.
> 
> The interface change of the mirror job that callers must consider is
> that after job completion, their local BDS pointers still point to the
> same node now. qemu-img must change its code accordingly (which makes it
> easier to understand); the other callers stays unchanged because after
> completion they don't do anything with the BDS, but just with the job,
> and the job is still owned by the source BDS.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c               | 32 +++++++++++++++++++++++++++++++-
>  block/mirror.c        | 23 +++++++----------------
>  include/block/block.h |  4 +++-
>  qemu-img.c            | 16 ++++++++--------
>  4 files changed, 49 insertions(+), 26 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 98fc17c..7c21659 100644
> --- a/block.c
> +++ b/block.c
> @@ -1095,7 +1095,7 @@ static BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
>      return child;
>  }
>  
> -void bdrv_detach_child(BdrvChild *child)
> +static void bdrv_detach_child(BdrvChild *child)
>  {
>      QLIST_REMOVE(child, next);
>      QLIST_REMOVE(child, next_parent);
> @@ -2228,6 +2228,36 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
>      bdrv_unref(bs_new);
>  }
>  
> +void bdrv_replace_in_backing_chain(BlockDriverState *old, BlockDriverState *new)
> +{
> +    assert(!bdrv_requests_pending(old));
> +    assert(!bdrv_requests_pending(new));
> +
> +    bdrv_ref(old);
> +
> +    if (old->blk) {
> +        /* As long as these fields aren't in BlockBackend, but in the top-level
> +         * BlockDriverState, it's not possible for a BDS to have two BBs.
> +         *
> +         * We really want to copy the fields from old to new, but we go for a
> +         * swap instead so that pointers aren't duplicated and cause trouble.
> +         * (Also, bdrv_swap() used to do the same.) */
> +        assert(!new->blk);
> +        swap_feature_fields(old, new);
> +    }
> +    change_parent_backing_link(old, new);
> +
> +    /* Change backing files if a previously independent node is added to the
> +     * chain. For active commit, we replace top by its own (indirect) backing
> +     * file and don't do anything here so we don't build a loop. */
> +    if (new->backing == NULL && !bdrv_chain_contains(backing_bs(old), new)) {
> +        bdrv_set_backing_hd(new, backing_bs(old));
> +        bdrv_set_backing_hd(old, NULL);

Wouldn't we want @old to keep its backing file?

Then bdrv_append() would basically be a special case of this function,
with it additionally decrementing the refcount of @bs_new.

Max

> +    }
> +
> +    bdrv_unref(old);
> +}
> +
>  static void bdrv_delete(BlockDriverState *bs)
>  {
>      assert(!bs->job);


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