[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 07/20] graph-lock: Fix GRAPH_RDLOCK_GUARD*() to be reader loc
From: |
Eric Blake |
Subject: |
Re: [PATCH 07/20] graph-lock: Fix GRAPH_RDLOCK_GUARD*() to be reader lock |
Date: |
Tue, 25 Apr 2023 15:36:03 -0500 |
User-agent: |
NeoMutt/20230407 |
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 <kwolf@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 <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
- Re: [PATCH 01/20] qcow2: Don't call bdrv_getlength() in coroutine_fns, (continued)
[PATCH 03/20] block: bdrv/blk_co_unref() for calls in coroutine context, Kevin Wolf, 2023/04/25
[PATCH 02/20] block: Consistently call bdrv_activate() outside coroutine, Kevin Wolf, 2023/04/25
[PATCH 07/20] graph-lock: Fix GRAPH_RDLOCK_GUARD*() to be reader lock, Kevin Wolf, 2023/04/25
- Re: [PATCH 07/20] graph-lock: Fix GRAPH_RDLOCK_GUARD*() to be reader lock,
Eric Blake <=
[PATCH 05/20] test-bdrv-drain: Don't modify the graph in coroutines, Kevin Wolf, 2023/04/25
[PATCH 04/20] block: Don't call no_coroutine_fns in qmp_block_resize(), Kevin Wolf, 2023/04/25
[PATCH 09/20] nbd: Remove nbd_co_flush() wrapper function, Kevin Wolf, 2023/04/25