[Qemu-devel] RFC [PATCH] Make bdrv_flush synchronous only and update callers

[Qemu-devel] RFC [PATCH] Make bdrv_flush synchronous only and update callers

[Charlie Shepherd]

This patch makes bdrv_flush a synchronous function and updates any callers from
a coroutine context to use bdrv_co_flush instead.

The motivation for this patch comes from the GSoC Continuation-Passing C
project. When coroutines were introduced, synchronous functions in the block
layer were converted to use asynchronous methods by dynamically detecting if
they were being run from a coroutine context by calling qemu_in_coroutine(), and
yielding if so. If they were not, they would spawn a new coroutine and poll
until the asynchronous counterpart finished.

However this approach does not work with CPC as the CPC translator converts all
functions annotated coroutine_fn to a different (continuation based) calling
convention. This means that coroutine_fn annotated functions cannot be called
from a non-coroutine context.

This patch is a Request For Comments on the approach of splitting these
"dynamic" functions into synchronous and asynchronous versions. This is easy for
bdrv_flush as it already has an asynchronous counterpart - bdrv_co_flush. The
only caller of bdrv_flush from a coroutine context is mirror_drain in
block/mirror.c - this should be annotated as a coroutine_fn as it calls
qemu_coroutine_yield().

If this approach meets with approval I will develop a patchset splitting the
other "dynamic" functions in the block layer. This will allow all coroutine
functions to have a coroutine_fn annotation that can be statically checked (CPC
can be used to verify annotations).

I have audited the other callers of bdrv_flush, they are included below:

block.c: bdrv_reopen_prepare, bdrv_close, bdrv_commit, bdrv_pwrite_sync
block/qcow2-cache.c: qcow2_cache_entry_flush, qcow2_cache_flush
block/qcow2-refcount.c: qcow2_update_snapshot_refcount
block/qcow2-snapshot.c: qcow2_write_snapshots
block/qcow2.c: qcow2_mark_dirty, qcow2_mark_clean
block/qed-check.c: qed_check_mark_clean
block/qed.c: bdrv_qed_open, bdrv_qed_close
blockdev.c: external_snapshot_prepare, do_drive_del
cpus.c: do_vm_stop
hw/block/nvme.c: nvme_clear_ctrl
qemu-io-cmds.c: flush_f
savevm.c: bdrv_fclose

---
 block.c        | 13 ++++---------
 block/mirror.c |  4 ++--
 2 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/block.c b/block.c
index 6c493ad..00d71df 100644
--- a/block.c
+++ b/block.c
@@ -4110,15 +4110,10 @@ int bdrv_flush(BlockDriverState *bs)
         .ret = NOT_DONE,
     };
 
-    if (qemu_in_coroutine()) {
-        /* Fast-path if already in coroutine context */
-        bdrv_flush_co_entry(&rwco);
-    } else {
-        co = qemu_coroutine_create(bdrv_flush_co_entry);
-        qemu_coroutine_enter(co, &rwco);
-        while (rwco.ret == NOT_DONE) {
-            qemu_aio_wait();
-        }
+    co = qemu_coroutine_create(bdrv_flush_co_entry);
+    qemu_coroutine_enter(co, &rwco);
+    while (rwco.ret == NOT_DONE) {
+        qemu_aio_wait();
     }
 
     return rwco.ret;
diff --git a/block/mirror.c b/block/mirror.c
index bed4a7e..3d5da7e 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -282,7 +282,7 @@ static void mirror_free_init(MirrorBlockJob *s)
     }
 }
 
-static void mirror_drain(MirrorBlockJob *s)
+static void coroutine_fn mirror_drain(MirrorBlockJob *s)
 {
     while (s->in_flight > 0) {
         qemu_coroutine_yield();
@@ -390,7 +390,7 @@ static void coroutine_fn mirror_run(void *opaque)
         should_complete = false;
         if (s->in_flight == 0 && cnt == 0) {
             trace_mirror_before_flush(s);
-            ret = bdrv_flush(s->target);
+            ret = bdrv_co_flush(s->target);
             if (ret < 0) {
                 if (mirror_error_action(s, false, -ret) == BDRV_ACTION_REPORT) {
                     goto immediate_exit;
-- 
1.8.3.2

Re: [Qemu-devel] RFC [PATCH] Make bdrv_flush synchronous only and update callers

[Stefan Hajnoczi]

On Thu, Jul 18, 2013 at 11:21:42PM +0200, Charlie Shepherd wrote:
> This patch makes bdrv_flush a synchronous function and updates any callers from
> a coroutine context to use bdrv_co_flush instead.
> 
> The motivation for this patch comes from the GSoC Continuation-Passing C
> project. When coroutines were introduced, synchronous functions in the block
> layer were converted to use asynchronous methods by dynamically detecting if
> they were being run from a coroutine context by calling qemu_in_coroutine(), and
> yielding if so. If they were not, they would spawn a new coroutine and poll
> until the asynchronous counterpart finished.
> 
> However this approach does not work with CPC as the CPC translator converts all
> functions annotated coroutine_fn to a different (continuation based) calling
> convention. This means that coroutine_fn annotated functions cannot be called
> from a non-coroutine context.
> 
> This patch is a Request For Comments on the approach of splitting these
> "dynamic" functions into synchronous and asynchronous versions. This is easy for
> bdrv_flush as it already has an asynchronous counterpart - bdrv_co_flush. The
> only caller of bdrv_flush from a coroutine context is mirror_drain in
> block/mirror.c - this should be annotated as a coroutine_fn as it calls
> qemu_coroutine_yield().
> 
> If this approach meets with approval I will develop a patchset splitting the
> other "dynamic" functions in the block layer. This will allow all coroutine
> functions to have a coroutine_fn annotation that can be statically checked (CPC
> can be used to verify annotations).
> 
> I have audited the other callers of bdrv_flush, they are included below:
> 
> block.c: bdrv_reopen_prepare, bdrv_close, bdrv_commit, bdrv_pwrite_sync

bdrv_pwrite_sync() is a dynamic function.  If called from coroutine
context it will yield (indirectly from bdrv_pwrite() or bdrv_flush()).

> block/qcow2-cache.c: qcow2_cache_entry_flush, qcow2_cache_flush
> block/qcow2-refcount.c: qcow2_update_snapshot_refcount
> block/qcow2-snapshot.c: qcow2_write_snapshots
> block/qcow2.c: qcow2_mark_dirty, qcow2_mark_clean

qcow2 runs in coroutine context, the coroutine_fn annotations are just
missing.  See block/qcow2.c:bdrv_qcow2 for the entry points like
qcow2_co_readv.

> block/qed-check.c: qed_check_mark_clean
> block/qed.c: bdrv_qed_open, bdrv_qed_close
> blockdev.c: external_snapshot_prepare, do_drive_del
> cpus.c: do_vm_stop
> hw/block/nvme.c: nvme_clear_ctrl
> qemu-io-cmds.c: flush_f
> savevm.c: bdrv_fclose

I think the rest are fine and your patch looks good.

Re: [Qemu-devel] RFC [PATCH] Make bdrv_flush synchronous only and update callers

[Kevin Wolf]

Am 19.07.2013 um 07:27 hat Stefan Hajnoczi geschrieben:
> On Thu, Jul 18, 2013 at 11:21:42PM +0200, Charlie Shepherd wrote:
> > This patch makes bdrv_flush a synchronous function and updates any callers from
> > a coroutine context to use bdrv_co_flush instead.
> > 
> > The motivation for this patch comes from the GSoC Continuation-Passing C
> > project. When coroutines were introduced, synchronous functions in the block
> > layer were converted to use asynchronous methods by dynamically detecting if
> > they were being run from a coroutine context by calling qemu_in_coroutine(), and
> > yielding if so. If they were not, they would spawn a new coroutine and poll
> > until the asynchronous counterpart finished.
> > 
> > However this approach does not work with CPC as the CPC translator converts all
> > functions annotated coroutine_fn to a different (continuation based) calling
> > convention. This means that coroutine_fn annotated functions cannot be called
> > from a non-coroutine context.
> > 
> > This patch is a Request For Comments on the approach of splitting these
> > "dynamic" functions into synchronous and asynchronous versions. This is easy for
> > bdrv_flush as it already has an asynchronous counterpart - bdrv_co_flush. The
> > only caller of bdrv_flush from a coroutine context is mirror_drain in
> > block/mirror.c - this should be annotated as a coroutine_fn as it calls
> > qemu_coroutine_yield().
> > 
> > If this approach meets with approval I will develop a patchset splitting the
> > other "dynamic" functions in the block layer. This will allow all coroutine
> > functions to have a coroutine_fn annotation that can be statically checked (CPC
> > can be used to verify annotations).
> > 
> > I have audited the other callers of bdrv_flush, they are included below:
> > 
> > block.c: bdrv_reopen_prepare, bdrv_close, bdrv_commit, bdrv_pwrite_sync
> 
> bdrv_pwrite_sync() is a dynamic function.  If called from coroutine
> context it will yield (indirectly from bdrv_pwrite() or bdrv_flush()).
> 
> > block/qcow2-cache.c: qcow2_cache_entry_flush, qcow2_cache_flush
> > block/qcow2-refcount.c: qcow2_update_snapshot_refcount
> > block/qcow2-snapshot.c: qcow2_write_snapshots
> > block/qcow2.c: qcow2_mark_dirty, qcow2_mark_clean
> 
> qcow2 runs in coroutine context, the coroutine_fn annotations are just
> missing.  See block/qcow2.c:bdrv_qcow2 for the entry points like
> qcow2_co_readv.

Yes, you can't rely on coroutine_fn, it's missing in many places where
it should be there. But that was still the optimistic view.

The truth is that the greatest part of the qcow2 functions can be called
from eiher coroutine or non-coroutine context. You get coroutine context
for read/write/discard/flush, but anything else like doing snapshots,
resizing, preallocating the image, writing compressed data also accesses
the same metadata management functions outside coroutines.

It's only getting worse for function like bdrv_pwrite().

Kevin

Re: [Qemu-devel] RFC [PATCH] Make bdrv_flush synchronous only and update callers

[Stefan Hajnoczi]

On Fri, Jul 19, 2013 at 10:37:11AM +0200, Kevin Wolf wrote:
> Am 19.07.2013 um 07:27 hat Stefan Hajnoczi geschrieben:
> > On Thu, Jul 18, 2013 at 11:21:42PM +0200, Charlie Shepherd wrote:
> > > This patch makes bdrv_flush a synchronous function and updates any callers from
> > > a coroutine context to use bdrv_co_flush instead.
> > > 
> > > The motivation for this patch comes from the GSoC Continuation-Passing C
> > > project. When coroutines were introduced, synchronous functions in the block
> > > layer were converted to use asynchronous methods by dynamically detecting if
> > > they were being run from a coroutine context by calling qemu_in_coroutine(), and
> > > yielding if so. If they were not, they would spawn a new coroutine and poll
> > > until the asynchronous counterpart finished.
> > > 
> > > However this approach does not work with CPC as the CPC translator converts all
> > > functions annotated coroutine_fn to a different (continuation based) calling
> > > convention. This means that coroutine_fn annotated functions cannot be called
> > > from a non-coroutine context.
> > > 
> > > This patch is a Request For Comments on the approach of splitting these
> > > "dynamic" functions into synchronous and asynchronous versions. This is easy for
> > > bdrv_flush as it already has an asynchronous counterpart - bdrv_co_flush. The
> > > only caller of bdrv_flush from a coroutine context is mirror_drain in
> > > block/mirror.c - this should be annotated as a coroutine_fn as it calls
> > > qemu_coroutine_yield().
> > > 
> > > If this approach meets with approval I will develop a patchset splitting the
> > > other "dynamic" functions in the block layer. This will allow all coroutine
> > > functions to have a coroutine_fn annotation that can be statically checked (CPC
> > > can be used to verify annotations).
> > > 
> > > I have audited the other callers of bdrv_flush, they are included below:
> > > 
> > > block.c: bdrv_reopen_prepare, bdrv_close, bdrv_commit, bdrv_pwrite_sync
> > 
> > bdrv_pwrite_sync() is a dynamic function.  If called from coroutine
> > context it will yield (indirectly from bdrv_pwrite() or bdrv_flush()).
> > 
> > > block/qcow2-cache.c: qcow2_cache_entry_flush, qcow2_cache_flush
> > > block/qcow2-refcount.c: qcow2_update_snapshot_refcount
> > > block/qcow2-snapshot.c: qcow2_write_snapshots
> > > block/qcow2.c: qcow2_mark_dirty, qcow2_mark_clean
> > 
> > qcow2 runs in coroutine context, the coroutine_fn annotations are just
> > missing.  See block/qcow2.c:bdrv_qcow2 for the entry points like
> > qcow2_co_readv.
> 
> Yes, you can't rely on coroutine_fn, it's missing in many places where
> it should be there. But that was still the optimistic view.
> 
> The truth is that the greatest part of the qcow2 functions can be called
> from eiher coroutine or non-coroutine context. You get coroutine context
> for read/write/discard/flush, but anything else like doing snapshots,
> resizing, preallocating the image, writing compressed data also accesses
> the same metadata management functions outside coroutines.
> 
> It's only getting worse for function like bdrv_pwrite().

A built-time check for coroutine_fn would be valuable if we ever hope to
get disciplined with this annotation.

The check can detect when a coroutine_fn is invoked outside coroutine
context.  I wonder if Coccinelle can detect this, although I never
figured out how to use it as a grep-like tool instead of just a
patch-like tool.

Stefan

Re: [Qemu-devel] RFC [PATCH] Make bdrv_flush synchronous only and update callers

[Gabriel Kerneis]

On Tue, Jul 23, 2013 at 02:05:15PM +0200, Stefan Hajnoczi wrote:
> A built-time check for coroutine_fn would be valuable if we ever hope to
> get disciplined with this annotation.
> 
> The check can detect when a coroutine_fn is invoked outside coroutine
> context.  I wonder if Coccinelle can detect this, although I never
> figured out how to use it as a grep-like tool instead of just a
> patch-like tool.

The recent "cps-inference" branch of CPC enables precisely that kind of check.
Charlie is using it to drive his modifications to QEMU and has already suggested
several improvements that I have implemented.  Hopefully we should reach
something fully covering QEMU by the end his GSoC.

If there is interest, I can post a script showing how to build it and use it to
check QEMU annotations (it does not require any modifications to QEMU, only a
couple of configure switches).

Best regards,
-- 
Gabriel

Re: [Qemu-devel] RFC [PATCH] Make bdrv_flush synchronous only and update callers

[Stefan Hajnoczi]

On Tue, Jul 23, 2013 at 01:10:35PM +0100, Gabriel Kerneis wrote:
> On Tue, Jul 23, 2013 at 02:05:15PM +0200, Stefan Hajnoczi wrote:
> > A built-time check for coroutine_fn would be valuable if we ever hope to
> > get disciplined with this annotation.
> > 
> > The check can detect when a coroutine_fn is invoked outside coroutine
> > context.  I wonder if Coccinelle can detect this, although I never
> > figured out how to use it as a grep-like tool instead of just a
> > patch-like tool.
> 
> The recent "cps-inference" branch of CPC enables precisely that kind of check.
> Charlie is using it to drive his modifications to QEMU and has already suggested
> several improvements that I have implemented.  Hopefully we should reach
> something fully covering QEMU by the end his GSoC.
> 
> If there is interest, I can post a script showing how to build it and use it to
> check QEMU annotations (it does not require any modifications to QEMU, only a
> couple of configure switches).

What is the status of CPC packaging in distros?

Stefan

Re: [Qemu-devel] RFC [PATCH] Make bdrv_flush synchronous only and update callers

[Gabriel Kerneis]

On Tue, Jul 23, 2013 at 03:36:28PM +0200, Stefan Hajnoczi wrote:
> On Tue, Jul 23, 2013 at 01:10:35PM +0100, Gabriel Kerneis wrote:
> > On Tue, Jul 23, 2013 at 02:05:15PM +0200, Stefan Hajnoczi wrote:
> > > A built-time check for coroutine_fn would be valuable if we ever hope to
> > > get disciplined with this annotation.
> > > 
> > > The check can detect when a coroutine_fn is invoked outside coroutine
> > > context.  I wonder if Coccinelle can detect this, although I never
> > > figured out how to use it as a grep-like tool instead of just a
> > > patch-like tool.
> > 
> > The recent "cps-inference" branch of CPC enables precisely that kind of check.
> > Charlie is using it to drive his modifications to QEMU and has already suggested
> > several improvements that I have implemented.  Hopefully we should reach
> > something fully covering QEMU by the end his GSoC.
> > 
> > If there is interest, I can post a script showing how to build it and use it to
> > check QEMU annotations (it does not require any modifications to QEMU, only a
> > couple of configure switches).
> 
> What is the status of CPC packaging in distros?

There will be an opam [1] package for it before the end of the week (based on
the CIL release planned for tomorrow).  I've also been working on a Debian
package but it's not ready yet unfortunately.

[1] http://opam.ocamlpro.com/

-- 
Gabriel

Re: [Qemu-devel] RFC [PATCH] Make bdrv_flush synchronous only and update callers

[Wenchao Xia]

  I am glad to have an accurate sync bdrv_flush(). Code looks fine.

Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

> This patch makes bdrv_flush a synchronous function and updates any callers from
> a coroutine context to use bdrv_co_flush instead.
> 
> The motivation for this patch comes from the GSoC Continuation-Passing C
> project. When coroutines were introduced, synchronous functions in the block
> layer were converted to use asynchronous methods by dynamically detecting if
> they were being run from a coroutine context by calling qemu_in_coroutine(), and
> yielding if so. If they were not, they would spawn a new coroutine and poll
> until the asynchronous counterpart finished.
> 
> However this approach does not work with CPC as the CPC translator converts all
> functions annotated coroutine_fn to a different (continuation based) calling
> convention. This means that coroutine_fn annotated functions cannot be called
> from a non-coroutine context.
> 
> This patch is a Request For Comments on the approach of splitting these
> "dynamic" functions into synchronous and asynchronous versions. This is easy for
> bdrv_flush as it already has an asynchronous counterpart - bdrv_co_flush. The
> only caller of bdrv_flush from a coroutine context is mirror_drain in
> block/mirror.c - this should be annotated as a coroutine_fn as it calls
> qemu_coroutine_yield().
> 
> If this approach meets with approval I will develop a patchset splitting the
> other "dynamic" functions in the block layer. This will allow all coroutine
> functions to have a coroutine_fn annotation that can be statically checked (CPC
> can be used to verify annotations).
> 
> I have audited the other callers of bdrv_flush, they are included below:
> 
> block.c: bdrv_reopen_prepare, bdrv_close, bdrv_commit, bdrv_pwrite_sync
> block/qcow2-cache.c: qcow2_cache_entry_flush, qcow2_cache_flush
> block/qcow2-refcount.c: qcow2_update_snapshot_refcount
> block/qcow2-snapshot.c: qcow2_write_snapshots
> block/qcow2.c: qcow2_mark_dirty, qcow2_mark_clean
> block/qed-check.c: qed_check_mark_clean
> block/qed.c: bdrv_qed_open, bdrv_qed_close
> blockdev.c: external_snapshot_prepare, do_drive_del
> cpus.c: do_vm_stop
> hw/block/nvme.c: nvme_clear_ctrl
> qemu-io-cmds.c: flush_f
> savevm.c: bdrv_fclose
> 
> ---
>   block.c        | 13 ++++---------
>   block/mirror.c |  4 ++--
>   2 files changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 6c493ad..00d71df 100644
> --- a/block.c
> +++ b/block.c
> @@ -4110,15 +4110,10 @@ int bdrv_flush(BlockDriverState *bs)
>           .ret = NOT_DONE,
>       };
> 
> -    if (qemu_in_coroutine()) {
> -        /* Fast-path if already in coroutine context */
> -        bdrv_flush_co_entry(&rwco);
> -    } else {
> -        co = qemu_coroutine_create(bdrv_flush_co_entry);
> -        qemu_coroutine_enter(co, &rwco);
> -        while (rwco.ret == NOT_DONE) {
> -            qemu_aio_wait();
> -        }
> +    co = qemu_coroutine_create(bdrv_flush_co_entry);
> +    qemu_coroutine_enter(co, &rwco);
> +    while (rwco.ret == NOT_DONE) {
> +        qemu_aio_wait();
>       }
> 
>       return rwco.ret;
> diff --git a/block/mirror.c b/block/mirror.c
> index bed4a7e..3d5da7e 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -282,7 +282,7 @@ static void mirror_free_init(MirrorBlockJob *s)
>       }
>   }
> 
> -static void mirror_drain(MirrorBlockJob *s)
> +static void coroutine_fn mirror_drain(MirrorBlockJob *s)
>   {
>       while (s->in_flight > 0) {
>           qemu_coroutine_yield();
> @@ -390,7 +390,7 @@ static void coroutine_fn mirror_run(void *opaque)
>           should_complete = false;
>           if (s->in_flight == 0 && cnt == 0) {
>               trace_mirror_before_flush(s);
> -            ret = bdrv_flush(s->target);
> +            ret = bdrv_co_flush(s->target);
>               if (ret < 0) {
>                   if (mirror_error_action(s, false, -ret) == BDRV_ACTION_REPORT) {
>                       goto immediate_exit;
> 


-- 
Best Regards

Wenchao Xia