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

[Hanna Reitz <hreitz@redhat.com>]

On 15.11.21 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.
>
>
> 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…

Hm.  Can we perhaps let block_crypto_amend_options_generic_luks() take 
the BQL just for the permission adjustment (i.e. the 
bdrv_child_refresh_perms() call)?  Would that work? :/

Hanna