Re: [RFC PATCH 3/6] Convert query-block/info_block to coroutine

[Fabiano Rosas]

Eric Blake <eblake@redhat.com> writes:

> On Tue, May 23, 2023 at 06:39:00PM -0300, Fabiano Rosas wrote:
>> From: Lin Ma <lma@suse.com>
>> 
>> Sometimes the query-block performs time-consuming I/O(say waiting for
>> the fstat of NFS complete), So let's make this QMP handler runs in a
>> coroutine.
>
> Grammar suggestions:
>
> Sometimes the query-block command performs time-consuming I/O (say
> waiting for the fstat of NFS to complete), so let's make this QMP
> handler run in a coroutine.
>

Thanks!

>> 
>> The following patch moves the fstat() into a thread pool.
>> 
>> Signed-off-by: Lin Ma <lma@suse.com>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>
>> ---
>> +++ b/qapi/block-core.json
>> @@ -838,7 +838,7 @@
>>  #    }
>>  ##
>>  { 'command': 'query-block', 'returns': ['BlockInfo'],
>> -  'allow-preconfig': true }
>> +  'allow-preconfig': true, 'coroutine': true }
>
> Rereading docs/devel/qapi-code-gen.rst:
>
> | Coroutine safety can be hard to prove, similar to thread safety.  Common
> | pitfalls are:
> | 
> | - The global mutex isn't held across ``qemu_coroutine_yield()``, so
> |   operations that used to assume that they execute atomically may have
> |   to be more careful to protect against changes in the global state.
> | 
> | - Nested event loops (``AIO_WAIT_WHILE()`` etc.) are problematic in
> |   coroutine context and can easily lead to deadlocks.  They should be
> |   replaced by yielding and reentering the coroutine when the condition
> |   becomes false.
> | 
> | Since the command handler may assume coroutine context, any callers
> | other than the QMP dispatcher must also call it in coroutine context.
> | In particular, HMP commands calling such a QMP command handler must be
> | marked ``.coroutine = true`` in hmp-commands.hx.
>
> I agree that you also need to change the HMP handler, but the commit
> message didn't mention that other than in the subject line.  The

Ok, I'll update the message for v2.

> commit message could also do a better job at explaining how you have
> audited that merely adding the coroutine marker is safe (ie. that all
> of the calls made by query_block are already coroutine safe).  While
> the intent behind this patch is on the right track, I'm not sure if I
> have enough information to say whether it is safe, or if there are
> other lurking problems we will need to fix first.

Fair point, I've been trusting the tests too much. A closer code audit
is necessary indeed.