Re: [PATCH 00/13] block: Simplify drain

[Stefan Hajnoczi]

On Tue, Nov 08, 2022 at 01:37:25PM +0100, Kevin Wolf wrote:
> I'm aware that exactly nobody has been looking forward to a series with
> this title, but it has to be. The way drain works means that we need to
> poll in bdrv_replace_child_noperm() and that makes things rather messy
> with Emanuele's multiqueue work because you must not poll while you hold
> the graph lock.
> 
> The other reason why it has to be is that drain is way too complex and
> there are too many different cases. Some simplification like this will
> hopefully make it considerably more maintainable. The diffstat probably
> tells something, too.
> 
> There are roughly speaking three parts in this series:
> 
> 1. Make BlockDriver.bdrv_drained_begin/end() non-coroutine_fn again,
>    which allows us to not poll on bdrv_drained_end() any more.
> 
> 2. Remove subtree drains. They are a considerable complication in the
>    whole drain machinery (in particular, they require polling in the
>    BdrvChildClass.attach/detach() callbacks that are called during
>    bdrv_replace_child_noperm()) and none of their users actually has a
>    good reason to use them.
> 
> 3. Finally get rid of polling in bdrv_replace_child_noperm() by
>    requiring that the child is already drained by the caller and calling
>    callbacks only once and not again for every nested drain section.
> 
> If necessary, a prefix of this series can be merged that covers only the
> first or the first two parts and it would still make sense.
> 
> Kevin Wolf (13):
>   qed: Don't yield in bdrv_qed_co_drain_begin()
>   test-bdrv-drain: Don't yield in .bdrv_co_drained_begin/end()
>   block: Revert .bdrv_drained_begin/end to non-coroutine_fn
>   block: Remove drained_end_counter
>   block: Inline bdrv_drain_invoke()
>   block: Drain invidual nodes during reopen
>   block: Don't use subtree drains in bdrv_drop_intermediate()
>   stream: Replace subtree drain with a single node drain
>   block: Remove subtree drains
>   block: Call drain callbacks only once
>   block: Remove ignore_bds_parents parameter from drain functions
>   block: Don't poll in bdrv_replace_child_noperm()
>   block: Remove poll parameter from bdrv_parent_drained_begin_single()
> 
>  include/block/block-global-state.h |   3 +
>  include/block/block-io.h           |  52 +---
>  include/block/block_int-common.h   |  17 +-
>  include/block/block_int-io.h       |  12 -
>  block.c                            | 132 ++++++-----
>  block/block-backend.c              |   4 +-
>  block/io.c                         | 281 ++++------------------
>  block/qed.c                        |  24 +-
>  block/replication.c                |   6 -
>  block/stream.c                     |  20 +-
>  block/throttle.c                   |   6 +-
>  blockdev.c                         |  13 -
>  blockjob.c                         |   2 +-
>  tests/unit/test-bdrv-drain.c       | 369 +++++++----------------------
>  14 files changed, 270 insertions(+), 671 deletions(-)

I have looked through all patches but don't understand the code well
enough to give an opinion or spot bugs. Removing subtree drains and
aio_poll() in bdrv_replace_child_noperm() are nice.

Acked-by: Stefan Hajnoczi <stefanha@redhat.com>

signature.asc
Description: PGP signature