Eric Blake <ebl...@redhat.com> writes:
> On Tue, May 23, 2023 at 06:39:00PM -0300, Fabiano Rosas wrote:
>> From: Lin Ma <l...@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 <l...@suse.com>
>> Signed-off-by: Fabiano Rosas <faro...@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.
