On 9/1/26 15:37, Andrey Drobyshev wrote: >> block_copy_reset_unallocated()'s up-front scan already queries the >> source, and the same query reports zero-ness. The copy loop re-queries >> per task and, having no answer up front, can only size a task by the >> copy buffer, so a mostly-zero image is cut into thousands of pieces >> that each turn out to read as zero. >> >> Cache the answer in zero_bitmap, published by zero_bitmap_valid once >> the scan is done. block_copy_task_create() then picks the method from >> the bitmap and ends the task where the answer changes, so a zero task >> never reaches into data and a copy task never swallows a zero run. The >> per-task query stays as the fallback until the flag is set, since CBW >> intercepts guest writes while the scan is still running. >> >> The scan and the query it replaces must resolve BDRV_BLOCK_ZERO against >> the same part of the chain, so the scan moves from bdrv_co_is_allocated() >> to bdrv_co_block_status_above() and the base selection is factored out >> into block_copy_status_base(). >> >> zero_bitmap is a plain HBitmap, not a BdrvDirtyBitmap: an internal cache >> has no business in query-named-block-nodes. One writer and readers gated >> by the flag need no mutex, but the publish needs ordering, hence >> store-release and load-acquire. It is allocated on demand, so sync=none >> and a standalone copy-before-write filter do not pay for it. >> >> 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/backup.c b/block/backup.c >> index d4713fa1cd4..11d70243e24 100644 >> --- a/block/backup.c >> +++ b/block/backup.c >> @@ -280,6 +280,7 @@ static int coroutine_fn backup_run(Job *job, Error >> **errp) >> offset += count; >> } >> block_copy_set_skip_unallocated(s->bcs, false); >> + block_copy_set_zero_bitmap_valid(s->bcs); >> } >> >> if (s->sync_mode == MIRROR_SYNC_MODE_NONE) { >> diff --git a/block/block-copy.c b/block/block-copy.c >> index d71d070dbd0..94d4f3dd69a 100644 >> --- a/block/block-copy.c >> +++ b/block/block-copy.c >> @@ -20,6 +20,8 @@ >> #include "block/block_int-io.h" >> #include "block/dirty-bitmap.h" >> #include "block/reqlist.h" >> +#include "qemu/hbitmap.h" >> +#include "qemu/host-utils.h" >> #include "system/block-backend.h" >> #include "qemu/units.h" >> #include "qemu/co-shared-resource.h" >> @@ -157,6 +159,10 @@ typedef struct BlockCopyState { >> bool skip_unallocated; /* atomic */ >> /* State fields that use a thread-safe API */ >> BdrvDirtyBitmap *copy_bitmap; >> + /* Clusters reading as zero; allocated on demand, frozen once valid. */ >> + HBitmap *zero_bitmap; >> + /* Published only after the scan, with skip_unallocated already false. >> */ >> + bool zero_bitmap_valid; /* atomic, store-release/load-acquire */ >> ProgressMeter *progress; >> SharedResource *mem; >> RateLimit rate_limit; >> @@ -190,6 +196,7 @@ block_copy_task_create(BlockCopyState *s, >> BlockCopyCallState *call_state, >> int64_t offset, int64_t bytes) >> { >> BlockCopyTask *task; >> + BlockCopyMethod method; >> int64_t max_chunk; >> >> QEMU_LOCK_GUARD(&s->lock); >> @@ -201,6 +208,27 @@ block_copy_task_create(BlockCopyState *s, >> BlockCopyCallState *call_state, >> return NULL; >> } >> >> + method = s->method; >> + >> + /* >> + * The scan already knows how this range reads: pick the method here and >> + * stop the task where the answer changes. >> + */ >> + if (qatomic_load_acquire(&s->zero_bitmap_valid)) { >> + int64_t boundary; >> + >> + if (hbitmap_get(s->zero_bitmap, offset)) { >> + method = COPY_WRITE_ZEROES; >> + boundary = hbitmap_next_zero(s->zero_bitmap, offset, bytes); >> + } else { >> + boundary = hbitmap_next_dirty(s->zero_bitmap, offset, bytes); >> + } >> + >> + if (boundary >= 0) { >> + bytes = boundary - offset; >> + } >> + } >> + >> assert(QEMU_IS_ALIGNED(offset, s->cluster_size)); >> bytes = QEMU_ALIGN_UP(bytes, s->cluster_size); >> >> @@ -215,7 +243,7 @@ block_copy_task_create(BlockCopyState *s, >> BlockCopyCallState *call_state, >> .task.func = block_copy_task_entry, >> .s = s, >> .call_state = call_state, >> - .method = s->method, >> + .method = method, >> }; >> reqlist_init_req(&s->reqs, &task->req, offset, bytes); >> >> @@ -271,6 +299,9 @@ void block_copy_state_free(BlockCopyState *s) >> >> ratelimit_destroy(&s->rate_limit); >> bdrv_release_dirty_bitmap(s->copy_bitmap); >> + if (s->zero_bitmap) { >> + hbitmap_free(s->zero_bitmap); >> + } >> shres_destroy(s->mem); >> g_free(s); >> } >> @@ -624,20 +655,24 @@ static coroutine_fn int block_copy_task_entry(AioTask >> *task) >> return ret; >> } >> >> +/* The scan and the per-task query must resolve BDRV_BLOCK_ZERO alike. */ >> +static GRAPH_RDLOCK BlockDriverState *block_copy_status_base(BlockCopyState >> *s) >> +{ >> + if (qatomic_read(&s->skip_unallocated)) { >> + return bdrv_backing_chain_next(s->source->bs); >> + } >> + >> + return NULL; >> +} >> + >> static coroutine_fn GRAPH_RDLOCK >> int block_copy_block_status(BlockCopyState *s, int64_t offset, int64_t >> bytes, >> int64_t *pnum) >> { >> int64_t num; >> - BlockDriverState *base; >> + BlockDriverState *base = block_copy_status_base(s); >> int ret; >> >> - if (qatomic_read(&s->skip_unallocated)) { >> - base = bdrv_backing_chain_next(s->source->bs); >> - } else { >> - base = NULL; >> - } >> - >> ret = bdrv_co_block_status_above(s->source->bs, base, offset, bytes, >> &num, >> NULL, NULL); >> if (ret < 0 || num < s->cluster_size) { >> @@ -657,16 +692,41 @@ int block_copy_block_status(BlockCopyState *s, int64_t >> offset, int64_t bytes, >> return ret; >> } >> >> +/* Only the scan allocates, and it runs before zero_bitmap_valid. */ >> +static HBitmap *block_copy_zero_bitmap(BlockCopyState *s) >> +{ >> + if (!s->zero_bitmap) { >> + s->zero_bitmap = hbitmap_alloc(s->len, ctz32(s->cluster_size)); > Hmm why it's ctz32() when cluster_size is int64_t ? Is this intentional > or an oversight? Intentional, and it is the same assumption the file already makes a few lines up: copy_bitmap is created by bdrv_create_dirty_bitmap(), whose granularity parameter is a uint32_t, and that call is handed the very same s->cluster_size. block/dirty-bitmap.c then reaches hbitmap_alloc() through ctz32() as well, so zero_bitmap and copy_bitmap end up with the same granularity by the same route.
The type is int64_t because the block layer passes sizes around as int64_t. The value cannot need more: it comes from bdi.cluster_size, which is an int. The one way to push it past 4G is an x-perf min-cluster-size above that, and copy_bitmap would truncate before zero_bitmap did - a pre-existing gap in the parameter checking rather than something this series introduces. Thank you for review, Den
