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

[Eric Blake]

On Thu, Apr 27, 2023 at 01:28:15PM +0200, Kevin Wolf wrote:
> 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.

Yeah, I ended up at the same point - writing the graph vs. grabbing a
reader/writer lock as a writer are not the same thing, so the comments
at the start of the file are unrelated to the TSA annotations.

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

clang has really dropped the ball on their cleanup attribute handling;
it's been a known issue for years now, and could make their static
checking so much more powerful if we didn't have to keep working
around it.

> 
> "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.

At this point, I don't have any wording suggestions, and appreciate
your responses confirming what I arrived at on my own, so my R-b still
stands.


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org