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

[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?

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