Re: [PATCH v4 20/25] block_int-common.h: assertion in the callers of BlockDriver function pointers

[Emanuele Giuseppe Esposito <eesposit@redhat.com>]

On 17/11/2021 13:51, Hanna Reitz wrote:
> On 17.11.21 12:33, Emanuele Giuseppe Esposito wrote:
>>
>>
>> On 15/11/2021 13:48, Hanna Reitz wrote:
>>> On 25.10.21 12:17, Emanuele Giuseppe Esposito wrote:
>>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>>> ---
>>>>   block.c | 17 +++++++++++++++++
>>>>   1 file changed, 17 insertions(+)
>>>>
>>>> diff --git a/block.c b/block.c
>>>> index 94bff5c757..40c4729b8d 100644
>>>> --- a/block.c
>>>> +++ b/block.c
>>>
>>> [...]
>>>
>>>> @@ -2148,6 +2152,7 @@ static void bdrv_child_perm(BlockDriverState 
>>>> *bs, BlockDriverState *child_bs,
>>>>                               uint64_t *nperm, uint64_t *nshared)
>>>>   {
>>>>       assert(bs->drv && bs->drv->bdrv_child_perm);
>>>> +    assert(qemu_in_main_thread());
>>>>       bs->drv->bdrv_child_perm(bs, c, role, reopen_queue,
>>>>                                parent_perm, parent_shared,
>>>>                                nperm, nshared);
>>>
>>> (Should’ve noticed earlier, but only did now...)
>>>
>>> First, this function is indirectly called by bdrv_refresh_perms(). I 
>>> understand that all perm-related functions are classified as GS.
>>>
>>> However, bdrv_co_invalidate_cache() invokes bdrv_refresh_perms. Being 
>>> declared in block/coroutine.h, it’s an I/O function, so it mustn’t 
>>> call such a GS function. BlockDriver.bdrv_co_invalidate_cache(), 
>>> bdrv_invalidate_cache(), and blk_invalidate_cache() are also 
>>> classified as I/O functions. Perhaps all of these functions should be 
>>> classified as GS functions?  I believe their callers and their 
>>> purpose would allow for this.
>>
>> I think that the *_invalidate_cache functions are I/O.
>> First of all, test-block-iothread.c calls bdrv_invalidate_cache in 
>> test_sync_op_invalidate_cache, which is purposefully called in an 
>> iothread. So that hints that we want it as I/O.
> 
> Hm, OK, but bdrv_co_invalidate_cache() calls bdrv_refresh_perms(), which 
> is a GS function, so that shouldn’t work, right?

Ok let's take a step back for one moment: can you tell me why the perm 
functions should be GS?

On one side I see they are also used by I/O, as we can see above. On the 
other side, I kinda see that permission should only be modified under 
BQL. But I don't have any valid point to sustain that.
So I wonder if you have any specific and more valid reason to put them 
as GS.

Maybe clarifying this will help finding a clean solution to this problem.

> 
>> (Small mistake I just noticed: blk_invalidate_cache has the BQL 
>> assertion even though it is rightly put in block-backend-io.h
>>
>>>
>>> Second, it’s called by bdrv_child_refresh_perms(), which is called by 
>>> block_crypto_amend_options_generic_luks().  This function is called 
>>> by block_crypto_co_amend_luks(), which is a BlockDriver.bdrv_co_amend 
>>> implementation, which is classified as an I/O function.
>>>
>>> Honestly, I don’t know how to fix that mess.  The best would be if we 
>>> could make the perm functions thread-safe and classify them as I/O, 
>>> but it seems to me like that’s impossible (I sure hope I’m wrong). On 
>>> the other hand, .bdrv_co_amend very much strikes me like a GS 
>>> function, but it isn’t.  I’m afraid it must work on nodes that are 
>>> not in the main context, and it launches a job, so AFAIU we 
>>> absolutely cannot run it under the BQL.
>>>
>>> It almost seems to me like we’d need a thread-safe variant of the 
>>> perm functions that’s allowed to fail when it cannot guarantee thread 
>>> safety or something.  Or perhaps I’m wrong and the perm functions can 
>>> actually be classified as thread-safe and I/O, that’d be great…
>>
>> I think that since we are currently only splitting and not taking care 
>> of the actual I/O thread safety, we can move the _perm functions in 
>> I/O, and add a nice TODO to double check their thread safety.
> 
> :/
> 
> I would really, really like to avoid that unless it’s clear that we can 
> make them thread-safe, or that there’s a way to take the BQL in I/O 
> functions to call GS functions.  But the latter still wouldn’t make the 
> perm functions I/O functions.  At most, I’d sort them under common 
> functions.
> 
>> I mean, if they are not thread-safe after the split it means they are 
>> not thread safe also right now.
> 
> Yes, sorry I wasn’t clear, I think there’s a pre-existing problem that 
> your series only unveils.  I don’t know whether it has implications in 
> practice yet.
> 
> Hanna
>