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

[Paolo Bonzini]

On 10/14/2011 01:54 PM, 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:
> > > > Add coroutine support for flush and apply the same emulation that
> > > > we already do for read/write.  bdrv_aio_flush is simplified to always
> > > > go through a coroutine.
> > > >
> > > > Signed-off-by: Paolo Bonzini<address@hidden>
> > >
> > > To make the implementation more consistent with read/write operations,
> > > wouldn't it make sense to provide a bdrv_co_flush() globally instead of
> > > using the synchronous version as the preferred public interface?
> What I was thinking of looks a bit different:
>
>      bdrv_flush
>      ->  create coroutine or just fast-path to bdrv_flush_co_entry
>         ->  bdrv_flush_co_entry
>            ->  bdrv_co_flush
>
> and
>
>      bdrv_co_flush
>      ->  driver
>
> And the reason for this is that bdrv_co_flush would be a function that
> does only little more than passing the function to the driver (just like
> most bdrv_* functions do), with no emulation going on at all.

It would still host the checks on BDRV_O_NO_FLUSH and bs->drv->*_flush. 
 It would be the same as bdrv_flush_co_entry is now, minus the 
marshalling in/out of the RwCo.

> Instead of taking a void* and working on a RwCo structure that is really
> meant for emulation, bdrv_co_flush would take a BlockDriverState and
> improve readability this way.

I see.  Yeah, that's doable, but I'd still need two coroutines (one for 
bdrv_flush, one for bdrv_aio_flush) and the patch would be bigger overall...

> The more complicated and ugly code would be left separated and only used
> for emulation. I think that would make it easier to understand the
> common path without being distracted by emulation code.

... and on the other hand the length of the call chain would increse. 
It easily gets confusing, it already is for me in the read/write case.

Would bdrv_co_flush be static or not?  If not, you also get an 
additional entry point of dubious additional value, i.e. more complexity.

> 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).

qemu-img convert might benefit from multiple in-flight requests if on of 
the endpoints is remote or perhaps even sparse, I guess.

Paolo