| Message ID | 20230609201910.12100-10-farosas@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Make raw_co_get_allocated_file_size asynchronous | expand |
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:
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
Hanna Czenczek <hreitz@redhat.com> writes: > 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. Hi, thanks for the reviews! Yes, there's been a lot of changes since this series was sent. I'll rebase it and re-evaluate. Stefan just sent an AioContext series which will probably help bring the complexity of down for this series. Let's see how it goes.
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); 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:
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(-)