| Message ID | 20221108123738.530873-11-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Simplify drain | expand |
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);
On 11/8/22 15:37, Kevin Wolf wrote: > 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. This paragraph breaks my brain :/ Still, I understand the new concept and believe that dropping ignore_bds_parents and just call the callbacks once (and stop the recursion when we found quiesce_counter > 0) is a good way.
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
On 08.11.22 13: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(-) I too would like parent_quiesce_counter to become `bool parent_quiesced`, but: Reviewed-by: Hanna Reitz <hreitz@redhat.com>
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) { 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. */ assert(new_bs->quiesce_counter <= new_bs_quiesce_counter); - 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()); /* 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 */ - 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);
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(-)