[PULL v3 00/50] Block layer patches

[PULL v3 00/50] Block layer patches

[Kevin Wolf]

The following changes since commit 48804eebd4a327e4b11f902ba80a00876ee53a43:

  Merge tag 'pull-misc-2022-12-14' of https://repo.or.cz/qemu/armbru into staging (2022-12-15 10:13:46 +0000)

are available in the Git repository at:

  https://repo.or.cz/qemu/kevin.git tags/for-upstream

for you to fetch changes up to 1b3ff9feb942c2ad0b01ac931e99ad451ab0ef39:

  block: GRAPH_RDLOCK for functions only called by co_wrappers (2022-12-15 16:08:23 +0100)

v3:
- Dropped "configure: Enable -Wthread-safety if present" because FreeBSD
  has TSA annotations in its pthread locking functions, so we would have
  to annotate the use of every lock in QEMU first before we can enable
  it.

v2:
- Changed TSA capability name to "mutex" to work with older clang
  versions. The tsan-build CI job succeeds now.

----------------------------------------------------------------
Block layer patches

- Code cleanups around block graph modification
- Simplify drain
- coroutine_fn correctness fixes, including splitting generated
  coroutine wrappers into co_wrapper (to be called only from
  non-coroutine context) and co_wrapper_mixed (both coroutine and
  non-coroutine context)
- Introduce a block graph rwlock

----------------------------------------------------------------
Emanuele Giuseppe Esposito (21):
      block-io: introduce coroutine_fn duplicates for bdrv_common_block_status_above callers
      block-copy: add coroutine_fn annotations
      nbd/server.c: add coroutine_fn annotations
      block-backend: replace bdrv_*_above with blk_*_above
      block/vmdk: add coroutine_fn annotations
      block: avoid duplicating filename string in bdrv_create
      block: distinguish between bdrv_create running in coroutine and not
      block: bdrv_create_file is a coroutine_fn
      block: rename generated_co_wrapper in co_wrapper_mixed
      block-coroutine-wrapper.py: introduce co_wrapper
      block-coroutine-wrapper.py: support functions without bs arg
      block-coroutine-wrapper.py: support also basic return types
      block: convert bdrv_create to co_wrapper
      block/dirty-bitmap: convert coroutine-only functions to co_wrapper
      graph-lock: Implement guard macros
      async: Register/unregister aiocontext in graph lock list
      block: wrlock in bdrv_replace_child_noperm
      block: remove unnecessary assert_bdrv_graph_writable()
      block: assert that graph read and writes are performed correctly
      block-coroutine-wrapper.py: introduce annotations that take the graph rdlock
      block: use co_wrapper_mixed_bdrv_rdlock in functions taking the rdlock

Kevin Wolf (24):
      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: Fix locking for bdrv_reopen_queue_child()
      block: Drain individual 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_begin/end.
      block: Drop out of coroutine in bdrv_do_drained_begin_quiesce()
      block: Don't poll in bdrv_replace_child_noperm()
      block: Remove poll parameter from bdrv_parent_drained_begin_single()
      block: Factor out bdrv_drain_all_begin_nopoll()
      Import clang-tsa.h
      clang-tsa: Add TSA_ASSERT() macro
      clang-tsa: Add macros for shared locks
      test-bdrv-drain: Fix incorrrect drain assumptions
      block: Fix locking in external_snapshot_prepare()
      graph-lock: TSA annotations for lock/unlock functions
      Mark assert_bdrv_graph_readable/writable() GRAPH_RD/WRLOCK
      block: GRAPH_RDLOCK for functions only called by co_wrappers

Paolo Bonzini (1):
      graph-lock: Introduce a lock to protect block graph operations

Vladimir Sementsov-Ogievskiy (4):
      block: Inline bdrv_detach_child()
      block: drop bdrv_remove_filter_or_cow_child
      block: bdrv_refresh_perms(): allow external tran
      block: refactor bdrv_list_refresh_perms to allow any list of nodes

 docs/devel/block-coroutine-wrapper.rst |   6 +-
 block/block-gen.h                      |  11 +-
 block/coroutines.h                     |  21 +-
 include/block/aio.h                    |   9 +
 include/block/block-common.h           |  27 ++-
 include/block/block-copy.h             |   5 +-
 include/block/block-global-state.h     |  15 +-
 include/block/block-io.h               | 136 +++++------
 include/block/block_int-common.h       |  49 ++--
 include/block/block_int-global-state.h |  17 --
 include/block/block_int-io.h           |  12 -
 include/block/block_int.h              |   1 +
 include/block/dirty-bitmap.h           |  10 +-
 include/block/graph-lock.h             | 280 +++++++++++++++++++++++
 include/qemu/clang-tsa.h               | 114 ++++++++++
 include/sysemu/block-backend-io.h      |  77 ++++---
 block.c                                | 404 ++++++++++++++++++---------------
 block/block-backend.c                  |  25 +-
 block/block-copy.c                     |  21 +-
 block/commit.c                         |   4 +-
 block/crypto.c                         |   2 +-
 block/dirty-bitmap.c                   |  88 +------
 block/graph-lock.c                     | 275 ++++++++++++++++++++++
 block/io.c                             | 367 ++++++++++--------------------
 block/parallels.c                      |   2 +-
 block/qcow.c                           |   2 +-
 block/qcow2.c                          |   4 +-
 block/qed.c                            |  28 ++-
 block/raw-format.c                     |   2 +-
 block/replication.c                    |   6 -
 block/stream.c                         |  26 ++-
 block/throttle.c                       |   8 +-
 block/vdi.c                            |   2 +-
 block/vhdx.c                           |   2 +-
 block/vmdk.c                           |  38 ++--
 block/vpc.c                            |   2 +-
 blockdev.c                             |  17 +-
 blockjob.c                             |   2 +-
 nbd/server.c                           |  47 ++--
 stubs/graph-lock.c                     |  10 +
 tests/unit/test-bdrv-drain.c           | 387 +++++++++----------------------
 util/async.c                           |   4 +
 scripts/block-coroutine-wrapper.py     | 133 ++++++++---
 block/meson.build                      |   2 +
 stubs/meson.build                      |   1 +
 45 files changed, 1574 insertions(+), 1127 deletions(-)
 create mode 100644 include/block/graph-lock.h
 create mode 100644 include/qemu/clang-tsa.h
 create mode 100644 block/graph-lock.c
 create mode 100644 stubs/graph-lock.c

Re: [PULL v3 00/50] Block layer patches

[Peter Maydell]

On Thu, 15 Dec 2022 at 15:24, Kevin Wolf <kwolf@redhat.com> wrote:
>
> The following changes since commit 48804eebd4a327e4b11f902ba80a00876ee53a43:
>
>   Merge tag 'pull-misc-2022-12-14' of https://repo.or.cz/qemu/armbru into staging (2022-12-15 10:13:46 +0000)
>
> are available in the Git repository at:
>
>   https://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to 1b3ff9feb942c2ad0b01ac931e99ad451ab0ef39:
>
>   block: GRAPH_RDLOCK for functions only called by co_wrappers (2022-12-15 16:08:23 +0100)
>
> v3:
> - Dropped "configure: Enable -Wthread-safety if present" because FreeBSD
>   has TSA annotations in its pthread locking functions, so we would have
>   to annotate the use of every lock in QEMU first before we can enable
>   it.
>
> v2:
> - Changed TSA capability name to "mutex" to work with older clang
>   versions. The tsan-build CI job succeeds now.
>
> ----------------------------------------------------------------
> Block layer patches
>
> - Code cleanups around block graph modification
> - Simplify drain
> - coroutine_fn correctness fixes, including splitting generated
>   coroutine wrappers into co_wrapper (to be called only from
>   non-coroutine context) and co_wrapper_mixed (both coroutine and
>   non-coroutine context)
> - Introduce a block graph rwlock


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/8.0
for any user-visible changes.

-- PMM

Re: [PULL v3 00/50] Block layer patches

[Fabiano Rosas]

Kevin Wolf <kwolf@redhat.com> writes:

> The following changes since commit 48804eebd4a327e4b11f902ba80a00876ee53a43:
>
>   Merge tag 'pull-misc-2022-12-14' of https://repo.or.cz/qemu/armbru into staging (2022-12-15 10:13:46 +0000)
>
> are available in the Git repository at:
>
>   https://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to 1b3ff9feb942c2ad0b01ac931e99ad451ab0ef39:
>
>   block: GRAPH_RDLOCK for functions only called by co_wrappers (2022-12-15 16:08:23 +0100)
>
> v3:
> - Dropped "configure: Enable -Wthread-safety if present" because FreeBSD
>   has TSA annotations in its pthread locking functions, so we would have
>   to annotate the use of every lock in QEMU first before we can enable
>   it.
>
> v2:
> - Changed TSA capability name to "mutex" to work with older clang
>   versions. The tsan-build CI job succeeds now.
>
> ----------------------------------------------------------------
> Block layer patches
>
> - Code cleanups around block graph modification
> - Simplify drain
> - coroutine_fn correctness fixes, including splitting generated
>   coroutine wrappers into co_wrapper (to be called only from
>   non-coroutine context) and co_wrapper_mixed (both coroutine and
>   non-coroutine context)
> - Introduce a block graph rwlock
>
> ----------------------------------------------------------------
> Emanuele Giuseppe Esposito (21):
>       block-io: introduce coroutine_fn duplicates for bdrv_common_block_status_above callers
>       block-copy: add coroutine_fn annotations
>       nbd/server.c: add coroutine_fn annotations
>       block-backend: replace bdrv_*_above with blk_*_above
>       block/vmdk: add coroutine_fn annotations
>       block: avoid duplicating filename string in bdrv_create
>       block: distinguish between bdrv_create running in coroutine and not
>       block: bdrv_create_file is a coroutine_fn
>       block: rename generated_co_wrapper in co_wrapper_mixed
>       block-coroutine-wrapper.py: introduce co_wrapper
>       block-coroutine-wrapper.py: support functions without bs arg
>       block-coroutine-wrapper.py: support also basic return types
>       block: convert bdrv_create to co_wrapper
>       block/dirty-bitmap: convert coroutine-only functions to co_wrapper
>       graph-lock: Implement guard macros
>       async: Register/unregister aiocontext in graph lock list
>       block: wrlock in bdrv_replace_child_noperm
>       block: remove unnecessary assert_bdrv_graph_writable()
>       block: assert that graph read and writes are performed correctly
>       block-coroutine-wrapper.py: introduce annotations that take the graph rdlock
>       block: use co_wrapper_mixed_bdrv_rdlock in functions taking the rdlock
>
> Kevin Wolf (24):
>       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: Fix locking for bdrv_reopen_queue_child()
>       block: Drain individual 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_begin/end.
>       block: Drop out of coroutine in bdrv_do_drained_begin_quiesce()
>       block: Don't poll in bdrv_replace_child_noperm()
>       block: Remove poll parameter from bdrv_parent_drained_begin_single()
>       block: Factor out bdrv_drain_all_begin_nopoll()

Hi, 

With today's master at c15dc499cc (Merge tag 'pull-misc-20221218'
of https://gitlab.com/rth7680/qemu into staging, 2022-12-19),

I get a test failure:

$ make check-avocado AVOCADO_TESTS=../tests/avocado/reverse_debugging.py:ReverseDebugging_X86_64.test_x86_64_pc
...
Output: qemu-system-x86_64: ../block/block-backend.c:2572:
blk_root_drained_poll: Assertion `blk->quiesce_counter' failed.

Bisect points to this patch
da0bd74434 (block: Factor out bdrv_drain_all_begin_nopoll(), 2022-12-07)

Command from avocado logs (paths cut to save space):

./qemu-system-x86_64 -display none -vga none \
-chardev socket,id=mon,path=monitor.sock \
-mon chardev=mon,mode=control -machine pc \
-chardev socket,id=console,path=console.sock,server=on,wait=off \
-serial chardev:console -icount shift=7,rr=record,rrfile=replay.bin,rrsnapshot=init \
-net none -drive file=disk.qcow2,if=none

Happens with arm as well:

./qemu-system-aarch64 -display none -vga none \
-chardev socket,id=mon,path=monitor.sock \
-mon chardev=mon,mode=control -machine virt \
-chardev socket,id=console,path=console.sock,server=on,wait=off \
-serial chardev:console -cpu cortex-a53 -icount \
 shift=7,rr=record,rrfile=replay.bin,rrsnapshot=init \
-net none -drive file=disk.qcow2,if=none \
-kernel vmlinuz

>       Import clang-tsa.h
>       clang-tsa: Add TSA_ASSERT() macro
>       clang-tsa: Add macros for shared locks
>       test-bdrv-drain: Fix incorrrect drain assumptions
>       block: Fix locking in external_snapshot_prepare()
>       graph-lock: TSA annotations for lock/unlock functions
>       Mark assert_bdrv_graph_readable/writable() GRAPH_RD/WRLOCK
>       block: GRAPH_RDLOCK for functions only called by co_wrappers
>
> Paolo Bonzini (1):
>       graph-lock: Introduce a lock to protect block graph operations
>
> Vladimir Sementsov-Ogievskiy (4):
>       block: Inline bdrv_detach_child()
>       block: drop bdrv_remove_filter_or_cow_child
>       block: bdrv_refresh_perms(): allow external tran
>       block: refactor bdrv_list_refresh_perms to allow any list of nodes
>

Re: [PULL v3 00/50] Block layer patches

[Peter Maydell]

On Mon, 19 Dec 2022 at 23:55, Fabiano Rosas <farosas@suse.de> wrote:
> With today's master at c15dc499cc (Merge tag 'pull-misc-20221218'
> of https://gitlab.com/rth7680/qemu into staging, 2022-12-19),
>
> I get a test failure:
>
> $ make check-avocado AVOCADO_TESTS=../tests/avocado/reverse_debugging.py:ReverseDebugging_X86_64.test_x86_64_pc
> ...
> Output: qemu-system-x86_64: ../block/block-backend.c:2572:
> blk_root_drained_poll: Assertion `blk->quiesce_counter' failed.
>
> Bisect points to this patch
> da0bd74434 (block: Factor out bdrv_drain_all_begin_nopoll(), 2022-12-07)

Yep, that does seem to be the commit that introduces the
regression. The problem is that the refactoring has broken
the early-return if replay_events_enabled() is true --
before the refactor that 'return' used to return direcly
from bdrv_drain_all_begin(); now it only returns from
bdrv_drain_all_begin_nopoll() and doesn't cause the
calling bdrv_drain_all_begin() to return early, so
execution continues into the following code when it should
not.

-- PMM