Signed-off-by: Vincent Vanlaer <libvirt-e6954efa@volkihar.be>
---
block/commit.c | 85 ++++++++++++++++++++++++++++----------------------
1 file changed, 48 insertions(+), 37 deletions(-)
diff --git a/block/commit.c b/block/commit.c
index 8dee25b313..9eedd1fa47 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -128,6 +128,51 @@ static void commit_clean(Job *job)
blk_unref(s->top);
}
+static int commit_iteration(CommitBlockJob *s, int64_t offset, int64_t *n, void *buf) {
+ 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);
+ }
+
+ copy = (ret >= 0 && ret & BDRV_BLOCK_ALLOCATED);
+ trace_commit_one_iteration(s, offset, *n, ret);
+ if (copy) {
+ assert(*n < SIZE_MAX);
+
+ 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;
+ }
+ }
+ /* Publish progress */
+ job_progress_update(&s->common.job, *n);
+
+ if (copy) {
+ block_job_ratelimit_processed_bytes(&s->common, *n);
+ }
+
+ return 0;
+}
+
static int coroutine_fn commit_run(Job *job, Error **errp)
{
CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
@@ -158,9 +203,6 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
buf = blk_blockalign(s->top, COMMIT_BUFFER_SIZE);
for (offset = 0; offset < len; offset += n) {
- bool copy;
- bool error_in_source = true;
-
/* Note that even when no rate limit is applied we need to yield
* with no pending I/O here so that bdrv_drain_all() returns.
*/
@@ -168,42 +210,11 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
if (job_is_cancelled(&s->common.job)) {
break;
}
- /* 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);
- }
- copy = (ret >= 0 && ret & BDRV_BLOCK_ALLOCATED);
- trace_commit_one_iteration(s, offset, n, ret);
- if (copy) {
- assert(n < SIZE_MAX);
-
- 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;
- continue;
- }
- }
- /* Publish progress */
- job_progress_update(&s->common.job, n);
+ ret = commit_iteration(s, offset, &n, buf);
- if (copy) {
- block_job_ratelimit_processed_bytes(&s->common, n);
+ if (ret < 0) {
+ return ret;
}
}
--
2.44.1On 01.09.24 17:24, Vincent Vanlaer wrote:
> +static int commit_iteration(CommitBlockJob *s, int64_t offset, int64_t *n, void *buf) {
Hmm over-80 line. Didn't you forget run ./checkpatch.pl ?
--
Best regards,
Vladimir
On 01.09.24 17:24, Vincent Vanlaer wrote:
> Signed-off-by: Vincent Vanlaer <libvirt-e6954efa@volkihar.be>
> ---
> block/commit.c | 85 ++++++++++++++++++++++++++++----------------------
> 1 file changed, 48 insertions(+), 37 deletions(-)
>
> diff --git a/block/commit.c b/block/commit.c
> index 8dee25b313..9eedd1fa47 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -128,6 +128,51 @@ static void commit_clean(Job *job)
> blk_unref(s->top);
> }
>
> +static int commit_iteration(CommitBlockJob *s, int64_t offset, int64_t *n, void *buf) {
> + 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);
> + }
> +
> + copy = (ret >= 0 && ret & BDRV_BLOCK_ALLOCATED);
> + trace_commit_one_iteration(s, offset, *n, ret);
> + if (copy) {
> + assert(*n < SIZE_MAX);
Probably a matter of taste, but I'd prefer working with local variable instead of dereferencing the pointer on every line.
Like this:
commit_iteration(..., int64_t bytes, int64_t *done, ...) {
... work with bytes variable ...
out:
*done = bytes;
return 0;
}
anyway:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> +
> + 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;
> + }
> + }
> + /* Publish progress */
> + job_progress_update(&s->common.job, *n);
> +
> + if (copy) {
> + block_job_ratelimit_processed_bytes(&s->common, *n);
> + }
> +
> + return 0;
> +}
> +
> static int coroutine_fn commit_run(Job *job, Error **errp)
> {
> CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
> @@ -158,9 +203,6 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
> buf = blk_blockalign(s->top, COMMIT_BUFFER_SIZE);
>
> for (offset = 0; offset < len; offset += n) {
> - bool copy;
> - bool error_in_source = true;
> -
> /* Note that even when no rate limit is applied we need to yield
> * with no pending I/O here so that bdrv_drain_all() returns.
> */
> @@ -168,42 +210,11 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
> if (job_is_cancelled(&s->common.job)) {
> break;
> }
> - /* 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);
> - }
>
> - copy = (ret >= 0 && ret & BDRV_BLOCK_ALLOCATED);
> - trace_commit_one_iteration(s, offset, n, ret);
> - if (copy) {
> - assert(n < SIZE_MAX);
> -
> - 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;
> - continue;
> - }
> - }
> - /* Publish progress */
> - job_progress_update(&s->common.job, n);
> + ret = commit_iteration(s, offset, &n, buf);
>
> - if (copy) {
> - block_job_ratelimit_processed_bytes(&s->common, n);
> + if (ret < 0) {
> + return ret;
> }
> }
>
--
Best regards,
Vladimir
© 2016 - 2026 Red Hat, Inc.