Re: [PATCH 00/18] block: Introduce a block graph rwlock

[Kevin Wolf]

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 <eesposit@redhat.com>

Thanks, applied to block-next.

Kevin