Re: [PATCH v2 00/15] block: Simplify drain

[Hanna Reitz <hreitz@redhat.com>]

On 18.11.22 18:40, 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.
>
> v2:
> - Rebased on master
> - Patch 3: Removed left over _co parts in function names
> - Patch 4: Updated function comments to reflect that we're not polling
>    any more
> - Patch 6 (new): Fix inconsistent AioContext locking for reopen code
> - Patch 9 (was 8): Added comment to clarify when polling is allowed
>    and the graph may change again
> - Patch 11 (was 10):
>    * Reworded some comments and the commit message.
>    * Dropped a now unnecessary assertion that was dropped only in a later
>      patch in v1 of the series.
>    * Changed 'int parent_quiesce_counter' into 'bool quiesced_parent'
> - Patch 12 (was 11): Don't remove ignore_bds_parents from
>    bdrv_drain_poll(), it is actually still a valid optimisation there
>    that makes polling O(n) instead of O(n²)
> - Patch 13 (new): Instead of only removing assert(!qemu_in_coroutine())
>    like in v1 of the series, drop out of coroutine context in
>    bdrv_do_drained_begin_quiesce() just to be sure that we'll never get
>    coroutine surprises in drain code.
> - Patch 14 (was 12): More and reworded comments to make things hopefully
>    a bit clearer

Thanks!

Reviewed-by: Hanna Reitz <hreitz@redhat.com>