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.
> 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);
>
pgpZKoyGL3W9g.pgp
Description: PGP signature
