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