Re: [PATCH v4 03/25] assertions for block global state API

[Hanna Reitz <hreitz@redhat.com>]

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