[libvirt] [Qemu-devel] QEMU interfaces for image streaming and post-copy block migration

[Avi Kivity]

  On 09/12/2010 03:25 PM, Anthony Liguori wrote:
> On 09/12/2010 07:41 AM, Avi Kivity wrote:
>>  On 09/07/2010 05:57 PM, Anthony Liguori wrote:
>>>> I agree that streaming should be generic, like block migration.  The
>>>> trivial generic implementation is:
>>>>
>>>> void bdrv_stream(BlockDriverState* bs)
>>>> {
>>>>      for (sector = 0; sector<  bdrv_getlength(bs); sector += n) {
>>>>          if (!bdrv_is_allocated(bs, sector,&n)) {
>>>
>>> Three problems here.  First problem is that bdrv_is_allocated is 
>>> synchronous. 
>>
>> Put the whole thing in a thread.
>
> It doesn't fix anything.  You don't want stream to serialize all I/O 
> operations.

Why would it serialize all I/O operations?  It's just like another vcpu 
issuing reads.

>
>>> The second problem is that streaming makes the most sense when it's 
>>> the smallest useful piece of work whereas bdrv_is_allocated() may 
>>> return a very large range.
>>>
>>> You could cap it here but you then need to make sure that cap is at 
>>> least cluster_size to avoid a lot of unnecessary I/O.
>>
>> That seems like a nice solution.  You probably want a multiple of the 
>> cluster size to retain efficiency.
>
> What you basically do is:
>
> stream_step_three():
>    complete()
>
> stream_step_two(offset, length):
>    bdrv_aio_readv(offset, length, buffer, stream_step_three)
>
> bdrv_aio_stream():
>     bdrv_aio_find_free_cluster(stream_step_two)

Isn't there a write() missing somewhere?

>
> And that's exactly what the current code looks like.  The only change 
> to the patch that this does is make some of qed's internals be block 
> layer interfaces.

Why do you need find_free_cluster()?  That's a physical offset thing.  
Just write to the same logical offset.

IOW:

     bdrv_aio_stream():
         bdrv_aio_read(offset, stream_2)

     stream_2():
         if all zeros:
             increment offset
             if more:
                 bdrv_aio_stream()
        bdrv_aio_write(offset, stream_3)

     stream_3():
         bdrv_aio_write(offset, stream_4)

     stream_4():
         increment offset
         if more:
              bdrv_aio_stream()


Of course, need to serialize wrt guest writes, which adds a bit more 
complexity.  I'll leave it to you to code the state machine for that.

>
> One of the things Stefan has mentioned is that a lot of the QED code 
> could be reused by other formats.  All formats implement things like 
> CoW on their own today but if you exposed interfaces like 
> bdrv_aio_find_free_cluster(), you could actually implement a lot more 
> in the generic block layer.
>
> So, I agree with you in principle that this all should be common 
> code.  I think it's a larger effort though.

Not that large I think; and it will make commit async as a side effect.

>>>
>>> The QED streaming implementation is 140 LOCs too so you quickly end 
>>> up adding more code to the block formats to support these new 
>>> interfaces than it takes to just implement it in the block format.
>>
>> bdrv_is_allocated() already exists (and is needed for commit), what 
>> else is needed?  cluster size?
>
> Synchronous implementations are not reusable to implement asynchronous 
> anything. 

Surely this is easy to fix, at least for qed.

What we need is thread infrastructure that allows us to convert between 
the two methods.

> But you need the code to be cluster aware too.

Yes, another variable in BlockDriverState.

>
>>> Third problem is that  streaming really requires being able to do 
>>> zero write detection in a meaningful way.  You don't want to always 
>>> do zero write detection so you need another interface to mark a 
>>> specific write as a write that should be checked for zeros.
>>
>> You can do that in bdrv_stream(), above, before the actual write, and 
>> call bdrv_unmap() if you detect zeros.
>
> My QED branch now does that FWIW.  At the moment, it only detects zero 
> reads to unallocated clusters and writes a special zero cluster 
> marker.  However, the detection code is in the generic path so once 
> the fsck() logic is working, we can implement a free list in QED.
>
> In QED, the detection code needs to have a lot of knowledge about 
> cluster boundaries and the format of the device.  In principle, this 
> should be common code but it's not for the same reason copy-on-write 
> is not common code today.

Parts of it are: commit.  Of course, that's horribly synchronous.

-- 
error compiling committee.c: too many arguments to function