12.01.2021 16:23, Max Reitz wrote:
On 26.10.20 18:18, Vladimir Sementsov-Ogievskiy wrote:This brings async request handling and block-status driven chunk sizes to backup out of the box, which improves backup performance.Signed-off-by: Vladimir Sementsov-Ogievskiy <[email protected]> --- block/backup.c | 187 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 121 insertions(+), 66 deletions(-)Reviewed-by: Max Reitz <[email protected]>
Thanks a lot! My way to this point was started in far 2016 summer.. And final concept is far-far away from my first attempt:)
Irrelevant notes below.diff --git a/block/backup.c b/block/backup.c index 449b763ce4..0d1cd64eab 100644 --- a/block/backup.c +++ b/block/backup.c(Just something I noted when looking for remaining instances of “ratelimit”: backup.c includes ratelimit.h, which I don’t think it needs to do any longer)
agree
[...]static int coroutine_fn backup_loop(BackupBlockJob *job) {[...]+ if (!block_copy_call_finished(s) && + job_is_cancelled(&job->common.job)) + {Just for the sake of clarity: If !block_copy_call_finished(), then job_is_cancelled() must be true, right? (I.e., could be an assertion instead of the second part of the condition. I don’t mind how it is, but then again, it did made me ask.)
Hmm, keeping in mind previous loop "while (!block_copy_call_finished(s) &&
!job_is_cancelled(&job->common.job)) {}", yes, you are right.
+ /* + * Note that we can't use job_yield() here, as it doesn't work for + * cancelled job. + */ + block_copy_call_cancel(s); + job->wait = true; + qemu_coroutine_yield(); + assert(block_copy_call_finished(s)); + ret = 0; + goto out; + } + + if (job_is_cancelled(&job->common.job) || + block_copy_call_succeeded(s)) + { + ret = 0; + goto out; + } + + if (block_copy_call_cancelled(s)) { + /* + * Job is not cancelled but only block-copy call. This is possible + * after job pause. Now the pause is finished, start new block-copy + * iteration. + */ + block_copy_call_free(s); + continue;If one were to avoid all continues, we could alternatively put the whole error handling into a block_copy_call_failed() condition, and have the block_copy_call_free() common for both the cancel and the fail case. *shrug*
Hmm. Same emotion, *shrug*:). I like my comment, describing exactly block_copy_call_cancelled() in corresponding if.. Being combined with failure case it would be less demonstrative. But I'm OK with your suggestion too.
+ } + + /* The only remaining case is failed block-copy call. */ + assert(block_copy_call_failed(s)); + + ret = block_copy_call_status(s, &error_is_read); + act = backup_error_action(job, error_is_read, -ret); + switch (act) { + case BLOCK_ERROR_ACTION_REPORT: + goto out; + case BLOCK_ERROR_ACTION_STOP: + /* + * Go to pause prior to starting new block-copy call on the next + * iteration. + */ + job_pause_point(&job->common.job); + break; + case BLOCK_ERROR_ACTION_IGNORE: + /* Proceed to new block-copy call to retry. */ + break; + default: + abort(); + } + + block_copy_call_free(s); } - out: - bdrv_dirty_iter_free(bdbi); +out: + block_copy_call_free(s);Reads a bit weird after the block_copy_call_free(s) at the end of the while (true) loop, but is entirely correct, of course. (And I can’t offer any better alternative.)
Yes, looks weird.. Probably we'll invent some good refactoring later. If the function doesn't fit into one screen, there must exist some good refactoring) -- Best regards, Vladimir
