Re: [PATCH 6/6] block/block-copy: increase buffered copy request

[Max Reitz]

On 03.10.19 19:15, Vladimir Sementsov-Ogievskiy wrote:
> No reason to limit buffered copy to one cluster. Let's allow up to 1
> MiB.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  include/block/block-copy.h |  2 +-
>  block/block-copy.c         | 44 +++++++++++++++++++++++++++-----------
>  2 files changed, 32 insertions(+), 14 deletions(-)


Er, oops, looks like I was a bit quick there...

> @@ -100,17 +101,28 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, 
> BdrvChild *target,
>          .mem = qemu_co_shared_amount_new(BLOCK_COPY_MAX_MEM),
>      };
>  
> -    s->copy_range_size = QEMU_ALIGN_DOWN(max_transfer, cluster_size),
> -    /*
> -     * Set use_copy_range, consider the following:
> -     * 1. Compression is not supported for copy_range.
> -     * 2. copy_range does not respect max_transfer (it's a TODO), so we 
> factor
> -     *    that in here. If max_transfer is smaller than the 
> job->cluster_size,
> -     *    we do not use copy_range (in that case it's zero after aligning 
> down
> -     *    above).
> -     */
> -    s->use_copy_range =
> -        !(write_flags & BDRV_REQ_WRITE_COMPRESSED) && s->copy_range_size > 0;
> +    if (max_transfer < cluster_size) {
> +        /*
> +         * copy_range does not respect max_transfer. We don't want to bother
> +         * with requests smaller than block-copy cluster size, so fallback to
> +         * buffered copying (read and write respect max_transfer on their
> +         * behalf).
> +         */
> +        s->use_copy_range = false;
> +        s->copy_size = cluster_size;
> +    } else if (write_flags & BDRV_REQ_WRITE_COMPRESSED) {
> +        /* Compression is not supported for copy_range */
> +        s->use_copy_range = false;
> +        s->copy_size = MAX(cluster_size, BLOCK_COPY_MAX_BUFFER);
> +    } else {
> +        /*
> +         * copy_range does not respect max_transfer (it's a TODO), so we 
> factor
> +         * that in here.
> +         */
> +        s->use_copy_range = true;
> +        s->copy_size = MIN(MAX(cluster_size, BLOCK_COPY_MAX_COPY_RANGE),

This is already part of max_transfer, isn’t it?

(That doesn’t make it wrong, but I think max_transfer will always be
less than or equal to MAX(cluster_size, BLOCK_COPY_MAX_COPY_RANGE) anyway.)

Max

> +                           QEMU_ALIGN_DOWN(max_transfer, cluster_size));

signature.asc
Description: OpenPGP digital signature