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 <[email protected]> >>> --- >>> 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?
>>
>> 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.
Furthermore, it sounds a bit strange to imply that there’d be a strict
separation between block-copy and its user. They work tightly together
on this. I don’t think it would hurt to be more concrete on what the
backup job does here instead of just alluding to it as being “the user”.
[...]
>>> @@ -415,16 +535,16 @@ static void backup_abort(Job *job)
>>> static void backup_clean(Job *job)
>>> {
>>> BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
>>> - BlockDriverState *bs = blk_bs(s->common.blk);
>>> + BlockCopyState *bcs = s->bcs;
>>>
>>> - if (s->copy_bitmap) {
>>> - bdrv_release_dirty_bitmap(bs, s->copy_bitmap);
>>> - s->copy_bitmap = NULL;
>>> - }
>>> + /*
>>> + * Zero pointer first, to not interleave with backup_drain during some
>>> + * yield. TODO: just block_copy_state_free(s->bcs) after backup_drain
>>> + * dropped.
>>> + */
>>
>> I suppose that‘s now. :-)
>
> Hmm, it's in Kevin's branch. Should I rebase on it?
Yep, that’s what I meant.
Max
signature.asc
Description: OpenPGP digital signature
