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
