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 <vsement...@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)
I think, I'll resend with [2.] soon, if no other ideas. Or I can leave it for
a follow-up.
I don’t quite like leaving puzzling together the
> bits to the reader, if we can avoid it.
>
> Max
>
--
Best regards,
Vladimir
