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 <kw...@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
