[Qemu-devel] [PATCH v3 0/2] block: Use bdrv_unref_child() for all children in bdrv_close()

[Qemu-devel] [PATCH v3 0/2] block: Use bdrv_unref_child() for all children in bdrv_close()

[Alberto Garcia]

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

[Qemu-devel] [PATCH v3 1/2] block: Use bdrv_unref_child() for all children in bdrv_close()

[Alberto Garcia]

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

Re: [Qemu-devel] [PATCH v3 1/2] block: Use bdrv_unref_child() for all children in bdrv_close()

[Alberto Garcia]

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

Re: [Qemu-devel] [PATCH v3 1/2] block: Use bdrv_unref_child() for all children in bdrv_close()

[Max Reitz]

[-- 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 --]

[Qemu-devel] [PATCH v3 2/2] block: Make bdrv_root_attach_child() unref child_bs on failure

[Alberto Garcia]

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

Re: [Qemu-devel] [PATCH v3 2/2] block: Make bdrv_root_attach_child() unref child_bs on failure

[Max Reitz]

[-- 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 --]

Re: [Qemu-devel] [PATCH v3 2/2] block: Make bdrv_root_attach_child() unref child_bs on failure

[Alberto Garcia]

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

Re: [Qemu-devel] [PATCH v3 2/2] block: Make bdrv_root_attach_child() unref child_bs on failure

[Max Reitz]

[-- 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 --]

Re: [Qemu-devel] [PATCH v3 0/2] block: Use bdrv_unref_child() for all children in bdrv_close()

[Max Reitz]

[-- 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 --]