Re: [PATCH hci-8.0 9/9] block/block-copy: coalesce write-zeroes tasks #VSTOR-134085

[no-reply@sashiko.virtuozzo.com]

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