1. Patchew
  2. QEMU
  3. View series

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

Kevin Wolf posted 22 patches 7 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20230929145157.45443-1-kwolf@redhat.com
Maintainers: Kevin Wolf <kwolf@redhat.com>, Hanna Reitz <hreitz@redhat.com>, John Snow <jsnow@redhat.com>, Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>, Stefan Hajnoczi <stefanha@redhat.com>, Fam Zheng <fam@euphon.net>, Ronnie Sahlberg <ronniesahlberg@gmail.com>, Paolo Bonzini <pbonzini@redhat.com>, Peter Lieven <pl@kamp.de>, Eric Blake <eblake@redhat.com>, "Denis V. Lunev" <den@openvz.org>, Alberto Garcia <berto@igalia.com>, Ilya Dryomov <idryomov@gmail.com>, Wen Congyang <wencongyang2@huawei.com>, Xie Changlong <xiechanglong.d@gmail.com>, Stefan Weil <sw@weilnetz.de>, Jeff Cody <codyprime@gmail.com>, Juan Quintela <quintela@redhat.com>, Peter Xu <peterx@redhat.com>, Fabiano Rosas <farosas@suse.de>, Leonardo Bras <leobras@redhat.com>, Cleber Rosa <crosa@redhat.com>
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(-)
[PATCH 00/22] block: Graph locking part 5 (protect children/parent links)
Posted by Kevin Wolf 7 months ago 
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
Re: [PATCH 00/22] block: Graph locking part 5 (protect children/parent links)
Posted by Stefan Hajnoczi 6 months, 3 weeks ago 
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>
Re: [PATCH 00/22] block: Graph locking part 5 (protect children/parent links)
Posted by Kevin Wolf 6 months, 3 weeks ago 
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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.