Re: [Qemu-devel] [PATCH V1 1/2] Implement sync modes for drive-backup.

[Paolo Bonzini <pbonzini@redhat.com>]

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 <imain@redhat.com>
> ---
>  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
>