On 15.11.21 13:27, Emanuele Giuseppe Esposito wrote:
@@ -586,12 +589,14 @@ void bdrv_unapply_subtree_drain(BdrvChild
*child, BlockDriverState *old_parent)
void coroutine_fn bdrv_co_drain(BlockDriverState *bs)
{
assert(qemu_in_coroutine());
+ assert(qemu_in_main_thread());
bdrv_drained_begin(bs);
bdrv_drained_end(bs);
}
void bdrv_drain(BlockDriverState *bs)
{
+ assert(qemu_in_main_thread());
bdrv_drained_begin(bs);
bdrv_drained_end(bs);
}
Why are these GS functions when both bdrv_drained_begin() and
bdrv_drained_end() are I/O functions?
I can understand making the drain_all functions GS functions, but it
seems weird to say it’s an I/O function when a single BDS is drained
via bdrv_drained_begin() and bdrv_drained_end(), but not via
bdrv_drain(), which just does both.
(I can see that there are no I/O path callers, but I still find it
strange.)
The problem as you saw is on the path callers: bdrv_drain seems to be
called only in main thread, while others or similar drains are also
coming from I/O. I would say maybe it's a better idea to just put
everything I/O, maybe (probably) there are cases not caught by the tests.
The only ones in global-state will be:
- bdrv_apply_subtree_drain and bdrv_unapply_subtree_drain (called only
in .attach and .detach callbacks of BdrvChildClass, run under BQL).
- bdrv_drain_all_end_quiesce (called only by bdrv_close thus under BQL).
- bdrv_drain_all_begin, bdrv_drain_all_end and bdrv_drain_all (have
already BQL assertions)
In general, this is the underlying problem with these assertions: if a
test doesn't test a specific code path, an unneeded assertion might
pass undetected.
I believe the I/O vs. GS classification should not only be done based on
functional concerns, i.e. who calls this function (is it called by an
I/O function?) and whether it can be thread-safe or not (does it call a
BQL function?), but also needs to be done semantically. I believe there
are some functions that we consider thread-safe but are also only called
by BQL functions, for example apparently bdrv_drain(),
bdrv_apply_subtree_drain(), and bdrv_unapply_subtree_drain(). Such
functions can functionally be considered both GS and I/O functions, so
the decision should be done semantically. I believe that it makes sense
to say all drain-related functions for a single subtree are I/O functions.
OTOH, functions operating on multiple trees in the block graph (i.e. all
functions that have some “_all_” in their name) should naturally be
considered GS functions, regardless of whether their implementation is
thread-safe or not, because they are by nature global functions.
That’s why I’m wondering why some drain functions are I/O and others are
GS; or why this block-status function is I/O and this one is GS. It may
make sense functionally, but semantically it’s strange.
I understand it may be difficult for you to know which functions relate
to each other and thus know these semantic relationships, but in most of
the series the split seems very reasonable to me, semantically. If it
didn’t, I said so. :)
Hanna