| Message ID | 6d1d5feaa53aa1ab127adb73d605dc4503e3abd5.1557754872.git.berto@igalia.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: Use bdrv_unref_child() for allchildren in bdrv_close() | expand |
On Mon 13 May 2019 03:46:17 PM CEST, Alberto Garcia wrote: > Now bdrv_close() unrefs all children (before this patch it was only > bs->file and bs->backing). As a result, none of the callers of > brvd_attach_child() should remove their reference to child_bs (because > this function effectively steals that reference). This patch updates a > couple of tests that where doing their own bdrv_unref(). s/that where doing/that were doing/ Max, if it's not too late can you fix that in your block branch? Berto
On 23.05.19 17:27, Alberto Garcia wrote: > On Mon 13 May 2019 03:46:17 PM CEST, Alberto Garcia wrote: >> Now bdrv_close() unrefs all children (before this patch it was only >> bs->file and bs->backing). As a result, none of the callers of >> brvd_attach_child() should remove their reference to child_bs (because >> this function effectively steals that reference). This patch updates a >> couple of tests that where doing their own bdrv_unref(). > > s/that where doing/that were doing/ > > Max, if it's not too late can you fix that in your block branch? Sure. It’s fixed now. Max
diff --git a/block.c b/block.c index 5c2c6aa761..3c3bd0f8d2 100644 --- a/block.c +++ b/block.c @@ -3842,22 +3842,12 @@ static void bdrv_close(BlockDriverState *bs) bs->drv = NULL; } - bdrv_set_backing_hd(bs, NULL, &error_abort); - - if (bs->file != NULL) { - bdrv_unref_child(bs, bs->file); - bs->file = NULL; - } - QLIST_FOREACH_SAFE(child, &bs->children, next, next) { - /* TODO Remove bdrv_unref() from drivers' close function and use - * bdrv_unref_child() here */ - if (child->bs->inherits_from == bs) { - child->bs->inherits_from = NULL; - } - bdrv_detach_child(child); + bdrv_unref_child(bs, child); } + bs->backing = NULL; + bs->file = NULL; g_free(bs->opaque); bs->opaque = NULL; atomic_set(&bs->copy_on_read, 0); diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c index eda90750eb..5534c2adf9 100644 --- a/tests/test-bdrv-drain.c +++ b/tests/test-bdrv-drain.c @@ -1436,12 +1436,6 @@ static void test_detach_indirect(bool by_parent_cb) bdrv_unref(parent_b); blk_unref(blk); - /* XXX Once bdrv_close() unref's children instead of just detaching them, - * this won't be necessary any more. */ - bdrv_unref(a); - bdrv_unref(a); - bdrv_unref(c); - g_assert_cmpint(a->refcnt, ==, 1); g_assert_cmpint(b->refcnt, ==, 1); g_assert_cmpint(c->refcnt, ==, 1); diff --git a/tests/test-bdrv-graph-mod.c b/tests/test-bdrv-graph-mod.c index 283dc84869..747c0bf8fc 100644 --- a/tests/test-bdrv-graph-mod.c +++ b/tests/test-bdrv-graph-mod.c @@ -116,7 +116,6 @@ static void test_update_perm_tree(void) g_assert_nonnull(local_err); error_free(local_err); - bdrv_unref(bs); blk_unref(root); }
bdrv_unref_child() does the following things: - Updates the child->bs->inherits_from pointer. - Calls bdrv_detach_child() to remove the BdrvChild from bs->children. - Calls bdrv_unref() to unref the child BlockDriverState. When bdrv_unref_child() was introduced in commit 33a604075c it was not used in bdrv_close() because the drivers that had additional children (like quorum or blkverify) had already called bdrv_unref() on their children during their own close functions. This was changed later (in 0bd6e91a7e for quorum, in 3e586be0b2 for blkverify) so there's no reason not to use bdrv_unref_child() in bdrv_close() anymore. After this there's also no need to remove bs->backing and bs->file separately from the rest of the children, so bdrv_close() can be simplified. Now bdrv_close() unrefs all children (before this patch it was only bs->file and bs->backing). As a result, none of the callers of brvd_attach_child() should remove their reference to child_bs (because this function effectively steals that reference). This patch updates a couple of tests that where doing their own bdrv_unref(). Signed-off-by: Alberto Garcia <berto@igalia.com> --- block.c | 16 +++------------- tests/test-bdrv-drain.c | 6 ------ tests/test-bdrv-graph-mod.c | 1 - 3 files changed, 3 insertions(+), 20 deletions(-)