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