On 18.07.24 22:22, Kevin Wolf wrote:
Am 26.06.2024 um 16:50 hat Vladimir Sementsov-Ogievskiy geschrieben:
Actually block job is not completed without the final flush. It's
rather unexpected to have broken target when job was successfully
completed long ago and now we fail to flush or process just
crashed/killed.
Mirror job already has mirror_flush() for this. So, it's OK.
Do this for commit job too.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@yandex-team.ru>
---
block/commit.c | 41 +++++++++++++++++++++++++++--------------
1 file changed, 27 insertions(+), 14 deletions(-)
diff --git a/block/commit.c b/block/commit.c
index 7c3fdcb0ca..81971692a2 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -134,6 +134,7 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
int64_t n = 0; /* bytes */
QEMU_AUTO_VFREE void *buf = NULL;
int64_t len, base_len;
+ bool need_final_flush = true;
len = blk_co_getlength(s->top);
if (len < 0) {
@@ -155,8 +156,8 @@ 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;
+ for (offset = 0; offset < len || need_final_flush; offset += n) {
In general, the control flow would be nicer to read if the final flush
weren't integrated into the loop, but just added after it.
But I assume this is pretty much required for pausing the job during
error handling in the final flush if you don't want to duplicate a lot
of the logic into a second loop?
Right, that's the reason.
+ bool copy = false;
bool error_in_source = true;
/* Note that even when no rate limit is applied we need to yield
@@ -166,22 +167,34 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
if (job_is_cancelled(&s->common.job)) {
break;
}
- /* Copy if allocated above the base */
- ret = blk_co_is_allocated_above(s->top, s->base_overlay, true,
- offset, COMMIT_BUFFER_SIZE, &n);
- copy = (ret > 0);
- 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 (offset < len) {
+ /* Copy if allocated above the base */
+ ret = blk_co_is_allocated_above(s->top, s->base_overlay, true,
+ offset, COMMIT_BUFFER_SIZE, &n);
+ copy = (ret > 0);
+ 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;
+ }
}
}
+ } else {
+ assert(need_final_flush);
+ ret = blk_co_flush(s->base);
+ if (ret < 0) {
+ error_in_source = false;
+ } else {
+ need_final_flush = false;
+ }
Should we set n = 0 in this block to avoid counting the last chunk twice
for the progress?
Oops, right. Will fix, thanks
}
+
if (ret < 0) {
BlockErrorAction action =
block_job_error_action(&s->common, s->on_error,
Kevin
--
Best regards,
Vladimir
