Re: [PATCH 00/18] block: Introduce a block graph rwlock
Am 07.12.2022 um 15:12 hat Emanuele Giuseppe Esposito geschrieben: > Am 07/12/2022 um 14:18 schrieb Kevin Wolf: > > This series supersedes the first half of Emanuele's "Protect the block > > layer with a rwlock: part 1". It introduces the basic infrastructure for > > protecting the block graph (specifically parent/child links) with a > > rwlock. Actually taking the reader lock in all necessary places is left > > for future series. > > > > Compared to Emanuele's series, this one adds patches to make use of > > clang's Thread Safety Analysis (TSA) feature in order to statically > > check at compile time that the places where we assert that we hold the > > lock actually do hold it. Once we cover all relevant places, the check > > can be extended to verify that all accesses of bs->children and > > bs->parents hold the lock. > > > > For reference, here is the more detailed version of our plan in > > Emanuele's words from his series: > > > > The aim is to replace the current AioContext lock with much > > fine-grained locks, aimed to protect only specific data. Currently > > the AioContext lock is used pretty much everywhere, and it's not > > even clear what it is protecting exactly. > > > > The aim of the rwlock is to cover graph modifications: more > > precisely, when a BlockDriverState parent or child list is modified > > or read, since it can be concurrently accessed by the main loop and > > iothreads. > > > > The main assumption is that the main loop is the only one allowed to > > perform graph modifications, and so far this has always been held by > > the current code. > > > > The rwlock is inspired from cpus-common.c implementation, and aims > > to reduce cacheline bouncing by having per-aiocontext counter of > > readers. All details and implementation of the lock are in patch 2. > > > > We distinguish between writer (main loop, under BQL) that modifies > > the graph, and readers (all other coroutines running in various > > AioContext), that go through the graph edges, reading ->parents > > and->children. The writer (main loop) has an "exclusive" access, > > so it first waits for current read to finish, and then prevents > > incoming ones from entering while it has the exclusive access. The > > readers (coroutines in multiple AioContext) are free to access the > > graph as long the writer is not modifying the graph. In case it is, > > they go in a CoQueue and sleep until the writer is done. > > > > In this and following series, we try to follow the following locking > > pattern: > > > > - bdrv_co_* functions that call BlockDriver callbacks always expect > > the lock to be taken, therefore they assert. > > > > - blk_co_* functions are called from external code outside the block > > layer, which should not have to care about the block layer's > > internal locking. Usually they also call blk_wait_while_drained(). > > Therefore they take the lock internally. > > > > The long term goal of this series is to eventually replace the > > AioContext lock, so that we can get rid of it once and for all. > > > > Emanuele Giuseppe Esposito (7): > > graph-lock: Implement guard macros > > async: Register/unregister aiocontext in graph lock list > > block: wrlock in bdrv_replace_child_noperm > > block: remove unnecessary assert_bdrv_graph_writable() > > block: assert that graph read and writes are performed correctly > > block-coroutine-wrapper.py: introduce annotations that take the graph > > rdlock > > block: use co_wrapper_mixed_bdrv_rdlock in functions taking the rdlock > > > > Kevin Wolf (10): > > block: Factor out bdrv_drain_all_begin_nopoll() > > Import clang-tsa.h > > clang-tsa: Add TSA_ASSERT() macro > > clang-tsa: Add macros for shared locks > > configure: Enable -Wthread-safety if present > > test-bdrv-drain: Fix incorrrect drain assumptions > > block: Fix locking in external_snapshot_prepare() > > graph-lock: TSA annotations for lock/unlock functions > > Mark assert_bdrv_graph_readable/writable() GRAPH_RD/WRLOCK > > block: GRAPH_RDLOCK for functions only called by co_wrappers > > > > Paolo Bonzini (1): > > graph-lock: Introduce a lock to protect block graph operations > > > Reviewed-by: Emanuele Giuseppe Esposito Thanks, applied to block-next. Kevin
Re: [PATCH 00/18] block: Introduce a block graph rwlock
Am 07.12.2022 um 15:12 hat Emanuele Giuseppe Esposito geschrieben: > > Emanuele Giuseppe Esposito (7): > > graph-lock: Implement guard macros > > async: Register/unregister aiocontext in graph lock list > > block: wrlock in bdrv_replace_child_noperm > > block: remove unnecessary assert_bdrv_graph_writable() > > block: assert that graph read and writes are performed correctly > > block-coroutine-wrapper.py: introduce annotations that take the graph > > rdlock > > block: use co_wrapper_mixed_bdrv_rdlock in functions taking the rdlock > > > > Kevin Wolf (10): > > block: Factor out bdrv_drain_all_begin_nopoll() > > Import clang-tsa.h > > clang-tsa: Add TSA_ASSERT() macro > > clang-tsa: Add macros for shared locks > > configure: Enable -Wthread-safety if present > > test-bdrv-drain: Fix incorrrect drain assumptions > > block: Fix locking in external_snapshot_prepare() > > graph-lock: TSA annotations for lock/unlock functions > > Mark assert_bdrv_graph_readable/writable() GRAPH_RD/WRLOCK > > block: GRAPH_RDLOCK for functions only called by co_wrappers > > > > Paolo Bonzini (1): > > graph-lock: Introduce a lock to protect block graph operations > > > Reviewed-by: Emanuele Giuseppe Esposito Thanks! > ^ I am curious to see if I am allowed to have my r-b also on my patches :) That's actually a good question. I wondered myself whether I should add my R-b to patches that I picked up from you, but which already have my S-o-b now, of course, and are possibly modified by me. I would say you're allowed as long as you actually reviewed them in the version I sent to make sure that I didn't mess them up. :-) And similarly I'll probably add my R-b on patches that contain code from you. Kevin
Re: [PATCH 00/18] block: Introduce a block graph rwlock
Am 07/12/2022 um 14:18 schrieb Kevin Wolf: > This series supersedes the first half of Emanuele's "Protect the block > layer with a rwlock: part 1". It introduces the basic infrastructure for > protecting the block graph (specifically parent/child links) with a > rwlock. Actually taking the reader lock in all necessary places is left > for future series. > > Compared to Emanuele's series, this one adds patches to make use of > clang's Thread Safety Analysis (TSA) feature in order to statically > check at compile time that the places where we assert that we hold the > lock actually do hold it. Once we cover all relevant places, the check > can be extended to verify that all accesses of bs->children and > bs->parents hold the lock. > > For reference, here is the more detailed version of our plan in > Emanuele's words from his series: > > The aim is to replace the current AioContext lock with much > fine-grained locks, aimed to protect only specific data. Currently > the AioContext lock is used pretty much everywhere, and it's not > even clear what it is protecting exactly. > > The aim of the rwlock is to cover graph modifications: more > precisely, when a BlockDriverState parent or child list is modified > or read, since it can be concurrently accessed by the main loop and > iothreads. > > The main assumption is that the main loop is the only one allowed to > perform graph modifications, and so far this has always been held by > the current code. > > The rwlock is inspired from cpus-common.c implementation, and aims > to reduce cacheline bouncing by having per-aiocontext counter of > readers. All details and implementation of the lock are in patch 2. > > We distinguish between writer (main loop, under BQL) that modifies > the graph, and readers (all other coroutines running in various > AioContext), that go through the graph edges, reading ->parents > and->children. The writer (main loop) has an "exclusive" access, > so it first waits for current read to finish, and then prevents > incoming ones from entering while it has the exclusive access. The > readers (coroutines in multiple AioContext) are free to access the > graph as long the writer is not modifying the graph. In case it is, > they go in a CoQueue and sleep until the writer is done. > > In this and following series, we try to follow the following locking > pattern: > > - bdrv_co_* functions that call BlockDriver callbacks always expect > the lock to be taken, therefore they assert. > > - blk_co_* functions are called from external code outside the block > layer, which should not have to care about the block layer's > internal locking. Usually they also call blk_wait_while_drained(). > Therefore they take the lock internally. > > The long term goal of this series is to eventually replace the > AioContext lock, so that we can get rid of it once and for all. > > Emanuele Giuseppe Esposito (7): > graph-lock: Implement guard macros > async: Register/unregister aiocontext in graph lock list > block: wrlock in bdrv_replace_child_noperm > block: remove unnecessary assert_bdrv_graph_writable() > block: assert that graph read and writes are performed correctly > block-coroutine-wrapper.py: introduce annotations that take the graph > rdlock > block: use co_wrapper_mixed_bdrv_rdlock in functions taking the rdlock > > Kevin Wolf (10): > block: Factor out bdrv_drain_all_begin_nopoll() > Import clang-tsa.h > clang-tsa: Add TSA_ASSERT() macro > clang-tsa: Add macros for shared locks > configure: Enable -Wthread-safety if present > test-bdrv-drain: Fix incorrrect drain assumptions > block: Fix locking in external_snapshot_prepare() > graph-lock: TSA annotations for lock/unlock functions > Mark assert_bdrv_graph_readable/writable() GRAPH_RD/WRLOCK > block: GRAPH_RDLOCK for functions only called by co_wrappers > > Paolo Bonzini (1): > graph-lock: Introduce a lock to protect block graph operations > Reviewed-by: Emanuele Giuseppe Esposito ^ I am curious to see if I am allowed to have my r-b also on my patches :) > configure | 1 + > block/coroutines.h | 19 +- > include/block/aio.h| 9 + > include/block/block-common.h | 9 +- > include/block/block-global-state.h | 1 + > include/block/block-io.h | 53 +++-- > include/block/block_int-common.h | 24 +-- > include/block/block_int-global-state.h | 17 -- > include/block/block_int.h | 1 + > include/block/graph-lock.h | 280 + > include/qemu/clang-tsa.h | 114 ++ > block.c| 24 ++- > block/graph-lock.c | 275 > block/io.c | 21 +- > blockdev.c
[PATCH 00/18] block: Introduce a block graph rwlock
This series supersedes the first half of Emanuele's "Protect the block layer with a rwlock: part 1". It introduces the basic infrastructure for protecting the block graph (specifically parent/child links) with a rwlock. Actually taking the reader lock in all necessary places is left for future series. Compared to Emanuele's series, this one adds patches to make use of clang's Thread Safety Analysis (TSA) feature in order to statically check at compile time that the places where we assert that we hold the lock actually do hold it. Once we cover all relevant places, the check can be extended to verify that all accesses of bs->children and bs->parents hold the lock. For reference, here is the more detailed version of our plan in Emanuele's words from his series: The aim is to replace the current AioContext lock with much fine-grained locks, aimed to protect only specific data. Currently the AioContext lock is used pretty much everywhere, and it's not even clear what it is protecting exactly. The aim of the rwlock is to cover graph modifications: more precisely, when a BlockDriverState parent or child list is modified or read, since it can be concurrently accessed by the main loop and iothreads. The main assumption is that the main loop is the only one allowed to perform graph modifications, and so far this has always been held by the current code. The rwlock is inspired from cpus-common.c implementation, and aims to reduce cacheline bouncing by having per-aiocontext counter of readers. All details and implementation of the lock are in patch 2. We distinguish between writer (main loop, under BQL) that modifies the graph, and readers (all other coroutines running in various AioContext), that go through the graph edges, reading ->parents and->children. The writer (main loop) has an "exclusive" access, so it first waits for current read to finish, and then prevents incoming ones from entering while it has the exclusive access. The readers (coroutines in multiple AioContext) are free to access the graph as long the writer is not modifying the graph. In case it is, they go in a CoQueue and sleep until the writer is done. In this and following series, we try to follow the following locking pattern: - bdrv_co_* functions that call BlockDriver callbacks always expect the lock to be taken, therefore they assert. - blk_co_* functions are called from external code outside the block layer, which should not have to care about the block layer's internal locking. Usually they also call blk_wait_while_drained(). Therefore they take the lock internally. The long term goal of this series is to eventually replace the AioContext lock, so that we can get rid of it once and for all. Emanuele Giuseppe Esposito (7): graph-lock: Implement guard macros async: Register/unregister aiocontext in graph lock list block: wrlock in bdrv_replace_child_noperm block: remove unnecessary assert_bdrv_graph_writable() block: assert that graph read and writes are performed correctly block-coroutine-wrapper.py: introduce annotations that take the graph rdlock block: use co_wrapper_mixed_bdrv_rdlock in functions taking the rdlock Kevin Wolf (10): block: Factor out bdrv_drain_all_begin_nopoll() Import clang-tsa.h clang-tsa: Add TSA_ASSERT() macro clang-tsa: Add macros for shared locks configure: Enable -Wthread-safety if present test-bdrv-drain: Fix incorrrect drain assumptions block: Fix locking in external_snapshot_prepare() graph-lock: TSA annotations for lock/unlock functions Mark assert_bdrv_graph_readable/writable() GRAPH_RD/WRLOCK block: GRAPH_RDLOCK for functions only called by co_wrappers Paolo Bonzini (1): graph-lock: Introduce a lock to protect block graph operations configure | 1 + block/coroutines.h | 19 +- include/block/aio.h| 9 + include/block/block-common.h | 9 +- include/block/block-global-state.h | 1 + include/block/block-io.h | 53 +++-- include/block/block_int-common.h | 24 +-- include/block/block_int-global-state.h | 17 -- include/block/block_int.h | 1 + include/block/graph-lock.h | 280 + include/qemu/clang-tsa.h | 114 ++ block.c| 24 ++- block/graph-lock.c | 275 block/io.c | 21 +- blockdev.c | 4 + stubs/graph-lock.c | 10 + tests/unit/test-bdrv-drain.c | 18 ++ util/async.c | 4 + scripts/block-coroutine-wrapper.py | 12 ++ block/meson.build | 1 + stubs/meson.build | 1 + 21 files changed, 820 insertions(+), 78 deletions(-) create
