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

[Kevin Wolf <kwolf@redhat.com>]

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

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 <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?

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.

> 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?

Kevin

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



[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]