[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/4] block: unify flush implementations
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH 2/4] block: unify flush implementations |
Date: |
Fri, 14 Oct 2011 16:02:44 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:7.0) Gecko/20110927 Thunderbird/7.0 |
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<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.
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
- [Qemu-devel] [PATCH 0/4] coroutinization of flush and discard (split out of NBD series), Paolo Bonzini, 2011/10/14
- [Qemu-devel] [PATCH 1/4] block: rename bdrv_co_rw_bh, Paolo Bonzini, 2011/10/14
- [Qemu-devel] [PATCH 3/4] block: drop redundant bdrv_flush implementation, Paolo Bonzini, 2011/10/14
- [Qemu-devel] [PATCH 2/4] block: unify flush implementations, Paolo Bonzini, 2011/10/14
- Re: [Qemu-devel] [PATCH 2/4] block: unify flush implementations, Stefan Hajnoczi, 2011/10/14
- Re: [Qemu-devel] [PATCH 2/4] block: unify flush implementations, Paolo Bonzini, 2011/10/14
[Qemu-devel] [PATCH 4/4] block: add bdrv_co_discard and bdrv_aio_discard support, Paolo Bonzini, 2011/10/14