Re: [PATCH hci-8.0 7/9] block/block-copy: track known-zero source clusters #VSTOR-134085

[no-reply@sashiko.virtuozzo.com]

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