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

[Kevin Wolf]

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.

>      } 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.

Kevin