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

[Vladimir Sementsov-Ogievskiy]

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.

> +
> +    /*
> +     * We just allow aio context change on our block backends. block_copy() 
> user
> +     * (now it's only backup) is responsible for source and target being in 
> same
> +     * aio context.
> +     */
> +    blk_set_disable_request_queuing(s->source, true);
> +    blk_set_allow_aio_context_change(s->source, true);
> +    blk_set_disable_request_queuing(s->target, true);
> +    blk_set_allow_aio_context_change(s->target, true);
> +

[..]

> @@ -760,21 +860,19 @@ BlockJob *backup_job_create(const char *job_id, 
> BlockDriverState *bs,
>        * For more information see commit f8d59dfb40bb and test
>        * tests/qemu-iotests/222

[..]

>       job->cluster_size = cluster_size;
> -    job->copy_bitmap = copy_bitmap;
> -    copy_bitmap = NULL;
> -    job->use_copy_range = !compress; /* compression isn't supported for it */
> -    job->copy_range_size = 
> MIN_NON_ZERO(blk_get_max_transfer(job->common.blk),
> -                                        blk_get_max_transfer(job->target));
> -    job->copy_range_size = MAX(job->cluster_size,
> -                               QEMU_ALIGN_UP(job->copy_range_size,
> -                                             job->cluster_size));
> -
> -    /* Required permissions are already taken with target's blk_new() */
> +
> +    /* Required permissions are already taken by block-copy-state target */
>       block_job_add_bdrv(&job->common, "target", target, 0, BLK_PERM_ALL,
>                          &error_abort);
>       job->len = len;


-- 
Best regards,
Vladimir