Re: [Qemu-devel] [PATCH 2/4] block: unify flush implementations

[Stefan Hajnoczi]

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