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 15:40:08 +0100	[thread overview]
Message-ID: <733d73da-f151-4cbd-843c-5c83419cda6c@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;

But bdrv_do_query_node_info() is marked GRAPH_RDLOCK, so the whole 
function must not be called without holding the lock.  I don’t 
understand why we need to explicitly take it another time.

>   - bdrv_do_query_node_info() doesn't need to acquire the AioContext
>     because it doesn't call aio_poll anymore;

In the past (very likely outdated, but still mentioning it) you’d need 
to take the AioContext just in general when operating on a block device 
that might be processed in a different AioContext than the main one, and 
the current code runs in the main context, i.e. which is the situation 
we have here.

Speaking of contexts, I wonder how the threading is actually supposed to 
work.  I assume QMP coroutines run in the main thread, so now we run 
bdrv_co_get_allocated_file_size() in the main thread – is that correct, 
or do we need to use bdrv_co_enter() like qmp_block_resize() does?  And 
so, if we run it in the main thread, is it OK not to acquire the 
AioContext around it to prevent interference from a potential I/O thread?

> 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(-)

This patch implicitly assumes that quite a lot of functions (at least 
bdrv_query_info(), bdrv_query_image_info(), bdrv_do_query_node_info()) 
are now run in coroutine context.  This assumption must be formalized by 
annotating them all with coroutine_fn, and ideally adding a _co_ into 
their name.

Also, those functions should be checked whether they call coroutine 
wrappers, and made to use the native coroutine version now if so. (At 
least I’d find that nicer, FWIW.)  I’ve seen at least bdrv_getlength() 
in bdrv_do_query_node_info(), which could be a bdrv_co_getlength().

> diff --git a/block.c b/block.c
> index abed744b60..f94cee8930 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_co_block_device_info(NULL, bs, flat, errp);
> +        BlockDeviceInfo *info = bdrv_block_device_info(NULL, bs, flat, errp);
>           if (!info) {
>               qapi_free_BlockDeviceInfoList(list);
>               return NULL;
> diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
> index 26116fe831..1049f9b006 100644
> --- a/block/monitor/block-hmp-cmds.c
> +++ b/block/monitor/block-hmp-cmds.c
> @@ -742,7 +742,7 @@ static void print_block_info(Monitor *mon, BlockInfo *info,
>       }
>   }
>   
> -void hmp_info_block(Monitor *mon, const QDict *qdict)
> +void coroutine_fn hmp_info_block(Monitor *mon, const QDict *qdict)
>   {
>       BlockInfoList *block_list, *info;
>       BlockDeviceInfoList *blockdev_list, *blockdev;
> diff --git a/block/qapi.c b/block/qapi.c
> index 20660e15d6..3b4bc0b782 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -41,10 +41,10 @@
>   #include "qemu/qemu-print.h"
>   #include "sysemu/block-backend.h"
>   
> -BlockDeviceInfo *coroutine_fn bdrv_co_block_device_info(BlockBackend *blk,
> -                                                        BlockDriverState *bs,
> -                                                        bool flat,
> -                                                        Error **errp)
> +BlockDeviceInfo *coroutine_fn bdrv_block_device_info(BlockBackend *blk,
> +                                                     BlockDriverState *bs,
> +                                                     bool flat,
> +                                                     Error **errp)
>   {
>       ImageInfo **p_image_info;
>       ImageInfo *backing_info;
> @@ -235,8 +235,6 @@ static void bdrv_do_query_node_info(BlockDriverState *bs,
>       int ret;
>       Error *err = NULL;
>   
> -    aio_context_acquire(bdrv_get_aio_context(bs));
> -
>       size = bdrv_getlength(bs);
>       if (size < 0) {
>           error_setg_errno(errp, -size, "Can't get image size '%s'",
> @@ -249,7 +247,9 @@ static void bdrv_do_query_node_info(BlockDriverState *bs,
>       info->filename        = g_strdup(bs->filename);
>       info->format          = g_strdup(bdrv_get_format_name(bs));
>       info->virtual_size    = size;
> -    info->actual_size     = bdrv_get_allocated_file_size(bs);
> +    bdrv_graph_co_rdlock();
> +    info->actual_size     = bdrv_co_get_allocated_file_size(bs);
> +    bdrv_graph_co_rdunlock();
>       info->has_actual_size = info->actual_size >= 0;
>       if (bs->encrypted) {
>           info->encrypted = true;
> @@ -305,7 +305,7 @@ static void bdrv_do_query_node_info(BlockDriverState *bs,
>       }
>   
>   out:
> -    aio_context_release(bdrv_get_aio_context(bs));
> +    return;
>   }
>   
>   /**
> @@ -668,7 +668,7 @@ bdrv_query_bds_stats(BlockDriverState *bs, bool blk_level)
>       return s;
>   }
>   
> -BlockInfoList *qmp_query_block(Error **errp)
> +BlockInfoList *coroutine_fn qmp_query_block(Error **errp)
>   {
>       BlockInfoList *head = NULL, **p_next = &head;
>       BlockBackend *blk;
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index 47d63d26db..996895f417 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -65,6 +65,7 @@ ERST
>           .help       = "show info of one block device or all block devices "
>                         "(-n: show named nodes; -v: show details)",
>           .cmd        = hmp_info_block,
> +        .coroutine  = true,
>       },
>   
>   SRST
> diff --git a/include/block/block-hmp-cmds.h b/include/block/block-hmp-cmds.h
> index 71113cd7ef..6d9152318f 100644
> --- a/include/block/block-hmp-cmds.h
> +++ b/include/block/block-hmp-cmds.h
> @@ -48,7 +48,7 @@ void hmp_eject(Monitor *mon, const QDict *qdict);
>   
>   void hmp_qemu_io(Monitor *mon, const QDict *qdict);
>   
> -void hmp_info_block(Monitor *mon, const QDict *qdict);
> +void coroutine_fn hmp_info_block(Monitor *mon, const QDict *qdict);
>   void hmp_info_blockstats(Monitor *mon, const QDict *qdict);
>   void hmp_info_block_jobs(Monitor *mon, const QDict *qdict);
>   void hmp_info_snapshots(Monitor *mon, const QDict *qdict);
> diff --git a/include/block/block-io.h b/include/block/block-io.h
> index f31e25cf56..43af816d75 100644
> --- a/include/block/block-io.h
> +++ b/include/block/block-io.h
> @@ -87,7 +87,7 @@ int64_t co_wrapper_mixed_bdrv_rdlock bdrv_getlength(BlockDriverState *bs);
>   int64_t coroutine_fn GRAPH_RDLOCK
>   bdrv_co_get_allocated_file_size(BlockDriverState *bs);
>   
> -int64_t co_wrapper_mixed_bdrv_rdlock
> +int64_t co_wrapper_bdrv_rdlock
>   bdrv_get_allocated_file_size(BlockDriverState *bs);
>   
>   BlockMeasureInfo *bdrv_measure(BlockDriver *drv, QemuOpts *opts,
> diff --git a/include/block/qapi.h b/include/block/qapi.h
> index 5cb0202791..c37cba2a09 100644
> --- a/include/block/qapi.h
> +++ b/include/block/qapi.h
> @@ -30,14 +30,10 @@
>   #include "block/snapshot.h"
>   #include "qapi/qapi-types-block-core.h"
>   
> -BlockDeviceInfo *coroutine_fn bdrv_co_block_device_info(BlockBackend *blk,
> -                                                        BlockDriverState *bs,
> -                                                        bool flat,
> -                                                        Error **errp);
> -BlockDeviceInfo *co_wrapper bdrv_block_device_info(BlockBackend *blk,
> -                                                   BlockDriverState *bs,
> -                                                   bool flat,
> -                                                   Error **errp);
> +BlockDeviceInfo *coroutine_fn bdrv_block_device_info(BlockBackend *blk,
> +                                                     BlockDriverState *bs,
> +                                                     bool flat,
> +                                                     Error **errp);

As noted in general above, please continue to call it 
bdrv_co_block_device_info(), though.  (I think) coroutine_fn functions 
should have a _co_ in them, except when that’s really not possible (i.e. 
when they’re QMP handlers).

Hanna

>   int bdrv_query_snapshot_info_list(BlockDriverState *bs,
>                                     SnapshotInfoList **p_list,
>                                     Error **errp);
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 9d4c92f2c9..a78dc92493 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -838,7 +838,7 @@
>   #    }
>   ##
>   { 'command': 'query-block', 'returns': ['BlockInfo'],
> -  'allow-preconfig': true }
> +  'allow-preconfig': true, 'coroutine': true }
>   
>   ##
>   # @BlockDeviceTimedStats: