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

[Paolo Bonzini <pbonzini@redhat.com>]

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.

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.

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.

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.

Paolo