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

[Max Reitz]

On 29.09.2015 17:22, Kevin Wolf wrote:
> Am 23.09.2015 um 19:08 hat Max Reitz geschrieben:
>> 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 <address@hidden>
>>> ---
>>>  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?
> 
> Would we? The operation I had in mind was: Given a backing file chain,
> one node in that chain and an independent node, replace the node in the
> chain. That is:
> 
>     A <- B <- C <- D
> 
>          X
> 
> becomes
> 
>     A <- X <- C <- D
> 
>          B
> 
> Of course, you could define a different operation, but this seemed to be
> the obvious one that the mirror completion needs.

Oops, apparently I missed the backing_bs(old) in
bdrv_set_backing_hd(new, ...).

>> Then bdrv_append() would basically be a special case of this function,
>> with it additionally decrementing the refcount of @bs_new.
> 
> Hm, less duplication sounds nice, but as long as the current way is
> technically correct, can we leave this for a cleanup on top?

And with the backing_bs() taken into account, it is not so duplicated
after all.

Max

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

signature.asc
Description: OpenPGP digital signature