On Fri, Oct 14, 2011 at 01:54:42PM +0200, Kevin Wolf wrote: > Am 14.10.2011 13:30, schrieb Paolo Bonzini: > > On 10/14/2011 01:08 PM, Kevin Wolf wrote: > >> Am 14.10.2011 10:41, schrieb Paolo Bonzini: > > Let me show how this might go. Right now you have > > > > bdrv_read/write > > -> bdrv_rw_co > > -> create qiov > > -> create coroutine or just fast-path to bdrv_rw_co_entry > > -> bdrv_rw_co_entry > > -> bdrv_co_do_readv/writev > > -> driver > > > > bdrv_co_readv/writev > > -> bdrv_co_do_readv/writev > > -> driver > > > > But starting from here, you might just as well reorganize it like this: > > > > bdrv_read/writev > > -> bdrv_rw_co > > -> create qiov > > -> bdrv_readv/writev > > > > bdrv_readv/writev > > -> create coroutine or just fast-path to bdrv_rw_co_entry > > -> bdrv_rw_co_entry > > -> bdrv_co_do_readv/writev > > -> driver > > > > and just drop bdrv_co_readv, since it would just be hard-coding the > > fast-path of bdrv_readv. > > I guess it's all a matter of taste. Stefan, what do you think? > > > Since some amount of synchronous I/O would likely always be there, for > > example in qemu-img, I think this unification would make more sense than > > providing two separate entrypoints for bdrv_co_flush and bdrv_flush. > > Actually, I'm not so sure about qemu-img. I think we have thought of > scenarios where converting it to a coroutine based version with a main > loop would be helpful (can't remember the details, though).
I'd like to completely remove synchronous bdrv_*(), including for qemu-img. It's just too tempting to call these functions in contexts where it is not okay to do so. The bdrv_co_*() functions are all tagged as coroutine_fn and make it clear that they can yield. We already have an event loop in qemu-img except it's the nested event loop in synchronous bdrv_*() emulation functions. The nested event loop is a mini event loop and can't really do things like timers. It would be nicer to remove it in favor of a single top-level event loop with the qemu-img code running in a coroutine. So I'm in favor of keeping bdrv_co_*() explicit for now and refactoring both hw/ and qemu-tool users of synchronous functions. Stefan
