Re: [Qemu-devel] [PATCH v2] block: Use bdrv_unref_child() for all children in bdrv_close()

[Max Reitz]

On 08.05.19 17:51, Alberto Garcia wrote:
> 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.
> 
> This patch also updates a couple of tests that were doing their own
> bdrv_unref().
> 
> Signed-off-by: Alberto Garcia <address@hidden>
> ---
>  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 9ae5c0ed2f..96f8e431da 100644
> --- a/block.c
> +++ b/block.c
> @@ -3843,22 +3843,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);
>  }

Hm, yes, the comment in test-bdrv-drain makes these changes obvious enough.

So the “problem” was that this patch effectively makes
bdrv_attach_child() take the reference to the child BDS on success.
Well, it always kind of did so; it did not increase the refcount for the
link it creates.  But someone still needed to invoke some unref function
to drop the refcount, which is no longer the case as of this patch.

But taking the reference only on success is a bit wonky.  Usually such
functions promise to always take the reference, even on error (they will
just bdrv_unref() the BDS then).  And looking at the callers, that seems
to make sense, actually.  All of them invoke bdrv_unref() on the child
if bdrv_attach_child() fails.  This extends to bdrv_root_attach_child()
-- two of its three callers do not bdrv_unref() the child on failure,
but that's just because they haven't even taken their own reference yet.
 So if bdrv_root_attach_child() always took the reference, they'd just
need to call bdrv_ref() before bdrv_root_attach_child() rather than
afterwards.

Do you agree?  If so, would you mind changing the behavior of
bdrv_attach_child() and bdrv_root_attach_child() and document it?

Max

signature.asc
Description: OpenPGP digital signature