Re: [PATCH hci-8.0 8/9] block/backup: pre-fill zero_bitmap for full/bitmap #VSTOR-134085

[no-reply@sashiko.virtuozzo.com]

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