On Tue, Apr 25, 2023 at 07:31:45PM +0200, Kevin Wolf wrote: > GRAPH_RDLOCK_GUARD() and GRAPH_RDLOCK_GUARD_MAINLOOP() only take a > reader lock for the graph, so the correct annotation for them to use is > TSA_ASSERT_SHARED rather than TSA_ASSERT.
The comments at the start of graph-lock.h state that there is only 1 writer (main loop, under BQL), and all others are readers (coroutines in varous AioContext) - but that's regarding the main BdrvGraphRWlock. I guess my confusion is over the act of writing the graph (only in the main loop) and using TSA annotations to check for safety. Am I correct that the reason graph_lockable_auto_lock() only needs a TSA_ASSERT_SHARED locking is that it is only reachable from the other threads (and not the main loop thread itself) to check that we are indeed in a point where we aren't contending with the main loop's writable lock? > > Signed-off-by: Kevin Wolf <kw...@redhat.com> > --- > include/block/graph-lock.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/block/graph-lock.h b/include/block/graph-lock.h > index 7ef391fab3..adaa3ed089 100644 > --- a/include/block/graph-lock.h > +++ b/include/block/graph-lock.h > @@ -210,7 +210,7 @@ typedef struct GraphLockable { } GraphLockable; > * unlocked. TSA_ASSERT() makes sure that the following calls know that we > * hold the lock while unlocking is left unchecked. > */ > -static inline GraphLockable * TSA_ASSERT(graph_lock) TSA_NO_TSA > +static inline GraphLockable * TSA_ASSERT_SHARED(graph_lock) TSA_NO_TSA > graph_lockable_auto_lock(GraphLockable *x) > { > bdrv_graph_co_rdlock(); But the act of grabbing a rdlock is definitely a SHARED not EXCLUSIVE action, so I think I agree with your changing of the annotation, even if the commit message could be slightly improved. Assuming I've sorted out my own confusion on the various lock terminoligies, Reviewed-by: Eric Blake <ebl...@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org