[PATCH v2 00/10] block: Make raw_co_get_allocated_file_size asynchronous

[PATCH v2 00/10] block: Make raw_co_get_allocated_file_size asynchronous

[Fabiano Rosas]

Hi,

The major change from the last version is that this time I'm moving
all of the callers of bdrv_get_allocated_file_size() into
coroutines. I had to make some temporary changes to avoid asserts
while not all the callers were converted.

I tried my best to explain why I think the changes are safe. To avoid
changing too much of the code I added a change that removes the
dependency of qmp_query_block from hmp_nbd_server_start, that way I
don't need to move all of the nbd code into a coroutine as well.

Based on:
 [PATCH v2 00/11] block: Re-enable the graph lock
 https://lore.kernel.org/r/20230605085711.21261-1-kwolf@redhat.com

changes:

  - fixed duplicated commit message [Lin]
  - clarified why we need to convert info-block [Claudio]
  - added my rationale of why the changes are safe [Eric]
  - converted all callers to coroutines [Kevin]
  - made hmp_nbd_server_start don't depend on qmp_query_block

CI run: https://gitlab.com/farosas/qemu/-/pipelines/895525156
===
v1:
https://lore.kernel.org/r/20230523213903.18418-1-farosas@suse.de

As discussed in another thread [1], here's an RFC addressing a VCPU
softlockup encountered when issuing QMP commands that target a disk
placed on NFS.

Since QMP commands happen with the qemu_global_mutex locked, any
command that takes too long to finish will block other threads waiting
to take the global mutex. One such thread could be a VCPU thread going
out of the guest to handle IO.

This is the case when issuing the QMP command query-block, which
eventually calls raw_co_get_allocated_file_size(). This function makes
an 'fstat' call that has been observed to take a long time (seconds)
over NFS.

NFS latency issues aside, we can improve the situation by not blocking
VCPU threads while the command is running.

Move the 'fstat' call into the thread-pool and make the necessary
adaptations to ensure raw_co_get_allocated_file_size runs in a
coroutine in the block driver aio_context.

1- Question about QMP and BQL
https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg03141.html

Fabiano Rosas (8):
  block: Remove bdrv_query_block_node_info
  block: Remove unnecessary variable in bdrv_block_device_info
  block: Allow the wrapper script to see functions declared in qapi.h
  block: Temporarily mark bdrv_co_get_allocated_file_size as mixed
  block: Convert bdrv_query_block_graph_info to coroutine
  block: Convert bdrv_block_device_info into co_wrapper
  block: Don't query all block devices at hmp_nbd_server_start
  block: Convert qmp_query_block() to coroutine_fn

João Silva (1):
  block: Add a thread-pool version of fstat

Lin Ma (1):
  block: Convert qmp_query_named_block_nodes to coroutine

 block/file-posix.c                 | 40 +++++++++++++++++--
 block/meson.build                  |  1 +
 block/monitor/block-hmp-cmds.c     | 22 ++++++-----
 block/qapi.c                       | 62 +++++++++---------------------
 blockdev.c                         |  6 +--
 hmp-commands-info.hx               |  1 +
 include/block/block-hmp-cmds.h     |  2 +-
 include/block/qapi.h               | 17 ++++----
 include/block/raw-aio.h            |  4 +-
 qapi/block-core.json               |  5 ++-
 qemu-img.c                         |  2 -
 scripts/block-coroutine-wrapper.py |  1 +
 12 files changed, 90 insertions(+), 73 deletions(-)

-- 
2.35.3

[PATCH v2 01/10] block: Remove bdrv_query_block_node_info

[Fabiano Rosas]

The last call site of this function has been removed by commit
c04d0ab026 ("qemu-img: Let info print block graph").

Reviewed-by: Claudio Fontana <cfontana@suse.de>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 block/qapi.c         | 27 ---------------------------
 include/block/qapi.h |  3 ---
 2 files changed, 30 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index f34f95e0ef..79bf80c503 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -309,33 +309,6 @@ out:
     aio_context_release(bdrv_get_aio_context(bs));
 }
 
-/**
- * bdrv_query_block_node_info:
- * @bs: block node to examine
- * @p_info: location to store node information
- * @errp: location to store error information
- *
- * Store image information about @bs in @p_info.
- *
- * @p_info will be set only on success. On error, store error in @errp.
- */
-void bdrv_query_block_node_info(BlockDriverState *bs,
-                                BlockNodeInfo **p_info,
-                                Error **errp)
-{
-    BlockNodeInfo *info;
-    ERRP_GUARD();
-
-    info = g_new0(BlockNodeInfo, 1);
-    bdrv_do_query_node_info(bs, info, errp);
-    if (*errp) {
-        qapi_free_BlockNodeInfo(info);
-        return;
-    }
-
-    *p_info = info;
-}
-
 /**
  * bdrv_query_image_info:
  * @bs: block node to examine
diff --git a/include/block/qapi.h b/include/block/qapi.h
index 18d48ddb70..8663971c58 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -36,9 +36,6 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
 int bdrv_query_snapshot_info_list(BlockDriverState *bs,
                                   SnapshotInfoList **p_list,
                                   Error **errp);
-void bdrv_query_block_node_info(BlockDriverState *bs,
-                                BlockNodeInfo **p_info,
-                                Error **errp);
 void bdrv_query_image_info(BlockDriverState *bs,
                            ImageInfo **p_info,
                            bool flat,
-- 
2.35.3

[PATCH v2 02/10] block: Remove unnecessary variable in bdrv_block_device_info

[Fabiano Rosas]

The commit 5d8813593f ("block/qapi: Let bdrv_query_image_info()
recurse") removed the loop where we set the 'bs0' variable, so now it
is just the same as 'bs'.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 block/qapi.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index 79bf80c503..1cbb0935ff 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -48,7 +48,7 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
 {
     ImageInfo **p_image_info;
     ImageInfo *backing_info;
-    BlockDriverState *bs0, *backing;
+    BlockDriverState *backing;
     BlockDeviceInfo *info;
     ERRP_GUARD();
 
@@ -145,7 +145,6 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
 
     info->write_threshold = bdrv_write_threshold_get(bs);
 
-    bs0 = bs;
     p_image_info = &info->image;
     info->backing_file_depth = 0;
 
@@ -153,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(bs0, p_image_info, flat, blk != NULL, errp);
+    bdrv_query_image_info(bs, p_image_info, flat, blk != NULL, errp);
     if (*errp) {
         qapi_free_BlockDeviceInfo(info);
         return NULL;
-- 
2.35.3

Re: [PATCH v2 02/10] block: Remove unnecessary variable in bdrv_block_device_info

[Philippe Mathieu-Daudé]

On 9/6/23 22:19, Fabiano Rosas wrote:
> The commit 5d8813593f ("block/qapi: Let bdrv_query_image_info()
> recurse") removed the loop where we set the 'bs0' variable, so now it
> is just the same as 'bs'.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   block/qapi.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v2 03/10] block: Allow the wrapper script to see functions declared in qapi.h

[Fabiano Rosas]

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.

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 fb4332bd66..7ad6a396a4 100644
--- a/block/meson.build
+++ b/block/meson.build
@@ -150,6 +150,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 d4a183db61..814b62df26 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

Re: [PATCH v2 03/10] block: Allow the wrapper script to see functions declared in qapi.h

[Hanna Czenczek]

On 09.06.23 22:19, Fabiano Rosas wrote:
> 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.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   block/meson.build                  | 1 +
>   scripts/block-coroutine-wrapper.py | 1 +
>   2 files changed, 2 insertions(+)

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

[PATCH v2 04/10] block: Temporarily mark bdrv_co_get_allocated_file_size as mixed

[Fabiano Rosas]

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>
---
 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 43af816d75..f31e25cf56 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_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

Re: [PATCH v2 04/10] block: Temporarily mark bdrv_co_get_allocated_file_size as mixed

[Hanna Czenczek]

On 09.06.23 22:19, Fabiano Rosas wrote:
> 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>
> ---
>   include/block/block-io.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

[PATCH v2 05/10] block: Convert bdrv_query_block_graph_info to coroutine

[Fabiano Rosas]

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 function is a candidate because it calls bdrv_do_query_node_info(),
which in turn calls bdrv_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_getlength(), bdrv_get_allocated_file_size(), bdrv_get_info(),
   bdrv_get_specific_info();

2) bdrv_refresh_filename(), bdrv_get_format_name(),
   bdrv_get_full_backing_filename(), bdrv_query_snapshot_info_list();

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 block/qapi.c         | 12 +++++++-----
 include/block/qapi.h |  6 +++++-
 qemu-img.c           |  2 --
 3 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index 1cbb0935ff..a2e71edaff 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -375,7 +375,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
@@ -384,15 +384,17 @@ 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)
 {
     BlockGraphInfo *info;
     BlockChildInfoList **children_list_tail;
     BdrvChild *c;
     ERRP_GUARD();
 
+    assert_bdrv_graph_readable();
+
     info = g_new0(BlockGraphInfo, 1);
     bdrv_do_query_node_info(bs, qapi_BlockGraphInfo_base(info), errp);
     if (*errp) {
@@ -408,7 +410,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 8663971c58..7035bcd1ae 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"
@@ -41,7 +42,10 @@ void bdrv_query_image_info(BlockDriverState *bs,
                            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 27f48051b0..8066286f5e 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2945,9 +2945,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);
-- 
2.35.3

Re: [PATCH v2 05/10] block: Convert bdrv_query_block_graph_info to coroutine

[Hanna Czenczek]

On 09.06.23 22:19, Fabiano Rosas wrote:
> 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 function is a candidate because it calls bdrv_do_query_node_info(),
> which in turn calls bdrv_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_getlength(), bdrv_get_allocated_file_size(), bdrv_get_info(),
>     bdrv_get_specific_info();
>
> 2) bdrv_refresh_filename(), bdrv_get_format_name(),
>     bdrv_get_full_backing_filename(), bdrv_query_snapshot_info_list();
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   block/qapi.c         | 12 +++++++-----
>   include/block/qapi.h |  6 +++++-
>   qemu-img.c           |  2 --
>   3 files changed, 12 insertions(+), 8 deletions(-)

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

[PATCH v2 06/10] block: Convert bdrv_block_device_info into co_wrapper

[Fabiano Rosas]

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 function is a candidate because it calls bdrv_query_image_info()
-> bdrv_do_query_node_info() -> bdrv_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_do_query_node_info() (see previous commit);

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 block/qapi.c         |  8 ++++----
 include/block/qapi.h | 12 ++++++++----
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index a2e71edaff..20660e15d6 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)
 {
     ImageInfo **p_image_info;
     ImageInfo *backing_info;
diff --git a/include/block/qapi.h b/include/block/qapi.h
index 7035bcd1ae..5cb0202791 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -30,10 +30,14 @@
 #include "block/snapshot.h"
 #include "qapi/qapi-types-block-core.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);
+BlockDeviceInfo *co_wrapper 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);
-- 
2.35.3

Re: [PATCH v2 06/10] block: Convert bdrv_block_device_info into co_wrapper

[Hanna Czenczek]

On 09.06.23 22:19, Fabiano Rosas wrote:
> 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 function is a candidate because it calls bdrv_query_image_info()
> -> bdrv_do_query_node_info() -> bdrv_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_do_query_node_info() (see previous commit);
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   block/qapi.c         |  8 ++++----
>   include/block/qapi.h | 12 ++++++++----
>   2 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/block/qapi.c b/block/qapi.c
> index a2e71edaff..20660e15d6 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)
>   {
>       ImageInfo **p_image_info;
>       ImageInfo *backing_info;
> diff --git a/include/block/qapi.h b/include/block/qapi.h
> index 7035bcd1ae..5cb0202791 100644
> --- a/include/block/qapi.h
> +++ b/include/block/qapi.h
> @@ -30,10 +30,14 @@
>   #include "block/snapshot.h"
>   #include "qapi/qapi-types-block-core.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);
> +BlockDeviceInfo *co_wrapper bdrv_block_device_info(BlockBackend *blk,
> +                                                   BlockDriverState *bs,
> +                                                   bool flat,
> +                                                   Error **errp);

bdrv_co_block_device_info() is now marked as GRAPH_RDLOCK, so should 
this use a co_wrapper_bdrv_rdlock instead?

Hanna

>   int bdrv_query_snapshot_info_list(BlockDriverState *bs,
>                                     SnapshotInfoList **p_list,
>                                     Error **errp);

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

[Fabiano Rosas]

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

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);
         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:
-- 
2.35.3

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

[Hanna Czenczek]

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:

[PATCH v2 08/10] block: Don't query all block devices at hmp_nbd_server_start

[Fabiano Rosas]

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.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 block/monitor/block-hmp-cmds.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index ca2599de44..26116fe831 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -394,7 +394,7 @@ 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;
 
@@ -419,18 +419,24 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
         return;
     }
 
-    /* Then try adding all block devices.  If one fails, close all and
+    /*
+     * 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) && !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,
         };
@@ -443,8 +449,6 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
         }
     }
 
-    qapi_free_BlockInfoList(block_list);
-
 exit:
     hmp_handle_error(mon, local_err);
 }
-- 
2.35.3

Re: [PATCH v2 08/10] block: Don't query all block devices at hmp_nbd_server_start

[Hanna Czenczek]

On 09.06.23 22:19, Fabiano Rosas wrote:
> 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.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   block/monitor/block-hmp-cmds.c | 20 ++++++++++++--------
>   1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
> index ca2599de44..26116fe831 100644
> --- a/block/monitor/block-hmp-cmds.c
> +++ b/block/monitor/block-hmp-cmds.c
> @@ -394,7 +394,7 @@ 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;
>   
> @@ -419,18 +419,24 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
>           return;
>       }
>   
> -    /* Then try adding all block devices.  If one fails, close all and
> +    /*
> +     * 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) && !blk_get_attached_dev(blk)) {

I’d like a comment here that historically, we’ve used qmp_query_block() 
here, and this is the same condition that it uses.  (Otherwise, it’s 
hard to see why it matters whether a device is attached or not.)

> +            continue;
> +        }
> +
> +        bs = bdrv_skip_implicit_filters(bs);
> +        if (!bs || !bs->drv) {

Same here.  Just checking blk_is_inserted() would make more sense in 
this place, but if you want to absolutely keep behavior unchanged, then 
there should be a comment here why this check is done (because 
bdrv_query_info() does it to determine whether info->inserted should be 
set, which was historically used to determine whether this BlockBackend 
can be exported).

>               continue;
>           }
>   
>           export = (NbdServerAddOptions) {
> -            .device         = info->value->device,
> +            .device         = g_strdup(blk_name(blk)),

Do we need to g_strdup() here?  We didn’t before, so I think this will 
leak export.device.

I know bdrv_query_info() uses g_strdup(), but that was released by the 
qapi_free_BlockInfoList() below, which is now removed without replacement.

(On that note, it also looks like qmp_nbd_server_add() can leak 
arg->name (i.e. device.name) if it is not set by the caller.  It also 
uses g_strdup() there, but never frees it.  It does free the export_opts 
it creates, and `arg` is put into it, but as a deep copy, so anything in 
`arg` is leaked.)

Hanna

>               .has_writable   = true,
>               .writable       = writable,
>           };
> @@ -443,8 +449,6 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
>           }
>       }
>   
> -    qapi_free_BlockInfoList(block_list);
> -
>   exit:
>       hmp_handle_error(mon, local_err);
>   }

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

[Fabiano Rosas]

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

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:
-- 
2.35.3

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

[Hanna Czenczek]

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:

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

[Hanna Czenczek]

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

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

[Fabiano Rosas]

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.

[PATCH v2 10/10] block: Add a thread-pool version of fstat

[Fabiano Rosas]

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>
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 ac1ed54811..45232dc0f9 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -227,6 +227,9 @@ typedef struct RawPosixAIOData {
         struct {
             unsigned long op;
         } zone_mgmt;
+        struct {
+            struct stat *st;
+        } fstat;
     };
 } RawPosixAIOData;
 
@@ -2614,6 +2617,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.
@@ -2853,11 +2884,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 0f63c2800c..1f2c678461 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

Re: [PATCH v2 10/10] block: Add a thread-pool version of fstat

[Hanna Czenczek]

On 09.06.23 22:19, Fabiano Rosas wrote:
> 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>
> 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(-)

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

Re: [PATCH v2 00/10] block: Make raw_co_get_allocated_file_size asynchronous

[Fabiano Rosas]

Fabiano Rosas <farosas@suse.de> writes:

> Hi,
>
> The major change from the last version is that this time I'm moving
> all of the callers of bdrv_get_allocated_file_size() into
> coroutines. I had to make some temporary changes to avoid asserts
> while not all the callers were converted.
>
> I tried my best to explain why I think the changes are safe. To avoid
> changing too much of the code I added a change that removes the
> dependency of qmp_query_block from hmp_nbd_server_start, that way I
> don't need to move all of the nbd code into a coroutine as well.
>
> Based on:
>  [PATCH v2 00/11] block: Re-enable the graph lock
>  https://lore.kernel.org/r/20230605085711.21261-1-kwolf@redhat.com
>
> changes:
>
>   - fixed duplicated commit message [Lin]
>   - clarified why we need to convert info-block [Claudio]
>   - added my rationale of why the changes are safe [Eric]
>   - converted all callers to coroutines [Kevin]
>   - made hmp_nbd_server_start don't depend on qmp_query_block
>
> CI run: https://gitlab.com/farosas/qemu/-/pipelines/895525156

Ping, this seems to have fallen through the cracks.

Re: [PATCH v2 00/10] block: Make raw_co_get_allocated_file_size asynchronous

[Claudio Fontana]

Hi all,

we currently have to maintain something downstream for this, since the current behavior can compound problems on top of existing bad NFS latency,
could someone continue to help reviewing this work?

Thanks,

Claudio


On 6/9/23 22:19, Fabiano Rosas wrote:
> Hi,
> 
> The major change from the last version is that this time I'm moving
> all of the callers of bdrv_get_allocated_file_size() into
> coroutines. I had to make some temporary changes to avoid asserts
> while not all the callers were converted.
> 
> I tried my best to explain why I think the changes are safe. To avoid
> changing too much of the code I added a change that removes the
> dependency of qmp_query_block from hmp_nbd_server_start, that way I
> don't need to move all of the nbd code into a coroutine as well.
> 
> Based on:
>  [PATCH v2 00/11] block: Re-enable the graph lock
>  https://lore.kernel.org/r/20230605085711.21261-1-kwolf@redhat.com
> 
> changes:
> 
>   - fixed duplicated commit message [Lin]
>   - clarified why we need to convert info-block [Claudio]
>   - added my rationale of why the changes are safe [Eric]
>   - converted all callers to coroutines [Kevin]
>   - made hmp_nbd_server_start don't depend on qmp_query_block
> 
> CI run: https://gitlab.com/farosas/qemu/-/pipelines/895525156
> ===
> v1:
> https://lore.kernel.org/r/20230523213903.18418-1-farosas@suse.de
> 
> As discussed in another thread [1], here's an RFC addressing a VCPU
> softlockup encountered when issuing QMP commands that target a disk
> placed on NFS.
> 
> Since QMP commands happen with the qemu_global_mutex locked, any
> command that takes too long to finish will block other threads waiting
> to take the global mutex. One such thread could be a VCPU thread going
> out of the guest to handle IO.
> 
> This is the case when issuing the QMP command query-block, which
> eventually calls raw_co_get_allocated_file_size(). This function makes
> an 'fstat' call that has been observed to take a long time (seconds)
> over NFS.
> 
> NFS latency issues aside, we can improve the situation by not blocking
> VCPU threads while the command is running.
> 
> Move the 'fstat' call into the thread-pool and make the necessary
> adaptations to ensure raw_co_get_allocated_file_size runs in a
> coroutine in the block driver aio_context.
> 
> 1- Question about QMP and BQL
> https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg03141.html
> 
> Fabiano Rosas (8):
>   block: Remove bdrv_query_block_node_info
>   block: Remove unnecessary variable in bdrv_block_device_info
>   block: Allow the wrapper script to see functions declared in qapi.h
>   block: Temporarily mark bdrv_co_get_allocated_file_size as mixed
>   block: Convert bdrv_query_block_graph_info to coroutine
>   block: Convert bdrv_block_device_info into co_wrapper
>   block: Don't query all block devices at hmp_nbd_server_start
>   block: Convert qmp_query_block() to coroutine_fn
> 
> João Silva (1):
>   block: Add a thread-pool version of fstat
> 
> Lin Ma (1):
>   block: Convert qmp_query_named_block_nodes to coroutine
> 
>  block/file-posix.c                 | 40 +++++++++++++++++--
>  block/meson.build                  |  1 +
>  block/monitor/block-hmp-cmds.c     | 22 ++++++-----
>  block/qapi.c                       | 62 +++++++++---------------------
>  blockdev.c                         |  6 +--
>  hmp-commands-info.hx               |  1 +
>  include/block/block-hmp-cmds.h     |  2 +-
>  include/block/qapi.h               | 17 ++++----
>  include/block/raw-aio.h            |  4 +-
>  qapi/block-core.json               |  5 ++-
>  qemu-img.c                         |  2 -
>  scripts/block-coroutine-wrapper.py |  1 +
>  12 files changed, 90 insertions(+), 73 deletions(-)
>