Re: [PATCH v2 2/7] block/block-copy: use block_status

[Andrey Shinkevich]

On 27/11/2019 21:08, Vladimir Sementsov-Ogievskiy wrote:
> Use bdrv_block_status_above to chose effective chunk size and to handle
> zeroes effectively.
> 
> This substitutes checking for just being allocated or not, and drops
> old code path for it. Assistance by backup job is dropped too, as
> caching block-status information is more difficult than just caching
> is-allocated information in our dirty bitmap, and backup job is not
> good place for this caching anyway.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>   block/block-copy.c | 67 +++++++++++++++++++++++++++++++++++++---------
>   block/trace-events |  1 +
>   2 files changed, 55 insertions(+), 13 deletions(-)
> 
> diff --git a/block/block-copy.c b/block/block-copy.c
> index 8602e2cae7..74295d93d5 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
> @@ -152,7 +152,7 @@ void block_copy_set_callbacks(
>    */
>   static int coroutine_fn block_copy_do_copy(BlockCopyState *s,
>                                              int64_t start, int64_t end,
> -                                           bool *error_is_read)
> +                                           bool zeroes, bool *error_is_read)
>   {
>       int ret;
>       int nbytes = MIN(end, s->len) - start;
> @@ -162,6 +162,18 @@ static int coroutine_fn 
> block_copy_do_copy(BlockCopyState *s,
>       assert(QEMU_IS_ALIGNED(end, s->cluster_size));
>       assert(end < s->len || end == QEMU_ALIGN_UP(s->len, s->cluster_size));
>   
> +    if (zeroes) {
> +        ret = bdrv_co_pwrite_zeroes(s->target, start, nbytes, s->write_flags 
> &
> +                                    ~BDRV_REQ_WRITE_COMPRESSED);
> +        if (ret < 0) {
> +            trace_block_copy_write_zeroes_fail(s, start, ret);
> +            if (error_is_read) {
> +                *error_is_read = false;
> +            }
> +        }
> +        return ret;
> +    }
> +
>       if (s->use_copy_range) {
>           ret = bdrv_co_copy_range(s->source, start, s->target, start, nbytes,
>                                    0, s->write_flags);
> @@ -225,6 +237,34 @@ out:
>       return ret;
>   }
>   
> +static int block_copy_block_status(BlockCopyState *s, int64_t offset,
> +                                   int64_t bytes, int64_t *pnum)
> +{
> +    int64_t num;
> +    BlockDriverState *base;
> +    int ret;
> +
> +    if (s->skip_unallocated && s->source->bs->backing) {
> +        base = s->source->bs->backing->bs;
> +    } else {
> +        base = NULL;
> +    }
> +
> +    ret = bdrv_block_status_above(s->source->bs, base, offset, bytes, &num,
> +                                  NULL, NULL);
> +    if (ret < 0 || num < s->cluster_size) {
> +        num = s->cluster_size;

/* Let the cluster be copied in case of error too */
> +        ret = BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_DATA;
> +    } else if (offset + num == s->len) {
> +        num = QEMU_ALIGN_UP(num, s->cluster_size);
> +    } else {
> +        num = QEMU_ALIGN_DOWN(num, s->cluster_size);
> +    }
> +
> +    *pnum = num;
> +    return ret;
> +}
> +
>   /*
>    * Check if the cluster starting at offset is allocated or not.
>    * return via pnum the number of contiguous clusters sharing this 
> allocation.
> @@ -301,7 +341,6 @@ int coroutine_fn block_copy(BlockCopyState *s,
>   {
>       int ret = 0;
>       int64_t end = bytes + start; /* bytes */
> -    int64_t status_bytes;
>       BlockCopyInFlightReq req;
>   
>       /*
> @@ -318,7 +357,7 @@ int coroutine_fn block_copy(BlockCopyState *s,
>       block_copy_inflight_req_begin(s, &req, start, end);
>   
>       while (start < end) {
> -        int64_t next_zero, chunk_end;
> +        int64_t next_zero, chunk_end, status_bytes;
>   
>           if (!bdrv_dirty_bitmap_get(s->copy_bitmap, start)) {
>               trace_block_copy_skip(s, start);
> @@ -336,23 +375,25 @@ int coroutine_fn block_copy(BlockCopyState *s,
>               chunk_end = next_zero;
>           }
>   
> -        if (s->skip_unallocated) {
> -            ret = block_copy_reset_unallocated(s, start, &status_bytes);
> -            if (ret == 0) {
> -                trace_block_copy_skip_range(s, start, status_bytes);
> -                start += status_bytes;
> -                continue;
> -            }
> -            /* Clamp to known allocated region */
> -            chunk_end = MIN(chunk_end, start + status_bytes);
> +        ret = block_copy_block_status(s, start, chunk_end - start,
> +                                      &status_bytes);
> +        if (s->skip_unallocated && !(ret & BDRV_BLOCK_ALLOCATED)) {
> +            bdrv_reset_dirty_bitmap(s->copy_bitmap, start, status_bytes);
> +            s->progress_reset_callback(s->progress_opaque);
> +            trace_block_copy_skip_range(s, start, status_bytes);
> +            start += status_bytes;
> +            continue;
>           }
>   
> +        chunk_end = MIN(chunk_end, start + status_bytes);
> +
>           trace_block_copy_process(s, start);
>   
>           bdrv_reset_dirty_bitmap(s->copy_bitmap, start, chunk_end - start);
>   
>           co_get_from_shres(s->mem, chunk_end - start);
> -        ret = block_copy_do_copy(s, start, chunk_end, error_is_read);
> +        ret = block_copy_do_copy(s, start, chunk_end, ret & BDRV_BLOCK_ZERO,
> +                                 error_is_read);
>           co_put_to_shres(s->mem, chunk_end - start);
>           if (ret < 0) {
>               bdrv_set_dirty_bitmap(s->copy_bitmap, start, chunk_end - start);
> diff --git a/block/trace-events b/block/trace-events
> index 6ba86decca..346537a1d2 100644
> --- a/block/trace-events
> +++ b/block/trace-events
> @@ -48,6 +48,7 @@ block_copy_process(void *bcs, int64_t start) "bcs %p start 
> %"PRId64
>   block_copy_copy_range_fail(void *bcs, int64_t start, int ret) "bcs %p start 
> %"PRId64" ret %d"
>   block_copy_read_fail(void *bcs, int64_t start, int ret) "bcs %p start 
> %"PRId64" ret %d"
>   block_copy_write_fail(void *bcs, int64_t start, int ret) "bcs %p start 
> %"PRId64" ret %d"
> +block_copy_write_zeroes_fail(void *bcs, int64_t start, int ret) "bcs %p 
> start %"PRId64" ret %d"
>   
>   # ../blockdev.c
>   qmp_block_job_cancel(void *job) "job %p"
> 

Reviewed-by: Andrey Shinkevich <address@hidden>
-- 
With the best regards,
Andrey Shinkevich