Re: [PATCH v2 09/10] block: Convert qmp_query_block() to coroutine_fn

From: Hanna Czenczek <hreitz@redhat.com>
To: Fabiano Rosas <farosas@suse.de>, qemu-devel@nongnu.org
Cc: qemu-block@nongnu.org, "Kevin Wolf" <kwolf@redhat.com>,
	"Markus Armbruster" <armbru@redhat.com>,
	"João Silva" <jsilva@suse.de>, "Lin Ma" <lma@suse.com>,
	"Claudio Fontana" <cfontana@suse.de>,
	"Dario Faggioli" <dfaggioli@suse.com>,
	"Eric Blake" <eblake@redhat.com>,
	"Dr. David Alan Gilbert" <dave@treblig.org>
Subject: Re: [PATCH v2 09/10] block: Convert qmp_query_block() to coroutine_fn
Date: Mon, 6 Nov 2023 16:02:52 +0100	[thread overview]
Message-ID: <b1f840cc-82c7-4ff5-8ab8-38382e425181@redhat.com> (raw)
In-Reply-To: <20230609201910.12100-10-farosas@suse.de>

On 09.06.23 22:19, Fabiano Rosas wrote:
> This is another caller of bdrv_get_allocated_file_size() that needs to
> be converted to a coroutine because that function will be made
> asynchronous when called (indirectly) from the QMP dispatcher.
>
> This QMP command is a candidate because it calls bdrv_do_query_node_info(),
> which in turn calls bdrv_get_allocated_file_size().
>
> We've determined bdrv_do_query_node_info() to be coroutine-safe (see
> previous commits), so we can just put this QMP command in a coroutine.
>
> Since qmp_query_block() now expects to run in a coroutine, its callers
> need to be converted as well. Convert hmp_info_block(), which calls
> only coroutine-safe code, including qmp_query_named_block_nodes()
> which has been converted to coroutine in the previous patches.
>
> Now that all callers of bdrv_[co_]block_device_info() are using the
> coroutine version, a few things happen:
>
>   - we can return to using bdrv_block_device_info() without a wrapper;
>
>   - bdrv_get_allocated_file_size() can stop being mixed;
>
>   - bdrv_co_get_allocated_file_size() needs to be put under the graph
>     lock because it is being called wthout the wrapper;
>
>   - bdrv_do_query_node_info() doesn't need to acquire the AioContext
>     because it doesn't call aio_poll anymore;
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   block.c                        |  2 +-
>   block/monitor/block-hmp-cmds.c |  2 +-
>   block/qapi.c                   | 18 +++++++++---------
>   hmp-commands-info.hx           |  1 +
>   include/block/block-hmp-cmds.h |  2 +-
>   include/block/block-io.h       |  2 +-
>   include/block/qapi.h           | 12 ++++--------
>   qapi/block-core.json           |  2 +-
>   8 files changed, 19 insertions(+), 22 deletions(-)

After this series has been sent, we got some usages of 
GRAPH_RDLOCK_GUARD_MAINLOOP() that no longer fit with this patch – I’ve 
also mentioned one case on patch 7, not yet realizing that this was a 
new thing.  Those must now be fixed (e.g. in qmp_query_block(), or in 
bdrv_snapshot_list()), or they’ll crash.

Hanna