* [PATCH hci-8.0 1/9] block/commit: pass BDRV_WANT_PRECISE to block-status #VSTOR-134085
2026-09-30 11:17 [PATCH hci-8.0 0/9] block: cheaper zero handling in backup and commit #VSTOR-134085 Denis V. Lunev
@ 2026-09-30 11:17 ` Denis V. Lunev
2026-09-30 11:20 ` no-reply
2026-09-30 11:17 ` [PATCH hci-8.0 2/9] iotests/040: cover large and fragmented commit runs #VSTOR-134085 Denis V. Lunev
` (8 subsequent siblings)
9 siblings, 1 reply; 20+ messages in thread
From: Denis V. Lunev @ 2026-09-30 11:17 UTC (permalink / raw)
To: svt-core; +Cc: andrey.drobyshev, den
From: Denis V. Lunev <den@openvz.org>
c33159dec790 replaced the want_zero bool with a bitmask of BDRV_WANT_*
flags. block/commit.c was missed and still passes "true", which is
BDRV_BLOCK_DATA, not BDRV_WANT_PRECISE. Convert it like the other
callers.
Fixes: c33159dec790 ("block: Expand block status mode from bool to flags")
Reviewed-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
CC: John Snow <jsnow@redhat.com>
CC: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
CC: Eric Blake <eblake@redhat.com>
---
block/commit.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/commit.c b/block/commit.c
index 0d9e1a16d7a..a2e807288b2 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -139,8 +139,8 @@ static int commit_iteration(CommitBlockJob *s, int64_t offset,
/* Copy if allocated above the base */
WITH_GRAPH_RDLOCK_GUARD() {
ret = bdrv_co_common_block_status_above(blk_bs(s->top),
- s->base_overlay, true, true, offset, COMMIT_BUFFER_SIZE,
- &bytes, NULL, NULL, NULL);
+ s->base_overlay, true, BDRV_WANT_PRECISE, offset,
+ COMMIT_BUFFER_SIZE, &bytes, NULL, NULL, NULL);
}
trace_commit_one_iteration(s, offset, bytes, ret);
--
2.53.0
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCH hci-8.0 2/9] iotests/040: cover large and fragmented commit runs #VSTOR-134085
2026-09-30 11:17 [PATCH hci-8.0 0/9] block: cheaper zero handling in backup and commit #VSTOR-134085 Denis V. Lunev
2026-09-30 11:17 ` [PATCH hci-8.0 1/9] block/commit: pass BDRV_WANT_PRECISE to block-status #VSTOR-134085 Denis V. Lunev
@ 2026-09-30 11:17 ` Denis V. Lunev
2026-09-30 11:19 ` no-reply
2026-09-30 11:17 ` [PATCH hci-8.0 3/9] block/commit: batch block-status queries #VSTOR-134085 Denis V. Lunev
` (7 subsequent siblings)
9 siblings, 1 reply; 20+ messages in thread
From: Denis V. Lunev @ 2026-09-30 11:17 UTC (permalink / raw)
To: svt-core; +Cc: andrey.drobyshev, den
From: Denis V. Lunev <den@openvz.org>
040 never commits runs long enough to span more than one block-status
query, so the answer that commit_iteration() is about to start caching
goes untested.
Add two cases on a base <- mid <- active chain, committing mid so the
job takes the regular commit path rather than active commit, and compare
base against a snapshot of mid taken before the commit. One case mixes
multi-megabyte data, zero and hole runs, the other fragments them down
to single clusters with every transition off the 512K boundary.
Reviewed-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
CC: John Snow <jsnow@redhat.com>
CC: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
tests/qemu-iotests/040 | 102 ++++++++++++++++++++++++++++++++++++-
tests/qemu-iotests/040.out | 4 +-
2 files changed, 103 insertions(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index 5c18e413ec9..e01c9385bae 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -25,13 +25,14 @@
import time
import os
import iotests
-from iotests import qemu_img, qemu_io
+from iotests import qemu_img, qemu_img_create, qemu_io, compare_images
import struct
import errno
backing_img = os.path.join(iotests.test_dir, 'backing.img')
mid_img = os.path.join(iotests.test_dir, 'mid.img')
test_img = os.path.join(iotests.test_dir, 'test.img')
+reference_img = os.path.join(iotests.test_dir, 'reference.img')
class ImageCommitTestCase(iotests.QMPTestCase):
'''Abstract base class for image commit test cases'''
@@ -951,6 +952,105 @@ class TestCommitWithOverriddenBacking(iotests.QMPTestCase):
self.vm.qmp('block-job-complete', device='commit')
self.vm.event_wait('BLOCK_JOB_COMPLETED')
+class TestCommitLargeRuns(iotests.QMPTestCase):
+ """Commit runs long enough to cross commit_iteration()'s cached span."""
+
+ MB = 1024 * 1024
+ CLUSTER = 64 * 1024
+
+ # Runs several COMMIT_BUFFER_SIZE (512K) chunks long, of every kind.
+ SIZE = 32 * MB
+ LAYOUT = [
+ (0, 4 * MB, 'data'),
+ (4 * MB, 6 * MB, 'hole'),
+ (10 * MB, 4 * MB, 'data'),
+ (14 * MB, 6 * MB, 'zero'),
+ (20 * MB, 4 * MB, 'data'),
+ (24 * MB, 8 * MB, 'hole'),
+ ]
+
+ # The same, with every transition off the 512K boundary.
+ SIZE_FRAGMENTED = 384 * CLUSTER # 24M
+ LAYOUT_FRAGMENTED = [
+ (0, 45 * CLUSTER, 'data'),
+ (45 * CLUSTER, 55 * CLUSTER, 'hole'),
+ (100 * CLUSTER, CLUSTER, 'data'),
+ (101 * CLUSTER, 49 * CLUSTER, 'zero'),
+ (150 * CLUSTER, 80 * CLUSTER, 'data'),
+ (230 * CLUSTER, CLUSTER, 'hole'),
+ (231 * CLUSTER, 69 * CLUSTER, 'data'),
+ (300 * CLUSTER, 83 * CLUSTER, 'zero'),
+ (383 * CLUSTER, CLUSTER, 'hole'),
+ ]
+
+ def setUp(self):
+ self.vm = iotests.VM()
+ self.vm.launch()
+
+ def tearDown(self):
+ self.vm.shutdown()
+ for img in (backing_img, mid_img, test_img, reference_img):
+ if os.path.exists(img):
+ os.remove(img)
+
+ def build_images(self, layout, size):
+ # A pattern of its own in base, so a misplaced cluster shows up.
+ qemu_img_create('-f', iotests.imgfmt, backing_img, str(size))
+ qemu_io('-c', f'write -P 0x11 0 {size}', backing_img)
+
+ qemu_img_create('-f', iotests.imgfmt, '-b', backing_img, '-F',
+ iotests.imgfmt, mid_img, str(size))
+ for offset, length, kind in layout:
+ if kind == 'data':
+ qemu_io('-c', f'write -P 0x22 {offset} {length}', mid_img)
+ elif kind == 'zero':
+ qemu_io('-c', f'write -z {offset} {length}', mid_img)
+
+ # What base must equal once mid is committed into it.
+ qemu_img('convert', '-f', iotests.imgfmt, '-O', iotests.imgfmt,
+ mid_img, reference_img)
+
+ # An empty layer above mid, so top_node=mid is not the active one.
+ qemu_img_create('-f', iotests.imgfmt, '-b', mid_img, '-F',
+ iotests.imgfmt, test_img, str(size))
+
+ self.vm.cmd('blockdev-add', {
+ 'node-name': 'base',
+ 'driver': iotests.imgfmt,
+ 'file': {'driver': 'file', 'filename': backing_img},
+ })
+ self.vm.cmd('blockdev-add', {
+ 'node-name': 'mid',
+ 'driver': iotests.imgfmt,
+ 'file': {'driver': 'file', 'filename': mid_img},
+ 'backing': 'base',
+ })
+ self.vm.cmd('blockdev-add', {
+ 'node-name': 'active',
+ 'driver': iotests.imgfmt,
+ 'file': {'driver': 'file', 'filename': test_img},
+ 'backing': 'mid',
+ })
+
+ def commit_and_verify(self):
+ self.vm.cmd('block-commit', job_id='commit0', device='active',
+ top_node='mid', base_node='base')
+ self.wait_until_completed(drive='commit0')
+
+ self.vm.cmd('blockdev-del', node_name='active')
+ self.vm.cmd('blockdev-del', node_name='mid')
+ self.vm.cmd('blockdev-del', node_name='base')
+ self.assertTrue(compare_images(reference_img, backing_img))
+
+ def test_commit_large_runs(self):
+ self.build_images(self.LAYOUT, self.SIZE)
+ self.commit_and_verify()
+
+ def test_commit_fragmented_runs(self):
+ self.build_images(self.LAYOUT_FRAGMENTED, self.SIZE_FRAGMENTED)
+ self.commit_and_verify()
+
+
if __name__ == '__main__':
iotests.main(supported_fmts=['qcow2', 'qed'],
supported_protocols=['file'])
diff --git a/tests/qemu-iotests/040.out b/tests/qemu-iotests/040.out
index 1bb1dc5f0ea..d2e2a2d98f6 100644
--- a/tests/qemu-iotests/040.out
+++ b/tests/qemu-iotests/040.out
@@ -1,5 +1,5 @@
-.................................................................
+...................................................................
----------------------------------------------------------------------
-Ran 65 tests
+Ran 67 tests
OK
--
2.53.0
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCH hci-8.0 3/9] block/commit: batch block-status queries #VSTOR-134085
2026-09-30 11:17 [PATCH hci-8.0 0/9] block: cheaper zero handling in backup and commit #VSTOR-134085 Denis V. Lunev
2026-09-30 11:17 ` [PATCH hci-8.0 1/9] block/commit: pass BDRV_WANT_PRECISE to block-status #VSTOR-134085 Denis V. Lunev
2026-09-30 11:17 ` [PATCH hci-8.0 2/9] iotests/040: cover large and fragmented commit runs #VSTOR-134085 Denis V. Lunev
@ 2026-09-30 11:17 ` Denis V. Lunev
2026-09-30 11:21 ` no-reply
2026-09-30 11:17 ` [PATCH hci-8.0 4/9] iotests/124: cover backup of zero clusters and holes #VSTOR-134085 Denis V. Lunev
` (6 subsequent siblings)
9 siblings, 1 reply; 20+ messages in thread
From: Denis V. Lunev @ 2026-09-30 11:17 UTC (permalink / raw)
To: svt-core; +Cc: andrey.drobyshev, den
From: Denis V. Lunev <den@openvz.org>
commit_iteration() asks for block status COMMIT_BUFFER_SIZE (512K) at a
time, so a long run above base pays one query per 512K for an answer
the whole run shares.
Query the remainder of the image instead and keep the answer in a
CommitStatus owned by commit_run(). No device or export writes top
above base_overlay while the job runs: they ask for
BLK_PERM_CONSISTENT_READ along with BLK_PERM_WRITE, and the job
withholds CONSISTENT_READ there, having to share WRITE or it would
block its own writes to base through the backing chain. A job target
asks for WRITE alone and is refused by the op blocker instead. From
filtered_base downwards CONSISTENT_READ is shared again, and there only
the job writes.
Copying is still bounded by the read buffer. Zeroes need no buffer, so
COMMIT_ZERO_CHUNK bounds them, keeping a cancel from waiting on a huge
write-zeroes, and an unallocated span is crossed in one step.
Reviewed-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
CC: John Snow <jsnow@redhat.com>
CC: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
block/commit.c | 59 ++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 48 insertions(+), 11 deletions(-)
diff --git a/block/commit.c b/block/commit.c
index a2e807288b2..80b89e6269d 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -31,8 +31,23 @@ enum {
* contiguous regions of the image is efficient.
*/
COMMIT_BUFFER_SIZE = 512 * 1024, /* in bytes */
+
+ /*
+ * Zeroes need no buffer, so they are bounded by this instead. It stays
+ * well under BDRV_REQUEST_MAX_BYTES and keeps a cancel from waiting on
+ * a multi-gigabyte write.
+ */
+ COMMIT_ZERO_CHUNK = 256 * 1024 * 1024, /* in bytes */
};
+/* Last block-status answer, covering [offset, end). Empty when equal. */
+typedef struct CommitStatus {
+ int64_t len;
+ int64_t offset;
+ int64_t end;
+ int ret;
+} CommitStatus;
+
typedef struct CommitBlockJob {
BlockJob common;
BlockDriverState *commit_top_bs;
@@ -129,26 +144,46 @@ static void commit_clean(Job *job)
}
static int commit_iteration(CommitBlockJob *s, int64_t offset,
- int64_t *requested_bytes, void *buf)
+ int64_t *requested_bytes, void *buf,
+ CommitStatus *st)
{
BlockErrorAction action;
- int64_t bytes = *requested_bytes;
+ int64_t bytes;
int ret = 0;
bool error_in_source = true;
- /* Copy if allocated above the base */
- WITH_GRAPH_RDLOCK_GUARD() {
- ret = bdrv_co_common_block_status_above(blk_bs(s->top),
- s->base_overlay, true, BDRV_WANT_PRECISE, offset,
- COMMIT_BUFFER_SIZE, &bytes, NULL, NULL, NULL);
+ assert(offset >= st->offset);
+
+ if (offset >= st->end) {
+ /* Copy if allocated above the base */
+ WITH_GRAPH_RDLOCK_GUARD() {
+ ret = bdrv_co_common_block_status_above(blk_bs(s->top),
+ s->base_overlay, true, BDRV_WANT_PRECISE, offset,
+ st->len - offset, &bytes, NULL, NULL, NULL);
+ }
+
+ if (ret < 0) {
+ trace_commit_one_iteration(s, offset, 0, ret);
+ goto fail;
+ }
+
+ st->offset = offset;
+ st->end = offset + bytes;
+ st->ret = ret;
}
- trace_commit_one_iteration(s, offset, bytes, ret);
+ ret = st->ret;
+ bytes = st->end - offset;
- if (ret < 0) {
- goto fail;
+ /* An unallocated span costs no I/O, so it is crossed in one step. */
+ if (ret & BDRV_BLOCK_ZERO) {
+ bytes = MIN(bytes, COMMIT_ZERO_CHUNK);
+ } else if (ret & BDRV_BLOCK_ALLOCATED) {
+ bytes = MIN(bytes, COMMIT_BUFFER_SIZE);
}
+ trace_commit_one_iteration(s, offset, bytes, ret);
+
if (ret & BDRV_BLOCK_ALLOCATED) {
if (ret & BDRV_BLOCK_ZERO) {
/*
@@ -214,6 +249,7 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
int64_t n = 0; /* bytes */
QEMU_AUTO_VFREE void *buf = NULL;
int64_t len, base_len;
+ CommitStatus st = { 0 };
len = blk_co_getlength(s->top);
if (len < 0) {
@@ -234,6 +270,7 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
}
buf = blk_blockalign(s->top, COMMIT_BUFFER_SIZE);
+ st.len = len;
for (offset = 0; offset < len; offset += n) {
/* Note that even when no rate limit is applied we need to yield
@@ -244,7 +281,7 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
break;
}
- ret = commit_iteration(s, offset, &n, buf);
+ ret = commit_iteration(s, offset, &n, buf, &st);
if (ret < 0) {
return ret;
--
2.53.0
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH hci-8.0 3/9] block/commit: batch block-status queries #VSTOR-134085
2026-09-30 11:17 ` [PATCH hci-8.0 3/9] block/commit: batch block-status queries #VSTOR-134085 Denis V. Lunev
@ 2026-09-30 11:21 ` no-reply
0 siblings, 0 replies; 20+ messages in thread
From: no-reply @ 2026-09-30 11:21 UTC (permalink / raw)
To: svt-core, Denis V. Lunev; +Cc: andrey.drobyshev, den
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] block/commit: unallocated zero span is still capped at COMMIT_ZERO_CHUNK despite comment
--
commit 1c010ab0f99a89e08cf4922150238581339cd7aa
Author: Denis V. Lunev <den@openvz.org>
block/commit: batch block-status queries #VSTOR-134085
This patch changes commit_iteration() to query block status for the whole
remainder of the image and cache the answer in a CommitStatus owned by
commit_run(). Copying of allocated data stays bounded by
COMMIT_BUFFER_SIZE. Zero spans are bounded by the new COMMIT_ZERO_CHUNK.
> diff --git a/block/commit.c b/block/commit.c
> index a2e807288b23..80b89e6269d8 100644
> --- a/block/commit.c
> +++ b/block/commit.c
[ ... ]
> @@ -129,26 +144,46 @@ static void commit_clean(Job *job)
[ ... ]
> + ret = st->ret;
> + bytes = st->end - offset;
>
> + /* An unallocated span costs no I/O, so it is crossed in one step. */
> + if (ret & BDRV_BLOCK_ZERO) {
> + bytes = MIN(bytes, COMMIT_ZERO_CHUNK);
> + } else if (ret & BDRV_BLOCK_ALLOCATED) {
> + bytes = MIN(bytes, COMMIT_BUFFER_SIZE);
> }
[Severity: Low]
This isn't a bug, but the comment and the commit message don't match what
the code does. The comment says "An unallocated span costs no I/O, so it is
crossed in one step", and the commit message says "an unallocated span is
crossed in one step".
Can a span that reports BDRV_BLOCK_ZERO without BDRV_BLOCK_ALLOCATED still
be capped at COMMIT_ZERO_CHUNK here?
bdrv_co_do_block_status() in block/io.c returns BDRV_BLOCK_ZERO without
BDRV_BLOCK_ALLOCATED in two cases:
- A supports_backing layer that has no cow_bs.
- BDRV_WANT_PRECISE mode with offset at or beyond the backing file size.
This can happen when top is larger than the backing of base_overlay.
For these spans, commit_iteration() takes the first branch:
if (ret & BDRV_BLOCK_ZERO) {
bytes = MIN(bytes, COMMIT_ZERO_CHUNK);
The later "if (ret & BDRV_BLOCK_ALLOCATED)" test is false, so no I/O is
done, but the span is still split into 256M steps. Each extra step goes
through block_job_ratelimit_sleep(), the job_is_cancelled() check and
job_progress_update().
Only the fall-through case, where neither flag is set, is crossed in one
step. Should the cap apply only when both BDRV_BLOCK_ZERO and
BDRV_BLOCK_ALLOCATED are set, or should the comment and commit message be
adjusted to match the current behaviour?
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20260930111739.3866427-1-den@openvz.org?part=3
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH hci-8.0 4/9] iotests/124: cover backup of zero clusters and holes #VSTOR-134085
2026-09-30 11:17 [PATCH hci-8.0 0/9] block: cheaper zero handling in backup and commit #VSTOR-134085 Denis V. Lunev
` (2 preceding siblings ...)
2026-09-30 11:17 ` [PATCH hci-8.0 3/9] block/commit: batch block-status queries #VSTOR-134085 Denis V. Lunev
@ 2026-09-30 11:17 ` Denis V. Lunev
2026-09-30 11:31 ` no-reply
2026-09-30 11:17 ` [PATCH hci-8.0 5/9] block/block-copy: don't reserve memory for zero tasks #VSTOR-134085 Denis V. Lunev
` (5 subsequent siblings)
9 siblings, 1 reply; 20+ messages in thread
From: Denis V. Lunev @ 2026-09-30 11:17 UTC (permalink / raw)
To: svt-core; +Cc: andrey.drobyshev, den
From: Denis V. Lunev <den@openvz.org>
The backup tests compare content only, and their sources hold data
alone, so nothing pins how backup treats zero clusters and holes. Back
up a source mixing data, write-zero and holes with sync=full, sync=top
and sync=bitmap, and check the target with qemu-img map, which tells a
copied cluster apart from a write-zero and from an untouched one.
hci-8.0: sync=full skips unallocated clusters since 6b1a4754713, so
the gaps of a sync=full target stay holes instead of turning into zero
clusters.
Reviewed-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
CC: John Snow <jsnow@redhat.com>
CC: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
tests/qemu-iotests/124 | 350 ++++++++++++++++++++++++++++++++++++-
tests/qemu-iotests/124.out | 4 +-
2 files changed, 351 insertions(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
index b2f4328e345..2616010817c 100755
--- a/tests/qemu-iotests/124
+++ b/tests/qemu-iotests/124
@@ -22,8 +22,11 @@
#
import os
+from collections import namedtuple
+
import iotests
-from iotests import try_remove
+from iotests import (compare_images, qemu_img_create, qemu_img_map, qemu_io,
+ try_remove)
from qemu.qmp.qmp_client import ExecuteError
@@ -748,6 +751,351 @@ class TestIncrementalBackupBlkdebug(TestIncrementalBackupBase):
self.check_backups()
+# Backup of sources mixing data, zero clusters and holes.
+
+Extent = namedtuple('Extent', ['start', 'length', 'kind'])
+Case = namedtuple('Case', ['size', 'layout', 'expected'], defaults=[None])
+
+source_img = os.path.join(iotests.test_dir, 'source')
+base_img = os.path.join(iotests.test_dir, 'base')
+target_img = os.path.join(iotests.test_dir, 'target')
+
+SIZE = 64 * 1024 * 1024
+MB = 1024 * 1024
+CLUSTER = 64 * 1024
+
+# Data, write-zero and holes, every combination at a cluster boundary.
+MIXED = Case(SIZE, [
+ (0, MB, 'data'),
+ (2 * MB, MB, 'zero'),
+ (5 * MB, 2 * MB, 'data'),
+ (8 * MB, MB, 'zero'),
+ (10 * MB, MB // 2, 'data'),
+ (20 * MB, 4 * MB, 'zero'),
+ (30 * MB, MB, 'data'),
+])
+
+# Single-cluster runs, and a zero run ending exactly at EOF.
+BOUNDARY_SIZE = 16 * MB
+BOUNDARY = Case(BOUNDARY_SIZE, [
+ (0, CLUSTER, 'zero'),
+ (CLUSTER, CLUSTER, 'data'),
+ (2 * CLUSTER, CLUSTER, 'zero'),
+ (3 * CLUSTER, CLUSTER, 'data'),
+ (4 * CLUSTER, 8 * MB, 'zero'),
+ (4 * CLUSTER + 8 * MB, CLUSTER, 'data'),
+ (BOUNDARY_SIZE - CLUSTER, CLUSTER, 'zero'),
+])
+
+# A zero run past the old block_copy_chunk_size() 16M cap.
+LARGE_ZERO = Case(48 * MB, [
+ (0, MB, 'data'),
+ (4 * MB, 32 * MB, 'zero'),
+ (40 * MB, MB, 'data'),
+])
+
+# Image size not a multiple of the cluster size: a partial tail cluster.
+TAIL_SIZE = 4 * MB + 4096
+TAIL_CLUSTER = (TAIL_SIZE // CLUSTER) * CLUSTER
+TAIL_DATA = Case(TAIL_SIZE, [
+ (0, MB, 'data'),
+ (2 * MB, MB, 'zero'),
+ (4 * MB, TAIL_SIZE - 4 * MB, 'data'),
+])
+# The partial tail cluster falls out of zero_bitmap, so it copies as data.
+TAIL_ZERO = Case(TAIL_SIZE, [
+ (0, MB, 'data'),
+ (2 * MB, TAIL_SIZE - 2 * MB, 'zero'),
+], [
+ (0, MB, 'data'),
+ (2 * MB, TAIL_CLUSTER - 2 * MB, 'zero'),
+ (TAIL_CLUSTER, TAIL_SIZE - TAIL_CLUSTER, 'data'),
+])
+
+
+def coalesce(extents):
+ out = []
+ for e in extents:
+ prev = out[-1] if out else None
+ adjacent = prev is not None and prev.start + prev.length == e.start
+ if adjacent and prev.kind == e.kind:
+ out[-1] = prev._replace(length=prev.length + e.length)
+ else:
+ out.append(e)
+ return out
+
+
+def layout_to_extents(layout, size, gap='hole'):
+ extents = []
+ pos = 0
+ for offset, length, kind in layout:
+ if offset > pos:
+ extents.append(Extent(pos, offset - pos, gap))
+ extents.append(Extent(offset, length, kind))
+ pos = offset + length
+ if pos < size:
+ extents.append(Extent(pos, size - pos, gap))
+ return coalesce(extents)
+
+
+def create_image(path, size, backing=None, opts=None):
+ args = ['-f', iotests.imgfmt]
+ if opts:
+ args += ['-o', opts]
+ if backing:
+ args += ['-b', backing, '-F', iotests.imgfmt]
+ qemu_img_create(*args, path, str(size))
+
+
+class TestBackupZeroClusters(iotests.QMPTestCase):
+ def setUp(self):
+ self.vm = iotests.VM()
+ self.vm.launch()
+
+ def tearDown(self):
+ self.vm.shutdown()
+ for img in (source_img, base_img, target_img):
+ if os.path.exists(img):
+ os.remove(img)
+
+ def hmp_write(self, drive, cmd):
+ res = self.vm.hmp_qemu_io(drive, cmd)
+ self.assertNotIn('error', res['return'].lower())
+
+ def write_layout(self, layout):
+ for offset, length, kind in layout:
+ opt = '-z' if kind == 'zero' else '-P 0x5a'
+ self.hmp_write('src', f'write {opt} {offset} {length}')
+
+ def assert_map(self, case, backing=False, gap='hole'):
+ # Content is not enough, pin the data/zero/hole split as well.
+ def classify(e):
+ is_hole = e['depth'] > 0 if backing else not e['present']
+ return 'hole' if is_hole else ('zero' if e['zero'] else 'data')
+
+ actual = coalesce([Extent(e['start'], e['length'], classify(e))
+ for e in qemu_img_map(target_img)])
+ layout = case.expected if case.expected else case.layout
+
+ self.assertEqual(actual, layout_to_extents(layout, case.size, gap))
+
+ def add_source(self, case=MIXED, backing=None, opts=None):
+ create_image(source_img, case.size, backing, opts)
+
+ self.vm.cmd('blockdev-add', {
+ 'node-name': 'src',
+ 'driver': iotests.imgfmt,
+ 'file': {'driver': 'file', 'filename': source_img},
+ })
+
+ # Write through the node, so an attached bitmap sees it.
+ self.write_layout(case.layout)
+
+ def dirty_layout(self, case):
+ # A new bitmap tracks nothing yet: dirty all, then re-apply.
+ self.vm.cmd('block-dirty-bitmap-add', node='src', name='bm0')
+ self.hmp_write('src', f'write -z 0 {case.size}')
+ self.write_layout(case.layout)
+
+ def do_backup(self, sync, case, target_backing=None, prefill=None,
+ **kwargs):
+ create_image(target_img, case.size, target_backing)
+
+ if prefill is not None:
+ # Not zero, so a skipped cluster is provably untouched.
+ qemu_io('-c', f'write -P {prefill} 0 {case.size}', target_img)
+
+ self.vm.cmd('blockdev-add', {
+ 'node-name': 'target',
+ 'driver': iotests.imgfmt,
+ 'file': {'driver': 'file', 'filename': target_img},
+ })
+
+ self.vm.cmd('blockdev-backup', device='src', target='target',
+ job_id='bk0', sync=sync, **kwargs)
+ self.wait_until_completed(drive='bk0')
+
+ self.vm.cmd('blockdev-del', node_name='target')
+ self.vm.cmd('blockdev-del', node_name='src')
+
+ def backup_and_check(self, sync, case, gap=None, backing=False,
+ **kwargs):
+ if gap is None:
+ # hci-8.0 sync=full skips unallocated clusters, see 6b1a4754713.
+ gap = 'hole' if sync == 'full' else 'zero'
+ self.do_backup(sync, case, **kwargs)
+ self.assertTrue(compare_images(source_img, target_img))
+ self.assert_map(case, backing=backing, gap=gap)
+
+ def test_full(self):
+ self.add_source()
+ self.backup_and_check('full', MIXED)
+
+ def test_full_zero_overwrite(self):
+ # full skips holes, so only check that zero overwrites prefill.
+ case = Case(SIZE, [(2 * MB, MB, 'zero')])
+ self.add_source(case)
+ self.do_backup('full', case, prefill=0xcc)
+ qemu_io('-c', f'read -P 0 {2 * MB} {MB}', target_img)
+
+ def test_bitmap(self):
+ self.add_source()
+ self.dirty_layout(MIXED)
+ self.backup_and_check('bitmap', MIXED, bitmap='bm0',
+ bitmap_mode='never')
+
+ def test_top(self):
+ # Non-zero backing data, so a hole and an explicit zero differ.
+ create_image(base_img, SIZE)
+ qemu_io('-c', f'write -P 0x33 0 {SIZE}', base_img)
+
+ self.add_source(backing=base_img)
+ self.backup_and_check('top', MIXED, gap='hole', backing=True,
+ target_backing=base_img)
+
+ def test_boundary_full(self):
+ self.add_source(BOUNDARY)
+ self.backup_and_check('full', BOUNDARY)
+
+ def test_boundary_bitmap(self):
+ self.add_source(BOUNDARY)
+ self.dirty_layout(BOUNDARY)
+ self.backup_and_check('bitmap', BOUNDARY, bitmap='bm0',
+ bitmap_mode='never')
+
+ def test_large_zero_full(self):
+ self.add_source(LARGE_ZERO)
+ self.backup_and_check('full', LARGE_ZERO)
+
+ def test_large_zero_bitmap(self):
+ self.add_source(LARGE_ZERO)
+ self.dirty_layout(LARGE_ZERO)
+ self.backup_and_check('bitmap', LARGE_ZERO, bitmap='bm0',
+ bitmap_mode='never')
+
+ def test_huge_zero(self):
+ # A zero run past the cap on a single write-zeroes request, so it
+ # has to be split. qemu-io caps one write at 2G, hence three.
+ case = Case(2560 * MB, [(0, 1024 * MB, 'zero'),
+ (1024 * MB, 1024 * MB, 'zero'),
+ (2048 * MB, 512 * MB, 'zero')])
+ self.add_source(case)
+ self.backup_and_check('full', case)
+
+ def test_tail_data(self):
+ self.add_source(TAIL_DATA)
+ self.backup_and_check('full', TAIL_DATA)
+
+ def test_tail_zero(self):
+ self.add_source(TAIL_ZERO)
+ self.backup_and_check('full', TAIL_ZERO)
+
+ def test_tail_zero_bitmap(self):
+ # Same tail rounding as test_tail_zero, via the bitmap scan.
+ self.add_source(TAIL_ZERO)
+ self.dirty_layout(TAIL_ZERO)
+ self.backup_and_check('bitmap', TAIL_ZERO, bitmap='bm0',
+ bitmap_mode='never')
+
+ def test_mixed_cluster(self):
+ # 4K source clusters, so content varies inside one 64K cluster.
+ case = Case(2 * CLUSTER, [
+ (0, 8 * 1024, 'zero'),
+ (8 * 1024, 8 * 1024, 'data'),
+ (CLUSTER, CLUSTER, 'zero'),
+ ], [
+ # A mixed cluster must be data, or the data at [8k, 16k) is lost.
+ (0, CLUSTER, 'data'),
+ (CLUSTER, CLUSTER, 'zero'),
+ ])
+ self.add_source(case, opts='cluster_size=4k')
+ self.backup_and_check('full', case)
+
+ def test_top_zero_broken(self):
+ # An overlay hole before an explicit-zero run: the zero prefix
+ # must stop at the hole, or the backing data under it is lost.
+ size = 2 * CLUSTER
+ create_image(base_img, size, opts='cluster_size=4k')
+ qemu_io('-c', f'write -P 0x33 0 {size}', base_img)
+
+ # [0, 4k) stays a hole, the zero run spills into cluster 1.
+ case = Case(size, [(4096, CLUSTER, 'zero')])
+ self.add_source(case, backing=base_img, opts='cluster_size=4k')
+
+ self.do_backup('top', case, target_backing=base_img)
+ self.assertTrue(compare_images(source_img, target_img))
+
+ def test_bitmap_straddle(self):
+ # One dirty run straddling a zero/data transition, both ways.
+ case = Case(SIZE, [])
+ self.add_source(case)
+ self.vm.cmd('block-dirty-bitmap-add', node='src', name='bm0')
+
+ # One contiguous dirty run each: [7M, 9M) zero to data, and
+ # [15M, 17M) data to zero.
+ layout = [
+ (7 * MB, MB, 'zero'),
+ (8 * MB, MB, 'data'),
+ (15 * MB, MB, 'data'),
+ (16 * MB, MB, 'zero'),
+ ]
+ self.write_layout(layout)
+
+ self.do_backup('bitmap', case, bitmap='bm0', bitmap_mode='never')
+ self.assertTrue(compare_images(source_img, target_img))
+ # Nothing outside the two dirty runs may reach the target.
+ self.assert_map(case._replace(layout=layout))
+
+ def test_bitmap_fragmented(self):
+ # Isolated single-cluster dirty spots: nothing outside them
+ # may reach the target.
+ case = Case(SIZE, [])
+ self.add_source(case)
+
+ # Written before the bitmap exists, so untracked.
+ self.hmp_write('src', f'write -P 0x5a 0 {16 * MB}')
+
+ self.vm.cmd('block-dirty-bitmap-add', node='src', name='bm0')
+
+ # The only dirty bit in the data region, with a pattern of its own.
+ data_island = 4 * MB
+ self.hmp_write('src', f'write -P 0x7b {data_island} {CLUSTER}')
+
+ # The only dirty bit in the hole region.
+ zero_island = 24 * MB
+ self.hmp_write('src', f'write -z {zero_island} {CLUSTER}')
+
+ self.do_backup('bitmap', case, bitmap='bm0', bitmap_mode='never',
+ prefill=0xcc)
+
+ # Only the two isolated spots should have reached target ...
+ qemu_io('-c', f'read -P 0x7b {data_island} {CLUSTER}', target_img)
+ qemu_io('-c', f'read -P 0 {zero_island} {CLUSTER}', target_img)
+
+ # ... everything else must still hold the prefill pattern.
+ qemu_io('-c', f'read -P 0xcc 0 {CLUSTER}', target_img)
+ qemu_io('-c', f'read -P 0xcc {8 * MB} {CLUSTER}', target_img)
+
+ def test_bitmap_clean_zero_gap(self):
+ # A clean zero run between two dirty ones must stay prefilled.
+ case = Case(16 * MB, [])
+ self.add_source(case)
+ self.hmp_write('src', f'write -z 0 {16 * MB}')
+
+ self.vm.cmd('block-dirty-bitmap-add', node='src', name='bm0')
+ self.write_layout([(2 * MB, MB, 'zero'), (6 * MB, MB, 'zero')])
+
+ self.do_backup('bitmap', case, bitmap='bm0', bitmap_mode='never',
+ prefill=0xcc)
+
+ qemu_io('-c', f'read -P 0 {2 * MB} {MB}', target_img)
+ qemu_io('-c', f'read -P 0 {6 * MB} {MB}', target_img)
+
+ qemu_io('-c', f'read -P 0xcc 0 {2 * MB}', target_img)
+ qemu_io('-c', f'read -P 0xcc {3 * MB} {3 * MB}', target_img)
+ qemu_io('-c', f'read -P 0xcc {7 * MB} {9 * MB}', target_img)
+
+
if __name__ == '__main__':
iotests.main(supported_fmts=['qcow2'],
supported_protocols=['file'])
diff --git a/tests/qemu-iotests/124.out b/tests/qemu-iotests/124.out
index fa16b5cceff..5ce2f9a2ed9 100644
--- a/tests/qemu-iotests/124.out
+++ b/tests/qemu-iotests/124.out
@@ -1,5 +1,5 @@
-.............
+..............................
----------------------------------------------------------------------
-Ran 13 tests
+Ran 30 tests
OK
--
2.53.0
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH hci-8.0 4/9] iotests/124: cover backup of zero clusters and holes #VSTOR-134085
2026-09-30 11:17 ` [PATCH hci-8.0 4/9] iotests/124: cover backup of zero clusters and holes #VSTOR-134085 Denis V. Lunev
@ 2026-09-30 11:31 ` no-reply
0 siblings, 0 replies; 20+ messages in thread
From: no-reply @ 2026-09-30 11:31 UTC (permalink / raw)
To: svt-core, Denis V. Lunev; +Cc: andrey.drobyshev, den
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] iotests: 124 TAIL_ZERO tests expect behaviour that only appears in later patches
--
commit 147936f45e8ec1c124ea1a5b3dbf035c9a679b89
Author: Denis V. Lunev <den@openvz.org>
iotests/124: cover backup of zero clusters and holes #VSTOR-134085
This commit adds iotest 124 cases that back up sources mixing data,
write-zero and holes with sync=full, sync=top and sync=bitmap. It checks
the target layout with qemu-img map. It covers boundary, large-zero,
tail-cluster and mixed-cluster cases.
> diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
> --- a/tests/qemu-iotests/124
> +++ b/tests/qemu-iotests/124
[ ... ]
> +# A zero run past the old block_copy_chunk_size() 16M cap.
> +LARGE_ZERO = Case(48 * MB, [
> + (0, MB, 'data'),
> + (4 * MB, 32 * MB, 'zero'),
> + (40 * MB, MB, 'data'),
> +])
[Severity: Medium]
The comment about the old block_copy_chunk_size() 16M cap refers to a
chunk-cap removal that only lands in the later patch "block/block-copy:
coalesce write-zeroes tasks". Is this comment accurate at this commit?
> +# Image size not a multiple of the cluster size: a partial tail cluster.
> +TAIL_SIZE = 4 * MB + 4096
> +TAIL_CLUSTER = (TAIL_SIZE // CLUSTER) * CLUSTER
[ ... ]
> +# The partial tail cluster falls out of zero_bitmap, so it copies as data.
> +TAIL_ZERO = Case(TAIL_SIZE, [
> + (0, MB, 'data'),
> + (2 * MB, TAIL_SIZE - 2 * MB, 'zero'),
> +], [
> + (0, MB, 'data'),
> + (2 * MB, TAIL_CLUSTER - 2 * MB, 'zero'),
> + (TAIL_CLUSTER, TAIL_SIZE - TAIL_CLUSTER, 'data'),
> +])
[Severity: Medium]
Can test_tail_zero and test_tail_zero_bitmap pass at this commit? The
expected 'data' extent at TAIL_CLUSTER appears to hold only after later
patches in the series.
block/block-copy.c has no zero_bitmap at this point, and git grep finds it
only in block/mirror.c. The zero_bitmap is added by the later patch
"block/block-copy: track known-zero source clusters", and
block_copy_mark_zero_prefix() there marks only
QEMU_ALIGN_DOWN(zero_count, cluster_size). The tail cluster then falls
outside the bitmap and is copied as data.
Here is what seems to happen at this commit. TAIL_ZERO writes zeroes from
2M to EOF on a 4M+4096 source. qcow2_co_pwrite_zeroes() skips tail handling
when offset+bytes equals the image end, so the whole partial last cluster
becomes a zero cluster on the source.
block_copy_block_status() then reports ZERO for the run and rounds it up to
the cluster size, because offset+num == s->len. After that,
block_copy_dirty_clusters() selects the write-zeroes method:
block/block-copy.c:block_copy_dirty_clusters() {
...
if (ret & BDRV_BLOCK_ZERO) {
task->method = COPY_WRITE_ZEROES;
}
...
}
block_copy_do_copy() calls bdrv_co_pwrite_zeroes() with nbytes clipped to
s->len. The request ends at the target EOF, so qcow2 again skips tail
handling.
The target would therefore report a zero extent up to EOF, while
assert_map() expects a 4096-byte 'data' extent at TAIL_CLUSTER. The
assert_map() comparison should fail at this commit and pass only at the end
of the series. That breaks bisectability.
The later patches that make the expectation true are "block/block-copy:
track known-zero source clusters" and "block/backup: pre-fill zero_bitmap
for full/bitmap". This was found by static reading and the tests were not
run.
Also, the commit message says the tests pin how backup treats zero clusters
and holes. Is that accurate for the TAIL_ZERO and LARGE_ZERO cases, which
depend on behaviour added later in the series?
Could these cases, or at least their comments and expectations, move into
the patches that introduce the behaviour?
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20260930111739.3866427-1-den@openvz.org?part=4
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH hci-8.0 5/9] block/block-copy: don't reserve memory for zero tasks #VSTOR-134085
2026-09-30 11:17 [PATCH hci-8.0 0/9] block: cheaper zero handling in backup and commit #VSTOR-134085 Denis V. Lunev
` (3 preceding siblings ...)
2026-09-30 11:17 ` [PATCH hci-8.0 4/9] iotests/124: cover backup of zero clusters and holes #VSTOR-134085 Denis V. Lunev
@ 2026-09-30 11:17 ` Denis V. Lunev
2026-09-30 11:22 ` no-reply
2026-09-30 11:17 ` [PATCH hci-8.0 6/9] block/block-copy: extract block_copy_set_task_method() #VSTOR-134085 Denis V. Lunev
` (4 subsequent siblings)
9 siblings, 1 reply; 20+ messages in thread
From: Denis V. Lunev @ 2026-09-30 11:17 UTC (permalink / raw)
To: svt-core; +Cc: andrey.drobyshev, den
From: Denis V. Lunev <den@openvz.org>
block_copy_dirty_clusters() charges task->req.bytes to the shared memory
pool for every task, but a COPY_WRITE_ZEROES task allocates no bounce
buffer, so there is nothing to account for. Add
block_copy_task_shres_bytes() and use it at all three call sites.
Zero tasks lose the throttling the pool gave them incidentally and are
bounded only by BLOCK_COPY_MAX_WORKERS (64).
Reviewed-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
CC: John Snow <jsnow@redhat.com>
CC: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
block/block-copy.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/block/block-copy.c b/block/block-copy.c
index e68d3d8b141..aecbe0c16ab 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -470,6 +470,11 @@ void block_copy_set_progress_meter(BlockCopyState *s, ProgressMeter *pm)
s->progress = pm;
}
+static uint64_t block_copy_task_shres_bytes(BlockCopyTask *task)
+{
+ return task->method == COPY_WRITE_ZEROES ? 0 : task->req.bytes;
+}
+
/*
* Takes ownership of @task
*
@@ -491,7 +496,7 @@ static coroutine_fn int block_copy_task_run(AioTaskPool *pool,
aio_task_pool_wait_slot(pool);
if (aio_task_pool_status(pool) < 0) {
- co_put_to_shres(task->s->mem, task->req.bytes);
+ co_put_to_shres(task->s->mem, block_copy_task_shres_bytes(task));
block_copy_task_end(task, -ECANCELED);
g_free(task);
return -ECANCELED;
@@ -636,7 +641,7 @@ static coroutine_fn int block_copy_task_entry(AioTask *task)
progress_work_done(s->progress, t->req.bytes);
}
}
- co_put_to_shres(s->mem, t->req.bytes);
+ co_put_to_shres(s->mem, block_copy_task_shres_bytes(t));
block_copy_task_end(t, ret);
if (s->discard_source && ret == 0) {
@@ -855,7 +860,7 @@ block_copy_dirty_clusters(BlockCopyCallState *call_state)
trace_block_copy_process(s, task->req.offset);
- co_get_from_shres(s->mem, task->req.bytes);
+ co_get_from_shres(s->mem, block_copy_task_shres_bytes(task));
offset = task_end(task);
bytes = end - offset;
--
2.53.0
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCH hci-8.0 6/9] block/block-copy: extract block_copy_set_task_method() #VSTOR-134085
2026-09-30 11:17 [PATCH hci-8.0 0/9] block: cheaper zero handling in backup and commit #VSTOR-134085 Denis V. Lunev
` (4 preceding siblings ...)
2026-09-30 11:17 ` [PATCH hci-8.0 5/9] block/block-copy: don't reserve memory for zero tasks #VSTOR-134085 Denis V. Lunev
@ 2026-09-30 11:17 ` Denis V. Lunev
2026-09-30 11:22 ` no-reply
2026-09-30 11:17 ` [PATCH hci-8.0 7/9] block/block-copy: track known-zero source clusters #VSTOR-134085 Denis V. Lunev
` (3 subsequent siblings)
9 siblings, 1 reply; 20+ messages in thread
From: Denis V. Lunev @ 2026-09-30 11:17 UTC (permalink / raw)
To: svt-core; +Cc: andrey.drobyshev, den
From: Denis V. Lunev <den@openvz.org>
Move the per-task block-status query and its skip_unallocated and
BDRV_BLOCK_ZERO handling out of block_copy_dirty_clusters() into
block_copy_set_task_method(). No behavior change. The next patch
changes that decision, and having it in one place keeps that change to
just the new logic.
The block-status result no longer lands in the caller's @ret, so drop
the note saying @ret may be positive at the out: label.
Reviewed-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
CC: John Snow <jsnow@redhat.com>
CC: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
block/block-copy.c | 46 ++++++++++++++++++++++++++++++----------------
1 file changed, 30 insertions(+), 16 deletions(-)
diff --git a/block/block-copy.c b/block/block-copy.c
index aecbe0c16ab..e210688b1df 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -780,6 +780,35 @@ int64_t coroutine_fn block_copy_reset_unallocated(BlockCopyState *s,
return ret;
}
+/*
+ * Decide how @task is copied: COPY_WRITE_ZEROES if it reads as zero. May
+ * shrink @task. Returns false if @task is to be skipped (already ended,
+ * not freed).
+ */
+static bool coroutine_fn GRAPH_RDLOCK
+block_copy_set_task_method(BlockCopyState *s, BlockCopyTask *task)
+{
+ int ret;
+ int64_t status_bytes;
+
+ ret = block_copy_block_status(s, task->req.offset, task->req.bytes,
+ &status_bytes);
+ assert(ret >= 0); /* never fail */
+ if (status_bytes < task->req.bytes) {
+ block_copy_task_shrink(task, status_bytes);
+ }
+ if (qatomic_read(&s->skip_unallocated) && !(ret & BDRV_BLOCK_ALLOCATED)) {
+ block_copy_task_end(task, 0);
+ trace_block_copy_skip_range(s, task->req.offset, task->req.bytes);
+ return false;
+ }
+ if (ret & BDRV_BLOCK_ZERO) {
+ task->method = COPY_WRITE_ZEROES;
+ }
+
+ return true;
+}
+
/*
* block_copy_dirty_clusters
*
@@ -812,7 +841,6 @@ block_copy_dirty_clusters(BlockCopyCallState *call_state)
while (bytes && aio_task_pool_status(aio) == 0 &&
!qatomic_read(&call_state->cancelled)) {
BlockCopyTask *task;
- int64_t status_bytes;
task = block_copy_task_create(s, call_state, offset, bytes);
if (!task) {
@@ -826,24 +854,12 @@ block_copy_dirty_clusters(BlockCopyCallState *call_state)
found_dirty = true;
- ret = block_copy_block_status(s, task->req.offset, task->req.bytes,
- &status_bytes);
- assert(ret >= 0); /* never fail */
- if (status_bytes < task->req.bytes) {
- block_copy_task_shrink(task, status_bytes);
- }
- if (qatomic_read(&s->skip_unallocated) &&
- !(ret & BDRV_BLOCK_ALLOCATED)) {
- block_copy_task_end(task, 0);
- trace_block_copy_skip_range(s, task->req.offset, task->req.bytes);
+ if (!block_copy_set_task_method(s, task)) {
offset = task_end(task);
bytes = end - offset;
g_free(task);
continue;
}
- if (ret & BDRV_BLOCK_ZERO) {
- task->method = COPY_WRITE_ZEROES;
- }
if (!call_state->ignore_ratelimit) {
uint64_t ns = ratelimit_calculate_delay(&s->rate_limit, 0);
@@ -884,8 +900,6 @@ out:
* block_copy_task_run. If it fails, it means some task already failed
* for real reason, let's return first failure.
* Still, assert that we don't rewrite failure by success.
- *
- * Note: ret may be positive here because of block-status result.
*/
assert(ret >= 0 || aio_task_pool_status(aio) < 0);
ret = aio_task_pool_status(aio);
--
2.53.0
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCH hci-8.0 7/9] block/block-copy: track known-zero source clusters #VSTOR-134085
2026-09-30 11:17 [PATCH hci-8.0 0/9] block: cheaper zero handling in backup and commit #VSTOR-134085 Denis V. Lunev
` (5 preceding siblings ...)
2026-09-30 11:17 ` [PATCH hci-8.0 6/9] block/block-copy: extract block_copy_set_task_method() #VSTOR-134085 Denis V. Lunev
@ 2026-09-30 11:17 ` Denis V. Lunev
2026-09-30 11:30 ` no-reply
2026-09-30 11:17 ` [PATCH hci-8.0 8/9] block/backup: pre-fill zero_bitmap for full/bitmap #VSTOR-134085 Denis V. Lunev
` (2 subsequent siblings)
9 siblings, 1 reply; 20+ messages in thread
From: Denis V. Lunev @ 2026-09-30 11:17 UTC (permalink / raw)
To: svt-core; +Cc: andrey.drobyshev, den
From: Denis V. Lunev <den@openvz.org>
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.
hci-8.0: 6b1a4754713 ("backup: skip copying unallocated for full
mode") scans for sync=full too and leaves skip_unallocated set after
the scan, choosing the base from top_mode instead. So
block_copy_status_base() keys on top_mode, and replaces the base
block_copy_is_cluster_allocated() computed on its own.
Reviewed-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
CC: John Snow <jsnow@redhat.com>
CC: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
block/backup.c | 1 +
block/block-copy.c | 117 ++++++++++++++++++++++++++++++-------
include/block/block-copy.h | 1 +
3 files changed, 98 insertions(+), 21 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index f7d8421812a..03ad1ad350b 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -284,6 +284,7 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
offset += count;
}
+ 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 e210688b1df..27a5d1a41b2 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"
@@ -159,7 +161,10 @@ typedef struct BlockCopyState {
/* State fields that use a thread-safe API */
BdrvDirtyBitmap *copy_bitmap;
bool top_mode;
-
+ /* Clusters reading as zero; allocated on demand, frozen once valid. */
+ HBitmap *zero_bitmap;
+ /* Published only once the scan is done. */
+ bool zero_bitmap_valid; /* atomic, store-release/load-acquire */
ProgressMeter *progress;
SharedResource *mem;
RateLimit rate_limit;
@@ -193,6 +198,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);
@@ -204,6 +210,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);
@@ -218,7 +245,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);
@@ -274,6 +301,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);
}
@@ -655,20 +685,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->top_mode)) {
+ 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->top_mode)) {
- 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) {
@@ -688,50 +722,79 @@ 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));
+ }
+
+ return s->zero_bitmap;
+}
+
+static void block_copy_mark_zero_prefix(BlockCopyState *s, int64_t offset,
+ int64_t zero_count)
+{
+ int64_t zero_bytes = QEMU_ALIGN_DOWN(zero_count, s->cluster_size);
+
+ if (zero_bytes > 0) {
+ hbitmap_set(block_copy_zero_bitmap(s), offset, zero_bytes);
+ }
+}
+
/*
* Check if the cluster starting at offset is allocated or not.
* return via pnum the number of contiguous clusters sharing this allocation.
+ * Also marks the zero prefix of the range in zero_bitmap.
*/
static int coroutine_fn GRAPH_RDLOCK
block_copy_is_cluster_allocated(BlockCopyState *s, int64_t offset,
int64_t *pnum)
{
BlockDriverState *bs = s->source->bs;
- BlockDriverState *base;
+ BlockDriverState *base = block_copy_status_base(s);
+ int64_t orig_offset = offset;
int64_t count, total_count = 0;
+ int64_t zero_count = 0;
+ bool zero_broken = false;
int64_t bytes = s->len - offset;
int ret;
- if (s->top_mode && s->source->bs->backing) {
- base = s->source->bs->backing->bs;
- } else {
- base = NULL;
- }
-
assert(QEMU_IS_ALIGNED(offset, s->cluster_size));
while (true) {
/* protected in backup_run() */
- ret = bdrv_co_is_allocated_above(bs, base, false, offset, bytes,
- &count);
+ ret = bdrv_co_block_status_above(bs, base, offset, bytes, &count,
+ NULL, NULL);
if (ret < 0) {
return ret;
}
+ if (!zero_broken) {
+ if (ret & BDRV_BLOCK_ZERO) {
+ zero_count += count;
+ } else {
+ zero_broken = true;
+ }
+ }
+
total_count += count;
- if (ret || count == 0) {
+ if ((ret & BDRV_BLOCK_ALLOCATED) || count == 0) {
/*
- * ret: partial segment(s) are considered allocated.
+ * BDRV_BLOCK_ALLOCATED: partial segment(s) are considered
+ * allocated.
* otherwise: unallocated tail is treated as an entire segment.
*/
*pnum = DIV_ROUND_UP(total_count, s->cluster_size);
- return ret;
+ block_copy_mark_zero_prefix(s, orig_offset, zero_count);
+ return !!(ret & BDRV_BLOCK_ALLOCATED);
}
/* Unallocated segment(s) with uncertain following segment(s) */
if (total_count >= s->cluster_size) {
*pnum = total_count / s->cluster_size;
+ block_copy_mark_zero_prefix(s, orig_offset, zero_count);
return 0;
}
@@ -791,6 +854,12 @@ block_copy_set_task_method(BlockCopyState *s, BlockCopyTask *task)
int ret;
int64_t status_bytes;
+ /* block_copy_task_create() already decided, from zero_bitmap. */
+ if (qatomic_load_acquire(&s->zero_bitmap_valid)) {
+ return true;
+ }
+
+ /* CBW filter could call this early. */
ret = block_copy_block_status(s, task->req.offset, task->req.bytes,
&status_bytes);
assert(ret >= 0); /* never fail */
@@ -1122,6 +1191,12 @@ void block_copy_set_options(BlockCopyState *s, bool skip_unallocated,
qatomic_set(&s->top_mode, top_mode);
}
+void block_copy_set_zero_bitmap_valid(BlockCopyState *s)
+{
+ block_copy_zero_bitmap(s);
+ qatomic_store_release(&s->zero_bitmap_valid, true);
+}
+
void block_copy_set_speed(BlockCopyState *s, uint64_t speed)
{
ratelimit_set_speed(&s->rate_limit, speed, BLOCK_COPY_SLICE_TIME);
diff --git a/include/block/block-copy.h b/include/block/block-copy.h
index d9304d1cf56..57e13f82f78 100644
--- a/include/block/block-copy.h
+++ b/include/block/block-copy.h
@@ -102,5 +102,6 @@ int64_t block_copy_cluster_size(BlockCopyState *s);
void block_copy_set_options(BlockCopyState *s, bool skip_unallocated,
bool top_mode);
void block_copy_set_global_error(BlockCopyState *s, int error);
+void block_copy_set_zero_bitmap_valid(BlockCopyState *s);
#endif /* BLOCK_COPY_H */
--
2.53.0
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH hci-8.0 7/9] block/block-copy: track known-zero source clusters #VSTOR-134085
2026-09-30 11:17 ` [PATCH hci-8.0 7/9] block/block-copy: track known-zero source clusters #VSTOR-134085 Denis V. Lunev
@ 2026-09-30 11:30 ` no-reply
0 siblings, 0 replies; 20+ messages in thread
From: no-reply @ 2026-09-30 11:30 UTC (permalink / raw)
To: svt-core, Denis V. Lunev; +Cc: den, andrey.drobyshev
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Low] block-copy: ctz32() on 64-bit cluster_size gives wrong zero_bitmap granularity
- [Low] block-copy: unaligned EOF tail cluster never marked zero in zero_bitmap
- [Low] block-copy: skip_unallocated doc stale; early return relies on implicit complete-scan invariant
--
commit 00cb83b3deb7d878b4202ae80e855cacc98ffc19
Author: Denis V. Lunev <den@openvz.org>
block/block-copy: track known-zero source clusters #VSTOR-134085
This patch caches the source's zero-ness in a new zero_bitmap, filled by
the block_copy_reset_unallocated() scan. The bitmap is published through
zero_bitmap_valid once the scan is done. After that,
block_copy_task_create() picks the copy method from the bitmap and ends
each task where the answer changes.
> diff --git a/block/block-copy.c b/block/block-copy.c
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
[ ... ]
> @@ -688,50 +722,79 @@ 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));
[Severity: Low]
Is ctz32() the right helper here? s->cluster_size is an int64_t, and
block_copy_state_new() accepts any power-of-two min_cluster_size up to
INT64_MAX.
For a cluster_size of 2^33 or more, the low 32 bits are zero. In that
case ctz32() returns 32, so hbitmap_alloc() gets granularity 32. The
zero_bitmap then describes different cluster boundaries than copy_bitmap.
Those boundaries would feed hbitmap_get() and hbitmap_next_zero() in
block_copy_task_create(). Would ctz64() be more appropriate? This only
matters for a very large x-perf min-cluster-size, so it may be a niche
case.
> + }
> +
> + return s->zero_bitmap;
> +}
> +
> +static void block_copy_mark_zero_prefix(BlockCopyState *s, int64_t offset,
> + int64_t zero_count)
> +{
> + int64_t zero_bytes = QEMU_ALIGN_DOWN(zero_count, s->cluster_size);
> +
> + if (zero_bytes > 0) {
> + hbitmap_set(block_copy_zero_bitmap(s), offset, zero_bytes);
> + }
> +}
[Severity: Low]
When s->len is not a multiple of cluster_size and the tail reads as
zero, QEMU_ALIGN_DOWN(zero_count, s->cluster_size) drops the tail
cluster. Is that intended?
The old per-task path went through block_copy_block_status(). It aligned
the final chunk up to a full cluster and could return BDRV_BLOCK_ZERO for
it.
Once zero_bitmap_valid is set, block_copy_set_task_method() returns early
and never re-queries. The tail is then read and written as data instead of
using COPY_WRITE_ZEROES. This is only a missed optimization of one
cluster-sized copy per job.
[ ... ]
> @@ -791,6 +854,12 @@ block_copy_set_task_method(BlockCopyState *s, BlockCopyTask *task)
> int ret;
> int64_t status_bytes;
>
> + /* block_copy_task_create() already decided, from zero_bitmap. */
> + if (qatomic_load_acquire(&s->zero_bitmap_valid)) {
> + return true;
> + }
[Severity: Low]
This isn't a bug, but the early return skips the skip_unallocated check
and the block_copy_task_shrink() call. That is correct only because
backup_run() sets the flag after the full scan. The scan has already
cleared every unallocated cluster from copy_bitmap.
The comment on BlockCopyState.skip_unallocated still says:
* In this case, block_copy() will query the source's allocation status,
* skip unallocated regions, clear them in the copy_bitmap, and invoke
* block_copy_reset_unallocated() every time it does.
That comment was not updated for the zero_bitmap short-circuit.
Should the complete-scan requirement be documented on
block_copy_set_zero_bitmap_valid(), or asserted? Otherwise a later caller
could publish the flag without a full scan. Unallocated clusters would
then be copied silently.
> +
> + /* CBW filter could call this early. */
> ret = block_copy_block_status(s, task->req.offset, task->req.bytes,
> &status_bytes);
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20260930111739.3866427-1-den@openvz.org?part=7
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH hci-8.0 8/9] block/backup: pre-fill zero_bitmap for full/bitmap #VSTOR-134085
2026-09-30 11:17 [PATCH hci-8.0 0/9] block: cheaper zero handling in backup and commit #VSTOR-134085 Denis V. Lunev
` (6 preceding siblings ...)
2026-09-30 11:17 ` [PATCH hci-8.0 7/9] block/block-copy: track known-zero source clusters #VSTOR-134085 Denis V. Lunev
@ 2026-09-30 11:17 ` Denis V. Lunev
2026-09-30 11:33 ` no-reply
2026-09-30 11:17 ` [PATCH hci-8.0 9/9] block/block-copy: coalesce write-zeroes tasks #VSTOR-134085 Denis V. Lunev
2026-09-30 12:49 ` [PATCH hci-8.0 0/9] block: cheaper zero handling in backup and commit #VSTOR-134085 Andrey Drobyshev
9 siblings, 1 reply; 20+ messages in thread
From: Denis V. Lunev @ 2026-09-30 11:17 UTC (permalink / raw)
To: svt-core; +Cc: andrey.drobyshev, den
From: Denis V. Lunev <den@openvz.org>
Only sync=top runs an up-front scan, so the previous patch did nothing
for the other modes. FULL and BITMAP know their complete copy_bitmap
before copying starts, and the same copy-before-write guarantee holds:
a cluster still dirty when the copy loop reaches it has not been written
since the scan.
Add block_copy_calculate_zero_bitmap(), driven one query at a time from
backup_scan_source() so the graph rdlock and the pause point are taken
per query, and query each confirmed span once however many dirty areas
it covers. The scan only fills zero_bitmap and never clears copy_bitmap,
so sync=full still writes zeroes for clusters the source never
allocated; it just stops re-querying to find that out. Unlike the
per-task queries it replaces the scan is serial, so a sparsely dirtied
job against a slow source may spend more time here than it saves. The
sync=top loop had the same shape, so both fold into
backup_scan_source().
sync=none keeps the per-task query. Its bitmap says "anything may be
copied", not "this will be copied", and it only ever copies what the
guest writes during the fleecing window, so scanning the whole image at
attach is the wrong trade.
hci-8.0: sync=full already runs block_copy_reset_unallocated() since
6b1a4754713, so it stays on that path and only sync=bitmap moves to
block_copy_calculate_zero_bitmap(). Unallocated clusters are not
written for sync=full here.
Reviewed-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
CC: John Snow <jsnow@redhat.com>
CC: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
block/backup.c | 95 +++++++++++++++++++++++---------------
block/block-copy.c | 41 ++++++++++++++++
include/block/block-copy.h | 4 ++
3 files changed, 104 insertions(+), 36 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 03ad1ad350b..eef4512b8bb 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -250,60 +250,83 @@ static void backup_init_bcs_bitmap(BackupBlockJob *job)
job_progress_set_remaining(&job->common.job, estimate);
}
-static int coroutine_fn backup_run(Job *job, Error **errp)
+/*
+ * Walk the source before copying starts: fill zero_bitmap and, for sync=top
+ * and sync=full, drop the clusters that are not allocated.
+ */
+static int coroutine_fn backup_scan_source(BackupBlockJob *s)
{
- BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
- int ret;
-
- backup_init_bcs_bitmap(s);
-
- if (s->sync_mode == MIRROR_SYNC_MODE_TOP ||
- s->sync_mode == MIRROR_SYNC_MODE_FULL)
- {
- int64_t offset = 0;
- int64_t count;
+ Job *job = &s->common.job;
+ bool skip = s->sync_mode == MIRROR_SYNC_MODE_TOP ||
+ s->sync_mode == MIRROR_SYNC_MODE_FULL;
+ int64_t offset, cached_end = 0, count;
+ int ret = 0;
- for (offset = 0; offset < s->len; ) {
- if (job_is_cancelled(job)) {
- return -ECANCELED;
- }
+ for (offset = 0; offset < s->len; offset += count) {
+ if (job_is_cancelled(job)) {
+ return -ECANCELED;
+ }
- job_pause_point(job);
+ job_pause_point(job);
- if (job_is_cancelled(job)) {
- return -ECANCELED;
- }
+ if (job_is_cancelled(job)) {
+ return -ECANCELED;
+ }
- /* rdlock protects the subsequent call to bdrv_is_allocated() */
- bdrv_graph_co_rdlock();
+ /* rdlock protects the block-status queries below */
+ bdrv_graph_co_rdlock();
+ if (skip) {
ret = block_copy_reset_unallocated(s->bcs, offset, &count);
- bdrv_graph_co_rdunlock();
- if (ret < 0) {
- return ret;
- }
+ } else {
+ block_copy_calculate_zero_bitmap(s->bcs, offset, &cached_end,
+ &count);
+ }
+ bdrv_graph_co_rdunlock();
- offset += count;
+ if (ret < 0) {
+ return ret;
}
- block_copy_set_zero_bitmap_valid(s->bcs);
}
- if (s->sync_mode == MIRROR_SYNC_MODE_NONE) {
+ block_copy_set_zero_bitmap_valid(s->bcs);
+
+ return 0;
+}
+
+static int coroutine_fn backup_run(Job *job, Error **errp)
+{
+ BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
+ int ret;
+
+ backup_init_bcs_bitmap(s);
+
+ switch (s->sync_mode) {
+ case MIRROR_SYNC_MODE_TOP:
+ case MIRROR_SYNC_MODE_FULL:
+ case MIRROR_SYNC_MODE_BITMAP:
+ ret = backup_scan_source(s);
+ if (ret < 0) {
+ return ret;
+ }
+ break;
+
+ case MIRROR_SYNC_MODE_NONE:
/*
* All bits are set in bcs bitmap to allow any cluster to be copied.
- * This does not actually require them to be copied.
+ * This does not actually require them to be copied. Yield until the
+ * job is cancelled and let the before_write notify callback service
+ * CoW requests.
*/
while (!job_is_cancelled(job)) {
- /*
- * Yield until the job is cancelled. We just let our before_write
- * notify callback service CoW requests.
- */
job_yield(job);
}
- } else {
- return backup_loop(s);
+ return 0;
+
+ default:
+ g_assert_not_reached();
}
- return 0;
+ return backup_loop(s);
}
static void coroutine_fn backup_pause(Job *job)
diff --git a/block/block-copy.c b/block/block-copy.c
index 27a5d1a41b2..a35fba45183 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -843,6 +843,47 @@ int64_t coroutine_fn block_copy_reset_unallocated(BlockCopyState *s,
return ret;
}
+/*
+ * One scan step, like block_copy_reset_unallocated(); @cached_end
+ * tracks progress across calls, @count how far @offset should move.
+ */
+void coroutine_fn GRAPH_RDLOCK
+block_copy_calculate_zero_bitmap(BlockCopyState *s, int64_t offset,
+ int64_t *cached_end, int64_t *count)
+{
+ int64_t dirty_offset, dirty_bytes, dirty_end;
+ bool found;
+
+ /* CBW mutates copy_bitmap from its own AioContext meanwhile. */
+ WITH_QEMU_LOCK_GUARD(&s->lock) {
+ found = bdrv_dirty_bitmap_next_dirty_area(s->copy_bitmap, offset,
+ s->len, INT64_MAX,
+ &dirty_offset, &dirty_bytes);
+ }
+
+ if (!found) {
+ *count = s->len - offset;
+ return;
+ }
+ dirty_end = dirty_offset + dirty_bytes;
+
+ if (*cached_end < dirty_end) {
+ int64_t clusters;
+ int64_t query_offset = MAX(dirty_offset, *cached_end);
+ int ret = block_copy_is_cluster_allocated(s, query_offset, &clusters);
+
+ if (ret >= 0) {
+ *cached_end = query_offset + clusters * s->cluster_size;
+ } else {
+ /* Best-effort: leave this area unmarked; it copies as data. */
+ *cached_end = dirty_end;
+ }
+ }
+
+ /* 0 if the query above didn't yet reach dirty_end: try again next call. */
+ *count = *cached_end >= dirty_end ? dirty_end - offset : 0;
+}
+
/*
* Decide how @task is copied: COPY_WRITE_ZEROES if it reads as zero. May
* shrink @task. Returns false if @task is to be skipped (already ended,
diff --git a/include/block/block-copy.h b/include/block/block-copy.h
index 57e13f82f78..40dd5432735 100644
--- a/include/block/block-copy.h
+++ b/include/block/block-copy.h
@@ -43,6 +43,10 @@ void block_copy_reset(BlockCopyState *s, int64_t offset, int64_t bytes);
int64_t coroutine_fn GRAPH_RDLOCK
block_copy_reset_unallocated(BlockCopyState *s, int64_t offset, int64_t *count);
+void coroutine_fn GRAPH_RDLOCK
+block_copy_calculate_zero_bitmap(BlockCopyState *s, int64_t offset,
+ int64_t *cached_end, int64_t *count);
+
int coroutine_fn block_copy(BlockCopyState *s, int64_t offset, int64_t bytes,
bool ignore_ratelimit, uint64_t timeout_ns,
BlockCopyAsyncCallbackFunc cb,
--
2.53.0
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH hci-8.0 8/9] block/backup: pre-fill zero_bitmap for full/bitmap #VSTOR-134085
2026-09-30 11:17 ` [PATCH hci-8.0 8/9] block/backup: pre-fill zero_bitmap for full/bitmap #VSTOR-134085 Denis V. Lunev
@ 2026-09-30 11:33 ` no-reply
0 siblings, 0 replies; 20+ messages in thread
From: no-reply @ 2026-09-30 11:33 UTC (permalink / raw)
To: Denis V. Lunev, svt-core; +Cc: den, andrey.drobyshev
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] block: block_copy_calculate_zero_bitmap() can return count 0 forever, hanging the scan
- [Low] block/backup: commit message says full/bitmap but only sync=bitmap uses the new scan
--
commit 252a85c484652c4a95f8a77f49b08e8150b81585
Author: Denis V. Lunev <den@openvz.org>
block/backup: pre-fill zero_bitmap for full/bitmap #VSTOR-134085
This commit adds block_copy_calculate_zero_bitmap() and folds the
up-front source scan into a new backup_scan_source(). The scan now
pre-fills zero_bitmap for sync=bitmap as well as sync=top. sync=full
stays on block_copy_reset_unallocated(), and sync=none keeps the
per-task query.
> block/backup: pre-fill zero_bitmap for full/bitmap #VSTOR-134085
This isn't a code bug, but the subject line and body say that both FULL
and BITMAP get the new up-front scan. Two quoted portions of the commit
message read this way:
"Add block_copy_calculate_zero_bitmap(), driven one query at a time
from backup_scan_source() ..."
"FULL and BITMAP know their complete copy_bitmap before copying
starts"
In the code, sync=full still takes the block_copy_reset_unallocated()
path, and only sync=bitmap calls block_copy_calculate_zero_bitmap(). The
trailing paragraph of the message says so itself: "sync=full already
runs block_copy_reset_unallocated() ... so it stays on that path and only
sync=bitmap moves to block_copy_calculate_zero_bitmap()".
That leaves the message contradicting itself. It also carries the
downstream "hci-8.0:" tag. Could the subject and body be reworded to
match the code?
[Severity: Low]
> diff --git a/block/backup.c b/block/backup.c
> --- a/block/backup.c
> +++ b/block/backup.c
[ ... ]
> +static int coroutine_fn backup_scan_source(BackupBlockJob *s)
> {
[ ... ]
> + for (offset = 0; offset < s->len; offset += count) {
> + if (job_is_cancelled(job)) {
> + return -ECANCELED;
> + }
> +
> + job_pause_point(job);
[ ... ]
> + bdrv_graph_co_rdlock();
> + if (skip) {
> + ret = block_copy_reset_unallocated(s->bcs, offset, &count);
> + } else {
> + block_copy_calculate_zero_bitmap(s->bcs, offset, &cached_end,
> + &count);
> + }
> + bdrv_graph_co_rdunlock();
> diff --git a/block/block-copy.c b/block/block-copy.c
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
[ ... ]
> + if (*cached_end < dirty_end) {
> + int64_t clusters;
> + int64_t query_offset = MAX(dirty_offset, *cached_end);
> + int ret = block_copy_is_cluster_allocated(s, query_offset, &clusters);
> +
> + if (ret >= 0) {
> + *cached_end = query_offset + clusters * s->cluster_size;
> + } else {
> + /* Best-effort: leave this area unmarked; it copies as data. */
> + *cached_end = dirty_end;
> + }
> + }
> +
> + /* 0 if the query above didn't yet reach dirty_end: try again next call. */
> + *count = *cached_end >= dirty_end ? dirty_end - offset : 0;
> +}
Can block_copy_calculate_zero_bitmap() return *count == 0 forever for
sync=bitmap?
block_copy_is_cluster_allocated() sets *pnum = DIV_ROUND_UP(total_count,
cluster_size). That is 0 when bdrv_co_block_status_above() returns
count == 0, for example at or beyond the source EOF. The function still
returns ret >= 0 in that case, since it has an explicit "count == 0"
branch.
If that happens, the ret >= 0 branch above sets
*cached_end = query_offset + 0 * s->cluster_size;
which is still below dirty_end. *count then stays 0.
In backup_scan_source(), "offset += count" does not advance, so the next
iteration repeats the identical query. job_pause_point() and
job_is_cancelled() do not yield unless the job is paused or cancelled.
That would leave the scan spinning without yielding in the job's
AioContext.
I could not confirm that s->len can exceed the real source length, for
example if the source shrinks during the job, so this may be hard to
reach.
The same "offset += count" shape already exists in
block_copy_reset_unallocated() for top/full. However, the sync=bitmap
path through block_copy_calculate_zero_bitmap() is new with this patch.
The later patch "block/block-copy: coalesce write-zeroes tasks" does not
touch this scan.
Would it make sense to force progress, for example by treating
clusters == 0 like the error path (cached_end = dirty_end), or by
clamping clusters to at least 1?
[Severity: Medium]
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20260930111739.3866427-1-den@openvz.org?part=8
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH hci-8.0 9/9] block/block-copy: coalesce write-zeroes tasks #VSTOR-134085
2026-09-30 11:17 [PATCH hci-8.0 0/9] block: cheaper zero handling in backup and commit #VSTOR-134085 Denis V. Lunev
` (7 preceding siblings ...)
2026-09-30 11:17 ` [PATCH hci-8.0 8/9] block/backup: pre-fill zero_bitmap for full/bitmap #VSTOR-134085 Denis V. Lunev
@ 2026-09-30 11:17 ` Denis V. Lunev
2026-09-30 11:34 ` no-reply
2026-09-30 12:49 ` [PATCH hci-8.0 0/9] block: cheaper zero handling in backup and commit #VSTOR-134085 Andrey Drobyshev
9 siblings, 1 reply; 20+ messages in thread
From: Denis V. Lunev @ 2026-09-30 11:17 UTC (permalink / raw)
To: svt-core; +Cc: andrey.drobyshev, den
From: Denis V. Lunev <den@openvz.org>
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.
hci-8.0: 6b1a4754713 made the 056 copy-before-write source read as
zeroes. One widened write-zeroes task now covers all of it, and the
rate limit only charges a task after it ran, so speed=1 no longer
keeps the job alive until the test cancels it. Cap the task with
x-perf max-chunk there.
Reviewed-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
CC: John Snow <jsnow@redhat.com>
CC: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
block/block-copy.c | 94 ++++++++++++++++++++++++++++++++++++++----
tests/qemu-iotests/056 | 3 +-
2 files changed, 89 insertions(+), 8 deletions(-)
diff --git a/block/block-copy.c b/block/block-copy.c
index a35fba45183..082257da7be 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -161,6 +161,8 @@ typedef struct BlockCopyState {
/* State fields that use a thread-safe API */
BdrvDirtyBitmap *copy_bitmap;
bool top_mode;
+ /* 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 once the scan is done. */
@@ -189,6 +191,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.
@@ -200,6 +230,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);
@@ -221,6 +252,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);
@@ -481,6 +516,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;
@@ -537,6 +573,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);
+ 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
*
@@ -568,13 +654,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:
diff --git a/tests/qemu-iotests/056 b/tests/qemu-iotests/056
index e006d1296d9..72ea28365ea 100755
--- a/tests/qemu-iotests/056
+++ b/tests/qemu-iotests/056
@@ -117,7 +117,8 @@ class TestCopyBeforeWrite(iotests.QMPTestCase):
self.vm.pause_drive("drive0")
self.vm.cmd('drive-backup', device='drive0',
sync='full', target=target_img,
- format="file", speed=1)
+ format="file", speed=1,
+ x_perf={'max-chunk': 65536})
self.vm.cmd('block-job-pause', device="drive0")
# Speed is low enough that this must be an uncopied range, which will
# trigger the before write notifier
--
2.53.0
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH hci-8.0 9/9] block/block-copy: coalesce write-zeroes tasks #VSTOR-134085
2026-09-30 11:17 ` [PATCH hci-8.0 9/9] block/block-copy: coalesce write-zeroes tasks #VSTOR-134085 Denis V. Lunev
@ 2026-09-30 11:34 ` no-reply
0 siblings, 0 replies; 20+ messages in thread
From: no-reply @ 2026-09-30 11:34 UTC (permalink / raw)
To: Denis V. Lunev, svt-core; +Cc: andrey.drobyshev, den
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] block-copy: widened write-zeroes task blocks overlapping writers and rate limit for ~2 GiB
- [Low] block-copy: write_zeroes_fail trace reports advanced offset in chunked fallback
--
commit f1b3fcf0ac0c96a51feac391dbd24c727baced51
Author: Denis V. Lunev <den@openvz.org>
block/block-copy: coalesce write-zeroes tasks #VSTOR-134085
This patch lets a write-zeroes task in block-copy grow to about
BDRV_REQUEST_MAX_BYTES instead of being capped at block_copy_chunk_size().
It adds block_copy_widen_zero_area() and a new block_copy_write_zeroes().
The new function first asks the target for BDRV_REQ_NO_FALLBACK and falls
back to chunked writes on -ENOTSUP. The patch also caps the chunk size in
qemu-iotests 056.
> diff --git a/block/block-copy.c b/block/block-copy.c
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
[ ... ]
> +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);
[Severity: Medium]
Can one widened task now hold its BlockReq for too long?
With this limit, block_copy_task_create() can register a single BlockReq of
up to about 2 GiB through reqlist_init_req().
A copy-before-write guest write that overlaps the range waits in
reqlist_wait_one() until one bdrv_co_pwrite_zeroes() of that whole size
completes.
The job rate limit only charges a task after it has run. Pause and cancel
also take effect only between tasks, so they wait for the whole request too.
The only mitigation left is the existing max-chunk option. The commit
message admits this effect for test 056, but the limit is changed for all
users.
Is there a reason not to bound the task size by something smaller by
default?
[ ... ]
> +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;
> + }
[Severity: Medium]
Is this a problem for slow targets?
The first request covers the whole widened range in one call. If the
target refuses the fast path, for example file-posix on a failing
fallocate, the range is then rewritten in chunks by the while loop below.
Overlapping writers stay blocked on the task's BlockReq during both
passes. NO_FALLBACK is also only optimistic, as the commit message says,
so slow zeroing paths such as a block device or a network filesystem can
still be reached.
[ ... ]
> + offset += n;
> + bytes -= n;
> + }
> +
> +out:
> + if (ret < 0) {
> + trace_block_copy_write_zeroes_fail(s, offset, ret);
[Severity: Low]
This isn't a bug, but the offset passed to trace_block_copy_write_zeroes_fail()
differs between paths.
In the chunked fallback loop, offset is advanced by n after each
successful chunk. If a later chunk fails, the trace reports the advanced
offset. If the NO_FALLBACK request fails, it reports the task start.
The old code always reported the task offset. Would it be better to report
the same value on both paths, for example by using a separate variable for
the loop position?
[ ... ]
> diff --git a/tests/qemu-iotests/056 b/tests/qemu-iotests/056
> --- a/tests/qemu-iotests/056
> +++ b/tests/qemu-iotests/056
[ ... ]
> @@ -117,7 +117,8 @@ class TestCopyBeforeWrite(iotests.QMPTestCase):
> self.vm.pause_drive("drive0")
> self.vm.cmd('drive-backup', device='drive0',
> sync='full', target=target_img,
> - format="file", speed=1)
> + format="file", speed=1,
> + x_perf={'max-chunk': 65536})
[Severity: Medium]
The test has to cap the chunk size by hand to keep the job rate-limited.
Does this show that the throttling behavior changed for real users, with
the workaround applied only in the test?
--
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20260930111739.3866427-1-den@openvz.org?part=9
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH hci-8.0 0/9] block: cheaper zero handling in backup and commit #VSTOR-134085
2026-09-30 11:17 [PATCH hci-8.0 0/9] block: cheaper zero handling in backup and commit #VSTOR-134085 Denis V. Lunev
` (8 preceding siblings ...)
2026-09-30 11:17 ` [PATCH hci-8.0 9/9] block/block-copy: coalesce write-zeroes tasks #VSTOR-134085 Denis V. Lunev
@ 2026-09-30 12:49 ` Andrey Drobyshev
9 siblings, 0 replies; 20+ messages in thread
From: Andrey Drobyshev @ 2026-09-30 12:49 UTC (permalink / raw)
To: Denis V. Lunev, svt-core; +Cc: den
No real findings from Sashiko
Ack, applied to hci-dev
Andrey
On 9/30/26 2:17 PM, Denis V. Lunev wrote:
> Backup and commit re-query the source block status for every task and
> size a task by the copy buffer, so a long run of zeroes turns into a
> crowd of small write-zeroes requests. This series reuses what the
> up-front scan already learned and lets one write-zeroes task cover a
> whole run.
>
> This is the series posted upstream as v3, all nine patches with
> Andrey's Reviewed-by:
> https://lore.kernel.org/qemu-devel/20260929155125.3151111-1-den@openvz.org/
>
> hci-8.0 carries 6b1a4754713 ("backup: skip copying unallocated for full
> mode"): sync=full runs the same up-front scan as sync=top, drops the
> clusters the source never allocated, and keeps skip_unallocated set
> afterwards, taking the block-status base from a separate top_mode flag.
> So the upstream cover letter case, a mostly unallocated image, is
> already cheap here. What the series buys on hci-8.0 is allocated zero
> clusters, the ones a guest write-zeroes or detect-zeroes leaves behind,
> in sync=full and in sync=bitmap, and the commit job.
>
> Where the port differs from upstream, 4/9, 7/9, 8/9 and 9/9 say so in
> an "hci-8.0:" paragraph:
> - 3/9: only a context conflict. hci-8.0 lacks upstream a6c131217d,
> which adds coroutine_fn to commit_iteration(), so the signature stays
> as it is here.
> - 4/9: iotest 124 expects a sync=full target to keep the source's
> holes, since the unallocated clusters are skipped rather than written
> as zeroes.
> - 7/9: block_copy_status_base() keys on top_mode instead of
> skip_unallocated, and the scan uses it too instead of computing the
> base on its own.
> - 8/9: sync=full stays on block_copy_reset_unallocated(); only
> sync=bitmap moves to block_copy_calculate_zero_bitmap().
> - 9/9: 6b1a4754713 also made the 056 copy-before-write source read as
> zeroes. One widened write-zeroes task now covers all of it before
> speed=1 can hold the job, so the test caps the task with x-perf
> max-chunk. Andrey, the 4/9 and 9/9 test changes are new with this
> port and not covered by your upstream review.
>
> Every patch builds on its own. On dev007 with qcow2, iotests 040 055 056
> 124 129 141 185 219 257 image-fleecing backup-discard-source
> copy-before-write commit-zero-blocks iothreads-commit-active and
> migrate-during-backup pass. vz-kernel-cbt fails there with and without
> the series, the host kernel has no vz CBT.
>
> On the same host, a 16G qcow2 source holding 1G of data, raw target on
> ext4, counting block_copy_process and commit_one_iteration trace events:
>
> before after
> sync=full, 15G unallocated 1024 0.44s 1024 0.44s
> sync=full, 15G zero clusters 16384 0.66s 1032 0.46s
> sync=bitmap, all dirty, same 16384 0.65s 1032 0.46s
> commit, 15G zero clusters 32768 2.84s 2288 2.70s
>
> Commit time is dominated by copying the 1G of data.
>
> Denis V. Lunev (9):
> block/commit: pass BDRV_WANT_PRECISE to block-status #VSTOR-134085
> iotests/040: cover large and fragmented commit runs #VSTOR-134085
> block/commit: batch block-status queries #VSTOR-134085
> iotests/124: cover backup of zero clusters and holes #VSTOR-134085
> block/block-copy: don't reserve memory for zero tasks #VSTOR-134085
> block/block-copy: extract block_copy_set_task_method() #VSTOR-134085
> block/block-copy: track known-zero source clusters #VSTOR-134085
> block/backup: pre-fill zero_bitmap for full/bitmap #VSTOR-134085
> block/block-copy: coalesce write-zeroes tasks #VSTOR-134085
>
> block/backup.c | 94 ++++++----
> block/block-copy.c | 309 +++++++++++++++++++++++++++-----
> block/commit.c | 59 +++++--
> include/block/block-copy.h | 5 +
> tests/qemu-iotests/040 | 102 ++++++++++-
> tests/qemu-iotests/040.out | 4 +-
> tests/qemu-iotests/056 | 3 +-
> tests/qemu-iotests/124 | 350 ++++++++++++++++++++++++++++++++++++-
> tests/qemu-iotests/124.out | 4 +-
> 9 files changed, 830 insertions(+), 100 deletions(-)
>
>
> base-commit: f2f4b3de1a1be3360a77ca6ab79762466d238804
^ permalink raw reply [flat|nested] 20+ messages in thread