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 don't see any callers that actually are in coroutine context, except
test_quiesce_co_drain_all() and test_drv_cb_co_drain_all() in
tests/unit/test-bdrv-drain.c
Adding
GLOBAL_STATE_CODE();
assert(!qemu_in_coroutine());
to the beginning of the function seems to not cause any test failures
except for the two unit tests already mentioned.
Best Regards,
Fiona
