[PATCH v5 00/31] block layer: split block APIs in global state and I/O

Emanuele Giuseppe Esposito posted 31 patches 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20211124064418.3120601-1-eesposit@redhat.com
Maintainers: Eduardo Habkost <ehabkost@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Markus Armbruster <armbru@redhat.com>, Juan Quintela <quintela@redhat.com>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Kevin Wolf <kwolf@redhat.com>, Eric Blake <eblake@redhat.com>, John Snow <jsnow@redhat.com>, Fam Zheng <fam@euphon.net>, Hanna Reitz <hreitz@redhat.com>, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
block/copy-before-write.h                   |    7 +
block/coroutines.h                          |    6 +
include/block/block-common.h                |  380 +++++
include/block/block-global-state.h          |  271 ++++
include/block/block-io.h                    |  322 ++++
include/block/block.h                       |  878 +----------
include/block/block_int-common.h            | 1204 +++++++++++++++
include/block/block_int-global-state.h      |  323 ++++
include/block/block_int-io.h                |  167 +++
include/block/block_int.h                   | 1475 +------------------
include/block/blockjob.h                    |    9 +
include/block/blockjob_int.h                |   28 +
include/block/snapshot.h                    |   13 +-
include/qemu/job.h                          |   31 +
include/qemu/main-loop.h                    |   13 +
include/sysemu/block-backend-common.h       |  102 ++
include/sysemu/block-backend-global-state.h |  121 ++
include/sysemu/block-backend-io.h           |  139 ++
include/sysemu/block-backend.h              |  269 +---
include/sysemu/blockdev.h                   |   13 +-
block.c                                     |  272 +++-
block/amend.c                               |   20 +
block/backup.c                              |    1 +
block/block-backend.c                       |  108 +-
block/commit.c                              |    4 +
block/copy-before-write.c                   |    2 +
block/create.c                              |   10 +
block/crypto.c                              |   18 +-
block/dirty-bitmap.c                        |    1 +
block/export/fuse.c                         |   16 +-
block/io.c                                  |   26 +
block/mirror.c                              |    4 +
block/monitor/bitmap-qmp-cmds.c             |    6 +
block/snapshot.c                            |   28 +
block/stream.c                              |    2 +
blockdev.c                                  |   28 +
blockjob.c                                  |   14 +
job.c                                       |   23 +
migration/savevm.c                          |    2 +
softmmu/cpus.c                              |    5 +
softmmu/qdev-monitor.c                      |    2 +
stubs/iothread-lock.c                       |    5 +
block/meson.build                           |    7 +-
43 files changed, 3758 insertions(+), 2617 deletions(-)
create mode 100644 include/block/block-common.h
create mode 100644 include/block/block-global-state.h
create mode 100644 include/block/block-io.h
create mode 100644 include/block/block_int-common.h
create mode 100644 include/block/block_int-global-state.h
create mode 100644 include/block/block_int-io.h
create mode 100644 include/sysemu/block-backend-common.h
create mode 100644 include/sysemu/block-backend-global-state.h
create mode 100644 include/sysemu/block-backend-io.h

[PATCH v5 00/31] block layer: split block APIs in global state and I/O

Posted by Emanuele Giuseppe Esposito 2 weeks ago
Currently, block layer APIs like block.h contain a mix of
functions that are either running in the main loop and under the
BQL, or are thread-safe functions and run in iothreads performing I/O.
The functions running under BQL also take care of modifying the
block graph, by using drain and/or aio_context_acquire/release.
This makes it very confusing to understand where each function
runs, and what assumptions it provided with regards to thread
safety.

We call the functions running under BQL "global state (GS) API", and
distinguish them from the thread-safe "I/O API".

The aim of this series is to split the relevant block headers in
global state and I/O sub-headers. The division will be done in
this way:
header.h will be split in header-global-state.h, header-io.h and
header-common.h. The latter will just contain the data structures
needed by header-global-state and header-io, and common helpers
that are neither in GS nor in I/O. header.h will remain for
legacy and to avoid changing all includes in all QEMU c files,
but will only include the two new headers. No function shall be
added in header.c .
Once we split all relevant headers, it will be much easier to see what
uses the AioContext lock and remove it, which is the overall main
goal of this and other series that I posted/will post.

In addition to splitting the relevant headers shown in this series,
it is also very helpful splitting the function pointers in some
block structures, to understand what runs under AioContext lock and
what doesn't. This is what patches 21-27 do.

Each function in the GS API will have an assertion, checking
that it is always running under BQL.
I/O functions are instead thread safe (or so should be), meaning
that they *can* run under BQL, but also in an iothread in another
AioContext. Therefore they do not provide any assertion, and
need to be audited manually to verify the correctness.

Adding assetions has helped finding 2 bugs already, as shown in
my series "Migration: fix missing iothread locking".

Tested this series by running unit tests, qemu-iotests and qtests
(x86_64).
Some functions in the GS API are used everywhere but not
properly tested. Therefore their assertion is never actually run in
the tests, so despite my very careful auditing, it is not impossible
to exclude that some will trigger while actually using QEMU.

Patch 1 introduces qemu_in_main_thread(), the function used in
all assertions. This had to be introduced otherwise all unit tests
would fail, since they run in the main loop but use the code in
stubs/iothread.c
Patches 2-27 (with the exception of patch 9-10, that are an additional
assert) are all structured in the same way: first we split the header
and in the next (or same, if small) patch we add assertions.
Patch 28-31 take care instead of the block layer permission API,
fixing some bugs where they are used in I/O functions.

Next steps once this get reviewed:
1) audit the GS API and replace the AioContext lock with drains,
or remove them when not necessary (requires further discussion).
2) [optional as it should be already the case] audit the I/O API
and check that thread safety is guaranteed

In order to keep this series a little bit smaller, move some
refactoring patches in another series, "block: minor refactoring
in preparation to the block layer API split".

Based-on: <20211124063640.3118897-1-eesposit@redhat.com>

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
v5 -> v6:
* In short, apply all Hanna's comments. More in details,
  the following functions in the following headers have been moved:
  block-backend:
    blk_replace_bs (to gs)
    blk_nb_sectors (to io)
    blk_name (to io)
    blk_set_perm (to io)
    blk_get_perm (to io)
    blk_drain (to io)
    blk_abort_aio_request (to io)
    blk_make_empty (to gs)
    blk_invalidate_cache (was in io, but had GS assertion)
    blk_aio_cancel (to gs)
  block:
    bdrv_replace_child_bs (to gs)
    bdrv_get_device_name (to io)
    bdrv_get_device_or_node_name (to io)
    bdrv_drained_end_no_poll (to io)
    bdrv_block_status (was in io, but had GS assertion)
    bdrv_drain (to io)
    bdrv_co_drain (to io)
    bdrv_make_zero (was in GS, but did not have the assertion)
    bdrv_save_vmstate (to io)
    bdrv_load_vmstate (to io)
    bdrv_aio_cancel_async (to io)
  block_int:
    bdrv_get_parent_name (to io)
    bdrv_apply_subtree_drain (to io)
    bdrv_unapply_subtree_drain (to io)
    bdrv_co_copy_range_from (was in io, but had GS assertion)
    bdrv_co_copy_range_to (was in io, but had GS assertion)
    ->bdrv_save_vmstate (to io)
    ->bdrv_load_vmstate (to io)

  coding style (assertion after definitions):
    bdrv_save_vmstate
    bdrv_load_vmstate
    block_job_next
    block_job_get

  new patches:
    block.c: modify .attach and .detach callbacks of child_of_bds
    introduce pre_run as JobDriver callback to handle
      bdrv_co_amend usage of permission function
    leave blk_set/get_perm as a TODO in fuse.c
    make sure bdrv_co_invalidate_cache does not use permissions
      if BQL is not held

  minor changes:
    put back TODO for include block/block.h in block-backend-common.h
    rebase on kwolf/block branch
    modify where are used assert_bdrv_graph_writable, due to rebase

v4 -> v5:
* Moved the following functions from block-io to
  block-global-state (+ assertion):
  - bdrv_apply_auto_read_only
  - bdrv_can_set_read_only
* Moved the following functions from block-backend-io to
  block-backend-global-state (+ assertion):
  - blk_ioctl
* added patch 4 to distinguish assertions added to static functions
  in block.c
* block/coroutines: it seems that blk_co_do_ioctl and
  blk_do_ioctl are global state too

v3 -> v4:
* blockdev.h (patch 14): blockdev_mark_auto_del, blockdev_auto_del
  and blk_legacy_dinfo as GS API.
* add copyright header to block.h, block-io.h and block-global-state.h
* rebase on current master (c5b2f55981)

v2 -> v3:
* rename "graph API" into "global state API"
* change order of patches, block.h comes before block-backend.h
* change GS and I/O comment headers to avoid redundancy, all other
  headers refer to block-global-state.h and block-io.h
* fix typo on GS and I/O headers
* use assert instead of g_assert
* move bdrv_pwrite_sync, bdrv_block_status and bdrv_co_copy_range_{from/to}
  to the I/O API
* change assert_bdrv_graph_writable implementation, since we need
  to introduce additional drains
* remove transactions API split
* add preparation patch for blockdev.h (patch 13)
* backup-top -> copy-on-write
* change I/O comment in job.h into a better meaningful explanation
* fix all warnings given by checkpatch, mostly due to /* */ to be
  split in separate lines
* rebase on current master (c09124dcb8), and split the following new functions:
	blk_replace_bs (I/O)
	bdrv_bsc_is_data (I/O)
	bdrv_bsc_invalidate_range (I/O)
	bdrv_bsc_fill (I/O)
	bdrv_new_open_driver_opts (GS)
	blk_get_max_hw_iov (I/O)
  they are all added in patches 4 and 6.

v1 -> v2:
* remove the iothread locking bug fix, and send it as separate patch
* rename graph API -> global state API
* better documented patch 1 (qemu_in_main_thread)
* add and split all other block layer headers
* fix warnings given by checkpatch on multiline comments

Emanuele Giuseppe Esposito (31):
  main-loop.h: introduce qemu_in_main_thread()
  include/block/block: split header into I/O and global state API
  assertions for block global state API
  include/sysemu/block-backend: split header into I/O and global state
    (GS) API
  block-backend: special comments for blk_set/get_perm due to fuse
  block/block-backend.c: assertions for block-backend
  include/block/block_int: split header into I/O and global state API
  assertions for block_int global state API
  block: introduce assert_bdrv_graph_writable
  block.c: modify .attach and .detach callbacks of child_of_bds
  include/block/blockjob_int.h: split header into I/O and GS API
  assertions for blockjob_int.h
  block.c: add assertions to static functions
  include/block/blockjob.h: global state API
  assertions for blockjob.h global state API
  include/sysemu/blockdev.h: global state API
  assertions for blockdev.h global state API
  include/block/snapshot: global state API + assertions
  block/copy-before-write.h: global state API + assertions
  block/coroutines: I/O API
  block_int-common.h: split function pointers in BlockDriver
  block_int-common.h: assertion in the callers of BlockDriver function
    pointers
  block_int-common.h: split function pointers in BdrvChildClass
  block_int-common.h: assertions in the callers of BdrvChildClass
    function pointers
  block-backend-common.h: split function pointers in BlockDevOps
  job.h: split function pointers in JobDriver
  job.h: assertions in the callers of JobDriver funcion pointers
  block.c: assert BQL lock held in bdrv_co_invalidate_cache
  jobs: introduce pre_run function in JobDriver
  crypto: delegate permission functions to JobDriver .pre_run
  block.c: assertions to the block layer permissions API

 block/copy-before-write.h                   |    7 +
 block/coroutines.h                          |    6 +
 include/block/block-common.h                |  380 +++++
 include/block/block-global-state.h          |  271 ++++
 include/block/block-io.h                    |  322 ++++
 include/block/block.h                       |  878 +----------
 include/block/block_int-common.h            | 1204 +++++++++++++++
 include/block/block_int-global-state.h      |  323 ++++
 include/block/block_int-io.h                |  167 +++
 include/block/block_int.h                   | 1475 +------------------
 include/block/blockjob.h                    |    9 +
 include/block/blockjob_int.h                |   28 +
 include/block/snapshot.h                    |   13 +-
 include/qemu/job.h                          |   31 +
 include/qemu/main-loop.h                    |   13 +
 include/sysemu/block-backend-common.h       |  102 ++
 include/sysemu/block-backend-global-state.h |  121 ++
 include/sysemu/block-backend-io.h           |  139 ++
 include/sysemu/block-backend.h              |  269 +---
 include/sysemu/blockdev.h                   |   13 +-
 block.c                                     |  272 +++-
 block/amend.c                               |   20 +
 block/backup.c                              |    1 +
 block/block-backend.c                       |  108 +-
 block/commit.c                              |    4 +
 block/copy-before-write.c                   |    2 +
 block/create.c                              |   10 +
 block/crypto.c                              |   18 +-
 block/dirty-bitmap.c                        |    1 +
 block/export/fuse.c                         |   16 +-
 block/io.c                                  |   26 +
 block/mirror.c                              |    4 +
 block/monitor/bitmap-qmp-cmds.c             |    6 +
 block/snapshot.c                            |   28 +
 block/stream.c                              |    2 +
 blockdev.c                                  |   28 +
 blockjob.c                                  |   14 +
 job.c                                       |   23 +
 migration/savevm.c                          |    2 +
 softmmu/cpus.c                              |    5 +
 softmmu/qdev-monitor.c                      |    2 +
 stubs/iothread-lock.c                       |    5 +
 block/meson.build                           |    7 +-
 43 files changed, 3758 insertions(+), 2617 deletions(-)
 create mode 100644 include/block/block-common.h
 create mode 100644 include/block/block-global-state.h
 create mode 100644 include/block/block-io.h
 create mode 100644 include/block/block_int-common.h
 create mode 100644 include/block/block_int-global-state.h
 create mode 100644 include/block/block_int-io.h
 create mode 100644 include/sysemu/block-backend-common.h
 create mode 100644 include/sysemu/block-backend-global-state.h
 create mode 100644 include/sysemu/block-backend-io.h

-- 
2.27.0


Re: [PATCH v5 00/31] block layer: split block APIs in global state and I/O

Posted by Stefan Hajnoczi 1 week, 1 day ago
On Wed, Nov 24, 2021 at 01:43:47AM -0500, Emanuele Giuseppe Esposito wrote:
> v5 -> v6:
> * In short, apply all Hanna's comments. More in details,
>   the following functions in the following headers have been moved:
>   block-backend:
>     blk_replace_bs (to gs)
>     blk_nb_sectors (to io)
>     blk_name (to io)
>     blk_set_perm (to io)
>     blk_get_perm (to io)
>     blk_drain (to io)
>     blk_abort_aio_request (to io)
>     blk_make_empty (to gs)
>     blk_invalidate_cache (was in io, but had GS assertion)
>     blk_aio_cancel (to gs)
>   block:
>     bdrv_replace_child_bs (to gs)
>     bdrv_get_device_name (to io)
>     bdrv_get_device_or_node_name (to io)
>     bdrv_drained_end_no_poll (to io)
>     bdrv_block_status (was in io, but had GS assertion)
>     bdrv_drain (to io)
>     bdrv_co_drain (to io)
>     bdrv_make_zero (was in GS, but did not have the assertion)
>     bdrv_save_vmstate (to io)
>     bdrv_load_vmstate (to io)
>     bdrv_aio_cancel_async (to io)
>   block_int:
>     bdrv_get_parent_name (to io)
>     bdrv_apply_subtree_drain (to io)
>     bdrv_unapply_subtree_drain (to io)
>     bdrv_co_copy_range_from (was in io, but had GS assertion)
>     bdrv_co_copy_range_to (was in io, but had GS assertion)
>     ->bdrv_save_vmstate (to io)
>     ->bdrv_load_vmstate (to io)
> 
>   coding style (assertion after definitions):
>     bdrv_save_vmstate
>     bdrv_load_vmstate
>     block_job_next
>     block_job_get
> 
>   new patches:
>     block.c: modify .attach and .detach callbacks of child_of_bds
>     introduce pre_run as JobDriver callback to handle
>       bdrv_co_amend usage of permission function
>     leave blk_set/get_perm as a TODO in fuse.c
>     make sure bdrv_co_invalidate_cache does not use permissions
>       if BQL is not held
> 
>   minor changes:
>     put back TODO for include block/block.h in block-backend-common.h
>     rebase on kwolf/block branch
>     modify where are used assert_bdrv_graph_writable, due to rebase

These changes sound fine to me. Hanna or Kevin can merge the series when
they are happy.

Stefan

Re: [PATCH v5 00/31] block layer: split block APIs in global state and I/O

Posted by Emanuele Giuseppe Esposito 1 week, 1 day ago

On 29/11/2021 14:32, Stefan Hajnoczi wrote:
> On Wed, Nov 24, 2021 at 01:43:47AM -0500, Emanuele Giuseppe Esposito wrote:
>> v5 -> v6:
>> * In short, apply all Hanna's comments. More in details,
>>    the following functions in the following headers have been moved:
>>    block-backend:
>>      blk_replace_bs (to gs)
>>      blk_nb_sectors (to io)
>>      blk_name (to io)
>>      blk_set_perm (to io)
>>      blk_get_perm (to io)
>>      blk_drain (to io)
>>      blk_abort_aio_request (to io)
>>      blk_make_empty (to gs)
>>      blk_invalidate_cache (was in io, but had GS assertion)
>>      blk_aio_cancel (to gs)
>>    block:
>>      bdrv_replace_child_bs (to gs)
>>      bdrv_get_device_name (to io)
>>      bdrv_get_device_or_node_name (to io)
>>      bdrv_drained_end_no_poll (to io)
>>      bdrv_block_status (was in io, but had GS assertion)
>>      bdrv_drain (to io)
>>      bdrv_co_drain (to io)
>>      bdrv_make_zero (was in GS, but did not have the assertion)
>>      bdrv_save_vmstate (to io)
>>      bdrv_load_vmstate (to io)
>>      bdrv_aio_cancel_async (to io)
>>    block_int:
>>      bdrv_get_parent_name (to io)
>>      bdrv_apply_subtree_drain (to io)
>>      bdrv_unapply_subtree_drain (to io)
>>      bdrv_co_copy_range_from (was in io, but had GS assertion)
>>      bdrv_co_copy_range_to (was in io, but had GS assertion)
>>      ->bdrv_save_vmstate (to io)
>>      ->bdrv_load_vmstate (to io)
>>
>>    coding style (assertion after definitions):
>>      bdrv_save_vmstate
>>      bdrv_load_vmstate
>>      block_job_next
>>      block_job_get
>>
>>    new patches:
>>      block.c: modify .attach and .detach callbacks of child_of_bds
>>      introduce pre_run as JobDriver callback to handle
>>        bdrv_co_amend usage of permission function
>>      leave blk_set/get_perm as a TODO in fuse.c
>>      make sure bdrv_co_invalidate_cache does not use permissions
>>        if BQL is not held
>>
>>    minor changes:
>>      put back TODO for include block/block.h in block-backend-common.h
>>      rebase on kwolf/block branch
>>      modify where are used assert_bdrv_graph_writable, due to rebase
> 
> These changes sound fine to me. Hanna or Kevin can merge the series when
> they are happy.
> 

Just a tiny nitpick, I would just not merge patch 10: "block.c: modify 
.attach and .detach callbacks of child_of_bds". It is not wrong and 
should produce no side effect, but does not do what it was intended for 
(ie using the drain to protect the bs's list of children). I will work 
on that in a future serie. Apologies.

Thank you,
Emanuele