Re: [PATCH hci-8.0 4/9] iotests/124: cover backup of zero clusters and holes #VSTOR-134085

[no-reply@sashiko.virtuozzo.com]

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