Commit dd62f1ca authored by Kevin Wolf's avatar Kevin Wolf
Browse files

block: Implement bdrv_append() without bdrv_swap() 



Remember all parent nodes and just change the pointers there instead of
swapping the contents of the BlockDriverState.

Handling of snapshot=on must be moved further down in bdrv_open()
because *pbs (which is the bs pointer in the BlockBackend) must already
be set before bdrv_append() is called. Otherwise bdrv_append() changes
the BB's pointer to the temporary snapshot, but bdrv_open() overwrites
it with the read-only original image.

We also need to be careful to update callers as the interface changes
(becomes less insane): Previously, the meaning of the two parameters was
inverted when bdrv_append() returns. Now any BDS pointers keep pointing
to the same node.

Signed-off-by: default avatarKevin Wolf <kwolf@redhat.com>
Reviewed-by: default avatarMax Reitz <mreitz@redhat.com>
Reviewed-by: default avatarFam Zheng <famz@redhat.com>
Reviewed-by: default avatarStefan Hajnoczi <stefanha@redhat.com>
parent d42a8a93

block.c

+84 −28
Original line number Diff line number Diff line
@@ -1516,15 +1516,6 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename, 


    bdrv_refresh_filename(bs);



    /* For snapshot=on, create a temporary qcow2 overlay. bs points to the

     * temporary snapshot afterwards. */

    if (snapshot_flags) {

        ret = bdrv_append_temp_snapshot(bs, snapshot_flags, &local_err);

        if (local_err) {

            goto close_and_fail;

        }

    }



    /* Check if any unknown options were used */

    if (options && (qdict_size(options) != 0)) {

        const QDictEntry *entry = qdict_first(options);
@@ -1556,6 +1547,16 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename, 


    QDECREF(options);

    *pbs = bs;



    /* For snapshot=on, create a temporary qcow2 overlay. bs points to the

     * temporary snapshot afterwards. */

    if (snapshot_flags) {

        ret = bdrv_append_temp_snapshot(bs, snapshot_flags, &local_err);

        if (local_err) {

            goto close_and_fail;

        }

    }



    return 0;



fail:
@@ -2000,20 +2001,6 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest, 


    bs_dest->enable_write_cache = bs_src->enable_write_cache;



    /* i/o throttled req */

    bs_dest->throttle_state     = bs_src->throttle_state,

    bs_dest->io_limits_enabled  = bs_src->io_limits_enabled;

    bs_dest->pending_reqs[0]    = bs_src->pending_reqs[0];

    bs_dest->pending_reqs[1]    = bs_src->pending_reqs[1];

    bs_dest->throttled_reqs[0]  = bs_src->throttled_reqs[0];

    bs_dest->throttled_reqs[1]  = bs_src->throttled_reqs[1];

    memcpy(&bs_dest->round_robin,

           &bs_src->round_robin,

           sizeof(bs_dest->round_robin));

    memcpy(&bs_dest->throttle_timers,

           &bs_src->throttle_timers,

           sizeof(ThrottleTimers));



    /* r/w error */

    bs_dest->on_read_error      = bs_src->on_read_error;

    bs_dest->on_write_error     = bs_src->on_write_error;
@@ -2027,10 +2014,25 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest, 
}



/* Fields that only need to be swapped if the contents of BDSes is swapped

 * rather than pointers being changed in the parents. */

 * rather than pointers being changed in the parents, and throttling fields

 * because only bdrv_swap() messes with internals of throttling. */

static void bdrv_move_reference_fields(BlockDriverState *bs_dest,

                                       BlockDriverState *bs_src)

{

    /* i/o throttled req */

    bs_dest->throttle_state     = bs_src->throttle_state,

    bs_dest->io_limits_enabled  = bs_src->io_limits_enabled;

    bs_dest->pending_reqs[0]    = bs_src->pending_reqs[0];

    bs_dest->pending_reqs[1]    = bs_src->pending_reqs[1];

    bs_dest->throttled_reqs[0]  = bs_src->throttled_reqs[0];

    bs_dest->throttled_reqs[1]  = bs_src->throttled_reqs[1];

    memcpy(&bs_dest->round_robin,

           &bs_src->round_robin,

           sizeof(bs_dest->round_robin));

    memcpy(&bs_dest->throttle_timers,

           &bs_src->throttle_timers,

           sizeof(ThrottleTimers));



    /* reference count */

    bs_dest->refcnt             = bs_src->refcnt;
@@ -2156,6 +2158,45 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old) 
    bdrv_rebind(bs_old);

}



static void change_parent_backing_link(BlockDriverState *from,

                                       BlockDriverState *to)

{

    BdrvChild *c, *next;



    QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) {

        assert(c->role != &child_backing);

        c->bs = to;

        QLIST_REMOVE(c, next_parent);

        QLIST_INSERT_HEAD(&to->parents, c, next_parent);

        bdrv_ref(to);

        bdrv_unref(from);

    }

    if (from->blk) {

        blk_set_bs(from->blk, to);

        if (!to->device_list.tqe_prev) {

            QTAILQ_INSERT_BEFORE(from, to, device_list);

        }

        QTAILQ_REMOVE(&bdrv_states, from, device_list);

    }

}



static void swap_feature_fields(BlockDriverState *bs_top,

                                BlockDriverState *bs_new)

{

    BlockDriverState tmp;



    bdrv_move_feature_fields(&tmp, bs_top);

    bdrv_move_feature_fields(bs_top, bs_new);

    bdrv_move_feature_fields(bs_new, &tmp);



    assert(!bs_new->throttle_state);

    if (bs_top->throttle_state) {

        assert(bs_top->io_limits_enabled);

        bdrv_io_limits_enable(bs_new, throttle_group_get_name(bs_top));

        bdrv_io_limits_disable(bs_top);

    }

}



/*

 * Add new bs contents at the top of an image chain while the chain is

 * live, while keeping required fields on the top layer.
@@ -2166,14 +2207,29 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old) 
 * bs_new must not be attached to a BlockBackend.

 *

 * This function does not create any image files.

 *

 * bdrv_append() takes ownership of a bs_new reference and unrefs it because

 * that's what the callers commonly need. bs_new will be referenced by the old

 * parents of bs_top after bdrv_append() returns. If the caller needs to keep a

 * reference of its own, it must call bdrv_ref().

 */

void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)

{

    bdrv_swap(bs_new, bs_top);

    assert(!bdrv_requests_pending(bs_top));

    assert(!bdrv_requests_pending(bs_new));



    bdrv_ref(bs_top);

    change_parent_backing_link(bs_top, bs_new);



    /* Some fields always stay on top of the backing file chain */

    swap_feature_fields(bs_top, bs_new);



    bdrv_set_backing_hd(bs_new, bs_top);

    bdrv_unref(bs_top);



    /* The contents of 'tmp' will become bs_top, as we are

     * swapping bs_new and bs_top contents. */

    bdrv_set_backing_hd(bs_top, bs_new);

    /* bs_new is now referenced by its new parents, we don't need the

     * additional reference any more. */

    bdrv_unref(bs_new);

}



static void bdrv_delete(BlockDriverState *bs)

blockdev.c

+1 −1
Original line number Diff line number Diff line
@@ -1546,7 +1546,7 @@ static void external_snapshot_commit(BlkTransactionState *common) 
    /* We don't need (or want) to use the transactional

     * bdrv_reopen_multiple() across all the entries at once, because we

     * don't want to abort all of them if one of them fails the reopen */

    bdrv_reopen(state->new_bs, state->new_bs->open_flags & ~BDRV_O_RDWR,

    bdrv_reopen(state->old_bs, state->old_bs->open_flags & ~BDRV_O_RDWR,

                NULL);



    aio_context_release(state->aio_context);