Skip to content

Commit

Permalink
block: bdrv_append(): don't consume reference
Browse files Browse the repository at this point in the history
We have too much comments for this feature. It seems better just don't
do it. Most of real users (tests don't count) have to create additional
reference.

Drop also comment in external_snapshot_prepare:
 - bdrv_append doesn't "remove" old bs in common sense, it sounds
   strange
 - the fact that bdrv_append can fail is obvious from the context
 - the fact that we must rollback all changes in transaction abort is
   known (it's the direct role of abort)

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20210428151804.439460-5-vsementsov@virtuozzo.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
  • Loading branch information
Vladimir Sementsov-Ogievskiy authored and kevmw committed Apr 30, 2021
1 parent 397f7cc commit ae9d441
Show file tree
Hide file tree
Showing 7 changed files with 8 additions and 31 deletions.
25 changes: 3 additions & 22 deletions block.c
Original file line number Diff line number Diff line change
Expand Up @@ -3213,11 +3213,6 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
goto out;
}

/* bdrv_append() consumes a strong reference to bs_snapshot
* (i.e. it will call bdrv_unref() on it) even on error, so in
* order to be able to return one, we have to increase
* bs_snapshot's refcount here */
bdrv_ref(bs_snapshot);
ret = bdrv_append(bs_snapshot, bs, errp);
if (ret < 0) {
bs_snapshot = NULL;
Expand Down Expand Up @@ -4679,36 +4674,22 @@ int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
* 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().
*/
int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
Error **errp)
{
int ret = bdrv_set_backing_hd(bs_new, bs_top, errp);
if (ret < 0) {
goto out;
return ret;
}

ret = bdrv_replace_node(bs_top, bs_new, errp);
if (ret < 0) {
bdrv_set_backing_hd(bs_new, NULL, &error_abort);
goto out;
return ret;
}

ret = 0;

out:
/*
* bs_new is now referenced by its new parents, we don't need the
* additional reference any more.
*/
bdrv_unref(bs_new);

return ret;
return 0;
}

static void bdrv_delete(BlockDriverState *bs)
Expand Down
1 change: 0 additions & 1 deletion block/backup-top.c
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,6 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,

bdrv_drained_begin(source);

bdrv_ref(top);
ret = bdrv_append(top, source, errp);
if (ret < 0) {
error_prepend(errp, "Cannot append backup-top filter: ");
Expand Down
1 change: 1 addition & 0 deletions block/commit.c
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,
commit_top_bs->total_sectors = top->total_sectors;

ret = bdrv_append(commit_top_bs, top, errp);
bdrv_unref(commit_top_bs); /* referenced by new parents or failed */
if (ret < 0) {
commit_top_bs = NULL;
goto fail;
Expand Down
3 changes: 0 additions & 3 deletions block/mirror.c
Original file line number Diff line number Diff line change
Expand Up @@ -1630,9 +1630,6 @@ static BlockJob *mirror_start_job(

bs_opaque->is_commit = target_is_backing;

/* bdrv_append takes ownership of the mirror_top_bs reference, need to keep
* it alive until block_job_create() succeeds even if bs has no parent. */
bdrv_ref(mirror_top_bs);
bdrv_drained_begin(bs);
ret = bdrv_append(mirror_top_bs, bs, errp);
bdrv_drained_end(bs);
Expand Down
4 changes: 0 additions & 4 deletions blockdev.c
Original file line number Diff line number Diff line change
Expand Up @@ -1576,10 +1576,6 @@ static void external_snapshot_prepare(BlkActionState *common,
goto out;
}

/* This removes our old bs and adds the new bs. This is an operation that
* can fail, so we need to do it in .prepare; undoing it for abort is
* always possible. */
bdrv_ref(state->new_bs);
ret = bdrv_append(state->new_bs, state->old_bs, errp);
if (ret < 0) {
goto out;
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/test-bdrv-drain.c
Original file line number Diff line number Diff line change
Expand Up @@ -1478,7 +1478,6 @@ static void test_append_to_drained(void)
g_assert_cmpint(base_s->drain_count, ==, 1);
g_assert_cmpint(base->in_flight, ==, 0);

/* Takes ownership of overlay, so we don't have to unref it later */
bdrv_append(overlay, base, &error_abort);
g_assert_cmpint(base->in_flight, ==, 0);
g_assert_cmpint(overlay->in_flight, ==, 0);
Expand All @@ -1495,6 +1494,7 @@ static void test_append_to_drained(void)
g_assert_cmpint(overlay->quiesce_counter, ==, 0);
g_assert_cmpint(overlay_s->drain_count, ==, 0);

bdrv_unref(overlay);
bdrv_unref(base);
blk_unref(blk);
}
Expand Down
3 changes: 3 additions & 0 deletions tests/unit/test-bdrv-graph-mod.c
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ static void test_update_perm_tree(void)
ret = bdrv_append(filter, bs, NULL);
g_assert_cmpint(ret, <, 0);

bdrv_unref(filter);
blk_unref(root);
}

Expand Down Expand Up @@ -202,6 +203,7 @@ static void test_should_update_child(void)
bdrv_append(filter, bs, &error_abort);
g_assert(target->backing->bs == bs);

bdrv_unref(filter);
bdrv_unref(bs);
blk_unref(root);
}
Expand Down Expand Up @@ -380,6 +382,7 @@ static void test_append_greedy_filter(void)
&error_abort);

bdrv_append(fl, base, &error_abort);
bdrv_unref(fl);
bdrv_unref(top);
}

Expand Down

0 comments on commit ae9d441

Please sign in to comment.