qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [RFC PATCH 1/3] block: Factor out bdrv_run_co()


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [RFC PATCH 1/3] block: Factor out bdrv_run_co()
Date: Wed, 20 May 2020 14:14:19 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.8.0

20.05.2020 12:09, Philippe Mathieu-Daudé wrote:
On 5/12/20 5:37 PM, Eric Blake wrote:
On 5/12/20 9:43 AM, Kevin Wolf wrote:
We have a few bdrv_*() functions that can either spawn a new coroutine
and wait for it with BDRV_POLL_WHILE() or use a fastpath if they are
alreeady running in a coroutine. All of them duplicate basically the

already

same code.

Factor the common code into a new function bdrv_run_co().

Signed-off-by: Kevin Wolf <address@hidden>
---
  block/io.c | 104 +++++++++++++++--------------------------------------
  1 file changed, 28 insertions(+), 76 deletions(-)

diff --git a/block/io.c b/block/io.c
index 7d30e61edc..c1badaadc9 100644
--- a/block/io.c
+++ b/block/io.c
@@ -891,6 +891,22 @@ static int bdrv_check_byte_request(BlockDriverState *bs, 
int64_t offset,
      return 0;
  }
+static int bdrv_run_co(BlockDriverState *bs, CoroutineEntry *entry,
+                       void *opaque, int *ret)
+{
+    if (qemu_in_coroutine()) {
+        /* Fast-path if already in coroutine context */
+        entry(opaque);
+    } else {
+        Coroutine *co = qemu_coroutine_create(entry, opaque);
+        *ret = NOT_DONE;
+        bdrv_coroutine_enter(bs, co);
+        BDRV_POLL_WHILE(bs, *ret == NOT_DONE);

For my reference, NOT_DONE is defined as INT_MAX, which does not seem to be 
used as a return value in other situations.

@@ -923,25 +939,15 @@ static int bdrv_prwv_co(BdrvChild *child, int64_t offset,
                          QEMUIOVector *qiov, bool is_write,
                          BdrvRequestFlags flags)
  {
-    Coroutine *co;
      RwCo rwco = {
          .child = child,
          .offset = offset,
          .qiov = qiov,
          .is_write = is_write,
-        .ret = NOT_DONE,
          .flags = flags,
      };
-    if (qemu_in_coroutine()) {
-        /* Fast-path if already in coroutine context */
-        bdrv_rw_co_entry(&rwco);
-    } else {
-        co = qemu_coroutine_create(bdrv_rw_co_entry, &rwco);
-        bdrv_coroutine_enter(child->bs, co);
-        BDRV_POLL_WHILE(child->bs, rwco.ret == NOT_DONE);
-    }
-    return rwco.ret;
+    return bdrv_run_co(child->bs, bdrv_rw_co_entry, &rwco, &rwco.ret);

So code that previously looped on NOT_DONE is obviously safe, while...

  }
  int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
@@ -2230,7 +2236,6 @@ typedef struct BdrvCoBlockStatusData {
      int64_t *map;
      BlockDriverState **file;
      int ret;
-    bool done;
  } BdrvCoBlockStatusData;
  int coroutine_fn bdrv_co_block_status_from_file(BlockDriverState *bs,
@@ -2492,7 +2497,6 @@ static void coroutine_fn 
bdrv_block_status_above_co_entry(void *opaque)
                                             data->want_zero,
                                             data->offset, data->bytes,
                                             data->pnum, data->map, data->file);
-    data->done = true;
      aio_wait_kick();

...code that looped on something else now has to be checked that data->ret is 
still being set to something useful.  Fortunately that is true here.

@@ -2669,22 +2663,13 @@ static inline int
  bdrv_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
                  bool is_read)
  {
-    if (qemu_in_coroutine()) {
-        return bdrv_co_rw_vmstate(bs, qiov, pos, is_read);
-    } else {
-        BdrvVmstateCo data = {
-            .bs         = bs,
-            .qiov       = qiov,
-            .pos        = pos,
-            .is_read    = is_read,
-            .ret        = -EINPROGRESS,
-        };
-        Coroutine *co = qemu_coroutine_create(bdrv_co_rw_vmstate_entry, &data);
-
-        bdrv_coroutine_enter(bs, co);
-        BDRV_POLL_WHILE(bs, data.ret == -EINPROGRESS);
-        return data.ret;

It's a little harder to see whether -EINPROGRESS might ever be returned by a 
driver, but again this looks safe.

Maybe add a comment regarding -EINPROGRESS before calling bdrv_run_co() in 
bdrv_rw_vmstate()?

Reviewed-by: Philippe Mathieu-Daudé <address@hidden>


Hi!

Actually, I've sent a v2 of this patch:
"[PATCH v2] block: Factor out bdrv_run_co()"
https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg05437.html


--
Best regards,
Vladimir



reply via email to

[Prev in Thread] Current Thread [Next in Thread]