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

[Vladimir Sementsov-Ogievskiy]

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

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

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

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

However, I'd prefer block-copy to be separate enough and have clear interface,
still these series don't bring it to this final point.

> 
> [...]
> 
>>>> @@ -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
> 


-- 
Best regards,
Vladimir