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

[Max Reitz]

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?

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