Re: [PATCH v11 04/14] block/backup: introduce BlockCopyState

[Vladimir Sementsov-Ogievskiy]

20.09.2019 16:26, Max Reitz wrote:
> On 20.09.19 14:56, Vladimir Sementsov-Ogievskiy wrote:
>> 20.09.2019 15:56, Vladimir Sementsov-Ogievskiy wrote:
>>> 20.09.2019 15:46, Max Reitz wrote:
>>>> On 13.09.19 20:25, Vladimir Sementsov-Ogievskiy wrote:
>>>>> 10.09.2019 13:23, 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>
>>>>>> ---
>>>>>
>>>>>
>>>>> [..]
>>>>>
>>>>>> +
>>>>>> +static BlockCopyState *block_copy_state_new(
>>>>>> +        BlockDriverState *source, BlockDriverState *target,
>>>>>> +        int64_t cluster_size, BdrvRequestFlags write_flags,
>>>>>> +        ProgressBytesCallbackFunc progress_bytes_callback,
>>>>>> +        ProgressResetCallbackFunc progress_reset_callback,
>>>>>> +        void *progress_opaque, Error **errp)
>>>>>> +{
>>>>>> +    BlockCopyState *s;
>>>>>> +    int ret;
>>>>>> +    uint64_t no_resize = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE |
>>>>>> +                         BLK_PERM_WRITE_UNCHANGED | BLK_PERM_GRAPH_MOD;
>>>>>> +    BdrvDirtyBitmap *copy_bitmap;
>>>>>> +
>>>>>> +    copy_bitmap = bdrv_create_dirty_bitmap(source, cluster_size, NULL, 
>>>>>> errp);
>>>>>> +    if (!copy_bitmap) {
>>>>>> +        return NULL;
>>>>>> +    }
>>>>>> +    bdrv_disable_dirty_bitmap(copy_bitmap);
>>>>>> +
>>>>>> +    s = g_new(BlockCopyState, 1);
>>>>>> +    *s = (BlockCopyState) {
>>>>>> +        .source = blk_new(bdrv_get_aio_context(source),
>>>>>> +                          BLK_PERM_CONSISTENT_READ, no_resize),
>>>>>> +        .target = blk_new(bdrv_get_aio_context(target),
>>>>>> +                          BLK_PERM_WRITE, no_resize),
>>>>>> +        .copy_bitmap = copy_bitmap,
>>>>>> +        .cluster_size = cluster_size,
>>>>>> +        .len = bdrv_dirty_bitmap_size(copy_bitmap),
>>>>>> +        .write_flags = write_flags,
>>>>>> +        .use_copy_range = !(write_flags & BDRV_REQ_WRITE_COMPRESSED),
>>>>>> +        .progress_bytes_callback = progress_bytes_callback,
>>>>>> +        .progress_reset_callback = progress_reset_callback,
>>>>>> +        .progress_opaque = progress_opaque,
>>>>>> +    };
>>>>>> +
>>>>>> +    s->copy_range_size = 
>>>>>> QEMU_ALIGN_UP(MIN(blk_get_max_transfer(s->source),
>>>>>> +                                           
>>>>>> blk_get_max_transfer(s->target)),
>>>>>> +                                       s->cluster_size);
>>>>>
>>>>> preexistent, but it obviously should be QEMU_ALIGN_DOWN. I can resend 
>>>>> with a separate
>>>>> fix, it may be fixed while queuing (if resend is not needed for other 
>>>>> reasons) or
>>>>> I'll send a follow-up fix later, whichever you prefer.
>>>>
>>>> Hm, true.  But then we’ll also need to handle the (unlikely, admittedly)
>>>> case where max_transfer < cluster_size so this would then return 0 (by
>>>> setting use_copy_range = false).  So how about this:
>>>
>>> Done in [PATCH v12 0/2] backup: copy_range fixes.
>>> If it is convenient I'll rebase these series on "[PATCH v12 0/2] backup: 
>>> copy_range fixes"
> 
> Oh, good.
> 
> I think taking copy_range fixes first would make more sense.  It seems
> that John still had some suggestion for it...?
> 
> Max
> 

Finally, only wording of the comment. I think, I can resend combined series 
with my final suggestion
of the comment (a bit tweaked John's one).

-- 
Best regards,
Vladimir