Re: [PATCH v2 03/24] block/snapshot: move drain outside of read-locked bdrv_snapshot_delete()

[Kevin Wolf <kwolf@redhat.com>]

Am 26.05.2025 um 12:33 hat Fiona Ebner geschrieben:
> Am 26.05.25 um 11:10 schrieb Kevin Wolf:
> > Am 23.05.2025 um 20:12 hat Andrey Drobyshev geschrieben:
> >> Okay, I've got a very simple and naive question to ask.  We've got this
> >> pattern recurring throughout the series:
> >>
> >>> GLOBAL_STATE_CODE();
> >>> bdrv_drain_all_begin();
> >>> bdrv_graph_rdlock_main_loop();
> >>>
> >>> ...
> >>>
> >>> bdrv_graph_rdunlock_main_loop();
> >>> bdrv_drain_all_end();
> >>
> >> bdrv_graph_rdlock_main_loop() doesn't actually take any locks, it
> >> asserts that we're running in the main thread and not in a coroutine.
> >> bdrv_graph_rdunlock_main_loop() does the same.
> >> GRAPH_RDLOCK_GUARD_MAINLOOP() does both those calls, in the beginning of
> >> a function and when leaving its scope, so essentially it also just does
> >> assert(qemu_in_main_thread() && !qemu_in_coroutine()).
> >>
> >> Therefore:
> >>
> >> 1. Is there any real benefit from using those
> >> {rdlock/rdunlock}_main_loop() constructions, or they're here due to
> >> historical reasons only?
> > 
> > It's the price we pay for the compiler to verify our locking rules.
> > 
> >> 2. Would it hurt if we only leave GRAPH_RDLOCK_GUARD_MAINLOOP() in all
> >> such occurrences?  At least when it's obvious we can't get out of the
> >> main thread.  That would simply deliver us from performing same checks
> >> several times, similar to what's done in commit 22/24 ("block/io: remove
> >> duplicate GLOBAL_STATE_CODE() in bdrv_do_drained_end()").
> > 
> > Once bdrv_drain_all_begin() is marked GRAPH_UNLOCKED, calling it after
> > GRAPH_RDLOCK_GUARD_MAINLOOP() would be wrong according to TSA rules
> > (which don't know anything about this being only a fake lock) and the
> > build would fail.
> 
> Note that I did not mark bdrv_drain_all_begin() as GRAPH_UNLOCKED in the
> series yet. The reason is that I wasn't fully sure if that is okay,
> given that it also can be called from a coroutine and does
> bdrv_co_yield_to_drain() then. But I suppose that doesn't do anything
> with the graph lock, so I'll add the GRAPH_UNLOCKED marker in v3.

I think it's still GRAPH_UNLOCKED even when called from a coroutine,
because otherwise the polling could wait for the calling coroutine to
make progress and release the lock, resulting in a deadlock.

Kevin