Re: [PATCH 07/20] graph-lock: Fix GRAPH_RDLOCK_GUARD*() to be reader lock

[Kevin Wolf <kwolf@redhat.com>]

Am 25.04.2023 um 22:36 hat Eric Blake geschrieben:
> 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 think much of that comment is actually unrelated to BdrvGraphRWlock
(which tracks lock/unlock operations of a single thread), but to graph
locking in general.

> 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?

TSA_ASSERT_SHARED is not a precondition requirement, but a postcondition
assertion. That is, callers of the function can assume that they hold
the lock after this function returns.

This should really be TSA_ACQUIRE_SHARED for graph_lockable_auto_lock(),
but as the comment above it states, this is impossible because TSA
doesn't understand unlocking via the cleanup attribute.

"shared" and "exclusive" in TSA map to "reader" and "writer" lock of a
RWLock. So since we're only taking a reader lock in this function, we
can only assert that the caller now holds a shared lock (which allows
you to call GRAPH_RDLOCK functions), but not an exclusive one like the
code previously suggested (this would allow you to call GRAPH_WRLOCK
functions).

I'm not sure if this fully addresses your confusion yet. Feel free to
ask if there are more unclear parts.

Kevin