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

[Charlie Shepherd <charlie@ctshepherd.com>]

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