There are a few more lines of code that can be removed around draining
code, but especially the logic can be simplified by removing unnecessary
parameters.

Due to the failure of the block-next branch, the first three patches
drop patches 14+15 of Kevin's drain cleanup series, and then redo
patch 15 in a slightly less satisfactory way that still enables the
remaining cleanups.  These reverts are not supposed to be applied;
either the offending patches are dropped from the branch, or if the
issue is fixed then my first three patches can go away.

The next three are taken from Emanuele's old subtree drain attempt
at removing the AioContext.  The main one is the second, which is needed
to avoid testcase failures, but I included all of them for simplicity.

Patch 7 fixes another latent bug exposed by the later cleanups, and while
looking for a fix I noticed a general lack of thread-safety in BlockBackend's
drain code.  There are some global properties that only need to be documented
and enforced to be set only at creation time (patches 8/9), but also
queued_requests is not protected by any mutex, which is fixed in patch 10.

Finally, patches 11-15 are the actual simplification.

Applies on top of block-next.

Paolo

Emanuele Giuseppe Esposito (3):
  test-bdrv-drain.c: remove test_detach_by_parent_cb()
  tests/unit/test-bdrv-drain.c: graph setup functions can't run in
    coroutines
  tests/qemu-iotests/030: test_stream_parallel should use
    auto_finalize=False

Paolo Bonzini (12):
  Revert "block: Remove poll parameter from
    bdrv_parent_drained_begin_single()"
  Revert "block: Don't poll in bdrv_replace_child_noperm()"
  block: Pull polling out of bdrv_parent_drained_begin_single()
  block-backend: enter aio coroutine only after drain
  nbd: a BlockExport always has a BlockBackend
  block-backend: make global properties write-once
  block-backend: always wait for drain before starting operation
  block-backend: make queued_requests thread-safe
  block: limit bdrv_co_yield_to_drain to drain_begin
  block: second argument of bdrv_do_drained_end is always NULL
  block: second argument of bdrv_do_drained_begin and bdrv_drain_poll is
    always NULL
  block: only get out of coroutine context for polling

 block.c                           | 109 ++++----------------
 block/block-backend.c             |  91 +++++++++-------
 block/commit.c                    |   4 +-
 block/export/export.c             |   2 +-
 block/io.c                        | 136 +++++++++---------------
 block/mirror.c                    |   4 +-
 block/parallels.c                 |   2 +-
 block/qcow.c                      |   2 +-
 block/qcow2.c                     |   2 +-
 block/qed.c                       |   2 +-
 block/stream.c                    |   4 +-
 block/vdi.c                       |   2 +-
 block/vhdx.c                      |   2 +-
 block/vmdk.c                      |   4 +-
 block/vpc.c                       |   2 +-
 include/block/block-io.h          |  29 +-----
 include/block/block_int-io.h      |  21 ++++
 include/sysemu/block-backend-io.h |   6 +-
 nbd/server.c                      |  15 ++-
 tests/qemu-iotests/030            |  12 +--
 tests/unit/test-bdrv-drain.c      | 166 +++++++++++++++---------------
 tests/unit/test-block-iothread.c  |   2 +-
 22 files changed, 258 insertions(+), 361 deletions(-)

-- 
2.38.1

Am 12.12.2022 um 13:59 hat Paolo Bonzini geschrieben:
> There are a few more lines of code that can be removed around draining
> code, but especially the logic can be simplified by removing unnecessary
> parameters.
> 
> Due to the failure of the block-next branch, the first three patches
> drop patches 14+15 of Kevin's drain cleanup series, and then redo
> patch 15 in a slightly less satisfactory way that still enables the
> remaining cleanups.  These reverts are not supposed to be applied;
> either the offending patches are dropped from the branch, or if the
> issue is fixed then my first three patches can go away.
> 
> The next three are taken from Emanuele's old subtree drain attempt
> at removing the AioContext.  The main one is the second, which is needed
> to avoid testcase failures, but I included all of them for simplicity.
> 
> Patch 7 fixes another latent bug exposed by the later cleanups, and while
> looking for a fix I noticed a general lack of thread-safety in BlockBackend's
> drain code.  There are some global properties that only need to be documented
> and enforced to be set only at creation time (patches 8/9), but also
> queued_requests is not protected by any mutex, which is fixed in patch 10.
> 
> Finally, patches 11-15 are the actual simplification.

Patches 12-15 actually look independent from the rest of the series, and
they look good to me. Could they be applied now even if the rest of the
series needs more discussion and a v2?

Kevin

Am 12.12.2022 um 13:59 hat Paolo Bonzini geschrieben:
> There are a few more lines of code that can be removed around draining
> code, but especially the logic can be simplified by removing unnecessary
> parameters.
> 
> Due to the failure of the block-next branch, the first three patches
> drop patches 14+15 of Kevin's drain cleanup series, and then redo
> patch 15 in a slightly less satisfactory way that still enables the
> remaining cleanups.  These reverts are not supposed to be applied;
> either the offending patches are dropped from the branch, or if the
> issue is fixed then my first three patches can go away.

Can you remind me which problem this was? The patches are in master now,
but I'm not sure if the latest version fixed whatever you had in mind.

> The next three are taken from Emanuele's old subtree drain attempt
> at removing the AioContext.  The main one is the second, which is needed
> to avoid testcase failures, but I included all of them for simplicity.
> 
> Patch 7 fixes another latent bug exposed by the later cleanups, and while
> looking for a fix I noticed a general lack of thread-safety in BlockBackend's
> drain code.  There are some global properties that only need to be documented
> and enforced to be set only at creation time (patches 8/9), but also
> queued_requests is not protected by any mutex, which is fixed in patch 10.
> 
> Finally, patches 11-15 are the actual simplification.
> 
> Applies on top of block-next.

Not any more. :-)

I found out that it applies on top of 6355f90eef, which may work for
some basic review, but the conflicts when rebasing seem non-trivial, so
we'll need a v2.

Kevin