> Task size was capped to block_copy_chunk_size() regardless of method,
> which sizes a task by the copy buffer. A write-zeroes task carries no
> buffer, so that cap only splits one long known-zero run into a crowd of
> small COPY_WRITE_ZEROES tasks, each with its own request against the
> target.
>
> block_copy_task_create() already decides from zero_bitmap how far a task
> may run, so add block_copy_widen_zero_area() to that decision: for a
> write-zeroes task, re-search the dirty area under BDRV_REQUEST_MAX_BYTES
> instead of the buffer chunk size, and let the existing clamp cut it back
> to where the zero run ends. That bound is INT_MAX rounded down to a
> sector, so it is aligned down at the use site, nothing bounding
> cluster_size. Widening is safe: an overlapping caller waits on the
> task's BlockReq via reqlist_wait_one() rather than observing it
> mid-flight.
>
> A request that large is only cheap where the target zeroes by metadata.
> BDRV_REQ_NO_FALLBACK in supported_zero_flags rules out the targets which
> cannot, qcow2 v2 and iscsi among them, but it is optimistic for the rest:
> file-posix advertises it at open and only learns from a failing
> fallocate. So the first widened request also asks for it, which costs
> nothing when the target obliges and fails without writing when it does
> not. The run then keeps every write-zeroes request at the buffer chunk
> size, and the range that found out is written in those chunks.
>
> Signed-off-by: Denis V. Lunev <[email protected]>
> CC: Vladimir Sementsov-Ogievskiy <[email protected]>
> CC: John Snow <[email protected]>
> CC: Andrey Drobyshev <[email protected]>
>
> diff --git a/block/block-copy.c b/block/block-copy.c
> index c0a33599384..16f3d048195 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
> @@ -38,6 +38,12 @@
> #define BLOCK_COPY_SLICE_TIME 100000000ULL /* ns */
> #define BLOCK_COPY_CLUSTER_SIZE_DEFAULT (1 << 16)
>
> +typedef enum {
> + ZERO_WIDEN_UNPROBED,
> + ZERO_WIDEN_ON,
> + ZERO_WIDEN_OFF,
> +} BlockCopyZeroWiden;
> +
> typedef enum {
> COPY_READ_WRITE_CLUSTER,
> COPY_READ_WRITE,
> @@ -159,6 +165,8 @@ typedef struct BlockCopyState {
> bool skip_unallocated; /* atomic */
> /* State fields that use a thread-safe API */
> BdrvDirtyBitmap *copy_bitmap;
> + /* Whether the target zeroes by metadata; see block_copy_write_zeroes().
> */
> + BlockCopyZeroWiden zero_widen; /* atomic */
> /* Clusters reading as zero; allocated on demand, frozen once valid. */
> HBitmap *zero_bitmap;
> /* Published only after the scan, with skip_unallocated already false. */
> @@ -187,6 +195,34 @@ static int64_t block_copy_chunk_size(BlockCopyState *s)
> }
> }
>
> +/*
> + * A write-zeroes task carries no buffer, so it may cover far more than
> + * block_copy_chunk_size(). Return how far it may run; the caller clamps it
> + * to where the zero run ends.
> + */
> +static int64_t block_copy_widen_zero_area(BlockCopyState *s,
> + BlockCopyCallState *call_state,
> + int64_t offset, int64_t search_end,
> + int64_t bytes)
> +{
> + int64_t aligned = QEMU_ALIGN_DOWN(BDRV_REQUEST_MAX_BYTES,
> + s->cluster_size);
> + int64_t zero_chunk = MIN_NON_ZERO(MAX(aligned, s->cluster_size),
> + call_state->max_chunk);
> + int64_t wide_offset, wide_bytes;
> +
> + if (!bdrv_dirty_bitmap_next_dirty_area(s->copy_bitmap, offset,
> search_end,
> + zero_chunk, &wide_offset,
> + &wide_bytes)) {
> + return bytes;
> + }
> +
> + /* @offset is dirty, so the search cannot have moved past it. */
> + assert(wide_offset == offset);
> +
> + return wide_bytes;
> +}
> +
> /*
> * Search for the first dirty area in offset/bytes range and create task at
> * the beginning of it.
> @@ -198,6 +234,7 @@ block_copy_task_create(BlockCopyState *s,
> BlockCopyCallState *call_state,
> BlockCopyTask *task;
> BlockCopyMethod method;
> int64_t max_chunk;
> + int64_t search_end = offset + bytes;
>
> QEMU_LOCK_GUARD(&s->lock);
> max_chunk = MIN_NON_ZERO(block_copy_chunk_size(s),
> call_state->max_chunk);
> @@ -219,6 +256,10 @@ block_copy_task_create(BlockCopyState *s,
> BlockCopyCallState *call_state,
>
> if (hbitmap_get(s->zero_bitmap, offset)) {
> method = COPY_WRITE_ZEROES;
> + if (qatomic_read(&s->zero_widen) != ZERO_WIDEN_OFF) {
There's a potential race here. Imagine:
1) task A is created and widened here;
2) task B is created and widened here;
3) in block_copy_write_zeroes() we get -ENOTSUP on task A and
set zero_widen=OFF.
4) in block_copy_write_zeroes() OFF != UNPROBED, we skip the probing
and go via slow path.
The race is minor, but still. Maybe we don't need tristate enum here
and might just make zero_widen a bool, so every fast-path request
carries NO_FALLBACK (not just the 1st request)?
> + bytes = block_copy_widen_zero_area(s, call_state, offset,
> + search_end, bytes);
> + }
> boundary = hbitmap_next_zero(s->zero_bitmap, offset, bytes);
> } else {
> boundary = hbitmap_next_dirty(s->zero_bitmap, offset, bytes);
> @@ -465,6 +506,8 @@ BlockCopyState *block_copy_state_new(BdrvChild *source,
> BdrvChild *target,
> .max_transfer = QEMU_ALIGN_DOWN(
> block_copy_max_transfer(source, target),
> cluster_size),
> + .zero_widen = target->bs->supported_zero_flags &
> BDRV_REQ_NO_FALLBACK ?
> + ZERO_WIDEN_UNPROBED : ZERO_WIDEN_OFF,
> };
>
> s->discard_source = discard_source;
> @@ -521,6 +564,54 @@ static coroutine_fn int block_copy_task_run(AioTaskPool
> *pool,
> return 0;
> }
>
> +/*
> + * Widening a write-zeroes request only pays off where the target zeroes by
> + * metadata. Ask the first one to fail instead of falling back to writing the
> + * zeroes out: it then costs nothing, and a target which would have written
> + * them keeps its requests small for the rest of the run.
> + */
> +static int coroutine_fn GRAPH_RDLOCK
> +block_copy_write_zeroes(BlockCopyState *s, int64_t offset, int64_t bytes,
> + bool *error_is_read)
> +{
> + BdrvRequestFlags flags = s->write_flags & ~BDRV_REQ_WRITE_COMPRESSED;
> + int64_t chunk = bytes;
> + int ret = 0;
> +
> + if (qatomic_read(&s->zero_widen) == ZERO_WIDEN_UNPROBED) {
> + ret = bdrv_co_pwrite_zeroes(s->target, offset, bytes,
> + flags | BDRV_REQ_NO_FALLBACK);
> + if (ret != -ENOTSUP) {
And what if it's -EIO? Why not 'if (ret == 0)'?
> + qatomic_set(&s->zero_widen, ZERO_WIDEN_ON);
> + goto out;
> + }
> +
> + /* Nothing has been written, so the whole range is still to do. */
> + qatomic_set(&s->zero_widen, ZERO_WIDEN_OFF);
> + chunk = block_copy_chunk_size(s);
block_copy_chunk_size() own doc says "Called with lock held", but we
aren't doing so here. Should we?
Andrey
--
Andrey Drobyshev <[email protected]>
