Re: [PATCH hci-8.0 3/9] block/commit: batch block-status queries #VSTOR-134085

[no-reply@sashiko.virtuozzo.com]

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