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 <vsementsov@virtuozzo.com>
---
block/backup.c | 187 ++++++++++++++++++++++++++++++++-----------------
1 file changed, 121 insertions(+), 66 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 449b763ce4..0d1cd64eab 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -44,41 +44,17 @@ typedef struct BackupBlockJob {
BlockdevOnError on_source_error;
BlockdevOnError on_target_error;
uint64_t len;
- uint64_t bytes_read;
int64_t cluster_size;
BackupPerf perf;
BlockCopyState *bcs;
+
+ bool wait;
+ BlockCopyCallState *bg_bcs_call;
} BackupBlockJob;
static const BlockJobDriver backup_job_driver;
-static void backup_progress_bytes_callback(int64_t bytes, void *opaque)
-{
- BackupBlockJob *s = opaque;
-
- s->bytes_read += bytes;
-}
-
-static int coroutine_fn backup_do_cow(BackupBlockJob *job,
- int64_t offset, uint64_t bytes,
- bool *error_is_read)
-{
- int ret = 0;
- int64_t start, end; /* bytes */
-
- start = QEMU_ALIGN_DOWN(offset, job->cluster_size);
- end = QEMU_ALIGN_UP(bytes + offset, job->cluster_size);
-
- trace_backup_do_cow_enter(job, start, offset, bytes);
-
- ret = block_copy(job->bcs, start, end - start, true, error_is_read);
-
- trace_backup_do_cow_return(job, offset, bytes, ret);
-
- return ret;
-}
-
static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret)
{
BdrvDirtyBitmap *bm;
@@ -158,53 +134,97 @@ static BlockErrorAction backup_error_action(BackupBlockJob *job,
}
}
-static bool coroutine_fn yield_and_check(BackupBlockJob *job)
+static void coroutine_fn backup_block_copy_callback(void *opaque)
{
- uint64_t delay_ns;
-
- if (job_is_cancelled(&job->common.job)) {
- return true;
- }
-
- /*
- * We need to yield even for delay_ns = 0 so that bdrv_drain_all() can
- * return. Without a yield, the VM would not reboot.
- */
- delay_ns = block_job_ratelimit_get_delay(&job->common, job->bytes_read);
- job->bytes_read = 0;
- job_sleep_ns(&job->common.job, delay_ns);
+ BackupBlockJob *s = opaque;
- if (job_is_cancelled(&job->common.job)) {
- return true;
+ if (s->wait) {
+ s->wait = false;
+ aio_co_wake(s->common.job.co);
+ } else {
+ job_enter(&s->common.job);
}
-
- return false;
}
static int coroutine_fn backup_loop(BackupBlockJob *job)
{
- bool error_is_read;
- int64_t offset;
- BdrvDirtyBitmapIter *bdbi;
+ BlockCopyCallState *s = NULL;
int ret = 0;
+ bool error_is_read;
+ BlockErrorAction act;
+
+ while (true) { /* retry loop */
+ job->bg_bcs_call = s = block_copy_async(job->bcs, 0,
+ QEMU_ALIGN_UP(job->len, job->cluster_size),
+ job->perf.max_workers, job->perf.max_chunk,
+ backup_block_copy_callback, job);
+
+ while (!block_copy_call_finished(s) &&
+ !job_is_cancelled(&job->common.job))
+ {
+ job_yield(&job->common.job);
+ }
- bdbi = bdrv_dirty_iter_new(block_copy_dirty_bitmap(job->bcs));
- while ((offset = bdrv_dirty_iter_next(bdbi)) != -1) {
- do {
- if (yield_and_check(job)) {
- goto out;
- }
- ret = backup_do_cow(job, offset, job->cluster_size, &error_is_read);
- if (ret < 0 && backup_error_action(job, error_is_read, -ret) ==
- BLOCK_ERROR_ACTION_REPORT)
- {
- goto out;
- }
- } while (ret < 0);
+ if (!block_copy_call_finished(s) &&
+ job_is_cancelled(&job->common.job))
+ {
+ /*
+ * 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;
+ }
+
+ /* 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);
+ job->bg_bcs_call = NULL;
return ret;
}
@@ -245,7 +265,13 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
int64_t count;
for (offset = 0; offset < s->len; ) {
- if (yield_and_check(s)) {
+ if (job_is_cancelled(job)) {
+ return -ECANCELED;
+ }
+
+ job_pause_point(job);
+
+ if (job_is_cancelled(job)) {
return -ECANCELED;
}
@@ -278,6 +304,33 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
return 0;
}
+static void coroutine_fn backup_pause(Job *job)
+{
+ BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
+
+ if (s->bg_bcs_call && !block_copy_call_finished(s->bg_bcs_call)) {
+ block_copy_call_cancel(s->bg_bcs_call);
+ s->wait = true;
+ qemu_coroutine_yield();
+ }
+}
+
+static void coroutine_fn backup_set_speed(BlockJob *job, int64_t speed)
+{
+ BackupBlockJob *s = container_of(job, BackupBlockJob, common);
+
+ /*
+ * block_job_set_speed() is called first from block_job_create(), when we
+ * don't yet have s->bcs.
+ */
+ if (s->bcs) {
+ block_copy_set_speed(s->bcs, speed);
+ if (s->bg_bcs_call) {
+ block_copy_kick(s->bg_bcs_call);
+ }
+ }
+}
+
static const BlockJobDriver backup_job_driver = {
.job_driver = {
.instance_size = sizeof(BackupBlockJob),
@@ -288,7 +341,9 @@ static const BlockJobDriver backup_job_driver = {
.commit = backup_commit,
.abort = backup_abort,
.clean = backup_clean,
- }
+ .pause = backup_pause,
+ },
+ .set_speed = backup_set_speed,
};
static int64_t backup_calculate_cluster_size(BlockDriverState *target,
@@ -485,8 +540,8 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
job->len = len;
job->perf = *perf;
- block_copy_set_progress_callback(bcs, backup_progress_bytes_callback, job);
block_copy_set_progress_meter(bcs, &job->common.job.progress);
+ block_copy_set_speed(bcs, speed);
/* Required permissions are already taken by backup-top target */
block_job_add_bdrv(&job->common, "target", target, 0, BLK_PERM_ALL,
--
2.21.3
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 <vsementsov@virtuozzo.com>
> ---
> block/backup.c | 187 ++++++++++++++++++++++++++++++++-----------------
> 1 file changed, 121 insertions(+), 66 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
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)
[...]
> 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.)
> + /*
> + * 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*
> + }
> +
> + /* 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.)
Max
> + job->bg_bcs_call = NULL;
> return ret;
> }
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 <vsementsov@virtuozzo.com>
>> ---
>> block/backup.c | 187 ++++++++++++++++++++++++++++++++-----------------
>> 1 file changed, 121 insertions(+), 66 deletions(-)
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
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
© 2016 - 2025 Red Hat, Inc.