Signed-off-by: Vincent Vanlaer <libvirt-e6954efa@volkihar.be>
---
block/commit.c | 61 ++++++++++++++++++++++++++++----------------------
1 file changed, 34 insertions(+), 27 deletions(-)
diff --git a/block/commit.c b/block/commit.c
index 078e54f51f..5c24c8b80a 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -129,51 +129,58 @@ static void commit_clean(Job *job)
}
static int commit_iteration(CommitBlockJob *s, int64_t offset,
- int64_t *n, void *buf)
+ int64_t *requested_bytes, void *buf)
{
+ int64_t bytes = *requested_bytes;
int ret = 0;
- bool copy;
bool error_in_source = true;
/* Copy if allocated above the base */
WITH_GRAPH_RDLOCK_GUARD() {
ret = bdrv_co_common_block_status_above(blk_bs(s->top),
s->base_overlay, true, true, offset, COMMIT_BUFFER_SIZE,
- n, NULL, NULL, NULL);
+ &bytes, NULL, NULL, NULL);
}
- copy = (ret >= 0 && ret & BDRV_BLOCK_ALLOCATED);
- trace_commit_one_iteration(s, offset, *n, ret);
- if (copy) {
- assert(*n < SIZE_MAX);
+ trace_commit_one_iteration(s, offset, bytes, ret);
- ret = blk_co_pread(s->top, offset, *n, buf, 0);
- if (ret >= 0) {
- ret = blk_co_pwrite(s->base, offset, *n, buf, 0);
- if (ret < 0) {
- error_in_source = false;
- }
- }
- }
if (ret < 0) {
- BlockErrorAction action = block_job_error_action(&s->common,
- s->on_error,
- error_in_source,
- -ret);
- if (action == BLOCK_ERROR_ACTION_REPORT) {
- return ret;
- } else {
- *n = 0;
- return 0;
+ goto fail;
+ }
+
+ if (ret & BDRV_BLOCK_ALLOCATED) {
+ assert(bytes < SIZE_MAX);
+
+ ret = blk_co_pread(s->top, offset, bytes, buf, 0);
+ if (ret < 0) {
+ goto fail;
}
+
+ ret = blk_co_pwrite(s->base, offset, bytes, buf, 0);
+ if (ret < 0) {
+ error_in_source = false;
+ goto fail;
+ }
+
+ block_job_ratelimit_processed_bytes(&s->common, bytes);
}
+
/* Publish progress */
- job_progress_update(&s->common.job, *n);
- if (copy) {
- block_job_ratelimit_processed_bytes(&s->common, *n);
+ job_progress_update(&s->common.job, bytes);
+
+ *requested_bytes = bytes;
+
+ return 0;
+fail:;
+ BlockErrorAction action = block_job_error_action(&s->common, s->on_error,
+ error_in_source, -ret);
+ if (action == BLOCK_ERROR_ACTION_REPORT) {
+ return ret;
}
+ *requested_bytes = 0;
+
return 0;
}
--
2.44.1On 26.10.24 19:30, Vincent Vanlaer wrote:
> Signed-off-by: Vincent Vanlaer <libvirt-e6954efa@volkihar.be>
> ---
> block/commit.c | 61 ++++++++++++++++++++++++++++----------------------
> 1 file changed, 34 insertions(+), 27 deletions(-)
>
> diff --git a/block/commit.c b/block/commit.c
> index 078e54f51f..5c24c8b80a 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -129,51 +129,58 @@ static void commit_clean(Job *job)
> }
>
> static int commit_iteration(CommitBlockJob *s, int64_t offset,
> - int64_t *n, void *buf)
> + int64_t *requested_bytes, void *buf)
> {
> + int64_t bytes = *requested_bytes;
> int ret = 0;
> - bool copy;
> bool error_in_source = true;
>
> /* Copy if allocated above the base */
> WITH_GRAPH_RDLOCK_GUARD() {
> ret = bdrv_co_common_block_status_above(blk_bs(s->top),
> s->base_overlay, true, true, offset, COMMIT_BUFFER_SIZE,
> - n, NULL, NULL, NULL);
> + &bytes, NULL, NULL, NULL);
> }
>
> - copy = (ret >= 0 && ret & BDRV_BLOCK_ALLOCATED);
> - trace_commit_one_iteration(s, offset, *n, ret);
> - if (copy) {
> - assert(*n < SIZE_MAX);
> + trace_commit_one_iteration(s, offset, bytes, ret);
>
> - ret = blk_co_pread(s->top, offset, *n, buf, 0);
> - if (ret >= 0) {
> - ret = blk_co_pwrite(s->base, offset, *n, buf, 0);
> - if (ret < 0) {
> - error_in_source = false;
> - }
> - }
> - }
> if (ret < 0) {
> - BlockErrorAction action = block_job_error_action(&s->common,
> - s->on_error,
> - error_in_source,
> - -ret);
> - if (action == BLOCK_ERROR_ACTION_REPORT) {
> - return ret;
> - } else {
> - *n = 0;
> - return 0;
> + goto fail;
> + }
> +
> + if (ret & BDRV_BLOCK_ALLOCATED) {
> + assert(bytes < SIZE_MAX);
> +
> + ret = blk_co_pread(s->top, offset, bytes, buf, 0);
> + if (ret < 0) {
> + goto fail;
> }
> +
> + ret = blk_co_pwrite(s->base, offset, bytes, buf, 0);
> + if (ret < 0) {
> + error_in_source = false;
> + goto fail;
> + }
> +
> + block_job_ratelimit_processed_bytes(&s->common, bytes);
> }
> +
> /* Publish progress */
> - job_progress_update(&s->common.job, *n);
>
> - if (copy) {
> - block_job_ratelimit_processed_bytes(&s->common, *n);
> + job_progress_update(&s->common.job, bytes);
> +
> + *requested_bytes = bytes;
> +
> + return 0;
With this extra semicolon removed:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
I'd also add an empty line before "fail:".
> +fail:;
> + BlockErrorAction action = block_job_error_action(&s->common, s->on_error,
> + error_in_source, -ret);
> + if (action == BLOCK_ERROR_ACTION_REPORT) {
> + return ret;
> }
>
> + *requested_bytes = 0;
> +
> return 0;
> }
>
--
Best regards,
Vladimir
On 18.11.24 10:37, Vladimir Sementsov-Ogievskiy wrote:
> On 26.10.24 19:30, Vincent Vanlaer wrote:
>> Signed-off-by: Vincent Vanlaer <libvirt-e6954efa@volkihar.be>
>> ---
>> block/commit.c | 61 ++++++++++++++++++++++++++++----------------------
>> 1 file changed, 34 insertions(+), 27 deletions(-)
>>
>> diff --git a/block/commit.c b/block/commit.c
>> index 078e54f51f..5c24c8b80a 100644
>> --- a/block/commit.c
>> +++ b/block/commit.c
>> @@ -129,51 +129,58 @@ static void commit_clean(Job *job)
>> }
>> static int commit_iteration(CommitBlockJob *s, int64_t offset,
>> - int64_t *n, void *buf)
>> + int64_t *requested_bytes, void *buf)
>> {
>> + int64_t bytes = *requested_bytes;
>> int ret = 0;
>> - bool copy;
>> bool error_in_source = true;
>> /* Copy if allocated above the base */
>> WITH_GRAPH_RDLOCK_GUARD() {
>> ret = bdrv_co_common_block_status_above(blk_bs(s->top),
>> s->base_overlay, true, true, offset, COMMIT_BUFFER_SIZE,
>> - n, NULL, NULL, NULL);
>> + &bytes, NULL, NULL, NULL);
>> }
>> - copy = (ret >= 0 && ret & BDRV_BLOCK_ALLOCATED);
>> - trace_commit_one_iteration(s, offset, *n, ret);
>> - if (copy) {
>> - assert(*n < SIZE_MAX);
>> + trace_commit_one_iteration(s, offset, bytes, ret);
>> - ret = blk_co_pread(s->top, offset, *n, buf, 0);
>> - if (ret >= 0) {
>> - ret = blk_co_pwrite(s->base, offset, *n, buf, 0);
>> - if (ret < 0) {
>> - error_in_source = false;
>> - }
>> - }
>> - }
>> if (ret < 0) {
>> - BlockErrorAction action = block_job_error_action(&s->common,
>> - s->on_error,
>> - error_in_source,
>> - -ret);
>> - if (action == BLOCK_ERROR_ACTION_REPORT) {
>> - return ret;
>> - } else {
>> - *n = 0;
>> - return 0;
>> + goto fail;
>> + }
>> +
>> + if (ret & BDRV_BLOCK_ALLOCATED) {
>> + assert(bytes < SIZE_MAX);
>> +
>> + ret = blk_co_pread(s->top, offset, bytes, buf, 0);
>> + if (ret < 0) {
>> + goto fail;
>> }
>> +
>> + ret = blk_co_pwrite(s->base, offset, bytes, buf, 0);
>> + if (ret < 0) {
>> + error_in_source = false;
>> + goto fail;
>> + }
>> +
>> + block_job_ratelimit_processed_bytes(&s->common, bytes);
>> }
>> +
>> /* Publish progress */
>> - job_progress_update(&s->common.job, *n);
>> - if (copy) {
>> - block_job_ratelimit_processed_bytes(&s->common, *n);
>> + job_progress_update(&s->common.job, bytes);
>> +
>> + *requested_bytes = bytes;
>> +
>> + return 0;
>
> With this extra semicolon removed:
I meant "fail:;"
I'll will touch this up when applying.
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>
> I'd also add an empty line before "fail:".
>
>
>> +fail:;
>> + BlockErrorAction action = block_job_error_action(&s->common, s->on_error,
>> + error_in_source, -ret);
>> + if (action == BLOCK_ERROR_ACTION_REPORT) {
>> + return ret;
>> }
>> + *requested_bytes = 0;
>> +
>> return 0;
>> }
>
--
Best regards,
Vladimir
© 2016 - 2026 Red Hat, Inc.