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

[Kevin Wolf]

Am 09.11.2022 um 19:05 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 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?

I'm not sure if it was strictly wrong in practice, but at least
unnecessary. It would have been wrong if a BdrvChildClass implemented
for example .drained_begin, but not .drain_end. But I think we always
implement all three of .drained_begin/poll/end or none of them.

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

Ok, but I would do that in a separate patch. Maybe 'bool drains_parent'.

> >           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?

Patch 12 removes both, but I can do it already here.

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

I'm sure I added it because I actually got an assertion failure, but I
can't reproduce it on this commit now. At the end of the series tests do
fail without this removed. I'll double check which commit is right one
to remove it.

> >       /* 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

It is the same place as in bdrv_do_drained_begin_quiesce().

> > -    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));
> >       }
> >   }

Kevin