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

[Kevin Wolf <kwolf@redhat.com>]

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

Am 10.10.2023 um 22:48 hat Stefan Hajnoczi geschrieben:
> 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>

Thanks, applied to the block branch.

Kevin

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