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 <kw...@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? > > 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