[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V1 1/2] Implement sync modes for drive-backup.
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH V1 1/2] Implement sync modes for drive-backup. |
Date: |
Mon, 01 Jul 2013 14:16:12 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130514 Thunderbird/17.0.6 |
Il 28/06/2013 04:28, Ian Main ha scritto:
> This patch adds sync-modes to the drive-backup interface and
> implements the FULL, NONE and TOP modes of synchronization.
>
> FULL performs as before copying the entire contents of the drive
> while preserving the point-in-time using CoW.
> NONE only copies new writes to the target drive.
> TOP copies changes to the topmost drive image and preserves the
> point-in-time using CoW.
>
> Signed-off-by: Ian Main <address@hidden>
> ---
> block/backup.c | 90
> +++++++++++++++++++++++++++++++----------------
> blockdev.c | 25 ++++++++-----
> include/block/block_int.h | 4 ++-
> qapi-schema.json | 4 +++
> qmp-commands.hx | 1 +
> 5 files changed, 85 insertions(+), 39 deletions(-)
>
> diff --git a/block/backup.c b/block/backup.c
> index 16105d4..9bad85f 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -37,6 +37,7 @@ typedef struct CowRequest {
> typedef struct BackupBlockJob {
> BlockJob common;
> BlockDriverState *target;
> + MirrorSyncMode sync_mode;
> RateLimit limit;
> BlockdevOnError on_source_error;
> BlockdevOnError on_target_error;
> @@ -247,40 +248,68 @@ static void coroutine_fn backup_run(void *opaque)
>
> bdrv_add_before_write_notifier(bs, &before_write);
>
> - for (; start < end; start++) {
> - bool error_is_read;
> -
> - if (block_job_is_cancelled(&job->common)) {
> - break;
> + if (job->sync_mode == MIRROR_SYNC_MODE_NONE) {
> + while (!block_job_is_cancelled(&job->common)) {
> + /* Sleep for 100ms (SLICE_TIME). Our only goal here is to
> + * catch when the job is cancelled. Otherwise we just let
> + * our before_write notify callback service CoW requests. */
> + block_job_sleep_ns(&job->common, rt_clock, SLICE_TIME);
I think there is no need to poll here. You can just do
qemu_coroutine_yield().
> }
> + } else {
> + /* Both FULL and TOP SYNC_MODE's require copying.. */
> + for (; start < end; start++) {
> + bool error_is_read;
>
> - /* we need to yield so that qemu_aio_flush() returns.
> - * (without, VM does not reboot)
> - */
> - if (job->common.speed) {
> - uint64_t delay_ns = ratelimit_calculate_delay(
> - &job->limit, job->sectors_read);
> - job->sectors_read = 0;
> - block_job_sleep_ns(&job->common, rt_clock, delay_ns);
> - } else {
> - block_job_sleep_ns(&job->common, rt_clock, 0);
> - }
> + if (block_job_is_cancelled(&job->common)) {
> + break;
> + }
>
> - if (block_job_is_cancelled(&job->common)) {
> - break;
> - }
> + /* we need to yield so that qemu_aio_flush() returns.
> + * (without, VM does not reboot)
> + */
> + if (job->common.speed) {
> + uint64_t delay_ns = ratelimit_calculate_delay(
> + &job->limit, job->sectors_read);
> + job->sectors_read = 0;
> + block_job_sleep_ns(&job->common, rt_clock, delay_ns);
> + } else {
> + block_job_sleep_ns(&job->common, rt_clock, 0);
> + }
>
> - ret = backup_do_cow(bs, start * BACKUP_SECTORS_PER_CLUSTER,
> - BACKUP_SECTORS_PER_CLUSTER, &error_is_read);
> - if (ret < 0) {
> - /* Depending on error action, fail now or retry cluster */
> - BlockErrorAction action =
> - backup_error_action(job, error_is_read, -ret);
> - if (action == BDRV_ACTION_REPORT) {
> + if (block_job_is_cancelled(&job->common)) {
> break;
> - } else {
> - start--;
> - continue;
> + }
> +
> + if (job->sync_mode == MIRROR_SYNC_MODE_TOP) {
> + int n, alloced;
> +
> + /* Check to see if these blocks are already in the backing
> file. */
> +
> + alloced =
> + bdrv_co_is_allocated_above(bs, bs->backing_hd,
> + start *
> BACKUP_SECTORS_PER_CLUSTER,
> + BACKUP_SECTORS_PER_CLUSTER,
> &n);
You can just use bdrv_co_is_allocated, instead of
bdrv_co_is_allocated-above.
> + /* The above call returns true if the given sector is in the
> + * topmost image. If that is the case then we must copy it
> as
> + * it has been modified from the original backing file.
> + * Otherwise we skip it. */
> + if (alloced == 0) {
> + continue;
> + }
> + }
> + /* FULL sync mode we copy the whole drive. */
> + ret = backup_do_cow(bs, start * BACKUP_SECTORS_PER_CLUSTER,
> + BACKUP_SECTORS_PER_CLUSTER, &error_is_read);
> + if (ret < 0) {
> + /* Depending on error action, fail now or retry cluster */
> + BlockErrorAction action =
> + backup_error_action(job, error_is_read, -ret);
> + if (action == BDRV_ACTION_REPORT) {
> + break;
> + } else {
> + start--;
> + continue;
> + }
> }
> }
> }
> @@ -300,7 +329,7 @@ static void coroutine_fn backup_run(void *opaque)
> }
>
> void backup_start(BlockDriverState *bs, BlockDriverState *target,
> - int64_t speed,
> + int64_t speed, MirrorSyncMode sync_mode,
> BlockdevOnError on_source_error,
> BlockdevOnError on_target_error,
> BlockDriverCompletionFunc *cb, void *opaque,
> @@ -335,6 +364,7 @@ void backup_start(BlockDriverState *bs, BlockDriverState
> *target,
> job->on_source_error = on_source_error;
> job->on_target_error = on_target_error;
> job->target = target;
> + job->sync_mode = sync_mode;
> job->common.len = len;
> job->common.co = qemu_coroutine_create(backup_run);
> qemu_coroutine_enter(job->common.co, job);
> diff --git a/blockdev.c b/blockdev.c
> index c5abd65..000dea6 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1430,17 +1430,13 @@ void qmp_drive_backup(const char *device, const char
> *target,
> Error **errp)
> {
> BlockDriverState *bs;
> - BlockDriverState *target_bs;
> + BlockDriverState *target_bs, *source;
> BlockDriver *drv = NULL;
> Error *local_err = NULL;
> int flags;
> int64_t size;
> int ret;
>
> - if (sync != MIRROR_SYNC_MODE_FULL) {
> - error_setg(errp, "only sync mode 'full' is currently supported");
> - return;
> - }
> if (!has_speed) {
> speed = 0;
> }
> @@ -1483,6 +1479,13 @@ void qmp_drive_backup(const char *device, const char
> *target,
>
> flags = bs->open_flags | BDRV_O_RDWR;
>
> + /* See if we have a backing HD we can use to create our new image
> + * on top of. */
> + source = bs->backing_hd;
Should the source be "bs" for MIRROR_SYNC_MODE_NONE? Also in this case
you may want to default the format to "qcow2" instead of bs's format.
Paolo
> + if (!source && sync == MIRROR_SYNC_MODE_TOP) {
> + sync = MIRROR_SYNC_MODE_FULL;
> + }
> +
> size = bdrv_getlength(bs);
> if (size < 0) {
> error_setg_errno(errp, -size, "bdrv_getlength failed");
> @@ -1491,8 +1494,14 @@ void qmp_drive_backup(const char *device, const char
> *target,
>
> if (mode != NEW_IMAGE_MODE_EXISTING) {
> assert(format && drv);
> - bdrv_img_create(target, format,
> - NULL, NULL, NULL, size, flags, &local_err, false);
> + if (sync == MIRROR_SYNC_MODE_TOP) {
> + bdrv_img_create(target, format, source->filename,
> + source->drv->format_name, NULL,
> + size, flags, &local_err, false);
> + } else {
> + bdrv_img_create(target, format, NULL, NULL, NULL,
> + size, flags, &local_err, false);
> + }
> }
>
> if (error_is_set(&local_err)) {
> @@ -1508,7 +1517,7 @@ void qmp_drive_backup(const char *device, const char
> *target,
> return;
> }
>
> - backup_start(bs, target_bs, speed, on_source_error, on_target_error,
> + backup_start(bs, target_bs, speed, sync, on_source_error,
> on_target_error,
> block_job_cb, bs, &local_err);
> if (local_err != NULL) {
> bdrv_delete(target_bs);
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index c6ac871..e45f2a0 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -404,6 +404,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState
> *target,
> * @bs: Block device to operate on.
> * @target: Block device to write to.
> * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
> + * @sync_mode: What parts of the disk image should be copied to the
> destination.
> * @on_source_error: The action to take upon error reading from the source.
> * @on_target_error: The action to take upon error writing to the target.
> * @cb: Completion function for the job.
> @@ -413,7 +414,8 @@ void mirror_start(BlockDriverState *bs, BlockDriverState
> *target,
> * until the job is cancelled or manually completed.
> */
> void backup_start(BlockDriverState *bs, BlockDriverState *target,
> - int64_t speed, BlockdevOnError on_source_error,
> + int64_t speed, MirrorSyncMode sync_mode,
> + BlockdevOnError on_source_error,
> BlockdevOnError on_target_error,
> BlockDriverCompletionFunc *cb, void *opaque,
> Error **errp);
> diff --git a/qapi-schema.json b/qapi-schema.json
> index cdd2d6b..b3f6c2a 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1807,6 +1807,10 @@
> # @format: #optional the format of the new destination, default is to
> # probe if @mode is 'existing', else the format of the source
> #
> +# @sync: what parts of the disk image should be copied to the destination
> +# (all the disk, only the sectors allocated in the topmost image, or
> +# only new I/O).
> +#
> # @mode: #optional whether and how QEMU should create a new image, default is
> # 'absolute-paths'.
> #
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index e075df4..f3f6b3d 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -957,6 +957,7 @@ Arguments:
>
> Example:
> -> { "execute": "drive-backup", "arguments": { "device": "drive0",
> + "sync": "full",
> "target": "backup.img" } }
> <- { "return": {} }
> EQMP
>
- Re: [Qemu-devel] [PATCH V1 1/2] Implement sync modes for drive-backup.,
Paolo Bonzini <=