qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Hanna Reitz
Subject: Re: [PATCH v2 00/15] block: Simplify drain
Date: Thu, 24 Nov 2022 19:26:22 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0

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>




reply via email to

[Prev in Thread] Current Thread [Next in Thread]