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

[Eric Blake]

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.

> 
> 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
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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org