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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

10.09.2019 11:39, Max Reitz wrote:
> On 10.09.19 10:12, Vladimir Sementsov-Ogievskiy wrote:
>> 10.09.2019 10:42, Max Reitz wrote:
>>> On 09.09.19 17:11, Vladimir Sementsov-Ogievskiy wrote:
>>>> 09.09.2019 17:24, Max Reitz wrote:
>>>>> On 09.09.19 16:12, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> 09.09.2019 15:59, Max Reitz wrote:
>>>>>>> On 30.08.19 18:12, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>>> Split copying code part from backup to "block-copy", including separate
>>>>>>>> state structure and function renaming. This is needed to share it with
>>>>>>>> backup-top filter driver in further commits.
>>>>>>>>
>>>>>>>> Notes:
>>>>>>>>
>>>>>>>> 1. As BlockCopyState keeps own BlockBackend objects, remaining
>>>>>>>> job->common.blk users only use it to get bs by blk_bs() call, so clear
>>>>>>>> job->commen.blk permissions set in block_job_create and add
>>>>>>>> job->source_bs to be used instead of blk_bs(job->common.blk), to keep
>>>>>>>> it more clear which bs we use when introduce backup-top filter in
>>>>>>>> further commit.
>>>>>>>>
>>>>>>>> 2. Rename s/initializing_bitmap/skip_unallocated/ to sound a bit better
>>>>>>>> as interface to BlockCopyState
>>>>>>>>
>>>>>>>> 3. Split is not very clean: there left some duplicated fields, backup
>>>>>>>> code uses some BlockCopyState fields directly, let's postpone it for
>>>>>>>> further improvements and keep this comment simpler for review.
>>>>>>>>
>>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>>>>> ---
>>>>>>>>      block/backup.c     | 357 ++++++++++++++++++++++++++++-----------------
>>>>>>>>      block/trace-events |  12 +-
>>>>>>>>      2 files changed, 231 insertions(+), 138 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/block/backup.c b/block/backup.c
>>>>>>>> index abb5099fa3..002dee4d7f 100644
>>>>>>>> --- a/block/backup.c
>>>>>>>> +++ b/block/backup.c
>>>>>>>> @@ -35,12 +35,43 @@ typedef struct CowRequest {
>>>>>>>>          CoQueue wait_queue; /* coroutines blocked on this request */
>>>>>>>>      } CowRequest;
>>>>>>>>      
>>>>>>>> +typedef void (*ProgressBytesCallbackFunc)(int64_t bytes, void *opaque);
>>>>>>>> +typedef void (*ProgressResetCallbackFunc)(void *opaque);
>>>>>>>> +typedef struct BlockCopyState {
>>>>>>>> +    BlockBackend *source;
>>>>>>>> +    BlockBackend *target;
>>>>>>>> +    BdrvDirtyBitmap *copy_bitmap;
>>>>>>>> +    int64_t cluster_size;
>>>>>>>> +    bool use_copy_range;
>>>>>>>> +    int64_t copy_range_size;
>>>>>>>> +    uint64_t len;
>>>>>>>> +
>>>>>>>> +    BdrvRequestFlags write_flags;
>>>>>>>> +
>>>>>>>> +    /*
>>>>>>>> +     * skip_unallocated: if true, on copy operation firstly reset areas
>>>>>>>> +     * unallocated in top layer of source (and then of course don't copy
>>>>>>>> +     * corresponding clusters). If some bytes reset, call
>>>>>>>> +     * progress_reset_callback.
>>>>>>>> +     */
>>>>>>>
>>>>>>> It isn’t quite clear that this refers to the copy_bitmap.  Maybe
>>>>>>> something like
>>>>>>>
>>>>>>> “If true, the copy operation prepares a sync=top job: It scans the
>>>>>>
>>>>>> hmm, now it's not refactored to scan it before copying loop, so it's not precise
>>>>>> wording..
>>>>>>
>>>>>>> source's top layer to find all unallocated areas and resets them in the
>>>>>>
>>>>>> Not all, but mostly inside block-copy requested area (but may be more)
>>>>>
>>>>> Sorry, I meant backup operation.
>>>>>
>>>>>>> copy_bitmap (so they will not be copied).  Whenever any such area is
>>>>>>> cleared, progress_reset_callback will be invoked.
>>>>>>> Once the whole top layer has been scanned, skip_unallocated is cleared
>>>>>>> and the actual copying begins.”
>>>>>>
>>>>>> Last sentence sounds like it's a block-copy who will clear skip_unallocated,
>>>>>> but it's not so. It's not very good design and may be refactored in future,
>>>>>> but for now, I'd better drop last sentence.
>>>>>
>>>>> But wasn’t that the point of this variable?  That it goes back to false
>>>>> once the bitmap is fully initialized?
>>>>
>>>> I just want to stress, that block-copy itself (which will be in a separate file,
>>>> so it would be clean enough, what is block-copy and what is its user) do not clear
>>>> this variable. It of course may track calls to  block_copy_reset_unallocated() and
>>>> clear this variable automatically, but it don't do it now. And your wording looks
>>>> for me like block-copy code may automatically clear this variable
>>>
>>> Hm, OK.
>>>
>>>>>
>>>>>>>
>>>>>>> instead?
>>>>>>
>>>>>> Or, what about the following mix:
>>>>>>
>>>>>> Used for job sync=top mode. If true, 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. User is assumed to call in background
>>>>>> block_copy_reset_unallocated() several times to cover the whole copied disk and
>>>>>> then clear skip_unallocated, to prevent extra effort.
>>>>>
>>>>> I don’t know.  The point of this variable is the initialization of the
>>>>> bitmap.  block-copy only uses this as a hint that it needs to
>>>>> participate in that initialization process, too, and cannot assume the
>>>>> bitmap to be fully allocated.
>>>>
>>>> Hmm, but where is a conflict of this and my wording? IMHO, I just documented
>>>> exactly what's written in the code.
>>>
>>> There’s no conflict because it isn’t mentioned; which is the problem I
>>> have with it.
>>>
>>> What I was really confused about is who consumes the variable.  It all
>>> becomes much clearer when I take it as a given that all of your
>>> description just is an imperative to block-copy.  That simply wasn’t
>>> clear to me.  (Which is why you don’t like my description, precisely
>>> because it tells the story from another POV, namely that of backup.)
>>>
>>> Furthermore, I just miss the big picture about it.  Why does the
>>> variable even exist?
>>
>> Too keep it simpler for now, we can improve it as a follow-up. I see
>> several solutions:
>>
>> 1. track sequential calls to block_copy_reset_unallocated() and when
>> it comes to the disk end - clear the variable
>>
>> 2. don't publish block_copy_reset_unallocated() at all, assuming sequential
>> calls to block_copy() and do like in (1.)
>>
>> 3. keep additional bitmap to track, what was already explored about being
>> allocated/unallocated (seems too much)
> 
> Sorry, over some editing I accidentally removed the meaning from what I
> wrote there.  I didn’t mean literally “Why does the variable exist” or
> “I don’t understand the big picture”.  I meant “The comment does not
> explain the big picture, for example, it does not explain why the
> variable even exists.”
> 


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


-- 
Best regards,
Vladimir