* [Qemu-devel] [PATCH v3 0/2] block: Use bdrv_unref_child() for all children in bdrv_close()
@ 2019-05-13 13:46 Alberto Garcia
2019-05-13 13:46 ` [Qemu-devel] [PATCH v3 1/2] " Alberto Garcia
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Alberto Garcia @ 2019-05-13 13:46 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Alberto Garcia, qemu-block, Max Reitz
Hi,
the first patch is the same as in v2 (with an updated commit
message). The second patch is new and makes bdrv_root_attach_child()
unref child_bs on failure, as suggested by Max.
Regards,
Berto
v2: https://lists.gnu.org/archive/html/qemu-block/2019-05/msg00325.html
v1: https://lists.gnu.org/archive/html/qemu-block/2019-03/msg01040.html
Alberto Garcia (2):
block: Use bdrv_unref_child() for all children in bdrv_close()
block: Make bdrv_root_attach_child() unref child_bs on failure
block.c | 41 ++++++++++++++++++++---------------------
block/block-backend.c | 3 +--
block/quorum.c | 1 -
blockjob.c | 2 +-
tests/test-bdrv-drain.c | 6 ------
tests/test-bdrv-graph-mod.c | 1 -
6 files changed, 22 insertions(+), 32 deletions(-)
--
2.11.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH v3 1/2] block: Use bdrv_unref_child() for all children in bdrv_close()
2019-05-13 13:46 [Qemu-devel] [PATCH v3 0/2] block: Use bdrv_unref_child() for all children in bdrv_close() Alberto Garcia
@ 2019-05-13 13:46 ` Alberto Garcia
2019-05-23 15:27 ` Alberto Garcia
2019-05-13 13:46 ` [Qemu-devel] [PATCH v3 2/2] block: Make bdrv_root_attach_child() unref child_bs on failure Alberto Garcia
2019-05-13 14:32 ` [Qemu-devel] [PATCH v3 0/2] block: Use bdrv_unref_child() for all children in bdrv_close() Max Reitz
2 siblings, 1 reply; 9+ messages in thread
From: Alberto Garcia @ 2019-05-13 13:46 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Alberto Garcia, qemu-block, Max Reitz
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(-)
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);
}
--
2.11.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH v3 2/2] block: Make bdrv_root_attach_child() unref child_bs on failure
2019-05-13 13:46 [Qemu-devel] [PATCH v3 0/2] block: Use bdrv_unref_child() for all children in bdrv_close() Alberto Garcia
2019-05-13 13:46 ` [Qemu-devel] [PATCH v3 1/2] " Alberto Garcia
@ 2019-05-13 13:46 ` Alberto Garcia
2019-05-13 14:31 ` Max Reitz
2019-05-13 14:32 ` [Qemu-devel] [PATCH v3 0/2] block: Use bdrv_unref_child() for all children in bdrv_close() Max Reitz
2 siblings, 1 reply; 9+ messages in thread
From: Alberto Garcia @ 2019-05-13 13:46 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Alberto Garcia, qemu-block, Max Reitz
A consequence of the previous patch is that bdrv_attach_child()
transfers the reference to child_bs from the caller to parent_bs,
which will drop it on bdrv_close() or when someone calls
bdrv_unref_child().
But this only happens when bdrv_attach_child() succeeds. If it fails
then the caller is responsible for dropping the reference to child_bs.
This patch makes bdrv_attach_child() take the reference also when
there is an error, freeing the caller for having to do it.
A similar situation happens with bdrv_root_attach_child(), so the
changes on this patch affect both functions.
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
block.c | 25 +++++++++++++++++--------
block/block-backend.c | 3 +--
block/quorum.c | 1 -
blockjob.c | 2 +-
4 files changed, 19 insertions(+), 12 deletions(-)
diff --git a/block.c b/block.c
index 3c3bd0f8d2..df727314ff 100644
--- a/block.c
+++ b/block.c
@@ -2208,6 +2208,13 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
}
}
+/*
+ * This function steals the reference to child_bs from the caller.
+ * That reference is later dropped by bdrv_root_unref_child().
+ *
+ * On failure NULL is returned, errp is set and the reference to
+ * child_bs is also dropped.
+ */
BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
const char *child_name,
const BdrvChildRole *child_role,
@@ -2220,6 +2227,7 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
ret = bdrv_check_update_perm(child_bs, NULL, perm, shared_perm, NULL, errp);
if (ret < 0) {
bdrv_abort_perm_update(child_bs);
+ bdrv_unref(child_bs);
return NULL;
}
@@ -2239,6 +2247,14 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
return child;
}
+/*
+ * This function transfers the reference to child_bs from the caller
+ * to parent_bs. That reference is later dropped by parent_bs on
+ * bdrv_close() or if someone calls bdrv_unref_child().
+ *
+ * On failure NULL is returned, errp is set and the reference to
+ * child_bs is also dropped.
+ */
BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
BlockDriverState *child_bs,
const char *child_name,
@@ -2366,12 +2382,9 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
/* If backing_hd was already part of bs's backing chain, and
* inherits_from pointed recursively to bs then let's update it to
* point directly to bs (else it will become NULL). */
- if (update_inherits_from) {
+ if (bs->backing && update_inherits_from) {
backing_hd->inherits_from = bs;
}
- if (!bs->backing) {
- bdrv_unref(backing_hd);
- }
out:
bdrv_refresh_limits(bs, NULL);
@@ -2569,10 +2582,6 @@ BdrvChild *bdrv_open_child(const char *filename,
}
c = bdrv_attach_child(parent, bs, bdref_key, child_role, errp);
- if (!c) {
- bdrv_unref(bs);
- return NULL;
- }
return c;
}
diff --git a/block/block-backend.c b/block/block-backend.c
index f78e82a707..7a621597e7 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -383,7 +383,6 @@ BlockBackend *blk_new_open(const char *filename, const char *reference,
blk->root = bdrv_root_attach_child(bs, "root", &child_root,
perm, BLK_PERM_ALL, blk, errp);
if (!blk->root) {
- bdrv_unref(bs);
blk_unref(blk);
return NULL;
}
@@ -791,12 +790,12 @@ void blk_remove_bs(BlockBackend *blk)
int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp)
{
ThrottleGroupMember *tgm = &blk->public.throttle_group_member;
+ bdrv_ref(bs);
blk->root = bdrv_root_attach_child(bs, "root", &child_root,
blk->perm, blk->shared_perm, blk, errp);
if (blk->root == NULL) {
return -EPERM;
}
- bdrv_ref(bs);
notifier_list_notify(&blk->insert_bs_notifiers, blk);
if (tgm->throttle_state) {
diff --git a/block/quorum.c b/block/quorum.c
index 352f729136..133ee18204 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1019,7 +1019,6 @@ static void quorum_add_child(BlockDriverState *bs, BlockDriverState *child_bs,
child = bdrv_attach_child(bs, child_bs, indexstr, &child_format, errp);
if (child == NULL) {
s->next_child_index--;
- bdrv_unref(child_bs);
goto out;
}
s->children = g_renew(BdrvChild *, s->children, s->num_children + 1);
diff --git a/blockjob.c b/blockjob.c
index 730101d282..b68fa4b13e 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -208,6 +208,7 @@ int block_job_add_bdrv(BlockJob *job, const char *name, BlockDriverState *bs,
{
BdrvChild *c;
+ bdrv_ref(bs);
c = bdrv_root_attach_child(bs, name, &child_job, perm, shared_perm,
job, errp);
if (c == NULL) {
@@ -215,7 +216,6 @@ int block_job_add_bdrv(BlockJob *job, const char *name, BlockDriverState *bs,
}
job->nodes = g_slist_prepend(job->nodes, c);
- bdrv_ref(bs);
bdrv_op_block_all(bs, job->blocker);
return 0;
--
2.11.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] block: Make bdrv_root_attach_child() unref child_bs on failure
2019-05-13 13:46 ` [Qemu-devel] [PATCH v3 2/2] block: Make bdrv_root_attach_child() unref child_bs on failure Alberto Garcia
@ 2019-05-13 14:31 ` Max Reitz
2019-05-13 15:52 ` Alberto Garcia
0 siblings, 1 reply; 9+ messages in thread
From: Max Reitz @ 2019-05-13 14:31 UTC (permalink / raw)
To: Alberto Garcia, qemu-devel; +Cc: Kevin Wolf, qemu-block
[-- Attachment #1: Type: text/plain, Size: 1399 bytes --]
On 13.05.19 15:46, Alberto Garcia wrote:
> A consequence of the previous patch is that bdrv_attach_child()
> transfers the reference to child_bs from the caller to parent_bs,
> which will drop it on bdrv_close() or when someone calls
> bdrv_unref_child().
>
> But this only happens when bdrv_attach_child() succeeds. If it fails
> then the caller is responsible for dropping the reference to child_bs.
>
> This patch makes bdrv_attach_child() take the reference also when
> there is an error, freeing the caller for having to do it.
>
> A similar situation happens with bdrv_root_attach_child(), so the
> changes on this patch affect both functions.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
> block.c | 25 +++++++++++++++++--------
> block/block-backend.c | 3 +--
> block/quorum.c | 1 -
> blockjob.c | 2 +-
> 4 files changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/block.c b/block.c
> index 3c3bd0f8d2..df727314ff 100644
> --- a/block.c
> +++ b/block.c
[...]
> @@ -2569,10 +2582,6 @@ BdrvChild *bdrv_open_child(const char *filename,
> }
>
> c = bdrv_attach_child(parent, bs, bdref_key, child_role, errp);
> - if (!c) {
> - bdrv_unref(bs);
> - return NULL;
> - }
>
> return c;
> }
(That could have been simplified even further. *shrug*)
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/2] block: Use bdrv_unref_child() for all children in bdrv_close()
2019-05-13 13:46 [Qemu-devel] [PATCH v3 0/2] block: Use bdrv_unref_child() for all children in bdrv_close() Alberto Garcia
2019-05-13 13:46 ` [Qemu-devel] [PATCH v3 1/2] " Alberto Garcia
2019-05-13 13:46 ` [Qemu-devel] [PATCH v3 2/2] block: Make bdrv_root_attach_child() unref child_bs on failure Alberto Garcia
@ 2019-05-13 14:32 ` Max Reitz
2 siblings, 0 replies; 9+ messages in thread
From: Max Reitz @ 2019-05-13 14:32 UTC (permalink / raw)
To: Alberto Garcia, qemu-devel; +Cc: Kevin Wolf, qemu-block
[-- Attachment #1: Type: text/plain, Size: 1056 bytes --]
On 13.05.19 15:46, Alberto Garcia wrote:
> Hi,
>
> the first patch is the same as in v2 (with an updated commit
> message). The second patch is new and makes bdrv_root_attach_child()
> unref child_bs on failure, as suggested by Max.
>
> Regards,
>
> Berto
>
> v2: https://lists.gnu.org/archive/html/qemu-block/2019-05/msg00325.html
> v1: https://lists.gnu.org/archive/html/qemu-block/2019-03/msg01040.html
>
> Alberto Garcia (2):
> block: Use bdrv_unref_child() for all children in bdrv_close()
> block: Make bdrv_root_attach_child() unref child_bs on failure
>
> block.c | 41 ++++++++++++++++++++---------------------
> block/block-backend.c | 3 +--
> block/quorum.c | 1 -
> blockjob.c | 2 +-
> tests/test-bdrv-drain.c | 6 ------
> tests/test-bdrv-graph-mod.c | 1 -
> 6 files changed, 22 insertions(+), 32 deletions(-)
Thanks for bearing with me, applied to my block branch:
https://git.xanclic.moe/XanClic/qemu/commits/branch/block
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] block: Make bdrv_root_attach_child() unref child_bs on failure
2019-05-13 14:31 ` Max Reitz
@ 2019-05-13 15:52 ` Alberto Garcia
2019-05-13 15:54 ` Max Reitz
0 siblings, 1 reply; 9+ messages in thread
From: Alberto Garcia @ 2019-05-13 15:52 UTC (permalink / raw)
To: Max Reitz, qemu-devel; +Cc: Kevin Wolf, qemu-block
On Mon 13 May 2019 04:31:16 PM CEST, Max Reitz wrote:
>> @@ -2569,10 +2582,6 @@ BdrvChild *bdrv_open_child(const char *filename,
>> }
>>
>> c = bdrv_attach_child(parent, bs, bdref_key, child_role, errp);
>> - if (!c) {
>> - bdrv_unref(bs);
>> - return NULL;
>> - }
>>
>> return c;
>> }
>
> (That could have been simplified even further. *shrug*)
Right, we can remove the 'c' variable altogether. Feel free to edit the
commit if you want.
Berto
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] block: Make bdrv_root_attach_child() unref child_bs on failure
2019-05-13 15:52 ` Alberto Garcia
@ 2019-05-13 15:54 ` Max Reitz
0 siblings, 0 replies; 9+ messages in thread
From: Max Reitz @ 2019-05-13 15:54 UTC (permalink / raw)
To: Alberto Garcia, qemu-devel; +Cc: Kevin Wolf, qemu-block
[-- Attachment #1: Type: text/plain, Size: 597 bytes --]
On 13.05.19 17:52, Alberto Garcia wrote:
> On Mon 13 May 2019 04:31:16 PM CEST, Max Reitz wrote:
>>> @@ -2569,10 +2582,6 @@ BdrvChild *bdrv_open_child(const char *filename,
>>> }
>>>
>>> c = bdrv_attach_child(parent, bs, bdref_key, child_role, errp);
>>> - if (!c) {
>>> - bdrv_unref(bs);
>>> - return NULL;
>>> - }
>>>
>>> return c;
>>> }
>>
>> (That could have been simplified even further. *shrug*)
>
> Right, we can remove the 'c' variable altogether. Feel free to edit the
> commit if you want.
OK, I’ll do that, then.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/2] block: Use bdrv_unref_child() for all children in bdrv_close()
2019-05-13 13:46 ` [Qemu-devel] [PATCH v3 1/2] " Alberto Garcia
@ 2019-05-23 15:27 ` Alberto Garcia
2019-05-23 15:57 ` Max Reitz
0 siblings, 1 reply; 9+ messages in thread
From: Alberto Garcia @ 2019-05-23 15:27 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, qemu-block, Max Reitz
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/2] block: Use bdrv_unref_child() for all children in bdrv_close()
2019-05-23 15:27 ` Alberto Garcia
@ 2019-05-23 15:57 ` Max Reitz
0 siblings, 0 replies; 9+ messages in thread
From: Max Reitz @ 2019-05-23 15:57 UTC (permalink / raw)
To: Alberto Garcia, qemu-devel; +Cc: Kevin Wolf, qemu-block
[-- Attachment #1: Type: text/plain, Size: 606 bytes --]
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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-05-23 16:00 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-13 13:46 [Qemu-devel] [PATCH v3 0/2] block: Use bdrv_unref_child() for all children in bdrv_close() Alberto Garcia
2019-05-13 13:46 ` [Qemu-devel] [PATCH v3 1/2] " Alberto Garcia
2019-05-23 15:27 ` Alberto Garcia
2019-05-23 15:57 ` Max Reitz
2019-05-13 13:46 ` [Qemu-devel] [PATCH v3 2/2] block: Make bdrv_root_attach_child() unref child_bs on failure Alberto Garcia
2019-05-13 14:31 ` Max Reitz
2019-05-13 15:52 ` Alberto Garcia
2019-05-13 15:54 ` Max Reitz
2019-05-13 14:32 ` [Qemu-devel] [PATCH v3 0/2] block: Use bdrv_unref_child() for all children in bdrv_close() Max Reitz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.