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

Mon, 06 Nov 2023 06:49:01 -0800 

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 <faro...@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:

Reply via email to