| Message ID | 20230929145157.45443-1-kwolf@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | block: Graph locking part 5 (protect children/parent links) | expand |
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>
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