diff mbox

[3/5] Convert BlockDriver to explicit coroutine annotations

Message ID 1375728247-1306-4-git-send-email-charlie@ctshepherd.com
State New
Headers show

Commit Message

Charlie Shepherd Aug. 5, 2013, 6:44 p.m. UTC 
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.
---
 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(-)

Comments

Gabriel Kerneis Aug. 5, 2013, 7:23 p.m. UTC | #1 
Hi Charlie,

Many thanks for this patch series.

On Mon, Aug 05, 2013 at 08:44:05PM +0200, Charlie Shepherd wrote:
> 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.

The commit message is incomplete. You also convert brdv_read and brdv_write.

Moreover:

> diff --git a/block/ssh.c b/block/ssh.c
> index d7e7bf8..66ac4c1 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -748,7 +748,7 @@ static int return_true(void *opaque)
>      return 1;
>  }
>  
> -static coroutine_fn void set_fd_handler(BDRVSSHState *s)
> +static void set_fd_handler(BDRVSSHState *s)
>  {
>      int r;
>      IOHandler *rd_handler = NULL, *wr_handler = NULL;
> @@ -769,7 +769,7 @@ static coroutine_fn void set_fd_handler(BDRVSSHState *s)
>      qemu_aio_set_fd_handler(s->sock, rd_handler, wr_handler, return_true, co);
>  }
>  
> -static coroutine_fn void clear_fd_handler(BDRVSSHState *s)
> +static void clear_fd_handler(BDRVSSHState *s)
>  {
>      DPRINTF("s->sock=%d", s->sock);
>      qemu_aio_set_fd_handler(s->sock, NULL, NULL, NULL, NULL);
> diff --git a/include/block/coroutine_int.h b/include/block/coroutine_int.h
> index f133d65..d0ab27d 100644
> --- a/include/block/coroutine_int.h
> +++ b/include/block/coroutine_int.h
> @@ -48,6 +48,6 @@ Coroutine *qemu_coroutine_new(void);
>  void qemu_coroutine_delete(Coroutine *co);
>  CoroutineAction qemu_coroutine_switch(Coroutine *from, Coroutine *to,
>                                        CoroutineAction action);
> -void coroutine_fn qemu_co_queue_run_restart(Coroutine *co);
> +void qemu_co_queue_run_restart(Coroutine *co);
>  
>  #endif

Adding coroutine_fn where it is necessary seems straightforward to me: just
follow the "callers of coroutine_fn should be coroutine_fn" rule (assuming you
got it right and did not over-annotate). On the other hand, you
should probably explain in the commit message why you *remove* those three
coroutine_fn annotations.

Best,
Charlie Shepherd Aug. 5, 2013, 7:33 p.m. UTC | #2 
On 05/08/2013 20:23, Gabriel Kerneis wrote:
> On Mon, Aug 05, 2013 at 08:44:05PM +0200, Charlie Shepherd wrote:
>> 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.
> The commit message is incomplete. You also convert brdv_read and brdv_write.

Thanks, I do need to mention this.

> Moreover:
>
>> diff --git a/block/ssh.c b/block/ssh.c
>> index d7e7bf8..66ac4c1 100644
>> --- a/block/ssh.c
>> +++ b/block/ssh.c
>> @@ -748,7 +748,7 @@ static int return_true(void *opaque)
>>       return 1;
>>   }
>>   
>> -static coroutine_fn void set_fd_handler(BDRVSSHState *s)
>> +static void set_fd_handler(BDRVSSHState *s)
>>   {
>>       int r;
>>       IOHandler *rd_handler = NULL, *wr_handler = NULL;
>> @@ -769,7 +769,7 @@ static coroutine_fn void set_fd_handler(BDRVSSHState *s)
>>       qemu_aio_set_fd_handler(s->sock, rd_handler, wr_handler, return_true, co);
>>   }
>>   
>> -static coroutine_fn void clear_fd_handler(BDRVSSHState *s)
>> +static void clear_fd_handler(BDRVSSHState *s)
>>   {
>>       DPRINTF("s->sock=%d", s->sock);
>>       qemu_aio_set_fd_handler(s->sock, NULL, NULL, NULL, NULL);
>> diff --git a/include/block/coroutine_int.h b/include/block/coroutine_int.h
>> index f133d65..d0ab27d 100644
>> --- a/include/block/coroutine_int.h
>> +++ b/include/block/coroutine_int.h
>> @@ -48,6 +48,6 @@ Coroutine *qemu_coroutine_new(void);
>>   void qemu_coroutine_delete(Coroutine *co);
>>   CoroutineAction qemu_coroutine_switch(Coroutine *from, Coroutine *to,
>>                                         CoroutineAction action);
>> -void coroutine_fn qemu_co_queue_run_restart(Coroutine *co);
>> +void qemu_co_queue_run_restart(Coroutine *co);
>>   
>>   #endif
> Adding coroutine_fn where it is necessary seems straightforward to me: just
> follow the "callers of coroutine_fn should be coroutine_fn" rule (assuming you
> got it right and did not over-annotate). On the other hand, you
> should probably explain in the commit message why you *remove* those three
> coroutine_fn annotations.

Yes that does merit some explanation.

Building the tree with cps inference warned that these functions were 
annotated spuriously. I initially thought this was because they called a 
coroutine function that hadn't been annotated, but it seems the 
*_handler functions called qemu_aio_set_fd_handler which, from my 
investigation, it seems does not need annotating. Therefore they were 
indeed spuriously annotated and so I removed the annotation.

qemu_co_queue_run_restart is a bit different. It is only called from 
coroutine_swap in qemu-coroutine.c, and it enters coroutines that were 
waiting but have now moved to the runnable state by the actions of the 
most recent coroutine (I believe). I think we discussed this on IRC on 
Thursday? It only calls qemu_coroutine_enter, and cannot yield, so again 
I removed the annotation. I'll add these explanations to the commit message.


Thanks,

Charlie
Gabriel Kerneis Aug. 5, 2013, 8:05 p.m. UTC | #3 
On Mon, Aug 05, 2013 at 08:33:10PM +0100, Charlie Shepherd wrote:
> Yes that does merit some explanation.

Thanks for the details.

> qemu_co_queue_run_restart is a bit different. It is only called from
> coroutine_swap in qemu-coroutine.c, and it enters coroutines that
> were waiting but have now moved to the runnable state by the actions
> of the most recent coroutine (I believe). I think we discussed this
> on IRC on Thursday? It only calls qemu_coroutine_enter, and cannot
> yield, so again I removed the annotation. I'll add these
> explanations to the commit message.

Or maybe split those in different commits altogether? I find it often easier to
review many small commits than a big one, each focused on an "atomic" change.
Kevin Wolf Aug. 6, 2013, 9:04 a.m. UTC | #4 
Am 05.08.2013 um 22:05 hat Gabriel Kerneis geschrieben:
> On Mon, Aug 05, 2013 at 08:33:10PM +0100, Charlie Shepherd wrote:
> > Yes that does merit some explanation.
> 
> Thanks for the details.
> 
> > qemu_co_queue_run_restart is a bit different. It is only called from
> > coroutine_swap in qemu-coroutine.c, and it enters coroutines that
> > were waiting but have now moved to the runnable state by the actions
> > of the most recent coroutine (I believe). I think we discussed this
> > on IRC on Thursday? It only calls qemu_coroutine_enter, and cannot
> > yield, so again I removed the annotation. I'll add these
> > explanations to the commit message.
> 
> Or maybe split those in different commits altogether? I find it often easier to
> review many small commits than a big one, each focused on an "atomic" change.

Yes, please. One logical change per commit, and no changes that aren't
mentioned in the commit message.

Kevin
Kevin Wolf Aug. 6, 2013, 9:24 a.m. UTC | #5 
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
Stefan Hajnoczi Aug. 7, 2013, 7:30 p.m. UTC | #6 
On Mon, Aug 05, 2013 at 08:33:10PM +0100, Charlie Shepherd wrote:
> On 05/08/2013 20:23, Gabriel Kerneis wrote:
> >On Mon, Aug 05, 2013 at 08:44:05PM +0200, Charlie Shepherd wrote:
> >>diff --git a/include/block/coroutine_int.h b/include/block/coroutine_int.h
> >>index f133d65..d0ab27d 100644
> >>--- a/include/block/coroutine_int.h
> >>+++ b/include/block/coroutine_int.h
> >>@@ -48,6 +48,6 @@ Coroutine *qemu_coroutine_new(void);
> >>  void qemu_coroutine_delete(Coroutine *co);
> >>  CoroutineAction qemu_coroutine_switch(Coroutine *from, Coroutine *to,
> >>                                        CoroutineAction action);
> >>-void coroutine_fn qemu_co_queue_run_restart(Coroutine *co);
> >>+void qemu_co_queue_run_restart(Coroutine *co);
> >>  #endif
> >Adding coroutine_fn where it is necessary seems straightforward to me: just
> >follow the "callers of coroutine_fn should be coroutine_fn" rule (assuming you
> >got it right and did not over-annotate). On the other hand, you
> >should probably explain in the commit message why you *remove* those three
> >coroutine_fn annotations.
> 
> Yes that does merit some explanation.
> 
> Building the tree with cps inference warned that these functions
> were annotated spuriously. I initially thought this was because they
> called a coroutine function that hadn't been annotated, but it seems
> the *_handler functions called qemu_aio_set_fd_handler which, from
> my investigation, it seems does not need annotating. Therefore they
> were indeed spuriously annotated and so I removed the annotation.
> 
> qemu_co_queue_run_restart is a bit different. It is only called from
> coroutine_swap in qemu-coroutine.c, and it enters coroutines that
> were waiting but have now moved to the runnable state by the actions
> of the most recent coroutine (I believe). I think we discussed this
> on IRC on Thursday? It only calls qemu_coroutine_enter, and cannot
> yield, so again I removed the annotation. I'll add these
> explanations to the commit message.

I have mixed feelings about removing coroutine_fn annotations from a
function when it does not yield or call other coroutine_fn functions.

These functions were probably written as part of a coroutine code path.
The coroutine_fn annotation tells me I'm in coroutine context.

By removing this information those modifying the code now need to
convert it back to coroutine_fn after auditing callers before they can
use coroutine context.

The thing is, these leaf functions are typically only called from
coroutine context anyway.  I think they should be left marked
coroutine_fn.

I'd compare this to a comment that says "lock foo is held across this
function" but the function doesn't use anything that lock foo protects.
Removing the comment isn't really helpful, you are throwing away
information that can be useful when modifying the function.

Stefan
Charlie Shepherd Aug. 8, 2013, 1:14 a.m. UTC | #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
Charlie Shepherd Aug. 8, 2013, 1:31 a.m. UTC | #8 
On 07/08/2013 20:30, Stefan Hajnoczi wrote:
> On Mon, Aug 05, 2013 at 08:33:10PM +0100, Charlie Shepherd wrote:
>> On 05/08/2013 20:23, Gabriel Kerneis wrote:
>>> On Mon, Aug 05, 2013 at 08:44:05PM +0200, Charlie Shepherd wrote:
>>>> diff --git a/include/block/coroutine_int.h b/include/block/coroutine_int.h
>>>> index f133d65..d0ab27d 100644
>>>> --- a/include/block/coroutine_int.h
>>>> +++ b/include/block/coroutine_int.h
>>>> @@ -48,6 +48,6 @@ Coroutine *qemu_coroutine_new(void);
>>>>   void qemu_coroutine_delete(Coroutine *co);
>>>>   CoroutineAction qemu_coroutine_switch(Coroutine *from, Coroutine *to,
>>>>                                         CoroutineAction action);
>>>> -void coroutine_fn qemu_co_queue_run_restart(Coroutine *co);
>>>> +void qemu_co_queue_run_restart(Coroutine *co);
>>>>   #endif
>>> Adding coroutine_fn where it is necessary seems straightforward to me: just
>>> follow the "callers of coroutine_fn should be coroutine_fn" rule (assuming you
>>> got it right and did not over-annotate). On the other hand, you
>>> should probably explain in the commit message why you *remove* those three
>>> coroutine_fn annotations.
>> Yes that does merit some explanation.
>>
>> Building the tree with cps inference warned that these functions
>> were annotated spuriously. I initially thought this was because they
>> called a coroutine function that hadn't been annotated, but it seems
>> the *_handler functions called qemu_aio_set_fd_handler which, from
>> my investigation, it seems does not need annotating. Therefore they
>> were indeed spuriously annotated and so I removed the annotation.
>>
>> qemu_co_queue_run_restart is a bit different. It is only called from
>> coroutine_swap in qemu-coroutine.c, and it enters coroutines that
>> were waiting but have now moved to the runnable state by the actions
>> of the most recent coroutine (I believe). I think we discussed this
>> on IRC on Thursday? It only calls qemu_coroutine_enter, and cannot
>> yield, so again I removed the annotation. I'll add these
>> explanations to the commit message.
> I have mixed feelings about removing coroutine_fn annotations from a
> function when it does not yield or call other coroutine_fn functions.
>
> These functions were probably written as part of a coroutine code path.
> The coroutine_fn annotation tells me I'm in coroutine context.
>
> By removing this information those modifying the code now need to
> convert it back to coroutine_fn after auditing callers before they can
> use coroutine context.
>
> The thing is, these leaf functions are typically only called from
> coroutine context anyway.  I think they should be left marked
> coroutine_fn.
>
> I'd compare this to a comment that says "lock foo is held across this
> function" but the function doesn't use anything that lock foo protects.
> Removing the comment isn't really helpful, you are throwing away
> information that can be useful when modifying the function.

I see, so in that case coroutine_fn is more a contract saying "this 
function can only be, and is only, called from a coroutine context"? 
That feels fair enough, but I'll amend the comment in coroutine.h to 
that effect.


Charlie
Gabriel Kerneis Aug. 8, 2013, 6:27 a.m. UTC | #9 
On Wed, Aug 07, 2013 at 09:30:25PM +0200, Stefan Hajnoczi wrote:
> I have mixed feelings about removing coroutine_fn annotations from a
> function when it does not yield or call other coroutine_fn functions.
> 
> These functions were probably written as part of a coroutine code path.
> The coroutine_fn annotation tells me I'm in coroutine context.

No, it tells you it is forbidden to call this function from outside coroutine
context. Which is false: if the function never yields, it is definitely correct
to call from somewhere else (unless there is some other invariant in qemu about
coroutine context?).

> By removing this information those modifying the code now need to
> convert it back to coroutine_fn after auditing callers before they can
> use coroutine context.

I don't understand this. You mean someone who, later, would decide to make a
version of the function that yields? In that case, wouldn't it make sense to
introduce an alternative coroutine_fn counter-part? (I see, however, how line of
reasoning might have led to the "dynamic functions" maze).

> I'd compare this to a comment that says "lock foo is held across this
> function" but the function doesn't use anything that lock foo protects.
> Removing the comment isn't really helpful, you are throwing away
> information that can be useful when modifying the function.

Except that the function might also makes sense outside of coroutine context,
and you are forcing people to allocate a spurious coroutine if they want to
use it.

Note that I'm arguing for the sake of defining precisely what Qemu developpers
expect when they read or write "coroutine_fn". As long as we can agree on that
point, and get it documented in coroutine.h, I'm fine.  From a performance
point-of-view, it matters only for the CPC backend and there aren't that many
spuriously-annotated functions anyway.  We could even benchmark the overhead at
the end of the GSoC, but I don't expect it to be significant (if there were
dozens of them, it would be a different story).

Best,
diff mbox

Patch

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);
     } 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) {
@@ -3759,9 +3759,9 @@  static BlockDriverAIOCB *bdrv_aio_rw_vector(BlockDriverState *bs,
 
     if (is_write) {
         qemu_iovec_to_buf(acb->qiov, 0, acb->bounce, qiov->size);
-        acb->ret = bs->drv->bdrv_write(bs, sector_num, acb->bounce, nb_sectors);
+        acb->ret = bs->drv->bdrv_co_write(bs, sector_num, acb->bounce, nb_sectors);
     } else {
-        acb->ret = bs->drv->bdrv_read(bs, sector_num, acb->bounce, nb_sectors);
+        acb->ret = bs->drv->bdrv_co_read(bs, sector_num, acb->bounce, nb_sectors);
     }
 
     qemu_bh_schedule(acb->bh);
diff --git a/block/blkdebug.c b/block/blkdebug.c
index ccb627a..9dd5697 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -349,7 +349,7 @@  static QemuOptsList runtime_opts = {
     },
 };
 
-static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn blkdebug_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVBlkdebugState *s = bs->opaque;
     QemuOpts *opts;
@@ -494,7 +494,7 @@  static void blkdebug_close(BlockDriverState *bs)
     }
 }
 
-static void suspend_request(BlockDriverState *bs, BlkdebugRule *rule)
+static void coroutine_fn suspend_request(BlockDriverState *bs, BlkdebugRule *rule)
 {
     BDRVBlkdebugState *s = bs->opaque;
     BlkdebugSuspendedReq r;
@@ -515,7 +515,7 @@  static void suspend_request(BlockDriverState *bs, BlkdebugRule *rule)
     g_free(r.tag);
 }
 
-static bool process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,
+static bool coroutine_fn process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,
     bool injected)
 {
     BDRVBlkdebugState *s = bs->opaque;
@@ -546,7 +546,7 @@  static bool process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,
     return injected;
 }
 
-static void blkdebug_debug_event(BlockDriverState *bs, BlkDebugEvent event)
+static void coroutine_fn blkdebug_debug_event(BlockDriverState *bs, BlkDebugEvent event)
 {
     BDRVBlkdebugState *s = bs->opaque;
     struct BlkdebugRule *rule, *next;
@@ -626,7 +626,7 @@  static BlockDriver bdrv_blkdebug = {
     .instance_size          = sizeof(BDRVBlkdebugState),
 
     .bdrv_parse_filename    = blkdebug_parse_filename,
-    .bdrv_file_open         = blkdebug_open,
+    .bdrv_co_file_open      = blkdebug_open,
     .bdrv_close             = blkdebug_close,
     .bdrv_getlength         = blkdebug_getlength,
 
diff --git a/block/blkverify.c b/block/blkverify.c
index 1d58cc3..0b89cfe 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -116,7 +116,7 @@  static QemuOptsList runtime_opts = {
     },
 };
 
-static int blkverify_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn blkverify_co_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVBlkverifyState *s = bs->opaque;
     QemuOpts *opts;
@@ -405,7 +405,7 @@  static BlockDriver bdrv_blkverify = {
     .instance_size          = sizeof(BDRVBlkverifyState),
 
     .bdrv_parse_filename    = blkverify_parse_filename,
-    .bdrv_file_open         = blkverify_open,
+    .bdrv_co_file_open      = blkverify_co_open,
     .bdrv_close             = blkverify_close,
     .bdrv_getlength         = blkverify_getlength,
 
diff --git a/block/bochs.c b/block/bochs.c
index d7078c0..c827bd4 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -108,7 +108,7 @@  static int bochs_probe(const uint8_t *buf, int buf_size, const char *filename)
     return 0;
 }
 
-static int bochs_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn bochs_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVBochsState *s = bs->opaque;
     int i;
@@ -228,7 +228,7 @@  static coroutine_fn int bochs_co_read(BlockDriverState *bs, int64_t sector_num,
     return ret;
 }
 
-static void bochs_close(BlockDriverState *bs)
+static void coroutine_fn bochs_close(BlockDriverState *bs)
 {
     BDRVBochsState *s = bs->opaque;
     g_free(s->catalog_bitmap);
@@ -238,8 +238,8 @@  static BlockDriver bdrv_bochs = {
     .format_name	= "bochs",
     .instance_size	= sizeof(BDRVBochsState),
     .bdrv_probe		= bochs_probe,
-    .bdrv_open		= bochs_open,
-    .bdrv_read          = bochs_co_read,
+    .bdrv_co_open	= bochs_open,
+    .bdrv_co_read	= bochs_co_read,
     .bdrv_close		= bochs_close,
 };
 
diff --git a/block/cloop.c b/block/cloop.c
index 6ea7cf4..ef5555f 100644
--- a/block/cloop.c
+++ b/block/cloop.c
@@ -53,7 +53,7 @@  static int cloop_probe(const uint8_t *buf, int buf_size, const char *filename)
     return 0;
 }
 
-static int cloop_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn cloop_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVCloopState *s = bs->opaque;
     uint32_t offsets_size, max_compressed_block_size = 1, i;
@@ -191,8 +191,8 @@  static BlockDriver bdrv_cloop = {
     .format_name    = "cloop",
     .instance_size  = sizeof(BDRVCloopState),
     .bdrv_probe     = cloop_probe,
-    .bdrv_open      = cloop_open,
-    .bdrv_read      = cloop_co_read,
+    .bdrv_co_open   = cloop_open,
+    .bdrv_co_read   = cloop_co_read,
     .bdrv_close     = cloop_close,
 };
 
diff --git a/block/cow.c b/block/cow.c
index 1cc2e89..c68c5ae 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -58,7 +58,7 @@  static int cow_probe(const uint8_t *buf, int buf_size, const char *filename)
         return 0;
 }
 
-static int cow_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn cow_co_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVCowState *s = bs->opaque;
     struct cow_header_v2 cow_header;
@@ -255,7 +255,7 @@  static void cow_close(BlockDriverState *bs)
 {
 }
 
-static int cow_create(const char *filename, QEMUOptionParameter *options)
+static int coroutine_fn cow_co_create(const char *filename, QEMUOptionParameter *options)
 {
     struct cow_header_v2 cow_header;
     struct stat st;
@@ -337,13 +337,13 @@  static BlockDriver bdrv_cow = {
     .instance_size  = sizeof(BDRVCowState),
 
     .bdrv_probe     = cow_probe,
-    .bdrv_open      = cow_open,
+    .bdrv_co_open   = cow_co_open,
     .bdrv_close     = cow_close,
-    .bdrv_create    = cow_create,
+    .bdrv_co_create = cow_co_create,
     .bdrv_has_zero_init     = bdrv_has_zero_init_1,
 
-    .bdrv_read              = cow_co_read,
-    .bdrv_write             = cow_co_write,
+    .bdrv_co_read           = cow_co_read,
+    .bdrv_co_write          = cow_co_write,
     .bdrv_co_is_allocated   = cow_co_is_allocated,
 
     .create_options = cow_create_options,
diff --git a/block/curl.c b/block/curl.c
index 6af8cb7..e2e34f4 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -395,7 +395,7 @@  static QemuOptsList runtime_opts = {
     },
 };
 
-static int curl_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn curl_co_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVCURLState *s = bs->opaque;
     CURLState *state = NULL;
@@ -622,7 +622,7 @@  static BlockDriver bdrv_http = {
 
     .instance_size          = sizeof(BDRVCURLState),
     .bdrv_parse_filename    = curl_parse_filename,
-    .bdrv_file_open         = curl_open,
+    .bdrv_co_file_open      = curl_co_open,
     .bdrv_close             = curl_close,
     .bdrv_getlength         = curl_getlength,
 
@@ -635,7 +635,7 @@  static BlockDriver bdrv_https = {
 
     .instance_size          = sizeof(BDRVCURLState),
     .bdrv_parse_filename    = curl_parse_filename,
-    .bdrv_file_open         = curl_open,
+    .bdrv_co_file_open      = curl_co_open,
     .bdrv_close             = curl_close,
     .bdrv_getlength         = curl_getlength,
 
@@ -648,7 +648,7 @@  static BlockDriver bdrv_ftp = {
 
     .instance_size          = sizeof(BDRVCURLState),
     .bdrv_parse_filename    = curl_parse_filename,
-    .bdrv_file_open         = curl_open,
+    .bdrv_co_file_open      = curl_co_open,
     .bdrv_close             = curl_close,
     .bdrv_getlength         = curl_getlength,
 
@@ -661,7 +661,7 @@  static BlockDriver bdrv_ftps = {
 
     .instance_size          = sizeof(BDRVCURLState),
     .bdrv_parse_filename    = curl_parse_filename,
-    .bdrv_file_open         = curl_open,
+    .bdrv_co_file_open      = curl_co_open,
     .bdrv_close             = curl_close,
     .bdrv_getlength         = curl_getlength,
 
@@ -674,7 +674,7 @@  static BlockDriver bdrv_tftp = {
 
     .instance_size          = sizeof(BDRVCURLState),
     .bdrv_parse_filename    = curl_parse_filename,
-    .bdrv_file_open         = curl_open,
+    .bdrv_co_file_open      = curl_co_open,
     .bdrv_close             = curl_close,
     .bdrv_getlength         = curl_getlength,
 
diff --git a/block/dmg.c b/block/dmg.c
index 3141cb5..346aa7d 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -92,7 +92,7 @@  static int read_uint32(BlockDriverState *bs, int64_t offset, uint32_t *result)
     return 0;
 }
 
-static int dmg_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn dmg_co_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVDMGState *s = bs->opaque;
     uint64_t info_begin,info_end,last_in_offset,last_out_offset;
@@ -378,8 +378,8 @@  static BlockDriver bdrv_dmg = {
     .format_name	= "dmg",
     .instance_size	= sizeof(BDRVDMGState),
     .bdrv_probe		= dmg_probe,
-    .bdrv_open		= dmg_open,
-    .bdrv_read          = dmg_co_read,
+    .bdrv_co_open	= dmg_co_open,
+    .bdrv_co_read	= dmg_co_read,
     .bdrv_close		= dmg_close,
 };
 
diff --git a/block/nbd.c b/block/nbd.c
index 9c480b8..3e037ba 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -256,7 +256,7 @@  static int nbd_config(BDRVNBDState *s, QDict *options)
 }
 
 
-static void nbd_coroutine_start(BDRVNBDState *s, struct nbd_request *request)
+static void coroutine_fn nbd_coroutine_start(BDRVNBDState *s, struct nbd_request *request)
 {
     int i;
 
@@ -334,7 +334,7 @@  static void nbd_restart_write(void *opaque)
     qemu_coroutine_enter(s->send_coroutine, NULL);
 }
 
-static int nbd_co_send_request(BDRVNBDState *s, struct nbd_request *request,
+static int coroutine_fn nbd_co_send_request(BDRVNBDState *s, struct nbd_request *request,
                                QEMUIOVector *qiov, int offset)
 {
     int rc, ret;
@@ -368,7 +368,7 @@  static int nbd_co_send_request(BDRVNBDState *s, struct nbd_request *request,
     return rc;
 }
 
-static void nbd_co_receive_reply(BDRVNBDState *s, struct nbd_request *request,
+static void coroutine_fn nbd_co_receive_reply(BDRVNBDState *s, struct nbd_request *request,
                                  struct nbd_reply *reply,
                                  QEMUIOVector *qiov, int offset)
 {
@@ -394,7 +394,7 @@  static void nbd_co_receive_reply(BDRVNBDState *s, struct nbd_request *request,
     }
 }
 
-static void nbd_coroutine_end(BDRVNBDState *s, struct nbd_request *request)
+static void coroutine_fn nbd_coroutine_end(BDRVNBDState *s, struct nbd_request *request)
 {
     int i = HANDLE_TO_INDEX(s, request->handle);
     s->recv_coroutine[i] = NULL;
@@ -463,7 +463,7 @@  static void nbd_teardown_connection(BlockDriverState *bs)
     closesocket(s->sock);
 }
 
-static int nbd_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn nbd_co_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVNBDState *s = bs->opaque;
     int result;
@@ -485,7 +485,7 @@  static int nbd_open(BlockDriverState *bs, QDict *options, int flags)
     return result;
 }
 
-static int nbd_co_readv_1(BlockDriverState *bs, int64_t sector_num,
+static int coroutine_fn nbd_co_readv_1(BlockDriverState *bs, int64_t sector_num,
                           int nb_sectors, QEMUIOVector *qiov,
                           int offset)
 {
@@ -510,7 +510,7 @@  static int nbd_co_readv_1(BlockDriverState *bs, int64_t sector_num,
 
 }
 
-static int nbd_co_writev_1(BlockDriverState *bs, int64_t sector_num,
+static int coroutine_fn nbd_co_writev_1(BlockDriverState *bs, int64_t sector_num,
                            int nb_sectors, QEMUIOVector *qiov,
                            int offset)
 {
@@ -542,7 +542,7 @@  static int nbd_co_writev_1(BlockDriverState *bs, int64_t sector_num,
  * remain aligned to 4K. */
 #define NBD_MAX_SECTORS 2040
 
-static int nbd_co_readv(BlockDriverState *bs, int64_t sector_num,
+static int coroutine_fn nbd_co_readv(BlockDriverState *bs, int64_t sector_num,
                         int nb_sectors, QEMUIOVector *qiov)
 {
     int offset = 0;
@@ -559,7 +559,7 @@  static int nbd_co_readv(BlockDriverState *bs, int64_t sector_num,
     return nbd_co_readv_1(bs, sector_num, nb_sectors, qiov, offset);
 }
 
-static int nbd_co_writev(BlockDriverState *bs, int64_t sector_num,
+static int coroutine_fn nbd_co_writev(BlockDriverState *bs, int64_t sector_num,
                          int nb_sectors, QEMUIOVector *qiov)
 {
     int offset = 0;
@@ -576,7 +576,7 @@  static int nbd_co_writev(BlockDriverState *bs, int64_t sector_num,
     return nbd_co_writev_1(bs, sector_num, nb_sectors, qiov, offset);
 }
 
-static int nbd_co_flush(BlockDriverState *bs)
+static int coroutine_fn nbd_co_flush(BlockDriverState *bs)
 {
     BDRVNBDState *s = bs->opaque;
     struct nbd_request request;
@@ -606,7 +606,7 @@  static int nbd_co_flush(BlockDriverState *bs)
     return -reply.error;
 }
 
-static int nbd_co_discard(BlockDriverState *bs, int64_t sector_num,
+static int coroutine_fn nbd_co_discard(BlockDriverState *bs, int64_t sector_num,
                           int nb_sectors)
 {
     BDRVNBDState *s = bs->opaque;
@@ -653,7 +653,7 @@  static BlockDriver bdrv_nbd = {
     .protocol_name       = "nbd",
     .instance_size       = sizeof(BDRVNBDState),
     .bdrv_parse_filename = nbd_parse_filename,
-    .bdrv_file_open      = nbd_open,
+    .bdrv_co_file_open   = nbd_co_open,
     .bdrv_co_readv       = nbd_co_readv,
     .bdrv_co_writev      = nbd_co_writev,
     .bdrv_close          = nbd_close,
@@ -667,7 +667,7 @@  static BlockDriver bdrv_nbd_tcp = {
     .protocol_name       = "nbd+tcp",
     .instance_size       = sizeof(BDRVNBDState),
     .bdrv_parse_filename = nbd_parse_filename,
-    .bdrv_file_open      = nbd_open,
+    .bdrv_co_file_open   = nbd_co_open,
     .bdrv_co_readv       = nbd_co_readv,
     .bdrv_co_writev      = nbd_co_writev,
     .bdrv_close          = nbd_close,
@@ -681,7 +681,7 @@  static BlockDriver bdrv_nbd_unix = {
     .protocol_name       = "nbd+unix",
     .instance_size       = sizeof(BDRVNBDState),
     .bdrv_parse_filename = nbd_parse_filename,
-    .bdrv_file_open      = nbd_open,
+    .bdrv_co_file_open   = nbd_co_open,
     .bdrv_co_readv       = nbd_co_readv,
     .bdrv_co_writev      = nbd_co_writev,
     .bdrv_close          = nbd_close,
diff --git a/block/parallels.c b/block/parallels.c
index 18b3ac0..1f9aaf9 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -68,7 +68,7 @@  static int parallels_probe(const uint8_t *buf, int buf_size, const char *filenam
     return 0;
 }
 
-static int parallels_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn parallels_co_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVParallelsState *s = bs->opaque;
     int i;
@@ -164,8 +164,8 @@  static BlockDriver bdrv_parallels = {
     .format_name	= "parallels",
     .instance_size	= sizeof(BDRVParallelsState),
     .bdrv_probe		= parallels_probe,
-    .bdrv_open		= parallels_open,
-    .bdrv_read          = parallels_co_read,
+    .bdrv_co_open	= parallels_co_open,
+    .bdrv_co_read	= parallels_co_read,
     .bdrv_close		= parallels_close,
 };
 
diff --git a/block/qcow.c b/block/qcow.c
index 5239bd6..04f59f2 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -92,7 +92,7 @@  static int qcow_probe(const uint8_t *buf, int buf_size, const char *filename)
         return 0;
 }
 
-static int qcow_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn qcow_co_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVQcowState *s = bs->opaque;
     int len, i, shift, ret;
@@ -651,7 +651,7 @@  static void qcow_close(BlockDriverState *bs)
     error_free(s->migration_blocker);
 }
 
-static int qcow_create(const char *filename, QEMUOptionParameter *options)
+static int coroutine_fn qcow_create(const char *filename, QEMUOptionParameter *options)
 {
     int header_size, backing_filename_len, l1_size, shift, i;
     QCowHeader header;
@@ -888,10 +888,10 @@  static BlockDriver bdrv_qcow = {
     .format_name	= "qcow",
     .instance_size	= sizeof(BDRVQcowState),
     .bdrv_probe		= qcow_probe,
-    .bdrv_open		= qcow_open,
+    .bdrv_co_open	= qcow_co_open,
     .bdrv_close		= qcow_close,
     .bdrv_reopen_prepare = qcow_reopen_prepare,
-    .bdrv_create	= qcow_create,
+    .bdrv_co_create	= qcow_create,
     .bdrv_has_zero_init     = bdrv_has_zero_init_1,
 
     .bdrv_co_readv          = qcow_co_readv,
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index cca76d4..50262d3 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -628,7 +628,7 @@  uint64_t qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
     return cluster_offset;
 }
 
-static int perform_cow(BlockDriverState *bs, QCowL2Meta *m, Qcow2COWRegion *r)
+static int coroutine_fn perform_cow(BlockDriverState *bs, QCowL2Meta *m, Qcow2COWRegion *r)
 {
     BDRVQcowState *s = bs->opaque;
     int ret;
@@ -657,7 +657,7 @@  static int perform_cow(BlockDriverState *bs, QCowL2Meta *m, Qcow2COWRegion *r)
     return 0;
 }
 
-int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m)
+int coroutine_fn qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m)
 {
     BDRVQcowState *s = bs->opaque;
     int i, j = 0, l2_index, ret;
@@ -783,7 +783,7 @@  out:
  *           information on cluster allocation may be invalid now. The caller
  *           must start over anyway, so consider *cur_bytes undefined.
  */
-static int handle_dependencies(BlockDriverState *bs, uint64_t guest_offset,
+static int coroutine_fn handle_dependencies(BlockDriverState *bs, uint64_t guest_offset,
     uint64_t *cur_bytes, QCowL2Meta **m)
 {
     BDRVQcowState *s = bs->opaque;
@@ -1149,7 +1149,7 @@  fail:
  *
  * Return 0 on success and -errno in error cases
  */
-int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
+int coroutine_fn qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
     int n_start, int n_end, int *num, uint64_t *host_offset, QCowL2Meta **m)
 {
     BDRVQcowState *s = bs->opaque;
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;
+}
+
 static int qcow2_set_key(BlockDriverState *bs, const char *key)
 {
     BDRVQcowState *s = bs->opaque;
@@ -1201,7 +1237,7 @@  static int qcow2_change_backing_file(BlockDriverState *bs,
     return qcow2_update_header(bs);
 }
 
-static int preallocate(BlockDriverState *bs)
+static int coroutine_fn preallocate(BlockDriverState *bs)
 {
     uint64_t nb_sectors;
     uint64_t offset;
@@ -1257,7 +1293,7 @@  static int preallocate(BlockDriverState *bs)
     return 0;
 }
 
-static int qcow2_create2(const char *filename, int64_t total_size,
+static int coroutine_fn qcow2_create2(const char *filename, int64_t total_size,
                          const char *backing_file, const char *backing_format,
                          int flags, size_t cluster_size, int prealloc,
                          QEMUOptionParameter *options, int version)
@@ -1394,7 +1430,7 @@  out:
     return ret;
 }
 
-static int qcow2_create(const char *filename, QEMUOptionParameter *options)
+static int coroutine_fn qcow2_co_create(const char *filename, QEMUOptionParameter *options)
 {
     const char *backing_file = NULL;
     const char *backing_fmt = NULL;
@@ -1781,10 +1817,10 @@  static BlockDriver bdrv_qcow2 = {
     .format_name        = "qcow2",
     .instance_size      = sizeof(BDRVQcowState),
     .bdrv_probe         = qcow2_probe,
-    .bdrv_open          = qcow2_open,
+    .bdrv_co_open       = qcow2_co_open,
     .bdrv_close         = qcow2_close,
     .bdrv_reopen_prepare  = qcow2_reopen_prepare,
-    .bdrv_create        = qcow2_create,
+    .bdrv_co_create     = qcow2_co_create,
     .bdrv_has_zero_init = bdrv_has_zero_init_1,
     .bdrv_co_is_allocated = qcow2_co_is_allocated,
     .bdrv_set_key       = qcow2_set_key,
diff --git a/block/qcow2.h b/block/qcow2.h
index 3b2d5cd..0b482ff 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -397,13 +397,13 @@  void qcow2_encrypt_sectors(BDRVQcowState *s, int64_t sector_num,
 
 int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
     int *num, uint64_t *cluster_offset);
-int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
+int coroutine_fn qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
     int n_start, int n_end, int *num, uint64_t *host_offset, QCowL2Meta **m);
 uint64_t qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
                                          uint64_t offset,
                                          int compressed_size);
 
-int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m);
+int coroutine_fn qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m);
 int qcow2_discard_clusters(BlockDriverState *bs, uint64_t offset,
     int nb_sectors);
 int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int nb_sectors);
diff --git a/block/qed.c b/block/qed.c
index f767b05..5f4ba79 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -373,7 +373,7 @@  static void bdrv_qed_rebind(BlockDriverState *bs)
     s->bs = bs;
 }
 
-static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn bdrv_qed_co_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVQEDState *s = bs->opaque;
     QEDHeader le_header;
@@ -603,7 +603,7 @@  out:
     return ret;
 }
 
-static int bdrv_qed_create(const char *filename, QEMUOptionParameter *options)
+static int coroutine_fn bdrv_qed_co_create(const char *filename, QEMUOptionParameter *options)
 {
     uint64_t image_size = 0;
     uint32_t cluster_size = QED_DEFAULT_CLUSTER_SIZE;
@@ -1570,10 +1570,10 @@  static BlockDriver bdrv_qed = {
 
     .bdrv_probe               = bdrv_qed_probe,
     .bdrv_rebind              = bdrv_qed_rebind,
-    .bdrv_open                = bdrv_qed_open,
+    .bdrv_co_open             = bdrv_qed_co_open,
     .bdrv_close               = bdrv_qed_close,
     .bdrv_reopen_prepare      = bdrv_qed_reopen_prepare,
-    .bdrv_create              = bdrv_qed_create,
+    .bdrv_co_create           = bdrv_qed_co_create,
     .bdrv_has_zero_init       = bdrv_has_zero_init_1,
     .bdrv_co_is_allocated     = bdrv_qed_co_is_allocated,
     .bdrv_make_empty          = bdrv_qed_make_empty,
diff --git a/block/raw-posix.c b/block/raw-posix.c
index ba721d3..e9f0890 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -335,7 +335,7 @@  fail:
     return ret;
 }
 
-static int raw_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn raw_co_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVRawState *s = bs->opaque;
 
@@ -1040,7 +1040,7 @@  static int64_t raw_get_allocated_file_size(BlockDriverState *bs)
     return (int64_t)st.st_blocks * 512;
 }
 
-static int raw_create(const char *filename, QEMUOptionParameter *options)
+static int coroutine_fn raw_co_create(const char *filename, QEMUOptionParameter *options)
 {
     int fd;
     int result = 0;
@@ -1193,12 +1193,12 @@  static BlockDriver bdrv_file = {
     .protocol_name = "file",
     .instance_size = sizeof(BDRVRawState),
     .bdrv_probe = NULL, /* no probe for protocols */
-    .bdrv_file_open = raw_open,
+    .bdrv_co_file_open = raw_co_open,
     .bdrv_reopen_prepare = raw_reopen_prepare,
     .bdrv_reopen_commit = raw_reopen_commit,
     .bdrv_reopen_abort = raw_reopen_abort,
     .bdrv_close = raw_close,
-    .bdrv_create = raw_create,
+    .bdrv_co_create = raw_co_create,
     .bdrv_has_zero_init = bdrv_has_zero_init_1,
     .bdrv_co_is_allocated = raw_co_is_allocated,
 
@@ -1325,7 +1325,7 @@  static int check_hdev_writable(BDRVRawState *s)
     return 0;
 }
 
-static int hdev_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn hdev_co_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVRawState *s = bs->opaque;
     int ret;
@@ -1498,7 +1498,7 @@  static coroutine_fn BlockDriverAIOCB *hdev_aio_discard(BlockDriverState *bs,
                        cb, opaque, QEMU_AIO_DISCARD|QEMU_AIO_BLKDEV);
 }
 
-static int hdev_create(const char *filename, QEMUOptionParameter *options)
+static int coroutine_fn hdev_co_create(const char *filename, QEMUOptionParameter *options)
 {
     int fd;
     int ret = 0;
@@ -1533,12 +1533,12 @@  static BlockDriver bdrv_host_device = {
     .protocol_name        = "host_device",
     .instance_size      = sizeof(BDRVRawState),
     .bdrv_probe_device  = hdev_probe_device,
-    .bdrv_file_open     = hdev_open,
+    .bdrv_co_file_open  = hdev_co_open,
     .bdrv_close         = raw_close,
     .bdrv_reopen_prepare = raw_reopen_prepare,
     .bdrv_reopen_commit  = raw_reopen_commit,
     .bdrv_reopen_abort   = raw_reopen_abort,
-    .bdrv_create        = hdev_create,
+    .bdrv_co_create     = hdev_co_create,
     .create_options     = raw_create_options,
 
     .bdrv_aio_readv	= raw_aio_readv,
@@ -1559,7 +1559,7 @@  static BlockDriver bdrv_host_device = {
 };
 
 #ifdef __linux__
-static int floppy_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn floppy_co_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVRawState *s = bs->opaque;
     int ret;
@@ -1657,12 +1657,12 @@  static BlockDriver bdrv_host_floppy = {
     .protocol_name      = "host_floppy",
     .instance_size      = sizeof(BDRVRawState),
     .bdrv_probe_device	= floppy_probe_device,
-    .bdrv_file_open     = floppy_open,
+    .bdrv_co_file_open  = floppy_co_open,
     .bdrv_close         = raw_close,
     .bdrv_reopen_prepare = raw_reopen_prepare,
     .bdrv_reopen_commit  = raw_reopen_commit,
     .bdrv_reopen_abort   = raw_reopen_abort,
-    .bdrv_create        = hdev_create,
+    .bdrv_co_create     = hdev_co_create,
     .create_options     = raw_create_options,
 
     .bdrv_aio_readv     = raw_aio_readv,
@@ -1680,7 +1680,7 @@  static BlockDriver bdrv_host_floppy = {
     .bdrv_eject         = floppy_eject,
 };
 
-static int cdrom_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn cdrom_co_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVRawState *s = bs->opaque;
 
@@ -1758,12 +1758,12 @@  static BlockDriver bdrv_host_cdrom = {
     .protocol_name      = "host_cdrom",
     .instance_size      = sizeof(BDRVRawState),
     .bdrv_probe_device	= cdrom_probe_device,
-    .bdrv_file_open     = cdrom_open,
+    .bdrv_co_file_open  = cdrom_co_open,
     .bdrv_close         = raw_close,
     .bdrv_reopen_prepare = raw_reopen_prepare,
     .bdrv_reopen_commit  = raw_reopen_commit,
     .bdrv_reopen_abort   = raw_reopen_abort,
-    .bdrv_create        = hdev_create,
+    .bdrv_co_create     = hdev_co_create,
     .create_options     = raw_create_options,
 
     .bdrv_aio_readv     = raw_aio_readv,
@@ -1787,7 +1787,7 @@  static BlockDriver bdrv_host_cdrom = {
 #endif /* __linux__ */
 
 #if defined (__FreeBSD__) || defined(__FreeBSD_kernel__)
-static int cdrom_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn cdrom_co_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVRawState *s = bs->opaque;
     int ret;
@@ -1879,12 +1879,12 @@  static BlockDriver bdrv_host_cdrom = {
     .protocol_name      = "host_cdrom",
     .instance_size      = sizeof(BDRVRawState),
     .bdrv_probe_device	= cdrom_probe_device,
-    .bdrv_file_open     = cdrom_open,
+    .bdrv_co_file_open  = cdrom_co_open,
     .bdrv_close         = raw_close,
     .bdrv_reopen_prepare = raw_reopen_prepare,
     .bdrv_reopen_commit  = raw_reopen_commit,
     .bdrv_reopen_abort   = raw_reopen_abort,
-    .bdrv_create        = hdev_create,
+    .bdrv_co_create     = hdev_co_create,
     .create_options     = raw_create_options,
 
     .bdrv_aio_readv     = raw_aio_readv,
diff --git a/block/raw.c b/block/raw.c
index ce10422..83fc9f8 100644
--- a/block/raw.c
+++ b/block/raw.c
@@ -3,7 +3,7 @@ 
 #include "block/block_int.h"
 #include "qemu/module.h"
 
-static int raw_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn raw_co_open(BlockDriverState *bs, QDict *options, int flags)
 {
     bs->sg = bs->file->sg;
     return 0;
@@ -95,7 +95,7 @@  static BlockDriverAIOCB *raw_aio_ioctl(BlockDriverState *bs,
    return bdrv_aio_ioctl(bs->file, req, buf, cb, opaque);
 }
 
-static int raw_create(const char *filename, QEMUOptionParameter *options)
+static int coroutine_fn raw_co_create(const char *filename, QEMUOptionParameter *options)
 {
     return bdrv_create_file(filename, options);
 }
@@ -120,7 +120,7 @@  static BlockDriver bdrv_raw = {
     /* It's really 0, but we need to make g_malloc() happy */
     .instance_size      = 1,
 
-    .bdrv_open          = raw_open,
+    .bdrv_co_open       = raw_co_open,
     .bdrv_close         = raw_close,
 
     .bdrv_reopen_prepare  = raw_reopen_prepare,
@@ -142,7 +142,7 @@  static BlockDriver bdrv_raw = {
     .bdrv_ioctl         = raw_ioctl,
     .bdrv_aio_ioctl     = raw_aio_ioctl,
 
-    .bdrv_create        = raw_create,
+    .bdrv_co_create     = raw_co_create,
     .create_options     = raw_create_options,
     .bdrv_has_zero_init = raw_has_zero_init,
 };
diff --git a/block/sheepdog.c b/block/sheepdog.c
index b397b5b..66c7c22 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1259,7 +1259,7 @@  static QemuOptsList runtime_opts = {
     },
 };
 
-static int sd_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn sd_co_open(BlockDriverState *bs, QDict *options, int flags)
 {
     int ret, fd;
     uint32_t vid = 0;
@@ -1454,7 +1454,7 @@  out:
     return ret;
 }
 
-static int sd_create(const char *filename, QEMUOptionParameter *options)
+static int coroutine_fn sd_co_create(const char *filename, QEMUOptionParameter *options)
 {
     int ret = 0;
     uint32_t vid = 0, base_vid = 0;
@@ -2186,7 +2186,7 @@  out:
     return found;
 }
 
-static int do_load_save_vmstate(BDRVSheepdogState *s, uint8_t *data,
+static int coroutine_fn do_load_save_vmstate(BDRVSheepdogState *s, uint8_t *data,
                                 int64_t pos, int size, int load)
 {
     bool create;
@@ -2344,9 +2344,9 @@  static BlockDriver bdrv_sheepdog = {
     .format_name    = "sheepdog",
     .protocol_name  = "sheepdog",
     .instance_size  = sizeof(BDRVSheepdogState),
-    .bdrv_file_open = sd_open,
-    .bdrv_close     = sd_close,
-    .bdrv_create    = sd_create,
+    .bdrv_co_file_open = sd_co_open,
+    .bdrv_close        = sd_close,
+    .bdrv_co_create    = sd_co_create,
     .bdrv_getlength = sd_getlength,
     .bdrv_truncate  = sd_truncate,
 
@@ -2371,9 +2371,9 @@  static BlockDriver bdrv_sheepdog_tcp = {
     .format_name    = "sheepdog",
     .protocol_name  = "sheepdog+tcp",
     .instance_size  = sizeof(BDRVSheepdogState),
-    .bdrv_file_open = sd_open,
-    .bdrv_close     = sd_close,
-    .bdrv_create    = sd_create,
+    .bdrv_co_file_open = sd_co_open,
+    .bdrv_close        = sd_close,
+    .bdrv_co_create    = sd_co_create,
     .bdrv_getlength = sd_getlength,
     .bdrv_truncate  = sd_truncate,
 
@@ -2398,9 +2398,9 @@  static BlockDriver bdrv_sheepdog_unix = {
     .format_name    = "sheepdog",
     .protocol_name  = "sheepdog+unix",
     .instance_size  = sizeof(BDRVSheepdogState),
-    .bdrv_file_open = sd_open,
-    .bdrv_close     = sd_close,
-    .bdrv_create    = sd_create,
+    .bdrv_co_file_open = sd_co_open,
+    .bdrv_close        = sd_close,
+    .bdrv_co_create    = sd_co_create,
     .bdrv_has_zero_init = bdrv_has_zero_init_1,
     .bdrv_getlength = sd_getlength,
     .bdrv_truncate  = sd_truncate,
diff --git a/block/snapshot.c b/block/snapshot.c
index 6c6d9de..541d83d 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -25,6 +25,8 @@ 
 #include "block/snapshot.h"
 #include "block/block_int.h"
 
+#define NOT_DONE 0x7fffffff
+
 int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
                        const char *name)
 {
@@ -81,6 +83,34 @@  int bdrv_snapshot_create(BlockDriverState *bs,
     return -ENOTSUP;
 }
 
+struct SnapOp {
+    BlockDriverState *bs;
+    int ret;
+};
+
+static void coroutine_fn bdrv_snapshot_open_entry(void *opaque)
+{
+    struct SnapOp *so = opaque;
+    so->ret = so->bs->drv->bdrv_co_open(so->bs, NULL, so->bs->open_flags);
+}
+
+static int bdrv_snapshot_open(BlockDriverState *bs)
+{
+    Coroutine *co;
+    struct SnapOp so = {
+        .bs = bs,
+        .ret = NOT_DONE,
+    };
+
+    co = qemu_coroutine_create(bdrv_snapshot_open_entry);
+    qemu_coroutine_enter(co, &so);
+    while (so.ret == NOT_DONE) {
+        qemu_aio_wait();
+    }
+
+    return so.ret;
+}
+
 int bdrv_snapshot_goto(BlockDriverState *bs,
                        const char *snapshot_id)
 {
@@ -97,7 +127,7 @@  int bdrv_snapshot_goto(BlockDriverState *bs,
     if (bs->file) {
         drv->bdrv_close(bs);
         ret = bdrv_snapshot_goto(bs->file, snapshot_id);
-        open_ret = drv->bdrv_open(bs, NULL, bs->open_flags);
+        open_ret = bdrv_snapshot_open(bs);
         if (open_ret < 0) {
             bdrv_delete(bs->file);
             bs->drv = NULL;
diff --git a/block/ssh.c b/block/ssh.c
index d7e7bf8..66ac4c1 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -608,7 +608,7 @@  static int connect_to_ssh(BDRVSSHState *s, QDict *options,
     return ret;
 }
 
-static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags)
+static int coroutine_fn ssh_co_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags)
 {
     BDRVSSHState *s = bs->opaque;
     int ret;
@@ -650,7 +650,7 @@  static QEMUOptionParameter ssh_create_options[] = {
     { NULL }
 };
 
-static int ssh_create(const char *filename, QEMUOptionParameter *options)
+static int coroutine_fn ssh_co_create(const char *filename, QEMUOptionParameter *options)
 {
     int r, ret;
     Error *local_err = NULL;
@@ -748,7 +748,7 @@  static int return_true(void *opaque)
     return 1;
 }
 
-static coroutine_fn void set_fd_handler(BDRVSSHState *s)
+static void set_fd_handler(BDRVSSHState *s)
 {
     int r;
     IOHandler *rd_handler = NULL, *wr_handler = NULL;
@@ -769,7 +769,7 @@  static coroutine_fn void set_fd_handler(BDRVSSHState *s)
     qemu_aio_set_fd_handler(s->sock, rd_handler, wr_handler, return_true, co);
 }
 
-static coroutine_fn void clear_fd_handler(BDRVSSHState *s)
+static void clear_fd_handler(BDRVSSHState *s)
 {
     DPRINTF("s->sock=%d", s->sock);
     qemu_aio_set_fd_handler(s->sock, NULL, NULL, NULL, NULL);
@@ -888,7 +888,7 @@  static coroutine_fn int ssh_co_readv(BlockDriverState *bs,
     return ret;
 }
 
-static int ssh_write(BDRVSSHState *s,
+static int coroutine_fn ssh_write(BDRVSSHState *s,
                      int64_t offset, size_t size,
                      QEMUIOVector *qiov)
 {
@@ -1049,8 +1049,8 @@  static BlockDriver bdrv_ssh = {
     .protocol_name                = "ssh",
     .instance_size                = sizeof(BDRVSSHState),
     .bdrv_parse_filename          = ssh_parse_filename,
-    .bdrv_file_open               = ssh_file_open,
-    .bdrv_create                  = ssh_create,
+    .bdrv_co_file_open            = ssh_co_file_open,
+    .bdrv_co_create               = ssh_co_create,
     .bdrv_close                   = ssh_close,
     .bdrv_has_zero_init           = ssh_has_zero_init,
     .bdrv_co_readv                = ssh_co_readv,
diff --git a/block/vdi.c b/block/vdi.c
index 8a91525..577a638 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -364,7 +364,7 @@  static int vdi_probe(const uint8_t *buf, int buf_size, const char *filename)
     return result;
 }
 
-static int vdi_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn vdi_co_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVVdiState *s = bs->opaque;
     VdiHeader header;
@@ -633,7 +633,7 @@  static int vdi_co_write(BlockDriverState *bs,
     return ret;
 }
 
-static int vdi_create(const char *filename, QEMUOptionParameter *options)
+static int coroutine_fn vdi_co_create(const char *filename, QEMUOptionParameter *options)
 {
     int fd;
     int result = 0;
@@ -775,17 +775,17 @@  static BlockDriver bdrv_vdi = {
     .format_name = "vdi",
     .instance_size = sizeof(BDRVVdiState),
     .bdrv_probe = vdi_probe,
-    .bdrv_open = vdi_open,
+    .bdrv_co_open = vdi_co_open,
     .bdrv_close = vdi_close,
     .bdrv_reopen_prepare = vdi_reopen_prepare,
-    .bdrv_create = vdi_create,
+    .bdrv_co_create = vdi_co_create,
     .bdrv_has_zero_init = bdrv_has_zero_init_1,
     .bdrv_co_is_allocated = vdi_co_is_allocated,
     .bdrv_make_empty = vdi_make_empty,
 
-    .bdrv_read = vdi_co_read,
+    .bdrv_co_read = vdi_co_read,
 #if defined(CONFIG_VDI_WRITE)
-    .bdrv_write = vdi_co_write,
+    .bdrv_co_write = vdi_co_write,
 #endif
 
     .bdrv_get_info = vdi_get_info,
diff --git a/block/vhdx.c b/block/vhdx.c
index e9704b1..af38098 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -715,7 +715,7 @@  exit:
 }
 
 
-static int vhdx_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn vhdx_co_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVVHDXState *s = bs->opaque;
     int ret = 0;
@@ -957,7 +957,7 @@  static BlockDriver bdrv_vhdx = {
     .format_name            = "vhdx",
     .instance_size          = sizeof(BDRVVHDXState),
     .bdrv_probe             = vhdx_probe,
-    .bdrv_open              = vhdx_open,
+    .bdrv_co_open           = vhdx_co_open,
     .bdrv_close             = vhdx_close,
     .bdrv_reopen_prepare    = vhdx_reopen_prepare,
     .bdrv_co_readv          = vhdx_co_readv,
diff --git a/block/vmdk.c b/block/vmdk.c
index a28fb5e..ae1dee4 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -767,7 +767,7 @@  exit:
     return ret;
 }
 
-static int vmdk_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn vmdk_co_open(BlockDriverState *bs, QDict *options, int flags)
 {
     int ret;
     BDRVVmdkState *s = bs->opaque;
@@ -1487,7 +1487,7 @@  static int filename_decompose(const char *filename, char *path, char *prefix,
     return VMDK_OK;
 }
 
-static int vmdk_create(const char *filename, QEMUOptionParameter *options)
+static int coroutine_fn vmdk_co_create(const char *filename, QEMUOptionParameter *options)
 {
     int fd, idx = 0;
     char desc[BUF_SIZE];
@@ -1765,13 +1765,13 @@  static BlockDriver bdrv_vmdk = {
     .format_name    = "vmdk",
     .instance_size  = sizeof(BDRVVmdkState),
     .bdrv_probe     = vmdk_probe,
-    .bdrv_open      = vmdk_open,
+    .bdrv_co_open   = vmdk_co_open,
     .bdrv_reopen_prepare = vmdk_reopen_prepare,
-    .bdrv_read      = vmdk_co_read,
-    .bdrv_write     = vmdk_co_write,
+    .bdrv_co_read   = vmdk_co_read,
+    .bdrv_co_write  = vmdk_co_write,
     .bdrv_co_write_zeroes = vmdk_co_write_zeroes,
     .bdrv_close     = vmdk_close,
-    .bdrv_create    = vmdk_create,
+    .bdrv_co_create = vmdk_co_create,
     .bdrv_co_flush_to_disk  = vmdk_co_flush,
     .bdrv_co_is_allocated   = vmdk_co_is_allocated,
     .bdrv_get_allocated_file_size  = vmdk_get_allocated_file_size,
diff --git a/block/vpc.c b/block/vpc.c
index fe4f311..6eb293a 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -155,7 +155,7 @@  static int vpc_probe(const uint8_t *buf, int buf_size, const char *filename)
     return 0;
 }
 
-static int vpc_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn vpc_co_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVVPCState *s = bs->opaque;
     int i;
@@ -683,7 +683,7 @@  static int create_fixed_disk(int fd, uint8_t *buf, int64_t total_size)
     return ret;
 }
 
-static int vpc_create(const char *filename, QEMUOptionParameter *options)
+static int coroutine_fn vpc_co_create(const char *filename, QEMUOptionParameter *options)
 {
     uint8_t buf[1024];
     struct vhd_footer *footer = (struct vhd_footer *) buf;
@@ -831,13 +831,13 @@  static BlockDriver bdrv_vpc = {
     .instance_size  = sizeof(BDRVVPCState),
 
     .bdrv_probe             = vpc_probe,
-    .bdrv_open              = vpc_open,
+    .bdrv_co_open           = vpc_co_open,
     .bdrv_close             = vpc_close,
     .bdrv_reopen_prepare    = vpc_reopen_prepare,
-    .bdrv_create            = vpc_create,
+    .bdrv_co_create         = vpc_co_create,
 
-    .bdrv_read              = vpc_co_read,
-    .bdrv_write             = vpc_co_write,
+    .bdrv_co_read           = vpc_co_read,
+    .bdrv_co_write          = vpc_co_write,
 
     .create_options         = vpc_create_options,
     .bdrv_has_zero_init     = vpc_has_zero_init,
diff --git a/block/vvfat.c b/block/vvfat.c
index 87b0279..4771a5d 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1065,7 +1065,7 @@  static void vvfat_parse_filename(const char *filename, QDict *options,
     qdict_put(options, "rw", qbool_from_int(rw));
 }
 
-static int vvfat_open(BlockDriverState *bs, QDict *options, int flags)
+static int coroutine_fn vvfat_co_open(BlockDriverState *bs, QDict *options, int flags)
 {
     BDRVVVFATState *s = bs->opaque;
     int cyls, heads, secs;
@@ -2886,7 +2886,7 @@  static int coroutine_fn vvfat_co_is_allocated(BlockDriverState *bs,
     return 1;
 }
 
-static int write_target_commit(BlockDriverState *bs, int64_t sector_num,
+static int coroutine_fn write_target_commit(BlockDriverState *bs, int64_t sector_num,
 	const uint8_t* buffer, int nb_sectors) {
     BDRVVVFATState* s = *((BDRVVVFATState**) bs->opaque);
     return try_commit(s);
@@ -2900,7 +2900,7 @@  static void write_target_close(BlockDriverState *bs) {
 
 static BlockDriver vvfat_write_target = {
     .format_name        = "vvfat_write_target",
-    .bdrv_write         = write_target_commit,
+    .bdrv_co_write      = write_target_commit,
     .bdrv_close         = write_target_close,
 };
 
@@ -2975,12 +2975,12 @@  static BlockDriver bdrv_vvfat = {
     .instance_size          = sizeof(BDRVVVFATState),
 
     .bdrv_parse_filename    = vvfat_parse_filename,
-    .bdrv_file_open         = vvfat_open,
+    .bdrv_co_file_open      = vvfat_co_open,
     .bdrv_close             = vvfat_close,
     .bdrv_rebind            = vvfat_rebind,
 
-    .bdrv_read              = vvfat_co_read,
-    .bdrv_write             = vvfat_co_write,
+    .bdrv_co_read           = vvfat_co_read,
+    .bdrv_co_write          = vvfat_co_write,
     .bdrv_co_is_allocated   = vvfat_co_is_allocated,
 };
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index c6ac871..bd92260 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -96,15 +96,15 @@  struct BlockDriver {
     void (*bdrv_reopen_commit)(BDRVReopenState *reopen_state);
     void (*bdrv_reopen_abort)(BDRVReopenState *reopen_state);
 
-    int (*bdrv_open)(BlockDriverState *bs, QDict *options, int flags);
-    int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags);
-    int (*bdrv_read)(BlockDriverState *bs, int64_t sector_num,
+    int coroutine_fn (*bdrv_co_open)(BlockDriverState *bs, QDict *options, int flags);
+    int coroutine_fn (*bdrv_co_file_open)(BlockDriverState *bs, QDict *options, int flags);
+    int coroutine_fn (*bdrv_co_read)(BlockDriverState *bs, int64_t sector_num,
                      uint8_t *buf, int nb_sectors);
-    int (*bdrv_write)(BlockDriverState *bs, int64_t sector_num,
+    int coroutine_fn (*bdrv_co_write)(BlockDriverState *bs, int64_t sector_num,
                       const uint8_t *buf, int nb_sectors);
     void (*bdrv_close)(BlockDriverState *bs);
     void (*bdrv_rebind)(BlockDriverState *bs);
-    int (*bdrv_create)(const char *filename, QEMUOptionParameter *options);
+    int coroutine_fn (*bdrv_co_create)(const char *filename, QEMUOptionParameter *options);
     int (*bdrv_set_key)(BlockDriverState *bs, const char *key);
     int (*bdrv_make_empty)(BlockDriverState *bs);
     /* aio */
diff --git a/include/block/coroutine.h b/include/block/coroutine.h
index 563dcde..75bc6f8 100644
--- a/include/block/coroutine.h
+++ b/include/block/coroutine.h
@@ -189,14 +189,14 @@  void qemu_co_rwlock_init(CoRwlock *lock);
  * of a parallel writer, control is transferred to the caller of the current
  * coroutine.
  */
-void qemu_co_rwlock_rdlock(CoRwlock *lock);
+void coroutine_fn qemu_co_rwlock_rdlock(CoRwlock *lock);
 
 /**
  * Write Locks the mutex. If the lock cannot be taken immediately because
  * of a parallel reader, control is transferred to the caller of the current
  * coroutine.
  */
-void qemu_co_rwlock_wrlock(CoRwlock *lock);
+void coroutine_fn qemu_co_rwlock_wrlock(CoRwlock *lock);
 
 /**
  * Unlocks the read/write lock and schedules the next coroutine that was
diff --git a/include/block/coroutine_int.h b/include/block/coroutine_int.h
index f133d65..d0ab27d 100644
--- a/include/block/coroutine_int.h
+++ b/include/block/coroutine_int.h
@@ -48,6 +48,6 @@  Coroutine *qemu_coroutine_new(void);
 void qemu_coroutine_delete(Coroutine *co);
 CoroutineAction qemu_coroutine_switch(Coroutine *from, Coroutine *to,
                                       CoroutineAction action);
-void coroutine_fn qemu_co_queue_run_restart(Coroutine *co);
+void qemu_co_queue_run_restart(Coroutine *co);
 
 #endif
diff --git a/qemu-coroutine-lock.c b/qemu-coroutine-lock.c
index d9fea49..200bfd2 100644
--- a/qemu-coroutine-lock.c
+++ b/qemu-coroutine-lock.c
@@ -145,7 +145,7 @@  void qemu_co_rwlock_init(CoRwlock *lock)
     qemu_co_queue_init(&lock->queue);
 }
 
-void qemu_co_rwlock_rdlock(CoRwlock *lock)
+void coroutine_fn qemu_co_rwlock_rdlock(CoRwlock *lock)
 {
     while (lock->writer) {
         qemu_co_queue_wait(&lock->queue);
@@ -169,7 +169,7 @@  void qemu_co_rwlock_unlock(CoRwlock *lock)
     }
 }
 
-void qemu_co_rwlock_wrlock(CoRwlock *lock)
+void coroutine_fn qemu_co_rwlock_wrlock(CoRwlock *lock)
 {
     while (lock->writer || lock->reader) {
         qemu_co_queue_wait(&lock->queue);