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