Message ID | 20221118174110.55183-1-kwolf@redhat.com (mailing list archive) |
---|---|
Headers | show |
Series | block: Simplify drain | expand |
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>
Am 18.11.2022 um 18:40 hat Kevin Wolf geschrieben: > 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. Thanks for the review, applied to block-next. Kevin