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

[Charlie Shepherd]

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