diff mbox series

[v5,01/15] block-io: introduce coroutine_fn duplicates for bdrv_common_block_status_above callers

Message ID 20221123114227.85757-2-eesposit@redhat.com (mailing list archive)
State New, archived
Headers show
Series Still more coroutine and various fixes in block layer | expand

Commit Message

Emanuele Giuseppe Esposito Nov. 23, 2022, 11:42 a.m. UTC 
bdrv_common_block_status_above() is a g_c_w, and it is being called by
many "wrapper" functions like bdrv_is_allocated(),
bdrv_is_allocated_above() and bdrv_block_status_above().

Because we want to eventually split the coroutine from non-coroutine
case in g_c_w, create duplicate wrappers that take care of directly
calling the same coroutine functions called in the g_c_w.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block/io.c               | 64 ++++++++++++++++++++++++++++++++++++++--
 include/block/block-io.h | 15 ++++++++++
 2 files changed, 76 insertions(+), 3 deletions(-)

Comments

Kevin Wolf Nov. 23, 2022, 4:29 p.m. UTC | #1 
Am 23.11.2022 um 12:42 hat Emanuele Giuseppe Esposito geschrieben:
> bdrv_common_block_status_above() is a g_c_w, and it is being called by
> many "wrapper" functions like bdrv_is_allocated(),
> bdrv_is_allocated_above() and bdrv_block_status_above().
> 
> Because we want to eventually split the coroutine from non-coroutine
> case in g_c_w, create duplicate wrappers that take care of directly
> calling the same coroutine functions called in the g_c_w.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  block/io.c               | 64 ++++++++++++++++++++++++++++++++++++++--
>  include/block/block-io.h | 15 ++++++++++
>  2 files changed, 76 insertions(+), 3 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 38e57d1f67..1bc05c8282 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -2533,6 +2533,19 @@ bdrv_co_common_block_status_above(BlockDriverState *bs,
>      return ret;
>  }
>  
> +int coroutine_fn bdrv_co_block_status_above(BlockDriverState *bs,
> +                                            BlockDriverState *base,
> +                                            int64_t offset, int64_t bytes,
> +                                            int64_t *pnum, int64_t *map,
> +                                            BlockDriverState **file)
> +{
> +    IO_CODE();
> +    /* If QEMU_IN_COROUTINE() fails, use bdrv_block_status_above() */
> +    QEMU_IN_COROUTINE();

This is an obvious patch order problem: The macro doesn't even exist
yet.

As I said, personally, I don't feel like putting QEMU_IN_COROUTINE()
assertions into every coroutine_fn is a useful thing to do. Static
analysis (maybe even with something vrc based in 'make check'? Paolo,
would this be realistic?) seems much preferable. But I'd like to hear
other opinions on this, too.

I feel the same way about the comment. Yes, of course, if you're not in
a coroutine, don't call the _co variant of a function, but the one
without it. But that goes without saying, doesn't it?

> +    return bdrv_co_common_block_status_above(bs, base, false, true, offset,
> +                                             bytes, pnum, map, file, NULL);
> +}

Apart from these considerations the patch looks right.

Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Paolo Bonzini Nov. 24, 2022, 7:04 a.m. UTC | #2 
Il mer 23 nov 2022, 17:29 Kevin Wolf <kwolf@redhat.com> ha scritto:

> As I said, personally, I don't feel like putting QEMU_IN_COROUTINE()
> assertions into every coroutine_fn is a useful thing to do. Static
> analysis (maybe even with something vrc based in 'make check'? Paolo,
> would this be realistic?)


Yes, using pyclang just to build the call graph should be much much faster
than doing all the static analysis. So it should be feasible to integrate
it with "make check". We can decide whether to bundle vrc, which is just a
dozen files, or install it from pypi (sooner or later configure will have
to be taught about that, too).

Paolo

>
diff mbox series

Patch

diff --git a/block/io.c b/block/io.c
index 38e57d1f67..1bc05c8282 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2533,6 +2533,19 @@  bdrv_co_common_block_status_above(BlockDriverState *bs,
     return ret;
 }
 
+int coroutine_fn bdrv_co_block_status_above(BlockDriverState *bs,
+                                            BlockDriverState *base,
+                                            int64_t offset, int64_t bytes,
+                                            int64_t *pnum, int64_t *map,
+                                            BlockDriverState **file)
+{
+    IO_CODE();
+    /* If QEMU_IN_COROUTINE() fails, use bdrv_block_status_above() */
+    QEMU_IN_COROUTINE();
+    return bdrv_co_common_block_status_above(bs, base, false, true, offset,
+                                             bytes, pnum, map, file, NULL);
+}
+
 int bdrv_block_status_above(BlockDriverState *bs, BlockDriverState *base,
                             int64_t offset, int64_t bytes, int64_t *pnum,
                             int64_t *map, BlockDriverState **file)
@@ -2578,6 +2591,24 @@  int coroutine_fn bdrv_co_is_zero_fast(BlockDriverState *bs, int64_t offset,
     return (pnum == bytes) && (ret & BDRV_BLOCK_ZERO);
 }
 
+int coroutine_fn bdrv_co_is_allocated(BlockDriverState *bs, int64_t offset,
+                                      int64_t bytes, int64_t *pnum)
+{
+    int ret;
+    int64_t dummy;
+    IO_CODE();
+    /* If QEMU_IN_COROUTINE() fails, use bdrv_is_allocated() */
+    QEMU_IN_COROUTINE();
+
+    ret = bdrv_co_common_block_status_above(bs, bs, true, false, offset,
+                                            bytes, pnum ? pnum : &dummy, NULL,
+                                            NULL, NULL);
+    if (ret < 0) {
+        return ret;
+    }
+    return !!(ret & BDRV_BLOCK_ALLOCATED);
+}
+
 int bdrv_is_allocated(BlockDriverState *bs, int64_t offset, int64_t bytes,
                       int64_t *pnum)
 {
@@ -2594,6 +2625,31 @@  int bdrv_is_allocated(BlockDriverState *bs, int64_t offset, int64_t bytes,
     return !!(ret & BDRV_BLOCK_ALLOCATED);
 }
 
+/* See bdrv_is_allocated_above for documentation */
+int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top,
+                                            BlockDriverState *base,
+                                            bool include_base, int64_t offset,
+                                            int64_t bytes, int64_t *pnum)
+{
+    int depth;
+    int ret;
+    IO_CODE();
+    /* If QEMU_IN_COROUTINE() fails, use bdrv_is_allocated_above() */
+    QEMU_IN_COROUTINE();
+
+    ret = bdrv_co_common_block_status_above(top, base, include_base, false,
+                                            offset, bytes, pnum, NULL, NULL,
+                                            &depth);
+    if (ret < 0) {
+        return ret;
+    }
+
+    if (ret & BDRV_BLOCK_ALLOCATED) {
+        return depth;
+    }
+    return 0;
+}
+
 /*
  * Given an image chain: ... -> [BASE] -> [INTER1] -> [INTER2] -> [TOP]
  *
@@ -2617,10 +2673,12 @@  int bdrv_is_allocated_above(BlockDriverState *top,
                             int64_t bytes, int64_t *pnum)
 {
     int depth;
-    int ret = bdrv_common_block_status_above(top, base, include_base, false,
-                                             offset, bytes, pnum, NULL, NULL,
-                                             &depth);
+    int ret;
     IO_CODE();
+
+    ret = bdrv_common_block_status_above(top, base, include_base, false,
+                                         offset, bytes, pnum, NULL, NULL,
+                                         &depth);
     if (ret < 0) {
         return ret;
     }
diff --git a/include/block/block-io.h b/include/block/block-io.h
index 92aaa7c1e9..72919254cd 100644
--- a/include/block/block-io.h
+++ b/include/block/block-io.h
@@ -94,14 +94,29 @@  bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs);
 int bdrv_block_status(BlockDriverState *bs, int64_t offset,
                       int64_t bytes, int64_t *pnum, int64_t *map,
                       BlockDriverState **file);
+
+int coroutine_fn bdrv_co_block_status_above(BlockDriverState *bs,
+                                            BlockDriverState *base,
+                                            int64_t offset, int64_t bytes,
+                                            int64_t *pnum, int64_t *map,
+                                            BlockDriverState **file);
 int bdrv_block_status_above(BlockDriverState *bs, BlockDriverState *base,
                             int64_t offset, int64_t bytes, int64_t *pnum,
                             int64_t *map, BlockDriverState **file);
+
+int coroutine_fn bdrv_co_is_allocated(BlockDriverState *bs, int64_t offset,
+                                      int64_t bytes, int64_t *pnum);
 int bdrv_is_allocated(BlockDriverState *bs, int64_t offset, int64_t bytes,
                       int64_t *pnum);
+
+int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top,
+                                            BlockDriverState *base,
+                                            bool include_base, int64_t offset,
+                                            int64_t bytes, int64_t *pnum);
 int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
                             bool include_base, int64_t offset, int64_t bytes,
                             int64_t *pnum);
+
 int coroutine_fn bdrv_co_is_zero_fast(BlockDriverState *bs, int64_t offset,
                                       int64_t bytes);