On 29.09.26 18:51, Denis V. Lunev wrote:
From: Denis V. Lunev <[email protected]>
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 nothing has aligned it to cluster_size and it is aligned down
at the use site. 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 a write-zeroes request asks for it while the target is
believed to oblige, which costs nothing when it does. A refusal ends the
widening, and the range is written in buffer-sized chunks, as is any
task widened before the refusal came. -ENOTSUP says the fast path is
unavailable, not that the target wrote nothing, so the range is
rewritten whole: zeroes over zeroes change nothing.
This may work bad with speed= parameter: big zero request will be accounted
as data, blocking further requests. Seems, we should not account fast-zeroes
as data.
Signed-off-by: Denis V. Lunev <[email protected]>
CC: Vladimir Sementsov-Ogievskiy <[email protected]>
CC: John Snow <[email protected]>
CC: Andrey Drobyshev <[email protected]>
---
block/block-copy.c | 94 ++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 87 insertions(+), 7 deletions(-)
diff --git a/block/block-copy.c b/block/block-copy.c
index c0a3359938..94ca2a10ed 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -159,6 +159,8 @@ typedef struct BlockCopyState {
bool skip_unallocated; /* atomic */
/* State fields that use a thread-safe API */
BdrvDirtyBitmap *copy_bitmap;
+ /* Whether a write-zeroes task may widen; see block_copy_write_zeroes(). */
+ bool 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 +189,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),
aligned < s->cluster_size case looks like a bug. I think we don't supprt
cluser_size > BDRV_REQUEST_MAX_BYTES anyway.
+ call_state->max_chunk);
max_chunk comes from experimental backup QAPI parameter x-perf.max_chunk. I
don't
think we should limit fast-zero requests by it. And it's unstable, so we can
change
the contract.
+ 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)) {
offset must be dirty at this point.. so bdrv_dirty_bitmap_next_zero() would be
enough,
we don't need bdrv_dirty_bitmap_next_dirty_area().
+ 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 +228,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 +250,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)) {
+ 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 +500,7 @@ 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,
};
s->discard_source = discard_source;
@@ -521,6 +557,56 @@ 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, so while the target is believed to oblige, a request asks to fail
+ * instead of falling back to writing the zeroes out. That costs nothing when
+ * it does oblige, and a refusal ends the widening 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;
+ int ret = 0;
+
+ if (qatomic_read(&s->zero_widen)) {
+ ret = bdrv_co_pwrite_zeroes(s->target, offset, bytes,
+ flags | BDRV_REQ_NO_FALLBACK);
+ if (ret != -ENOTSUP) {
+ goto out;
+ }
+
+ /* Redoing the range is safe: zeroes over zeroes change nothing. */
+ qatomic_set(&s->zero_widen, false);
+ }
+
+ WITH_QEMU_LOCK_GUARD(&s->lock) {
+ chunk = block_copy_chunk_size(s);
+ }
+
+ while (bytes) {
+ int64_t n = MIN(bytes, chunk);
+
+ ret = bdrv_co_pwrite_zeroes(s->target, offset, n, flags);
That's a problem actually. We do synchronous writing of large area: we lose
parllelism (max-workers), and probably block guest writes to the while big
area, until it written.
+ if (ret < 0) {
+ break;
+ }
+
+ offset += n;
+ bytes -= n;
+ }
+
+out:
+ if (ret < 0) {
+ trace_block_copy_write_zeroes_fail(s, offset, ret);
+ *error_is_read = false;
+ }
+
+ return ret;
+}
+
/*
* block_copy_do_copy
*
@@ -552,13 +638,7 @@ block_copy_do_copy(BlockCopyState *s, int64_t offset,
int64_t bytes,
switch (*method) {
case COPY_WRITE_ZEROES:
- ret = bdrv_co_pwrite_zeroes(s->target, offset, nbytes, s->write_flags &
- ~BDRV_REQ_WRITE_COMPRESSED);
- if (ret < 0) {
- trace_block_copy_write_zeroes_fail(s, offset, ret);
- *error_is_read = false;
- }
- return ret;
+ return block_copy_write_zeroes(s, offset, nbytes, error_is_read);
case COPY_RANGE_SMALL:
case COPY_RANGE_FULL:
--
Best regards,
Vladimir
