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

[Max Reitz]

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 <address@hidden>
>>>>>>> ---
>>>>>>>     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.”

Max

signature.asc
Description: OpenPGP digital signature