[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 15/16] block: Add and use bdrv_replace_in_backin
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH 15/16] block: Add and use bdrv_replace_in_backing_chain() |
Date: |
Wed, 30 Sep 2015 16:50:14 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 |
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