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

[Kevin Wolf <kwolf@redhat.com>]

Am 14.10.2011 14:42, schrieb 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<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?
>>>
>> 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.

Right.

By the way, I like how you handle all three backends in the same
function. I think this is a lot more readable than the solution used by
read/write (changing the function pointers on driver registration).

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

You already have two of them (bdrv_co_flush for AIO and
bdrv_flush_co_entry for synchronous), so I don't think it makes a
difference there.

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

Well, depends on what you're looking at. The call chain length would
increase for AIO and synchronous bdrv_flush, but it would become shorter
for bdrv_co_flush.

If we want to declare coroutines as the preferred interface, I think
such a change makes sense.

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

I think I would make it public. The one that has to go eventually is the
synchronous bdrv_flush(). Which is another reason why I wouldn't design
everything around it.

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

Quite possible, yes.

Kevin