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

[Kevin Wolf <kwolf@redhat.com>]

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<pbonzini@redhat.com>
>>
>> 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?
> 
> I thought about it, but then it turned out that I would have
> 
>          bdrv_flush
>          -> create coroutine or just fast-path to bdrv_flush_co_entry
>             -> bdrv_flush_co_entry
>                -> driver
> 
> and
> 
>          bdrv_co_flush
>          -> bdrv_flush_co_entry
>             -> driver
> 
> In other words, the code would be exactly the same, save for an "if 
> (qemu_in_coroutine())".  The reason is that, unlike read/write, neither 
> flush nor discard take a qiov.

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.

> In general, I think that with Stefan's cleanup having specialized 
> coroutine versions has in general a much smaller benefit.  The code 
> reading benefit of naming routines like bdrv_co_* is already lost, for 
> example, since bdrv_read can yield when called for coroutine context.

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.

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.

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

Kevin