Re: [PATCH v2 07/10] block: Convert qmp_query_named_block_nodes to coroutine

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>
Subject: Re: [PATCH v2 07/10] block: Convert qmp_query_named_block_nodes to coroutine
Date: Mon, 6 Nov 2023 13:51:04 +0100	[thread overview]
Message-ID: <622d78ae-1732-46e1-9e45-0a4bff8075df@redhat.com> (raw)
In-Reply-To: <20230609201910.12100-8-farosas@suse.de>

On 09.06.23 22:19, Fabiano Rosas wrote:
> From: Lin Ma <lma@suse.com>
>
> We're converting callers of bdrv_get_allocated_file_size() to run in
> coroutines because that function will be made asynchronous when called
> (indirectly) from the QMP dispatcher.
>
> This QMP command is a candidate because it indirectly calls
> bdrv_get_allocated_file_size() through bdrv_block_device_info() ->
> bdrv_query_image_info() -> bdrv_query_image_info().
>
> The previous patches have determined that bdrv_query_image_info() and
> bdrv_do_query_node_info() are coroutine-safe so we can just make the
> QMP command run in a coroutine.
>
> Signed-off-by: Lin Ma <lma@suse.com>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   block.c              | 2 +-
>   blockdev.c           | 6 +++---
>   qapi/block-core.json | 3 ++-
>   3 files changed, 6 insertions(+), 5 deletions(-)

(I see patch 9 does something with HMP code, but) hmp_info_block() calls 
qmp_query_named_block_nodes(), and I don’t think it may call such a 
coroutine_fn directly.

> diff --git a/block.c b/block.c
> index f94cee8930..abed744b60 100644
> --- a/block.c
> +++ b/block.c
> @@ -6148,7 +6148,7 @@ BlockDeviceInfoList *bdrv_named_nodes_list(bool flat,
>   
>       list = NULL;
>       QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) {
> -        BlockDeviceInfo *info = bdrv_block_device_info(NULL, bs, flat, errp);
> +        BlockDeviceInfo *info = bdrv_co_block_device_info(NULL, bs, flat, errp);

As far as I understand, only functions marked as coroutine_fn may call 
other coroutine_fn.  Regardless of whether bdrv_named_nodes_list() is 
only called by another coroutine_fn, we still have to mark it as 
coroutine_fn, too (and probably rename it to bdrv_co_named_nodes_list()).

Also, this function (bdrv_named_nodes_list()) uses 
GRAPH_RDLOCK_GUARD_MAINLOOP().  Is that the correct thing to use in a 
coroutine context?

Hanna

>           if (!info) {
>               qapi_free_BlockDeviceInfoList(list);
>               return NULL;
> diff --git a/blockdev.c b/blockdev.c
> index e6eba61484..8b5f7d06c8 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2818,9 +2818,9 @@ void qmp_drive_backup(DriveBackup *backup, Error **errp)
>       blockdev_do_action(&action, errp);
>   }
>   
> -BlockDeviceInfoList *qmp_query_named_block_nodes(bool has_flat,
> -                                                 bool flat,
> -                                                 Error **errp)
> +BlockDeviceInfoList *coroutine_fn qmp_query_named_block_nodes(bool has_flat,
> +                                                              bool flat,
> +                                                              Error **errp)
>   {
>       bool return_flat = has_flat && flat;
>   
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 5dd5f7e4b0..9d4c92f2c9 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1972,7 +1972,8 @@
>   { 'command': 'query-named-block-nodes',
>     'returns': [ 'BlockDeviceInfo' ],
>     'data': { '*flat': 'bool' },
> -  'allow-preconfig': true }
> +  'allow-preconfig': true,
> +  'coroutine': true}
>   
>   ##
>   # @XDbgBlockGraphNodeType: