qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH 3/5] Convert BlockDriver to explicit coroutine a


From: Charlie Shepherd
Subject: Re: [Qemu-devel] [PATCH 3/5] Convert BlockDriver to explicit coroutine annotations
Date: Thu, 08 Aug 2013 02:14:04 +0100
User-agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/20130620 Thunderbird/17.0.7

On 06/08/2013 10:24, Kevin Wolf wrote:
Am 05.08.2013 um 20:44 hat Charlie Shepherd geschrieben:
This patch converts the .bdrv_open, .bdrv_file_open and .bdrv_create members of 
struct BlockDriver
to be explicitly annotated as coroutine_fn, rather than yielding dynamically 
depending on whether
they are executed in a coroutine context or not.
I wonder whether this patch should be split into one for each converted
BlockDriver function. It would probably make review easier.

For those function where you actually just correct the coroutine_fn
annotation, but whose behaviour doesn't change, a single patch for all
might be enough.

  block.c                       | 16 +++++++--------
  block/blkdebug.c              | 10 ++++-----
  block/blkverify.c             |  4 ++--
  block/bochs.c                 |  8 ++++----
  block/cloop.c                 |  6 +++---
  block/cow.c                   | 12 +++++------
  block/curl.c                  | 12 +++++------
  block/dmg.c                   |  6 +++---
  block/nbd.c                   | 28 ++++++++++++-------------
  block/parallels.c             |  6 +++---
  block/qcow.c                  |  8 ++++----
  block/qcow2-cluster.c         |  8 ++++----
  block/qcow2.c                 | 48 +++++++++++++++++++++++++++++++++++++------
  block/qcow2.h                 |  4 ++--
  block/qed.c                   |  8 ++++----
  block/raw-posix.c             | 34 +++++++++++++++---------------
  block/raw.c                   |  8 ++++----
  block/sheepdog.c              | 24 +++++++++++-----------
  block/snapshot.c              | 32 ++++++++++++++++++++++++++++-
  block/ssh.c                   | 14 ++++++-------
  block/vdi.c                   | 12 +++++------
  block/vhdx.c                  |  4 ++--
  block/vmdk.c                  | 12 +++++------
  block/vpc.c                   | 12 +++++------
  block/vvfat.c                 | 12 +++++------
  include/block/block_int.h     | 10 ++++-----
  include/block/coroutine.h     |  4 ++--
  include/block/coroutine_int.h |  2 +-
  qemu-coroutine-lock.c         |  4 ++--
  30 files changed, 218 insertions(+), 152 deletions(-)

diff --git a/block.c b/block.c
index 6c493ad..aaa122c 100644
--- a/block.c
+++ b/block.c
@@ -374,7 +374,7 @@ static void coroutine_fn bdrv_create_co_entry(void *opaque)
      CreateCo *cco = opaque;
      assert(cco->drv);
- cco->ret = cco->drv->bdrv_create(cco->filename, cco->options);
+    cco->ret = cco->drv->bdrv_co_create(cco->filename, cco->options);
  }
int bdrv_create(BlockDriver *drv, const char* filename,
@@ -390,7 +390,7 @@ int bdrv_create(BlockDriver *drv, const char* filename,
          .ret = NOT_DONE,
      };
- if (!drv->bdrv_create) {
+    if (!drv->bdrv_co_create) {
          ret = -ENOTSUP;
          goto out;
      }
@@ -697,7 +697,7 @@ static int bdrv_open_common(BlockDriverState *bs, 
BlockDriverState *file,
      /* bdrv_open() with directly using a protocol as drv. This layer is 
already
       * opened, so assign it to bs (while file becomes a closed 
BlockDriverState)
       * and return immediately. */
-    if (file != NULL && drv->bdrv_file_open) {
+    if (file != NULL && drv->bdrv_co_file_open) {
          bdrv_swap(file, bs);
          return 0;
      }
@@ -728,10 +728,10 @@ static int bdrv_open_common(BlockDriverState *bs, 
BlockDriverState *file,
      bs->enable_write_cache = !!(flags & BDRV_O_CACHE_WB);
/* Open the image, either directly or using a protocol */
-    if (drv->bdrv_file_open) {
+    if (drv->bdrv_co_file_open) {
          assert(file == NULL);
          assert(drv->bdrv_parse_filename || filename != NULL);
-        ret = drv->bdrv_file_open(bs, options, open_flags);
+        ret = drv->bdrv_co_file_open(bs, options, open_flags);
bdrv_open_common() isn't coroutine_fn, though?

Ah well, I see that you change it in a later patch. Please make sure
that the code is in a consistent state after each single commit.

For this series, I think this suggests that you indeed split by converted
function, but put everything related to this function in one patch. For
example, the bdrv_open_common() conversion would be in the same patch as
this hunk.

Yeah, it's in this kind of scenario that I struggled to split up the patch series properly. I'll try and implement your approach in the next version.

      } else {
          if (file == NULL) {
              qerror_report(ERROR_CLASS_GENERIC_ERROR, "Can't use '%s' as a "
@@ -742,7 +742,7 @@ static int bdrv_open_common(BlockDriverState *bs, 
BlockDriverState *file,
          }
          assert(file != NULL);
          bs->file = file;
-        ret = drv->bdrv_open(bs, options, open_flags);
+        ret = drv->bdrv_co_open(bs, options, open_flags);
      }
if (ret < 0) {
diff --git a/block/qcow2.c b/block/qcow2.c
index 0eceefe..2ed0bb6 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -58,6 +58,10 @@ typedef struct {
  #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
  #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
+
+#define NOT_DONE 0x7fffffff
+
+
  static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
  {
      const QCowHeader *cow_header = (const void *)buf;
@@ -315,7 +319,7 @@ static QemuOptsList qcow2_runtime_opts = {
      },
  };
-static int qcow2_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn qcow2_co_open(BlockDriverState *bs, QDict *options, 
int flags)
  {
      BDRVQcowState *s = bs->opaque;
      int len, i, ret = 0;
@@ -590,6 +594,38 @@ static int qcow2_open(BlockDriverState *bs, QDict 
*options, int flags)
      return ret;
  }
+struct QOpenCo {
+    BlockDriverState *bs;
+    QDict *options;
+    int flags;
+    int ret;
+};
+
+static void coroutine_fn qcow2_co_open_entry(void *opaque)
+{
+    struct QOpenCo *qo = opaque;
+
+    qo->ret = qcow2_co_open(qo->bs, qo->options, qo->flags);
+}
+
+static int qcow2_open(BlockDriverState *bs, QDict *options, int flags)
+{
+    Coroutine *co;
+    struct QOpenCo qo = {
+        .bs = bs,
+        .options = options,
+        .flags = flags,
+        .ret = NOT_DONE,
+    };
+
+    co = qemu_coroutine_create(qcow2_co_open_entry);
+    qemu_coroutine_enter(co, &qo);
+    while (qo.ret == NOT_DONE) {
+        qemu_aio_wait();
+    }
+    return qo.ret;
+}
I think it would be better to convert qcow2_invalidate_cache() to
coroutine_fn (which you apparently do in patch 4) so that it can
directly call qcow2_co_open, and to keep this coroutine wrapper in
the block.c function.

Ok I'll make this change.


Charlie




reply via email to

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