qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Kevin Wolf
Subject: Re: [Qemu-devel] RFC [PATCH] Make bdrv_flush synchronous only and update callers
Date: Fri, 19 Jul 2013 10:37:11 +0200
User-agent: Mutt/1.5.21 (2010-09-15)

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]