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 <vsement...@virtuozzo.com>
> ---
> 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 <andrey.shinkev...@virtuozzo.com>
--
With the best regards,
Andrey Shinkevich
