On 25.07.23 19:40, Vladimir Sementsov-Ogievskiy wrote:
From: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
Actually block job is not completed without this 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.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@yandex-team.ru>
---
block/backup.c | 7 +++++--
block/commit.c | 2 +-
block/mirror.c | 4 ++++
block/stream.c | 7 ++++++-
blockjob.c | 18 ++++++++++++++++++
include/block/blockjob_int.h | 11 +++++++++++
6 files changed, 45 insertions(+), 4 deletions(-)
Yes, that’s a good change.
[...]
diff --git a/block/stream.c b/block/stream.c
index e522bbdec5..f7e8b35e94 100644
--- a/block/stream.c
+++ b/block/stream.c
[...]
@@ -207,6 +207,11 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
}
}
+ ret = block_job_final_target_flush(&s->common, s->target_bs);
+ if (error == 0) {
+ error = ret;
+ }
In all other jobs, this function is invoked only if the job was
successful, but here it’s called unconditionally. I don’t mind one way
or the other, but I think it should be consistent. (Mainly just because
inconsistency makes me wonder whether there’s an undocumented reason for
it.)
+
/* Do not remove the backing file if an error was there but ignored. */
return error;
}
diff --git a/blockjob.c b/blockjob.c
index 25fe8e625d..313e586b0d 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -611,3 +611,21 @@ AioContext *block_job_get_aio_context(BlockJob *job)
GLOBAL_STATE_CODE();
return job->job.aio_context;
}
+
+int coroutine_fn
+block_job_final_target_flush(BlockJob *job, BlockDriverState *target_bs)
+{
+ int ret;
+
Should we mark this as IO_CODE()?
+ WITH_GRAPH_RDLOCK_GUARD() {
+ ret = bdrv_co_flush(target_bs);
+ }
+
+ if (ret < 0 && !block_job_is_internal(job)) {
+ qapi_event_send_block_job_error(job->job.id,
+ IO_OPERATION_TYPE_WRITE,
+ BLOCK_ERROR_ACTION_REPORT);
+ }
Would it make sense to rely on block_job_error_action() instead? If so,
should we use the on-target-error setting?
I have no informed opinion on this, but I think using
block_job_error_action() does come to mind, so if we consciously decide
against it, that’s probably worth a comment, too.
Hanna
+
+ return ret;
+}
