* [PATCH v3 00/11] block: Convert qmp_query_block into a coroutine
@ 2024-04-09 14:59 Fabiano Rosas
2024-04-09 14:59 ` [PATCH v3 01/11] block: Allow the wrapper script to see functions declared in qapi.h Fabiano Rosas
` (10 more replies)
0 siblings, 11 replies; 12+ messages in thread
From: Fabiano Rosas @ 2024-04-09 14:59 UTC (permalink / raw
To: qemu-devel
Cc: qemu-block, Kevin Wolf, Hanna Reitz, Markus Armbruster,
João Silva, Lin Ma, Claudio Fontana, Dario Faggioli,
Eric Blake, Stefan Hajnoczi, Paolo Bonzini
Hi, it's been a while since the last version, so a recap:
This series converts qmp_query_block() & qmp_query_named_block_nodes()
to coroutines so we can yield from them all the way back into the main
loop. This addresses a vcpu softlockup encountered when querying a
disk placed on NFS.
If the NFS server happens to have high latency, an fstat() issued from
raw_co_get_allocated_file_size() could take seconds while the whole
QMP command is holding the BQL and blocks a vcpu thread going out of
the guest to handle IO.
This scenario is clearly undesireable since a query command is of much
lower priority than the vcpu thread doing actual work.
Move the 'fstat' call into the thread-pool and make the necessary
adaptations to ensure the whole QMP command that calls
raw_co_get_allocated_file_size() runs in a coroutine.
Changes since v2:
- Do the changes more gradually to make it easier to reason about the
safety of the change.
- Patch 4 addresses the issue I asked about recently on the ml [1]
about how to avoid dispatching the QMP command during an aio_poll().
- Converted qmp_query_block and qmp_query_named_block_nodes in a
single patch to avoid having hmp_info_block call a coroutine_fn out
of coroutine context.
On v2, Hanna asked:
"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?"
The QMP coroutines and also bdrv_co_get_allocated_file_size() run in
the main thread. This series doesn't change that. The difference is
that instead of bdrv_co_get_allocated_file_size() yielding back to
bdrv_poll(), it now yields back to the main loop.
As for thread safety, that's basically what I asked about in [1], so
I'm still gathering information and don't have a definite answer for
it. Since we don't have the AioContext lock anymore, it seems that
safety is now dependant on not dispatching the QMP command while other
operations are ongoing.
Still, for this particular case of fstat(), I don't think interference
of an I/O thread could cause any problems, as long as the file
descriptor is not closed prematurely. The fstat() manual already
mentions that it is succeptible to return old information in some
cases.
CI run: https://gitlab.com/farosas/qemu/-/pipelines/1244905208
1- Advice on block QMP command coroutines
https://lore.kernel.org/r/87bk6trl9i.fsf@suse.de
Initial discussion:
https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg03141.html
v1:
https://lore.kernel.org/r/20230523213903.18418-1-farosas@suse.de
v2:
https://lore.kernel.org/r/20230609201910.12100-1-farosas@suse.de
Fabiano Rosas (9):
block: Allow the wrapper script to see functions declared in qapi.h
block: Temporarily mark bdrv_co_get_allocated_file_size as mixed
block: Take the graph lock in bdrv_snapshot_list
block: Reschedule query-block during qcow2 invalidation
block: Run bdrv_do_query_node_info in a coroutine
block: Convert bdrv_query_block_graph_info to coroutine
block: Convert bdrv_query_image_info to coroutine
block: Convert bdrv_block_device_info into co_wrapper
block: Don't query all block devices at hmp_nbd_server_start
João Silva (1):
block: Add a thread-pool version of fstat
Lin Ma (1):
block: Convert qmp_query_block and qmp_query_named_block_nodes to
coroutine
block.c | 9 +++--
block/file-posix.c | 40 +++++++++++++++++--
block/meson.build | 1 +
block/mirror.c | 1 +
block/monitor/block-hmp-cmds.c | 34 +++++++++++-----
block/qapi.c | 63 +++++++++++++++---------------
block/qcow2.c | 20 ++++++++++
block/replication.c | 1 +
block/snapshot.c | 2 +-
blockdev.c | 8 ++--
blockjob.c | 1 +
hmp-commands-info.hx | 1 +
include/block/block-common.h | 1 +
include/block/block-global-state.h | 3 +-
include/block/block-hmp-cmds.h | 2 +-
include/block/qapi.h | 24 ++++++++----
include/block/raw-aio.h | 4 +-
migration/block.c | 1 +
qapi/block-core.json | 5 ++-
qemu-img.c | 3 --
scripts/block-coroutine-wrapper.py | 1 +
21 files changed, 157 insertions(+), 68 deletions(-)
--
2.35.3
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 01/11] block: Allow the wrapper script to see functions declared in qapi.h
2024-04-09 14:59 [PATCH v3 00/11] block: Convert qmp_query_block into a coroutine Fabiano Rosas
@ 2024-04-09 14:59 ` Fabiano Rosas
2024-04-09 14:59 ` [PATCH v3 02/11] block: Temporarily mark bdrv_co_get_allocated_file_size as mixed Fabiano Rosas
` (9 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Fabiano Rosas @ 2024-04-09 14:59 UTC (permalink / raw
To: qemu-devel
Cc: qemu-block, Kevin Wolf, Hanna Reitz, Markus Armbruster,
João Silva, Lin Ma, Claudio Fontana, Dario Faggioli,
Eric Blake, Stefan Hajnoczi, Paolo Bonzini, John Snow,
Cleber Rosa
The following patches will add co_wrapper annotations to functions
declared in qapi.h. Add that header to the set of files used by
block-coroutine-wrapper.py.
Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
block/meson.build | 1 +
scripts/block-coroutine-wrapper.py | 1 +
2 files changed, 2 insertions(+)
diff --git a/block/meson.build b/block/meson.build
index e1f03fd773..8fe684d301 100644
--- a/block/meson.build
+++ b/block/meson.build
@@ -154,6 +154,7 @@ block_gen_c = custom_target('block-gen.c',
'../include/block/dirty-bitmap.h',
'../include/block/block_int-io.h',
'../include/block/block-global-state.h',
+ '../include/block/qapi.h',
'../include/sysemu/block-backend-global-state.h',
'../include/sysemu/block-backend-io.h',
'coroutines.h'
diff --git a/scripts/block-coroutine-wrapper.py b/scripts/block-coroutine-wrapper.py
index dbbde99e39..067b203801 100644
--- a/scripts/block-coroutine-wrapper.py
+++ b/scripts/block-coroutine-wrapper.py
@@ -44,6 +44,7 @@ def gen_header():
#include "block/block-gen.h"
#include "block/block_int.h"
#include "block/dirty-bitmap.h"
+#include "block/qapi.h"
"""
--
2.35.3
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 02/11] block: Temporarily mark bdrv_co_get_allocated_file_size as mixed
2024-04-09 14:59 [PATCH v3 00/11] block: Convert qmp_query_block into a coroutine Fabiano Rosas
2024-04-09 14:59 ` [PATCH v3 01/11] block: Allow the wrapper script to see functions declared in qapi.h Fabiano Rosas
@ 2024-04-09 14:59 ` Fabiano Rosas
2024-04-09 14:59 ` [PATCH v3 03/11] block: Take the graph lock in bdrv_snapshot_list Fabiano Rosas
` (8 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Fabiano Rosas @ 2024-04-09 14:59 UTC (permalink / raw
To: qemu-devel
Cc: qemu-block, Kevin Wolf, Hanna Reitz, Markus Armbruster,
João Silva, Lin Ma, Claudio Fontana, Dario Faggioli,
Eric Blake, Stefan Hajnoczi, Paolo Bonzini
Some callers of this function are about to be converted to run in
coroutines, so allow it to be executed both inside and outside a
coroutine while we convert all the callers.
This will be reverted once all callers of bdrv_do_query_node_info run
in a coroutine.
Signed-off-by: Fabiano Rosas <farosas@suse.de>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
---
include/block/block-io.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/block/block-io.h b/include/block/block-io.h
index b49e0537dd..349d7760a1 100644
--- a/include/block/block-io.h
+++ b/include/block/block-io.h
@@ -86,7 +86,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_bdrv_rdlock
+int64_t co_wrapper_mixed_bdrv_rdlock
bdrv_get_allocated_file_size(BlockDriverState *bs);
BlockMeasureInfo *bdrv_measure(BlockDriver *drv, QemuOpts *opts,
--
2.35.3
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 03/11] block: Take the graph lock in bdrv_snapshot_list
2024-04-09 14:59 [PATCH v3 00/11] block: Convert qmp_query_block into a coroutine Fabiano Rosas
2024-04-09 14:59 ` [PATCH v3 01/11] block: Allow the wrapper script to see functions declared in qapi.h Fabiano Rosas
2024-04-09 14:59 ` [PATCH v3 02/11] block: Temporarily mark bdrv_co_get_allocated_file_size as mixed Fabiano Rosas
@ 2024-04-09 14:59 ` Fabiano Rosas
2024-04-09 14:59 ` [PATCH v3 04/11] block: Reschedule query-block during qcow2 invalidation Fabiano Rosas
` (7 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Fabiano Rosas @ 2024-04-09 14:59 UTC (permalink / raw
To: qemu-devel
Cc: qemu-block, Kevin Wolf, Hanna Reitz, Markus Armbruster,
João Silva, Lin Ma, Claudio Fontana, Dario Faggioli,
Eric Blake, Stefan Hajnoczi, Paolo Bonzini
This function has up until now always ran in the main loop, outside of
a coroutine. We're about to make it run inside a coroutine so start
actually taking the graph lock.
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
block/snapshot.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/snapshot.c b/block/snapshot.c
index 8242b4abac..3db9536ca2 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -389,7 +389,7 @@ int bdrv_snapshot_list(BlockDriverState *bs,
QEMUSnapshotInfo **psn_info)
{
GLOBAL_STATE_CODE();
- GRAPH_RDLOCK_GUARD_MAINLOOP();
+ GRAPH_RDLOCK_GUARD();
BlockDriver *drv = bs->drv;
BlockDriverState *fallback_bs = bdrv_snapshot_fallback(bs);
--
2.35.3
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 04/11] block: Reschedule query-block during qcow2 invalidation
2024-04-09 14:59 [PATCH v3 00/11] block: Convert qmp_query_block into a coroutine Fabiano Rosas
` (2 preceding siblings ...)
2024-04-09 14:59 ` [PATCH v3 03/11] block: Take the graph lock in bdrv_snapshot_list Fabiano Rosas
@ 2024-04-09 14:59 ` Fabiano Rosas
2024-04-09 14:59 ` [PATCH v3 05/11] block: Run bdrv_do_query_node_info in a coroutine Fabiano Rosas
` (6 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Fabiano Rosas @ 2024-04-09 14:59 UTC (permalink / raw
To: qemu-devel
Cc: qemu-block, Kevin Wolf, Hanna Reitz, Markus Armbruster,
João Silva, Lin Ma, Claudio Fontana, Dario Faggioli,
Eric Blake, Stefan Hajnoczi, Paolo Bonzini, John Snow,
Vladimir Sementsov-Ogievskiy, Wen Congyang, Xie Changlong,
Fam Zheng, Peter Xu
There is a small window at the end of block device migration when
devices are being re-activated. This includes a resetting of some
fields of BDRVQcow2State at qcow2_co_invalidate_cache(). A concurrent
QMP query-block command can call qcow2_get_specific_info() during this
window and see the cleared values, which leads to an assert:
qcow2_get_specific_info: Assertion `false' failed
This is the same issue as Gitlab #1933, which has already been
resolved[1], but there the fix applied only to non-coroutine
commands. Once we move query-block to a coroutine the problem will
manifest again.
Add an operation blocker to the invalidation function to block the
query info path during this window.
Instead of failing query-block, which would be disruptive to users,
use the blocker to know when to reschedule the coroutine back into the
iohandler so it doesn't run while the BDRVQcow2State is inconsistent.
To avoid failing query-block when all block operations are blocked,
unblock the INFO operation at various places. This preserves the prior
situations where query-block used to work.
1 - https://gitlab.com/qemu-project/qemu/-/issues/1933
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
block.c | 1 +
block/mirror.c | 1 +
block/qcow2.c | 20 ++++++++++++++++++++
block/replication.c | 1 +
blockjob.c | 1 +
include/block/block-common.h | 1 +
migration/block.c | 1 +
7 files changed, 26 insertions(+)
diff --git a/block.c b/block.c
index 468cf5e67d..01478c5471 100644
--- a/block.c
+++ b/block.c
@@ -1296,6 +1296,7 @@ static void GRAPH_WRLOCK bdrv_backing_attach(BdrvChild *c)
parent->backing_blocker);
bdrv_op_unblock(backing_hd, BLOCK_OP_TYPE_BACKUP_TARGET,
parent->backing_blocker);
+ bdrv_op_unblock(backing_hd, BLOCK_OP_TYPE_INFO, parent->backing_blocker);
}
static void bdrv_backing_detach(BdrvChild *c)
diff --git a/block/mirror.c b/block/mirror.c
index 1bdce3b657..9f952f3fdd 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1191,6 +1191,7 @@ static void mirror_complete(Job *job, Error **errp)
error_setg(&s->replace_blocker,
"block device is in use by block-job-complete");
bdrv_op_block_all(s->to_replace, s->replace_blocker);
+ bdrv_op_unblock(s->to_replace, BLOCK_OP_TYPE_INFO, s->replace_blocker);
bdrv_ref(s->to_replace);
}
diff --git a/block/qcow2.c b/block/qcow2.c
index 956128b409..b0ec8009e3 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2834,6 +2834,7 @@ qcow2_co_invalidate_cache(BlockDriverState *bs, Error **errp)
BdrvChild *data_file;
int flags = s->flags;
QCryptoBlock *crypto = NULL;
+ Error *blocker = NULL;
QDict *options;
int ret;
@@ -2845,6 +2846,17 @@ qcow2_co_invalidate_cache(BlockDriverState *bs, Error **errp)
crypto = s->crypto;
s->crypto = NULL;
+ /*
+ * When qcow2_do_open() below reads the qcow header, it yields to
+ * wait for the I/O which allows a concurrent QMP query-block
+ * command to be dispatched on the same context before
+ * BDRVQcow2State has been completely repopulated. Block the
+ * query-info operation during this window to avoid having
+ * qcow2_get_specific_info() access bogus values.
+ */
+ error_setg(&blocker, "invalidating cached metadata");
+ bdrv_op_block(bs, BLOCK_OP_TYPE_INFO, blocker);
+
/*
* Do not reopen s->data_file (i.e., have qcow2_do_close() not close it,
* and then prevent qcow2_do_open() from opening it), because this function
@@ -2864,6 +2876,8 @@ qcow2_co_invalidate_cache(BlockDriverState *bs, Error **errp)
qemu_co_mutex_lock(&s->lock);
ret = qcow2_do_open(bs, options, flags, false, errp);
qemu_co_mutex_unlock(&s->lock);
+ bdrv_op_unblock(bs, BLOCK_OP_TYPE_INFO, blocker);
+ g_free(blocker);
qobject_unref(options);
if (ret < 0) {
error_prepend(errp, "Could not reopen qcow2 layer: ");
@@ -5240,6 +5254,12 @@ qcow2_get_specific_info(BlockDriverState *bs, Error **errp)
ImageInfoSpecific *spec_info;
QCryptoBlockInfo *encrypt_info = NULL;
+ if (qemu_in_coroutine() &&
+ bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_INFO, errp)) {
+ errp = NULL;
+ aio_co_reschedule_self(iohandler_get_aio_context());
+ }
+
if (s->crypto != NULL) {
encrypt_info = qcrypto_block_get_info(s->crypto, errp);
if (!encrypt_info) {
diff --git a/block/replication.c b/block/replication.c
index ca6bd0a720..459d644673 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -577,6 +577,7 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
}
bdrv_op_block_all(top_bs, s->blocker);
bdrv_op_unblock(top_bs, BLOCK_OP_TYPE_DATAPLANE, s->blocker);
+ bdrv_op_unblock(top_bs, BLOCK_OP_TYPE_INFO, s->blocker);
bdrv_graph_wrunlock();
diff --git a/blockjob.c b/blockjob.c
index d5f29e14af..f0df345982 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -244,6 +244,7 @@ int block_job_add_bdrv(BlockJob *job, const char *name, BlockDriverState *bs,
job->nodes = g_slist_prepend(job->nodes, c);
bdrv_op_block_all(bs, job->blocker);
+ bdrv_op_unblock(bs, BLOCK_OP_TYPE_INFO, job->blocker);
return 0;
}
diff --git a/include/block/block-common.h b/include/block/block-common.h
index a846023a09..4458617179 100644
--- a/include/block/block-common.h
+++ b/include/block/block-common.h
@@ -364,6 +364,7 @@ typedef enum BlockOpType {
BLOCK_OP_TYPE_RESIZE,
BLOCK_OP_TYPE_STREAM,
BLOCK_OP_TYPE_REPLACE,
+ BLOCK_OP_TYPE_INFO,
BLOCK_OP_TYPE_MAX,
} BlockOpType;
diff --git a/migration/block.c b/migration/block.c
index 2b9054889a..3f3bc2748f 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -451,6 +451,7 @@ static int init_blk_migration(QEMUFile *f)
alloc_aio_bitmap(bmds);
error_setg(&bmds->blocker, "block device is in use by migration");
bdrv_op_block_all(bs, bmds->blocker);
+ bdrv_op_unblock(bs, BLOCK_OP_TYPE_INFO, bmds->blocker);
}
}
--
2.35.3
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 05/11] block: Run bdrv_do_query_node_info in a coroutine
2024-04-09 14:59 [PATCH v3 00/11] block: Convert qmp_query_block into a coroutine Fabiano Rosas
` (3 preceding siblings ...)
2024-04-09 14:59 ` [PATCH v3 04/11] block: Reschedule query-block during qcow2 invalidation Fabiano Rosas
@ 2024-04-09 14:59 ` Fabiano Rosas
2024-04-09 14:59 ` [PATCH v3 06/11] block: Convert bdrv_query_block_graph_info to coroutine Fabiano Rosas
` (5 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Fabiano Rosas @ 2024-04-09 14:59 UTC (permalink / raw
To: qemu-devel
Cc: qemu-block, Kevin Wolf, Hanna Reitz, Markus Armbruster,
João Silva, Lin Ma, Claudio Fontana, Dario Faggioli,
Eric Blake, Stefan Hajnoczi, Paolo Bonzini
Move this function into a coroutine so we can convert the whole
qmp_query_block command into a coroutine in the next patches.
Placing the entire command in a coroutine allow us to yield all the
way back to the main loop, releasing the BQL and unblocking the main
loop.
When the whole conversion is completed, we'll be able to avoid a
priority inversion that happens when a QMP command calls a slow
(buggy) system call and blocks the vcpu thread from doing mmio due to
contention on the BQL.
About coroutine safety:
Most callees have coroutine versions themselves and thus are safe to
call in a coroutine. The remaining ones:
- bdrv_refresh_filename, bdrv_get_full_backing_filename: String
manipulation, nothing that would be unsafe for use in coroutines;
- bdrv_get_format_name: Just accesses a field;
- bdrv_get_specific_info, bdrv_query_snapshot_info_list: No locks or
anything that would poll or block.
(using a mixed wrapper for now, but after all callers are converted,
this can become a coroutine exclusively)
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
- used the coroutine version of the called functions when available
---
block/qapi.c | 11 ++++++-----
include/block/qapi.h | 8 ++++++++
2 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/block/qapi.c b/block/qapi.c
index 2b5793f1d9..26a9510345 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -225,8 +225,9 @@ int bdrv_query_snapshot_info_list(BlockDriverState *bs,
* Helper function for other query info functions. Store information about @bs
* in @info, setting @errp on error.
*/
-static void GRAPH_RDLOCK
-bdrv_do_query_node_info(BlockDriverState *bs, BlockNodeInfo *info, Error **errp)
+void coroutine_fn
+bdrv_co_do_query_node_info(BlockDriverState *bs, BlockNodeInfo *info,
+ Error **errp)
{
int64_t size;
const char *backing_filename;
@@ -234,7 +235,7 @@ bdrv_do_query_node_info(BlockDriverState *bs, BlockNodeInfo *info, Error **errp)
int ret;
Error *err = NULL;
- size = bdrv_getlength(bs);
+ size = bdrv_co_getlength(bs);
if (size < 0) {
error_setg_errno(errp, -size, "Can't get image size '%s'",
bs->exact_filename);
@@ -246,13 +247,13 @@ bdrv_do_query_node_info(BlockDriverState *bs, BlockNodeInfo *info, Error **errp)
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);
+ info->actual_size = bdrv_co_get_allocated_file_size(bs);
info->has_actual_size = info->actual_size >= 0;
if (bs->encrypted) {
info->encrypted = true;
info->has_encrypted = true;
}
- if (bdrv_get_info(bs, &bdi) >= 0) {
+ if (bdrv_co_get_info(bs, &bdi) >= 0) {
if (bdi.cluster_size != 0) {
info->cluster_size = bdi.cluster_size;
info->has_cluster_size = true;
diff --git a/include/block/qapi.h b/include/block/qapi.h
index 54c48de26a..234730b3de 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -25,6 +25,7 @@
#ifndef BLOCK_QAPI_H
#define BLOCK_QAPI_H
+#include "block/block-common.h"
#include "block/graph-lock.h"
#include "block/snapshot.h"
#include "qapi/qapi-types-block-core.h"
@@ -49,4 +50,11 @@ void bdrv_image_info_specific_dump(ImageInfoSpecific *info_spec,
const char *prefix,
int indentation);
void bdrv_node_info_dump(BlockNodeInfo *info, int indentation, bool protocol);
+
+void coroutine_fn GRAPH_RDLOCK
+bdrv_co_do_query_node_info(BlockDriverState *bs, BlockNodeInfo *info,
+ Error **errp);
+void co_wrapper_bdrv_rdlock
+bdrv_do_query_node_info(BlockDriverState *bs, BlockNodeInfo *info,
+ Error **errp);
#endif
--
2.35.3
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 06/11] block: Convert bdrv_query_block_graph_info to coroutine
2024-04-09 14:59 [PATCH v3 00/11] block: Convert qmp_query_block into a coroutine Fabiano Rosas
` (4 preceding siblings ...)
2024-04-09 14:59 ` [PATCH v3 05/11] block: Run bdrv_do_query_node_info in a coroutine Fabiano Rosas
@ 2024-04-09 14:59 ` Fabiano Rosas
2024-04-09 14:59 ` [PATCH v3 07/11] block: Convert bdrv_query_image_info " Fabiano Rosas
` (4 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Fabiano Rosas @ 2024-04-09 14:59 UTC (permalink / raw
To: qemu-devel
Cc: qemu-block, Kevin Wolf, Hanna Reitz, Markus Armbruster,
João Silva, Lin Ma, Claudio Fontana, Dario Faggioli,
Eric Blake, Stefan Hajnoczi, Paolo Bonzini
We're converting callers of bdrv_co_get_allocated_file_size() to run
in coroutines because that function will be made asynchronous when
called (indirectly) from the QMP dispatcher.
This function is a candidate because it calls bdrv_do_query_node_info(),
which in turn calls bdrv_co_get_allocated_file_size().
All the functions called from bdrv_do_query_node_info() onwards are
coroutine-safe, either have a coroutine version themselves[1] or are
mostly simple code/string manipulation[2].
1) bdrv_co_getlength(), bdrv_co_get_allocated_file_size(),
bdrv_co_get_info();
2) bdrv_refresh_filename(), bdrv_get_format_name(),
bdrv_get_full_backing_filename(), bdrv_query_snapshot_info_list(),
bdrv_get_specific_info();
Signed-off-by: Fabiano Rosas <farosas@suse.de>
Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
---
block/qapi.c | 14 ++++++++------
include/block/qapi.h | 6 +++++-
qemu-img.c | 3 ---
3 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/block/qapi.c b/block/qapi.c
index 26a9510345..7b1cf48246 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -370,7 +370,7 @@ fail:
}
/**
- * bdrv_query_block_graph_info:
+ * bdrv_co_query_block_graph_info:
* @bs: root node to start from
* @p_info: location to store image information
* @errp: location to store error information
@@ -379,17 +379,19 @@ fail:
*
* @p_info will be set only on success. On error, store error in @errp.
*/
-void bdrv_query_block_graph_info(BlockDriverState *bs,
- BlockGraphInfo **p_info,
- Error **errp)
+void coroutine_fn
+bdrv_co_query_block_graph_info(BlockDriverState *bs, BlockGraphInfo **p_info,
+ Error **errp)
{
ERRP_GUARD();
BlockGraphInfo *info;
BlockChildInfoList **children_list_tail;
BdrvChild *c;
+ assert_bdrv_graph_readable();
+
info = g_new0(BlockGraphInfo, 1);
- bdrv_do_query_node_info(bs, qapi_BlockGraphInfo_base(info), errp);
+ bdrv_co_do_query_node_info(bs, qapi_BlockGraphInfo_base(info), errp);
if (*errp) {
goto fail;
}
@@ -403,7 +405,7 @@ void bdrv_query_block_graph_info(BlockDriverState *bs,
QAPI_LIST_APPEND(children_list_tail, c_info);
c_info->name = g_strdup(c->name);
- bdrv_query_block_graph_info(c->bs, &c_info->info, errp);
+ bdrv_co_query_block_graph_info(c->bs, &c_info->info, errp);
if (*errp) {
goto fail;
}
diff --git a/include/block/qapi.h b/include/block/qapi.h
index 234730b3de..76be9cc3e5 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -41,7 +41,11 @@ bdrv_query_snapshot_info_list(BlockDriverState *bs,
void GRAPH_RDLOCK
bdrv_query_image_info(BlockDriverState *bs, ImageInfo **p_info, bool flat,
bool skip_implicit_filters, Error **errp);
-void GRAPH_RDLOCK
+
+void coroutine_fn GRAPH_RDLOCK
+bdrv_co_query_block_graph_info(BlockDriverState *bs, BlockGraphInfo **p_info,
+ Error **errp);
+void co_wrapper_bdrv_rdlock
bdrv_query_block_graph_info(BlockDriverState *bs, BlockGraphInfo **p_info,
Error **errp);
diff --git a/qemu-img.c b/qemu-img.c
index 7668f86769..19db8f18fc 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2958,10 +2958,7 @@ static BlockGraphInfoList *collect_image_info_list(bool image_opts,
* duplicate the backing chain information that we obtain by walking
* the chain manually here.
*/
- bdrv_graph_rdlock_main_loop();
bdrv_query_block_graph_info(bs, &info, &err);
- bdrv_graph_rdunlock_main_loop();
-
if (err) {
error_report_err(err);
blk_unref(blk);
--
2.35.3
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 07/11] block: Convert bdrv_query_image_info to coroutine
2024-04-09 14:59 [PATCH v3 00/11] block: Convert qmp_query_block into a coroutine Fabiano Rosas
` (5 preceding siblings ...)
2024-04-09 14:59 ` [PATCH v3 06/11] block: Convert bdrv_query_block_graph_info to coroutine Fabiano Rosas
@ 2024-04-09 14:59 ` Fabiano Rosas
2024-04-09 14:59 ` [PATCH v3 08/11] block: Convert bdrv_block_device_info into co_wrapper Fabiano Rosas
` (3 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Fabiano Rosas @ 2024-04-09 14:59 UTC (permalink / raw
To: qemu-devel
Cc: qemu-block, Kevin Wolf, Hanna Reitz, Markus Armbruster,
João Silva, Lin Ma, Claudio Fontana, Dario Faggioli,
Eric Blake, Stefan Hajnoczi, Paolo Bonzini
This function is a caller of bdrv_do_query_node_info(), which have
been converted to a coroutine. Convert this function as well so we're
closer from having the whole qmp_query_block as a single coroutine.
Also remove the wrapper for bdrv_co_do_query_node_info() now that all
its callers are converted.
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
block/qapi.c | 16 +++++++---------
include/block/qapi.h | 8 ++++----
2 files changed, 11 insertions(+), 13 deletions(-)
diff --git a/block/qapi.c b/block/qapi.c
index 7b1cf48246..5e263960a9 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -304,7 +304,7 @@ bdrv_co_do_query_node_info(BlockDriverState *bs, BlockNodeInfo *info,
}
/**
- * bdrv_query_image_info:
+ * bdrv_co_query_image_info:
* @bs: block node to examine
* @p_info: location to store image information
* @flat: skip backing node information
@@ -325,17 +325,15 @@ bdrv_co_do_query_node_info(BlockDriverState *bs, BlockNodeInfo *info,
*
* @p_info will be set only on success. On error, store error in @errp.
*/
-void bdrv_query_image_info(BlockDriverState *bs,
- ImageInfo **p_info,
- bool flat,
- bool skip_implicit_filters,
- Error **errp)
+void coroutine_fn
+bdrv_co_query_image_info(BlockDriverState *bs, ImageInfo **p_info, bool flat,
+ bool skip_implicit_filters, Error **errp)
{
ERRP_GUARD();
ImageInfo *info;
info = g_new0(ImageInfo, 1);
- bdrv_do_query_node_info(bs, qapi_ImageInfo_base(info), errp);
+ bdrv_co_do_query_node_info(bs, qapi_ImageInfo_base(info), errp);
if (*errp) {
goto fail;
}
@@ -353,8 +351,8 @@ void bdrv_query_image_info(BlockDriverState *bs,
}
if (backing) {
- bdrv_query_image_info(backing, &info->backing_image, false,
- skip_implicit_filters, errp);
+ bdrv_co_query_image_info(backing, &info->backing_image, false,
+ skip_implicit_filters, errp);
if (*errp) {
goto fail;
}
diff --git a/include/block/qapi.h b/include/block/qapi.h
index 76be9cc3e5..5f7e3a690e 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -38,7 +38,10 @@ int GRAPH_RDLOCK
bdrv_query_snapshot_info_list(BlockDriverState *bs,
SnapshotInfoList **p_list,
Error **errp);
-void GRAPH_RDLOCK
+void coroutine_fn GRAPH_RDLOCK
+bdrv_co_query_image_info(BlockDriverState *bs, ImageInfo **p_info, bool flat,
+ bool skip_implicit_filters, Error **errp);
+void co_wrapper_bdrv_rdlock
bdrv_query_image_info(BlockDriverState *bs, ImageInfo **p_info, bool flat,
bool skip_implicit_filters, Error **errp);
@@ -58,7 +61,4 @@ void bdrv_node_info_dump(BlockNodeInfo *info, int indentation, bool protocol);
void coroutine_fn GRAPH_RDLOCK
bdrv_co_do_query_node_info(BlockDriverState *bs, BlockNodeInfo *info,
Error **errp);
-void co_wrapper_bdrv_rdlock
-bdrv_do_query_node_info(BlockDriverState *bs, BlockNodeInfo *info,
- Error **errp);
#endif
--
2.35.3
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 08/11] block: Convert bdrv_block_device_info into co_wrapper
2024-04-09 14:59 [PATCH v3 00/11] block: Convert qmp_query_block into a coroutine Fabiano Rosas
` (6 preceding siblings ...)
2024-04-09 14:59 ` [PATCH v3 07/11] block: Convert bdrv_query_image_info " Fabiano Rosas
@ 2024-04-09 14:59 ` Fabiano Rosas
2024-04-09 14:59 ` [PATCH v3 09/11] block: Don't query all block devices at hmp_nbd_server_start Fabiano Rosas
` (2 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Fabiano Rosas @ 2024-04-09 14:59 UTC (permalink / raw
To: qemu-devel
Cc: qemu-block, Kevin Wolf, Hanna Reitz, Markus Armbruster,
João Silva, Lin Ma, Claudio Fontana, Dario Faggioli,
Eric Blake, Stefan Hajnoczi, Paolo Bonzini
We're converting callers of bdrv_co_get_allocated_file_size() to run
in coroutines because that function will be made asynchronous when
called (indirectly) from the QMP dispatcher.
This function is a candidate because it calls bdrv_query_image_info()
-> bdrv_co_do_query_node_info() -> bdrv_co_get_allocated_file_size().
It is safe to turn this is a coroutine because the code it calls is
made up of either simple accessors and string manipulation functions
[1] or it has already been determined to be safe [2].
1) bdrv_refresh_filename(), bdrv_is_read_only(),
blk_enable_write_cache(), bdrv_cow_bs(), blk_get_public(),
throttle_group_get_name(), bdrv_write_threshold_get(),
bdrv_query_dirty_bitmaps(), throttle_group_get_config(),
bdrv_filter_or_cow_bs(), bdrv_skip_implicit_filters()
2) bdrv_co_do_query_node_info() (see previous commits);
This was the only caller of bdrv_query_image_info(), so we can remove
the wrapper for that function now.
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
- used co_wrapper_bdrv_rdlock instead of co_wrapper
---
block/qapi.c | 10 +++++-----
include/block/qapi.h | 13 ++++++-------
2 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/block/qapi.c b/block/qapi.c
index 5e263960a9..9a59e5606f 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -41,10 +41,10 @@
#include "qemu/qemu-print.h"
#include "sysemu/block-backend.h"
-BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
- BlockDriverState *bs,
- bool flat,
- Error **errp)
+BlockDeviceInfo *coroutine_fn bdrv_co_block_device_info(BlockBackend *blk,
+ BlockDriverState *bs,
+ bool flat,
+ Error **errp)
{
ERRP_GUARD();
ImageInfo **p_image_info;
@@ -152,7 +152,7 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
* Skip automatically inserted nodes that the user isn't aware of for
* query-block (blk != NULL), but not for query-named-block-nodes
*/
- bdrv_query_image_info(bs, p_image_info, flat, blk != NULL, errp);
+ bdrv_co_query_image_info(bs, p_image_info, flat, blk != NULL, errp);
if (*errp) {
qapi_free_BlockDeviceInfo(info);
return NULL;
diff --git a/include/block/qapi.h b/include/block/qapi.h
index 5f7e3a690e..9f0e957963 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -30,10 +30,12 @@
#include "block/snapshot.h"
#include "qapi/qapi-types-block-core.h"
-BlockDeviceInfo * GRAPH_RDLOCK
-bdrv_block_device_info(BlockBackend *blk, BlockDriverState *bs,
- bool flat, Error **errp);
-
+BlockDeviceInfo *coroutine_fn GRAPH_RDLOCK
+bdrv_co_block_device_info(BlockBackend *blk, BlockDriverState *bs, bool flat,
+ Error **errp);
+BlockDeviceInfo *co_wrapper_bdrv_rdlock
+bdrv_block_device_info(BlockBackend *blk, BlockDriverState *bs, bool flat,
+ Error **errp);
int GRAPH_RDLOCK
bdrv_query_snapshot_info_list(BlockDriverState *bs,
SnapshotInfoList **p_list,
@@ -41,9 +43,6 @@ bdrv_query_snapshot_info_list(BlockDriverState *bs,
void coroutine_fn GRAPH_RDLOCK
bdrv_co_query_image_info(BlockDriverState *bs, ImageInfo **p_info, bool flat,
bool skip_implicit_filters, Error **errp);
-void co_wrapper_bdrv_rdlock
-bdrv_query_image_info(BlockDriverState *bs, ImageInfo **p_info, bool flat,
- bool skip_implicit_filters, Error **errp);
void coroutine_fn GRAPH_RDLOCK
bdrv_co_query_block_graph_info(BlockDriverState *bs, BlockGraphInfo **p_info,
--
2.35.3
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 09/11] block: Don't query all block devices at hmp_nbd_server_start
2024-04-09 14:59 [PATCH v3 00/11] block: Convert qmp_query_block into a coroutine Fabiano Rosas
` (7 preceding siblings ...)
2024-04-09 14:59 ` [PATCH v3 08/11] block: Convert bdrv_block_device_info into co_wrapper Fabiano Rosas
@ 2024-04-09 14:59 ` Fabiano Rosas
2024-04-09 14:59 ` [PATCH v3 10/11] block: Convert qmp_query_block and qmp_query_named_block_nodes to coroutine Fabiano Rosas
2024-04-09 14:59 ` [PATCH v3 11/11] block: Add a thread-pool version of fstat Fabiano Rosas
10 siblings, 0 replies; 12+ messages in thread
From: Fabiano Rosas @ 2024-04-09 14:59 UTC (permalink / raw
To: qemu-devel
Cc: qemu-block, Kevin Wolf, Hanna Reitz, Markus Armbruster,
João Silva, Lin Ma, Claudio Fontana, Dario Faggioli,
Eric Blake, Stefan Hajnoczi, Paolo Bonzini
We're currently doing a full query-block just to enumerate the devices
for qmp_nbd_server_add and then discarding the BlockInfoList
afterwards. Alter hmp_nbd_server_start to instead iterate explicitly
over the block_backends list.
This allows the removal of the dependency on qmp_query_block from
hmp_nbd_server_start. This is desirable because we're about to move
qmp_query_block into a coroutine and don't need to change the NBD code
at the same time.
Add the GRAPH_RDLOCK_GUARD_MAINLOOP macro because
bdrv_skip_implicit_filters() needs the graph lock.
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
- add a comment explaining some checks are done to preserve previous
behavior;
- we need the strdup when assigning .device to preserve const. Just
add a matching g_free();
- about the possible leak at qmp_nbd_server_add() unrelated to this
patch, commit 8675cbd68b ("nbd: Utilize QAPI_CLONE for type
conversion") mentions that the QAPI visitor will already free
arg->name.
---
block/monitor/block-hmp-cmds.c | 32 ++++++++++++++++++++++++--------
1 file changed, 24 insertions(+), 8 deletions(-)
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index d954bec6f1..9db587c661 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -387,10 +387,12 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
bool writable = qdict_get_try_bool(qdict, "writable", false);
bool all = qdict_get_try_bool(qdict, "all", false);
Error *local_err = NULL;
- BlockInfoList *block_list, *info;
+ BlockBackend *blk;
SocketAddress *addr;
NbdServerAddOptions export;
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
if (writable && !all) {
error_setg(&local_err, "-w only valid together with -a");
goto exit;
@@ -415,29 +417,43 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
/* Then try adding all block devices. If one fails, close all and
* exit.
*/
- block_list = qmp_query_block(NULL);
+ for (blk = blk_all_next(NULL); blk; blk = blk_all_next(blk)) {
+ BlockDriverState *bs = blk_bs(blk);
- for (info = block_list; info; info = info->next) {
- if (!info->value->inserted) {
+ if (!*blk_name(blk)) {
+ continue;
+ }
+
+ /*
+ * Note: historically we used to call qmp_query_block() to get
+ * the list of block devices. The two 'continue' cases below
+ * are the same as used by that function and are here to
+ * preserve behavior.
+ */
+
+ if (!blk_get_attached_dev(blk)) {
+ continue;
+ }
+
+ bs = bdrv_skip_implicit_filters(bs);
+ if (!bs || !bs->drv) {
continue;
}
export = (NbdServerAddOptions) {
- .device = info->value->device,
+ .device = g_strdup(blk_name(blk)),
.has_writable = true,
.writable = writable,
};
qmp_nbd_server_add(&export, &local_err);
-
+ g_free(export.device);
if (local_err != NULL) {
qmp_nbd_server_stop(NULL);
break;
}
}
- qapi_free_BlockInfoList(block_list);
-
exit:
hmp_handle_error(mon, local_err);
}
--
2.35.3
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 10/11] block: Convert qmp_query_block and qmp_query_named_block_nodes to coroutine
2024-04-09 14:59 [PATCH v3 00/11] block: Convert qmp_query_block into a coroutine Fabiano Rosas
` (8 preceding siblings ...)
2024-04-09 14:59 ` [PATCH v3 09/11] block: Don't query all block devices at hmp_nbd_server_start Fabiano Rosas
@ 2024-04-09 14:59 ` Fabiano Rosas
2024-04-09 14:59 ` [PATCH v3 11/11] block: Add a thread-pool version of fstat Fabiano Rosas
10 siblings, 0 replies; 12+ messages in thread
From: Fabiano Rosas @ 2024-04-09 14:59 UTC (permalink / raw
To: qemu-devel
Cc: qemu-block, Kevin Wolf, Hanna Reitz, Markus Armbruster,
João Silva, Lin Ma, Claudio Fontana, Dario Faggioli,
Eric Blake, Stefan Hajnoczi, Paolo Bonzini,
Dr. David Alan Gilbert
From: Lin Ma <lma@suse.com>
Convert the remaining functions to make the QMP commands query-block
and query-named-block-nodes run in their entirety in a coroutine. With
this, any yield from those commands will return all the way back to
the main loop. This releases the BQL and the main loop and avoids
having the QMP command block another more important task from running.
Both commands need to be converted at once because hmp_info_block
calls both and it needs to be moved to a coroutine as well.
Now the wrapper for bdrv_co_get_allocated_file_size() can be made not
mixed and the wrapper for bdrv_co_block_device_info() can be removed.
Signed-off-by: Lin Ma <lma@suse.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
block.c | 8 ++++----
block/monitor/block-hmp-cmds.c | 2 +-
block/qapi.c | 12 ++++++------
blockdev.c | 8 ++++----
hmp-commands-info.hx | 1 +
include/block/block-global-state.h | 3 ++-
include/block/block-hmp-cmds.h | 2 +-
include/block/block-io.h | 2 +-
include/block/qapi.h | 3 ---
qapi/block-core.json | 5 +++--
10 files changed, 23 insertions(+), 23 deletions(-)
diff --git a/block.c b/block.c
index 01478c5471..cba28f07fc 100644
--- a/block.c
+++ b/block.c
@@ -6207,18 +6207,18 @@ BlockDriverState *bdrv_find_node(const char *node_name)
}
/* Put this QMP function here so it can access the static graph_bdrv_states. */
-BlockDeviceInfoList *bdrv_named_nodes_list(bool flat,
- Error **errp)
+BlockDeviceInfoList *coroutine_fn bdrv_co_named_nodes_list(bool flat,
+ Error **errp)
{
BlockDeviceInfoList *list;
BlockDriverState *bs;
GLOBAL_STATE_CODE();
- GRAPH_RDLOCK_GUARD_MAINLOOP();
+ GRAPH_RDLOCK_GUARD();
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);
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 9db587c661..8ceff59688 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -738,7 +738,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 9a59e5606f..c4514295ec 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -418,8 +418,8 @@ fail:
}
/* @p_info will be set only on success. */
-static void GRAPH_RDLOCK
-bdrv_query_info(BlockBackend *blk, BlockInfo **p_info, Error **errp)
+static void GRAPH_RDLOCK coroutine_fn
+bdrv_co_query_info(BlockBackend *blk, BlockInfo **p_info, Error **errp)
{
BlockInfo *info = g_malloc0(sizeof(*info));
BlockDriverState *bs = blk_bs(blk);
@@ -451,7 +451,7 @@ bdrv_query_info(BlockBackend *blk, BlockInfo **p_info, Error **errp)
}
if (bs && bs->drv) {
- info->inserted = bdrv_block_device_info(blk, bs, false, errp);
+ info->inserted = bdrv_co_block_device_info(blk, bs, false, errp);
if (info->inserted == NULL) {
goto err;
}
@@ -661,13 +661,13 @@ 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;
Error *local_err = NULL;
- GRAPH_RDLOCK_GUARD_MAINLOOP();
+ GRAPH_RDLOCK_GUARD();
for (blk = blk_all_next(NULL); blk; blk = blk_all_next(blk)) {
BlockInfoList *info;
@@ -677,7 +677,7 @@ BlockInfoList *qmp_query_block(Error **errp)
}
info = g_malloc0(sizeof(*info));
- bdrv_query_info(blk, &info->value, &local_err);
+ bdrv_co_query_info(blk, &info->value, &local_err);
if (local_err) {
error_propagate(errp, local_err);
g_free(info);
diff --git a/blockdev.c b/blockdev.c
index 057601dcf0..fe3226c8c4 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2744,13 +2744,13 @@ 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;
- return bdrv_named_nodes_list(return_flat, errp);
+ return bdrv_co_named_nodes_list(return_flat, errp);
}
XDbgBlockGraph *qmp_x_debug_query_block_graph(Error **errp)
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index ad1b1306e3..c0dad1fc86 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-global-state.h b/include/block/block-global-state.h
index bd7cecd1cf..ee87a0c7d9 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -196,7 +196,8 @@ void bdrv_aio_cancel(BlockAIOCB *acb);
int bdrv_has_zero_init_1(BlockDriverState *bs);
int coroutine_mixed_fn GRAPH_RDLOCK bdrv_has_zero_init(BlockDriverState *bs);
BlockDriverState *bdrv_find_node(const char *node_name);
-BlockDeviceInfoList *bdrv_named_nodes_list(bool flat, Error **errp);
+BlockDeviceInfoList *coroutine_fn bdrv_co_named_nodes_list(bool flat,
+ Error **errp);
XDbgBlockGraph * GRAPH_RDLOCK bdrv_get_xdbg_block_graph(Error **errp);
BlockDriverState *bdrv_lookup_bs(const char *device,
const char *node_name,
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 349d7760a1..b49e0537dd 100644
--- a/include/block/block-io.h
+++ b/include/block/block-io.h
@@ -86,7 +86,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 9f0e957963..9274b76814 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -33,9 +33,6 @@
BlockDeviceInfo *coroutine_fn GRAPH_RDLOCK
bdrv_co_block_device_info(BlockBackend *blk, BlockDriverState *bs, bool flat,
Error **errp);
-BlockDeviceInfo *co_wrapper_bdrv_rdlock
-bdrv_block_device_info(BlockBackend *blk, BlockDriverState *bs, bool flat,
- Error **errp);
int GRAPH_RDLOCK
bdrv_query_snapshot_info_list(BlockDriverState *bs,
SnapshotInfoList **p_list,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 746d1694c2..4a0a336431 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -849,7 +849,7 @@
# }
##
{ 'command': 'query-block', 'returns': ['BlockInfo'],
- 'allow-preconfig': true }
+ 'allow-preconfig': true, 'coroutine': true }
##
# @BlockDeviceTimedStats:
@@ -1997,7 +1997,8 @@
{ 'command': 'query-named-block-nodes',
'returns': [ 'BlockDeviceInfo' ],
'data': { '*flat': 'bool' },
- 'allow-preconfig': true }
+ 'allow-preconfig': true,
+ 'coroutine': true}
##
# @XDbgBlockGraphNodeType:
--
2.35.3
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 11/11] block: Add a thread-pool version of fstat
2024-04-09 14:59 [PATCH v3 00/11] block: Convert qmp_query_block into a coroutine Fabiano Rosas
` (9 preceding siblings ...)
2024-04-09 14:59 ` [PATCH v3 10/11] block: Convert qmp_query_block and qmp_query_named_block_nodes to coroutine Fabiano Rosas
@ 2024-04-09 14:59 ` Fabiano Rosas
10 siblings, 0 replies; 12+ messages in thread
From: Fabiano Rosas @ 2024-04-09 14:59 UTC (permalink / raw
To: qemu-devel
Cc: qemu-block, Kevin Wolf, Hanna Reitz, Markus Armbruster,
João Silva, Lin Ma, Claudio Fontana, Dario Faggioli,
Eric Blake, Stefan Hajnoczi, Paolo Bonzini
From: João Silva <jsilva@suse.de>
The fstat call can take a long time to finish when running over
NFS. Add a version of it that runs in the thread pool.
Adapt one of its users, raw_co_get_allocated_file size to use the new
version. That function is called via QMP under the qemu_global_mutex
so it has a large chance of blocking VCPU threads in case it takes too
long to finish.
Reviewed-by: Claudio Fontana <cfontana@suse.de>
Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
Signed-off-by: João Silva <jsilva@suse.de>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
block/file-posix.c | 40 +++++++++++++++++++++++++++++++++++++---
include/block/raw-aio.h | 4 +++-
2 files changed, 40 insertions(+), 4 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index 35684f7e21..6fbf961244 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -226,6 +226,9 @@ typedef struct RawPosixAIOData {
struct {
unsigned long op;
} zone_mgmt;
+ struct {
+ struct stat *st;
+ } fstat;
};
} RawPosixAIOData;
@@ -2616,6 +2619,34 @@ static void raw_close(BlockDriverState *bs)
}
}
+static int handle_aiocb_fstat(void *opaque)
+{
+ RawPosixAIOData *aiocb = opaque;
+
+ if (fstat(aiocb->aio_fildes, aiocb->fstat.st) < 0) {
+ return -errno;
+ }
+
+ return 0;
+}
+
+static int coroutine_fn raw_co_fstat(BlockDriverState *bs, struct stat *st)
+{
+ BDRVRawState *s = bs->opaque;
+ RawPosixAIOData acb;
+
+ acb = (RawPosixAIOData) {
+ .bs = bs,
+ .aio_fildes = s->fd,
+ .aio_type = QEMU_AIO_FSTAT,
+ .fstat = {
+ .st = st,
+ },
+ };
+
+ return raw_thread_pool_submit(handle_aiocb_fstat, &acb);
+}
+
/**
* Truncates the given regular file @fd to @offset and, when growing, fills the
* new space according to @prealloc.
@@ -2860,11 +2891,14 @@ static int64_t coroutine_fn raw_co_getlength(BlockDriverState *bs)
static int64_t coroutine_fn raw_co_get_allocated_file_size(BlockDriverState *bs)
{
struct stat st;
- BDRVRawState *s = bs->opaque;
+ int ret;
- if (fstat(s->fd, &st) < 0) {
- return -errno;
+ ret = raw_co_fstat(bs, &st);
+
+ if (ret) {
+ return ret;
}
+
return (int64_t)st.st_blocks * 512;
}
diff --git a/include/block/raw-aio.h b/include/block/raw-aio.h
index 20e000b8ef..0c6af8dc32 100644
--- a/include/block/raw-aio.h
+++ b/include/block/raw-aio.h
@@ -31,6 +31,7 @@
#define QEMU_AIO_ZONE_REPORT 0x0100
#define QEMU_AIO_ZONE_MGMT 0x0200
#define QEMU_AIO_ZONE_APPEND 0x0400
+#define QEMU_AIO_FSTAT 0x0800
#define QEMU_AIO_TYPE_MASK \
(QEMU_AIO_READ | \
QEMU_AIO_WRITE | \
@@ -42,7 +43,8 @@
QEMU_AIO_TRUNCATE | \
QEMU_AIO_ZONE_REPORT | \
QEMU_AIO_ZONE_MGMT | \
- QEMU_AIO_ZONE_APPEND)
+ QEMU_AIO_ZONE_APPEND | \
+ QEMU_AIO_FSTAT)
/* AIO flags */
#define QEMU_AIO_MISALIGNED 0x1000
--
2.35.3
^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2024-04-09 15:02 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-09 14:59 [PATCH v3 00/11] block: Convert qmp_query_block into a coroutine Fabiano Rosas
2024-04-09 14:59 ` [PATCH v3 01/11] block: Allow the wrapper script to see functions declared in qapi.h Fabiano Rosas
2024-04-09 14:59 ` [PATCH v3 02/11] block: Temporarily mark bdrv_co_get_allocated_file_size as mixed Fabiano Rosas
2024-04-09 14:59 ` [PATCH v3 03/11] block: Take the graph lock in bdrv_snapshot_list Fabiano Rosas
2024-04-09 14:59 ` [PATCH v3 04/11] block: Reschedule query-block during qcow2 invalidation Fabiano Rosas
2024-04-09 14:59 ` [PATCH v3 05/11] block: Run bdrv_do_query_node_info in a coroutine Fabiano Rosas
2024-04-09 14:59 ` [PATCH v3 06/11] block: Convert bdrv_query_block_graph_info to coroutine Fabiano Rosas
2024-04-09 14:59 ` [PATCH v3 07/11] block: Convert bdrv_query_image_info " Fabiano Rosas
2024-04-09 14:59 ` [PATCH v3 08/11] block: Convert bdrv_block_device_info into co_wrapper Fabiano Rosas
2024-04-09 14:59 ` [PATCH v3 09/11] block: Don't query all block devices at hmp_nbd_server_start Fabiano Rosas
2024-04-09 14:59 ` [PATCH v3 10/11] block: Convert qmp_query_block and qmp_query_named_block_nodes to coroutine Fabiano Rosas
2024-04-09 14:59 ` [PATCH v3 11/11] block: Add a thread-pool version of fstat Fabiano Rosas
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.