Re: [Qemu-block] [PATCH v10 04/14] block/backup: introduce BlockCopyState

[Vladimir Sementsov-Ogievskiy]

10.09.2019 13:14, Max Reitz wrote:
> On 10.09.19 11:22, Vladimir Sementsov-Ogievskiy wrote:
> 
> [...]
> 
>> Ok, than
>>
>> 4. Postpone improvements for a follow-up (anyway, finally, block-copy should
>> use block_status to copy by larger chunks, like mirror does), and improve the
>> comment like this:
>>
>> """
>> Used for job sync=top mode, which currently works as follows (the size of the
>> comment definitely shows unclean design, but this is a TODO to improve it):
>> If job started in sync=top mode, which means that we want to copy only parts
>> allocated in top layer, job should behave like this:
>>
>> 1. Create block-copy state with skip_unallocated = true.
>> 2. Then, block_copy() will automatically check for allocation in top layer,
>> and do not copy areas which are not allocated in top layer. So, for example,
>> copy-before-write operations in backup works correctly even before [3.]
>> 3. Sequentially call block_copy_reset_unallocated() to cover the whole source
>> node, copy_bitmap will be updated correspondingly.
>> 4. Unset skip_unallocated variable in block-copy state, to avoid extra (as
>> everything is covered by [3.]) block-status queries in block_copy() calls
>> 5. Do sequential copying by loop of block_copy() calls, all needed allocation
>> information is already in copy_bitmap.
>>
>>   From block_copy() side, it behaves like this:
>> If skip_unallocated is set, block_copy() will reset in copy_bitmap areas
>> unallocated in top image (so they will not be copied). Whenever any such
>> area is cleared, progress_reset_callback will be invoked. Note, that
>> progress_reset_callback is called from block_copy_reset_unallocated() too.
>> """
> 
> Can this not be simplified?
> 
> """
> Used by sync=top jobs, which first scan the source node for unallocated
> areas and clear them in the copy_bitmap.  During this process, the
> bitmap is thus not fully initialized: It may still have bits set for
> areas that are unallocated and should actually not be copied.
> 
> This is indicated by skip_unallocated.
> 
> In this case, block_copy() will query the source’s allocation status,
> skip unallocated regions, clear them in the copy_bitmap, and invoke
> block_copy_reset_unallocated() every time it does.
> 
> Otherwise, block_copy() copies everything that’s dirty in the copy_bitmap.
> """
> 

OK, thanks)


-- 
Best regards,
Vladimir