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

[Max Reitz]

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?  I don’t quite like leaving puzzling together the
bits to the reader, if we can avoid it.

Max

signature.asc
Description: OpenPGP digital signature