Re: [PATCH 00/22] block: Graph locking part 5 (protect children/parent links)

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6089 bytes --]

On Fri, Sep 29, 2023 at 04:51:35PM +0200, Kevin Wolf wrote:
> After all the preparation in previous series, this series reaches an
> important milestone for the graph locking work: TSA can now verify that
> all accesses to the children and parent lists of nodes happen under the
> graph lock.
> 
> However, this is not the end of the graph locking work yet. On the one
> hand, graph locking annotations aren't consistently present on all
> functions and having an unannotated function in the middle of the call
> chain means that TSA doesn't check if the locking is consistent (in
> fact, we know that functions like bdrv_unref() are still called in
> places where they strictly speaking must not be called).
> 
> On the other hand, the graph consists of more than just the children and
> parent lists. While it might be possible to convince me that the global
> node lists (graph_bdrv_states/all_bdrv_states) are safe because
> iothreads shouldn't access them, at least BlockDriverState.file/backing
> still need proper locking.
> 
> There may be other fields in BlockDriverState that need to be covered
> by the lock, too. For example, Stefan said that he would look into
> annotating BlockLimits accesses to be protected by the graph lock, too.
> 
> Emanuele Giuseppe Esposito (1):
>   block: Mark drain related functions GRAPH_RDLOCK
> 
> Kevin Wolf (21):
>   test-bdrv-drain: Don't call bdrv_graph_wrlock() in coroutine context
>   block-coroutine-wrapper: Add no_co_wrapper_bdrv_rdlock functions
>   block: Take graph rdlock in bdrv_inactivate_all()
>   block: Mark bdrv_first_blk() and bdrv_is_root_node() GRAPH_RDLOCK
>   block: Mark bdrv_parent_cb_resize() and callers GRAPH_RDLOCK
>   block: Mark bdrv_snapshot_fallback() and callers GRAPH_RDLOCK
>   block: Take graph rdlock in parts of reopen
>   block: Mark bdrv_get_xdbg_block_graph() and callers GRAPH_RDLOCK
>   block: Mark bdrv_refresh_filename() and callers GRAPH_RDLOCK
>   block: Mark bdrv_primary_child() and callers GRAPH_RDLOCK
>   block: Mark bdrv_get_parent_name() and callers GRAPH_RDLOCK
>   block: Mark bdrv_amend_options() and callers GRAPH_RDLOCK
>   qcow2: Mark qcow2_signal_corruption() and callers GRAPH_RDLOCK
>   qcow2: Mark qcow2_inactivate() and callers GRAPH_RDLOCK
>   qcow2: Mark check_constraints_on_bitmap() GRAPH_RDLOCK
>   block: Mark bdrv_op_is_blocked() and callers GRAPH_RDLOCK
>   block: Mark bdrv_apply_auto_read_only() and callers GRAPH_RDLOCK
>   block: Mark bdrv_get_specific_info() and callers GRAPH_RDLOCK
>   block: Protect bs->parents with graph_lock
>   block: Protect bs->children with graph_lock
>   block: Add assertion for bdrv_graph_wrlock()
> 
>  block/qcow2.h                               | 187 ++++++++++++--------
>  block/vhdx.h                                |   5 +-
>  include/block/block-common.h                |   7 +-
>  include/block/block-global-state.h          |  34 ++--
>  include/block/block-io.h                    |  42 +++--
>  include/block/block_int-common.h            |  69 ++++----
>  include/block/block_int-io.h                |   7 +-
>  include/block/graph-lock.h                  |   3 +-
>  include/block/qapi.h                        |  23 ++-
>  include/block/snapshot.h                    |  24 +--
>  include/sysemu/block-backend-global-state.h |   4 +-
>  block.c                                     | 120 +++++++++----
>  block/backup.c                              |   1 +
>  block/block-backend.c                       |   9 +-
>  block/bochs.c                               |   2 +
>  block/cloop.c                               |   2 +
>  block/commit.c                              |   1 +
>  block/crypto.c                              |   4 +-
>  block/curl.c                                |   2 +
>  block/dmg.c                                 |   2 +
>  block/export/export.c                       |   4 +
>  block/gluster.c                             |   2 +
>  block/graph-lock.c                          |   3 +-
>  block/io.c                                  |  45 ++++-
>  block/iscsi.c                               |   2 +
>  block/monitor/block-hmp-cmds.c              |   5 +
>  block/nbd.c                                 |   3 +-
>  block/nfs.c                                 |   2 +-
>  block/parallels.c                           |   3 +
>  block/qapi-sysemu.c                         |  11 +-
>  block/qapi.c                                |  11 +-
>  block/qcow.c                                |   3 +
>  block/qcow2-bitmap.c                        |  38 ++--
>  block/qcow2-cache.c                         |  11 +-
>  block/qcow2-cluster.c                       |  62 +++----
>  block/qcow2-refcount.c                      |  80 +++++----
>  block/qcow2.c                               |  72 +++++---
>  block/quorum.c                              |   4 +-
>  block/raw-format.c                          |   2 +
>  block/rbd.c                                 |   4 +
>  block/replication.c                         |  21 ++-
>  block/snapshot.c                            |  54 +++++-
>  block/vdi.c                                 |   3 +
>  block/vhdx.c                                |   4 +
>  block/vmdk.c                                |  53 +++---
>  block/vpc.c                                 |   3 +
>  block/vvfat.c                               |   2 +
>  blockdev.c                                  |  44 +++++
>  blockjob.c                                  |   1 +
>  migration/block.c                           |   2 +
>  migration/migration-hmp-cmds.c              |   2 +
>  qemu-img.c                                  |  16 ++
>  qemu-io-cmds.c                              |   3 +
>  tests/unit/test-bdrv-drain.c                |  15 +-
>  tests/unit/test-block-iothread.c            |   8 +
>  scripts/block-coroutine-wrapper.py          |  10 +-
>  56 files changed, 769 insertions(+), 387 deletions(-)
> 
> -- 
> 2.41.0
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]