Re: [PATCH 10/13] block: Call drain callbacks only once

[Vladimir Sementsov-Ogievskiy]

On 11/8/22 15:37, Kevin Wolf wrote:
> We only need to call both the BlockDriver's callback and the parent
> callbacks when going from undrained to drained or vice versa. A second
> drain section doesn't make a difference for the driver or the parent,
> they weren't supposed to send new requests before and after the second
> drain.
>
> One thing that gets in the way is the 'ignore_bds_parents' parameter in
> bdrv_do_drained_begin_quiesce() and bdrv_do_drained_end(): If it is true
> for the first drain, bs->quiesce_counter will be non-zero, but the
> parent callbacks still haven't been called, so a second drain where it
> is false would still have to call them.
>
> Instead of keeping track of this, let's just get rid of the parameter.
> It was introduced in commit 6cd5c9d7b2d as an optimisation so that
> during bdrv_drain_all(), we wouldn't recursively drain all parents up to
> the root for each node, resulting in quadratic complexity. As it happens,
> calling the callbacks only once solves the same problem, so as of this
> patch, we'll still have O(n) complexity and ignore_bds_parents is not
> needed any more.
>
> This patch only ignores the 'ignore_bds_parents' parameter. It will be
> removed in a separate patch.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block.c                      | 13 ++++++-------
>   block/io.c                   | 24 +++++++++++++-----------
>   tests/unit/test-bdrv-drain.c | 16 ++++++++++------
>   3 files changed, 29 insertions(+), 24 deletions(-)
>
> diff --git a/block.c b/block.c
> index 9d082631d9..8878586f6e 100644
> --- a/block.c
> +++ b/block.c
> @@ -2816,7 +2816,6 @@ static void bdrv_replace_child_noperm(BdrvChild *child,
>   {
>       BlockDriverState *old_bs = child->bs;
>       int new_bs_quiesce_counter;
> -    int drain_saldo;
>   
>       assert(!child->frozen);
>       assert(old_bs != new_bs);
> @@ -2827,15 +2826,13 @@ static void bdrv_replace_child_noperm(BdrvChild *child,
>       }
>   
>       new_bs_quiesce_counter = (new_bs ? new_bs->quiesce_counter : 0);
> -    drain_saldo = new_bs_quiesce_counter - child->parent_quiesce_counter;
>   
>       /*
>        * If the new child node is drained but the old one was not, flush
>        * all outstanding requests to the old child node.
>        */
> -    while (drain_saldo > 0 && child->klass->drained_begin) {
> +    if (new_bs_quiesce_counter && !child->parent_quiesce_counter) {

Looks like checking for child->klass->drained_begin was a wrong thing even 
prepatch?

Also, parent_quiesce_counter actually becomes a boolean variable.. Should we 
stress it by new type and name?

>           bdrv_parent_drained_begin_single(child, true);
> -        drain_saldo--;
>       }
>   
>       if (old_bs) {
> @@ -2859,7 +2856,6 @@ static void bdrv_replace_child_noperm(BdrvChild *child,
>            * more often.
>            */

the comment above ^^^ should be updated, we are not going to call drained_end 
more than once anyway

>           assert(new_bs->quiesce_counter <= new_bs_quiesce_counter);

do we still need this assertion and the comment at all?

> -        drain_saldo += new_bs->quiesce_counter - new_bs_quiesce_counter;
>   
>           if (child->klass->attach) {
>               child->klass->attach(child);
> @@ -2869,10 +2865,13 @@ static void bdrv_replace_child_noperm(BdrvChild *child,
>       /*
>        * If the old child node was drained but the new one is not, allow
>        * requests to come in only after the new node has been attached.
> +     *
> +     * Update new_bs_quiesce_counter because bdrv_parent_drained_begin_single()
> +     * polls, which could have changed the value.
>        */
> -    while (drain_saldo < 0 && child->klass->drained_end) {
> +    new_bs_quiesce_counter = (new_bs ? new_bs->quiesce_counter : 0);
> +    if (!new_bs_quiesce_counter && child->parent_quiesce_counter) {
>           bdrv_parent_drained_end_single(child);
> -        drain_saldo++;
>       }
>   }
>   
> diff --git a/block/io.c b/block/io.c
> index 870a25d7a5..87c7a92f15 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -62,7 +62,7 @@ void bdrv_parent_drained_end_single(BdrvChild *c)
>   {
>       IO_OR_GS_CODE();
>   
> -    assert(c->parent_quiesce_counter > 0);
> +    assert(c->parent_quiesce_counter == 1);
>       c->parent_quiesce_counter--;
>       if (c->klass->drained_end) {
>           c->klass->drained_end(c);
> @@ -109,6 +109,7 @@ static bool bdrv_parent_drained_poll(BlockDriverState *bs, 
> BdrvChild *ignore,
>   void bdrv_parent_drained_begin_single(BdrvChild *c, bool poll)
>   {
>       IO_OR_GS_CODE();
> +    assert(c->parent_quiesce_counter == 0);
>       c->parent_quiesce_counter++;
>       if (c->klass->drained_begin) {
>           c->klass->drained_begin(c);
> @@ -352,16 +353,16 @@ void bdrv_do_drained_begin_quiesce(BlockDriverState *bs,
>                                      BdrvChild *parent, bool ignore_bds_parents)
>   {
>       IO_OR_GS_CODE();
> -    assert(!qemu_in_coroutine());

why that is dropped? seems unrelated to the commit

>   
>       /* Stop things in parent-to-child order */
>       if (qatomic_fetch_inc(&bs->quiesce_counter) == 0) {
>           aio_disable_external(bdrv_get_aio_context(bs));
> -    }
>   
> -    bdrv_parent_drained_begin(bs, parent, ignore_bds_parents);
> -    if (bs->drv && bs->drv->bdrv_drain_begin) {
> -        bs->drv->bdrv_drain_begin(bs);
> +        /* TODO Remove ignore_bds_parents, we don't consider it any more */
> +        bdrv_parent_drained_begin(bs, parent, false);
> +        if (bs->drv && bs->drv->bdrv_drain_begin) {
> +            bs->drv->bdrv_drain_begin(bs);
> +        }
>       }
>   }
>   
> @@ -412,13 +413,14 @@ static void bdrv_do_drained_end(BlockDriverState *bs, BdrvChild *parent,
>       assert(bs->quiesce_counter > 0);
>   
>       /* Re-enable things in child-to-parent order */

the comment should be moved too, I think

> -    if (bs->drv && bs->drv->bdrv_drain_end) {
> -        bs->drv->bdrv_drain_end(bs);
> -    }
> -    bdrv_parent_drained_end(bs, parent, ignore_bds_parents);
> -
>       old_quiesce_counter = qatomic_fetch_dec(&bs->quiesce_counter);
>       if (old_quiesce_counter == 1) {
> +        if (bs->drv && bs->drv->bdrv_drain_end) {
> +            bs->drv->bdrv_drain_end(bs);
> +        }
> +        /* TODO Remove ignore_bds_parents, we don't consider it any more */
> +        bdrv_parent_drained_end(bs, parent, false);
> +
>           aio_enable_external(bdrv_get_aio_context(bs));
>       }
>   }
> diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
> index dda08de8db..172bc6debc 100644
> --- a/tests/unit/test-bdrv-drain.c
> +++ b/tests/unit/test-bdrv-drain.c
> @@ -296,7 +296,11 @@ static void test_quiesce_common(enum drain_type 
> drain_type, bool recursive)
>   
>       do_drain_begin(drain_type, bs);
>   
> -    g_assert_cmpint(bs->quiesce_counter, ==, 1);
> +    if (drain_type == BDRV_DRAIN_ALL) {
> +        g_assert_cmpint(bs->quiesce_counter, ==, 2);
> +    } else {
> +        g_assert_cmpint(bs->quiesce_counter, ==, 1);
> +    }
>       g_assert_cmpint(backing->quiesce_counter, ==, !!recursive);
>   
>       do_drain_end(drain_type, bs);
> @@ -348,8 +352,8 @@ static void test_nested(void)
>   
>       for (outer = 0; outer < DRAIN_TYPE_MAX; outer++) {
>           for (inner = 0; inner < DRAIN_TYPE_MAX; inner++) {
> -            int backing_quiesce = (outer != BDRV_DRAIN) +
> -                                  (inner != BDRV_DRAIN);
> +            int backing_quiesce = (outer == BDRV_DRAIN_ALL) +
> +                                  (inner == BDRV_DRAIN_ALL);
>   
>               g_assert_cmpint(bs->quiesce_counter, ==, 0);
>               g_assert_cmpint(backing->quiesce_counter, ==, 0);
> @@ -359,10 +363,10 @@ static void test_nested(void)
>               do_drain_begin(outer, bs);
>               do_drain_begin(inner, bs);
>   
> -            g_assert_cmpint(bs->quiesce_counter, ==, 2);
> +            g_assert_cmpint(bs->quiesce_counter, ==, 2 + !!backing_quiesce);
>               g_assert_cmpint(backing->quiesce_counter, ==, backing_quiesce);
> -            g_assert_cmpint(s->drain_count, ==, 2);
> -            g_assert_cmpint(backing_s->drain_count, ==, backing_quiesce);
> +            g_assert_cmpint(s->drain_count, ==, 1);
> +            g_assert_cmpint(backing_s->drain_count, ==, !!backing_quiesce);
>   
>               do_drain_end(inner, bs);
>               do_drain_end(outer, bs);

--
Best regards,
Vladimir