Patchwork [V6,1/3] Implement sync modes for drive-backup.

login
register
mail settings
Submitter Ian Main
Date July 22, 2013, 10:09 p.m.
Message ID <1374530960-22031-2-git-send-email-imain@redhat.com>
Download mbox | patch
Permalink /patch/260825/
State New
Headers show

Comments

Ian Main - July 22, 2013, 10:09 p.m.
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.

For sync mode TOP are creating a new target image using the same backing
file as the original disk image.  Then any new data that has been laid
on top of it since creation is copied in the main backup_run() loop.
There is an extra check in the 'TOP' case so that we don't bother to copy
all the data of the backing file as it already exists in the target.
This is where the bdrv_co_is_allocated() is used to determine if the
data exists in the topmost layer or below.

Also any new data being written is intercepted via the write_notifier
hook which ends up calling backup_do_cow() to copy old data out before
it gets overwritten.

For mode 'NONE' we create the new target image and only copy in the
original data from the disk image starting from the time the call was
made.  This preserves the point in time data by only copying the parts
that are *going to change* to the target image.  This way we can
reconstruct the final image by checking to see if the given block exists
in the new target image first, and if it does not, you can get it from
the original image.  This is basically an optimization allowing you to
do point-in-time snapshots with low overhead vs the 'FULL' version.

Since there is no old data to copy out the loop in backup_run() for the
NONE case just calls qemu_coroutine_yield() which only wakes up after
an event (usually cancel in this case).  The rest is handled by the
before_write notifier which again calls backup_do_cow() to write out
the old data so it can be preserved.

Signed-off-by: Ian Main <imain@redhat.com>
---
 block/backup.c            | 91 +++++++++++++++++++++++++++++++----------------
 blockdev.c                | 36 ++++++++++++-------
 include/block/block_int.h |  4 ++-
 qapi-schema.json          |  4 +--
 qmp-commands.hx           |  2 ++
 5 files changed, 92 insertions(+), 45 deletions(-)
Kevin Wolf - July 24, 2013, 10:55 a.m.
Am 23.07.2013 um 00:09 hat Ian Main geschrieben:
> 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.
> 
> For sync mode TOP are creating a new target image using the same backing
> file as the original disk image.  Then any new data that has been laid
> on top of it since creation is copied in the main backup_run() loop.
> There is an extra check in the 'TOP' case so that we don't bother to copy
> all the data of the backing file as it already exists in the target.
> This is where the bdrv_co_is_allocated() is used to determine if the
> data exists in the topmost layer or below.
> 
> Also any new data being written is intercepted via the write_notifier
> hook which ends up calling backup_do_cow() to copy old data out before
> it gets overwritten.
> 
> For mode 'NONE' we create the new target image and only copy in the
> original data from the disk image starting from the time the call was
> made.  This preserves the point in time data by only copying the parts
> that are *going to change* to the target image.  This way we can
> reconstruct the final image by checking to see if the given block exists
> in the new target image first, and if it does not, you can get it from
> the original image.  This is basically an optimization allowing you to
> do point-in-time snapshots with low overhead vs the 'FULL' version.
> 
> Since there is no old data to copy out the loop in backup_run() for the
> NONE case just calls qemu_coroutine_yield() which only wakes up after
> an event (usually cancel in this case).  The rest is handled by the
> before_write notifier which again calls backup_do_cow() to write out
> the old data so it can be preserved.
> 
> Signed-off-by: Ian Main <imain@redhat.com>
> ---
>  block/backup.c            | 91 +++++++++++++++++++++++++++++++----------------
>  blockdev.c                | 36 ++++++++++++-------
>  include/block/block_int.h |  4 ++-
>  qapi-schema.json          |  4 +--
>  qmp-commands.hx           |  2 ++
>  5 files changed, 92 insertions(+), 45 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index 16105d4..68abd23 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,69 @@ 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)) {
> +            /* Yield until the job is cancelled.  We just let our before_write
> +             * notify callback service CoW requests. */
> +            job->common.busy = false;
> +            qemu_coroutine_yield();
> +            job->common.busy = true;
>          }
> +    } 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(bs, start * BACKUP_SECTORS_PER_CLUSTER,
> +                                         BACKUP_SECTORS_PER_CLUSTER, &n);
> +                /* 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;

At first I was going to suggest that you use n for optimising the case
where you can skip multiple clusters.

But I think we have a much more serious problem: We're working at
BACKUP_SECTORS_PER_CLUSTER granularity, which currently consists of 128
512-byte sectors, each of which can have a different allocation status.

If the first one is unallocated, you skip the other 127 sectors as well,
without ever checking if they need to be copied. In the same way, the
code below can copy sectors which shouldn't be copied in fact. (When
fixing this, be aware that backup_do_cow() does round its parameters
once more.)

> +                }
> +            }
> +            /* 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 +330,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 +365,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..482029e 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;
>      }
> @@ -1465,10 +1461,13 @@ void qmp_drive_backup(const char *device, const char *target,
>          return;
>      }
>  
> -    if (!has_format) {
> -        format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : bs->drv->format_name;
> -    }
> -    if (format) {
> +    if (mode == NEW_IMAGE_MODE_EXISTING) {
> +        format = bs->drv->format_name;
> +    } else {
> +        if (!has_format) {
> +            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
> +            return;
> +        }
>          drv = bdrv_find_format(format);
>          if (!drv) {
>              error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);

Is this part really related to the new sync modes or should it be a
separate patch?

Unconditionally overriding format for NEW_IMAGE_MODE_EXISTING is
definitely wrong. It's the user's choice which COW format to use for the
backup image. There's no reason why it has to be the same format as the
image that is being backed up.

Before, bs->drv->format_name was a default for the case where a new
image had to be created and no format was given; and the format of
existing images could be probed. This is still what makes most sense to
me. What's even the goal with this change?

> @@ -1483,6 +1482,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;
> +    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 +1497,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 +1520,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 7b9fef1..8a9bcc5 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1816,8 +1816,8 @@
>  #          is a device, the existing file/device will be used as the new
>  #          destination.  If it does not exist, a new file will be created.
>  #
> -# @format: #optional the format of the new destination, default is to
> -#          probe if @mode is 'existing', else the format of the source
> +# @format: #optional If @mode is 'existing' the format will be probed.
> +#          If the format requires a new file then format is required.

This modifies drive-mirror rather than drive-backup.

Also, the last sentence doesn't make sense. Is s/format/mode/ what you
meant?

Kevin
Ian Main - July 24, 2013, 5:57 p.m.
On Wed, Jul 24, 2013 at 12:55:43PM +0200, Kevin Wolf wrote:
> Am 23.07.2013 um 00:09 hat Ian Main geschrieben:
> > 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.
> > 
> > For sync mode TOP are creating a new target image using the same backing
> > file as the original disk image.  Then any new data that has been laid
> > on top of it since creation is copied in the main backup_run() loop.
> > There is an extra check in the 'TOP' case so that we don't bother to copy
> > all the data of the backing file as it already exists in the target.
> > This is where the bdrv_co_is_allocated() is used to determine if the
> > data exists in the topmost layer or below.
> > 
> > Also any new data being written is intercepted via the write_notifier
> > hook which ends up calling backup_do_cow() to copy old data out before
> > it gets overwritten.
> > 
> > For mode 'NONE' we create the new target image and only copy in the
> > original data from the disk image starting from the time the call was
> > made.  This preserves the point in time data by only copying the parts
> > that are *going to change* to the target image.  This way we can
> > reconstruct the final image by checking to see if the given block exists
> > in the new target image first, and if it does not, you can get it from
> > the original image.  This is basically an optimization allowing you to
> > do point-in-time snapshots with low overhead vs the 'FULL' version.
> > 
> > Since there is no old data to copy out the loop in backup_run() for the
> > NONE case just calls qemu_coroutine_yield() which only wakes up after
> > an event (usually cancel in this case).  The rest is handled by the
> > before_write notifier which again calls backup_do_cow() to write out
> > the old data so it can be preserved.
> > 
> > Signed-off-by: Ian Main <imain@redhat.com>
> > ---
> >  block/backup.c            | 91 +++++++++++++++++++++++++++++++----------------
> >  blockdev.c                | 36 ++++++++++++-------
> >  include/block/block_int.h |  4 ++-
> >  qapi-schema.json          |  4 +--
> >  qmp-commands.hx           |  2 ++
> >  5 files changed, 92 insertions(+), 45 deletions(-)
> > 
> > diff --git a/block/backup.c b/block/backup.c
> > index 16105d4..68abd23 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,69 @@ 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)) {
> > +            /* Yield until the job is cancelled.  We just let our before_write
> > +             * notify callback service CoW requests. */
> > +            job->common.busy = false;
> > +            qemu_coroutine_yield();
> > +            job->common.busy = true;
> >          }
> > +    } 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(bs, start * BACKUP_SECTORS_PER_CLUSTER,
> > +                                         BACKUP_SECTORS_PER_CLUSTER, &n);
> > +                /* 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;
> 
> At first I was going to suggest that you use n for optimising the case
> where you can skip multiple clusters.
> 
> But I think we have a much more serious problem: We're working at
> BACKUP_SECTORS_PER_CLUSTER granularity, which currently consists of 128
> 512-byte sectors, each of which can have a different allocation status.
> 
> If the first one is unallocated, you skip the other 127 sectors as well,
> without ever checking if they need to be copied. In the same way, the
> code below can copy sectors which shouldn't be copied in fact. (When
> fixing this, be aware that backup_do_cow() does round its parameters
> once more.)

Doh!  For some reason I thought it checked them all but I see now that
it does not.  I will change it to go sector by sector.

> > +                }
> > +            }
> > +            /* 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 +330,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 +365,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..482029e 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;
> >      }
> > @@ -1465,10 +1461,13 @@ void qmp_drive_backup(const char *device, const char *target,
> >          return;
> >      }
> >  
> > -    if (!has_format) {
> > -        format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : bs->drv->format_name;
> > -    }
> > -    if (format) {
> > +    if (mode == NEW_IMAGE_MODE_EXISTING) {
> > +        format = bs->drv->format_name;
> > +    } else {
> > +        if (!has_format) {
> > +            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
> > +            return;
> > +        }
> >          drv = bdrv_find_format(format);
> >          if (!drv) {
> >              error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
> 
> Is this part really related to the new sync modes or should it be a
> separate patch?
> 
> Unconditionally overriding format for NEW_IMAGE_MODE_EXISTING is
> definitely wrong. It's the user's choice which COW format to use for the
> backup image. There's no reason why it has to be the same format as the
> image that is being backed up.

Yes, good catch.
 
> Before, bs->drv->format_name was a default for the case where a new
> image had to be created and no format was given; and the format of
> existing images could be probed. This is still what makes most sense to
> me. What's even the goal with this change?

It was suggested by a reviewer that we make the format a required
argument to the API.  This mostly started because sync mode NONE
requires a format which supports backing files in order to read from the
original as per the crazy backing_hd trick.  

I personally don't think it matters too much about the default as it would
likely be libvirt using the API, or someone else who is going to have to
know what they are doing.


> > @@ -1483,6 +1482,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;
> > +    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 +1497,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 +1520,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 7b9fef1..8a9bcc5 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -1816,8 +1816,8 @@
> >  #          is a device, the existing file/device will be used as the new
> >  #          destination.  If it does not exist, a new file will be created.
> >  #
> > -# @format: #optional the format of the new destination, default is to
> > -#          probe if @mode is 'existing', else the format of the source
> > +# @format: #optional If @mode is 'existing' the format will be probed.
> > +#          If the format requires a new file then format is required.
> 
> This modifies drive-mirror rather than drive-backup.

I've been bit by this a few times now..
 
> Also, the last sentence doesn't make sense. Is s/format/mode/ what you
> meant?

Yes that is right.  It sounds like I should just revert that part of the
patch however.

	Ian
Eric Blake - July 24, 2013, 8:32 p.m.
On 07/24/2013 04:55 AM, Kevin Wolf wrote:

> Unconditionally overriding format for NEW_IMAGE_MODE_EXISTING is
> definitely wrong. It's the user's choice which COW format to use for the
> backup image. There's no reason why it has to be the same format as the
> image that is being backed up.
> 
> Before, bs->drv->format_name was a default for the case where a new
> image had to be created and no format was given; and the format of
> existing images could be probed. This is still what makes most sense to
> me. What's even the goal with this change?

Furthermore, I'm proposing that for 1.6, we should make the format
argument mandatory for drive-backup.  We made it optional for
drive-mirror, to allow for probing, but there have been CVEs in the past
due to probing of a raw file gone wrong.  We can always relax a
mandatory argument into an optional one in 1.7, if we decide that
probing can be done safely, but we can never turn an optional argument
into a mandatory one once the initial release bakes in the option.  It
would make the code a lot simpler to just have a mandatory format
argument, instead of having to bake in and document hueristics on which
format is picked when the caller doesn't provide one.
Ian Main - July 24, 2013, 10:01 p.m.
On Wed, Jul 24, 2013 at 02:32:53PM -0600, Eric Blake wrote:
> On 07/24/2013 04:55 AM, Kevin Wolf wrote:
> 
> > Unconditionally overriding format for NEW_IMAGE_MODE_EXISTING is
> > definitely wrong. It's the user's choice which COW format to use for the
> > backup image. There's no reason why it has to be the same format as the
> > image that is being backed up.
> > 
> > Before, bs->drv->format_name was a default for the case where a new
> > image had to be created and no format was given; and the format of
> > existing images could be probed. This is still what makes most sense to
> > me. What's even the goal with this change?

Actually I think that code is wrong.  If we are using
NEW_IMAGE_MODE_EXISTING then format doesn't get used.  We just end up
using bdrv_open() below to open the existing image.  Format should not
be specified for an existing image.
 
> Furthermore, I'm proposing that for 1.6, we should make the format
> argument mandatory for drive-backup.  We made it optional for
> drive-mirror, to allow for probing, but there have been CVEs in the past
> due to probing of a raw file gone wrong.  We can always relax a
> mandatory argument into an optional one in 1.7, if we decide that
> probing can be done safely, but we can never turn an optional argument
> into a mandatory one once the initial release bakes in the option.  It
> would make the code a lot simpler to just have a mandatory format
> argument, instead of having to bake in and document hueristics on which
> format is picked when the caller doesn't provide one.

So I made format mandatory in the last patch but only for
NEW_IMAGE_MODE_ABSOLUTE_PATHS.  It actually doesn't make sense to
specify the format of an existing image so I left it optional as an
argument, but it will throw an error if it's not specified for the case
where we create a new image.

That make sense?

	Ian
Kevin Wolf - July 25, 2013, 11:23 a.m.
Am 25.07.2013 um 00:01 hat Ian Main geschrieben:
> On Wed, Jul 24, 2013 at 02:32:53PM -0600, Eric Blake wrote:
> > On 07/24/2013 04:55 AM, Kevin Wolf wrote:
> > 
> > > Unconditionally overriding format for NEW_IMAGE_MODE_EXISTING is
> > > definitely wrong. It's the user's choice which COW format to use for the
> > > backup image. There's no reason why it has to be the same format as the
> > > image that is being backed up.
> > > 
> > > Before, bs->drv->format_name was a default for the case where a new
> > > image had to be created and no format was given; and the format of
> > > existing images could be probed. This is still what makes most sense to
> > > me. What's even the goal with this change?
> 
> Actually I think that code is wrong.  If we are using
> NEW_IMAGE_MODE_EXISTING then format doesn't get used.  We just end up
> using bdrv_open() below to open the existing image.  Format should not
> be specified for an existing image.

That bdrv_open() gets drv passed, which is the block driver specified by
format. If you pass NULL instead, bdrv_open() tries to probe the format,
which is unreliable (raw is indistinguishable from other formats) and
as Eric notes has led to security problems in the past.

> > Furthermore, I'm proposing that for 1.6, we should make the format
> > argument mandatory for drive-backup.  We made it optional for
> > drive-mirror, to allow for probing, but there have been CVEs in the past
> > due to probing of a raw file gone wrong.  We can always relax a
> > mandatory argument into an optional one in 1.7, if we decide that
> > probing can be done safely, but we can never turn an optional argument
> > into a mandatory one once the initial release bakes in the option.  It
> > would make the code a lot simpler to just have a mandatory format
> > argument, instead of having to bake in and document hueristics on which
> > format is picked when the caller doesn't provide one.
> 
> So I made format mandatory in the last patch but only for
> NEW_IMAGE_MODE_ABSOLUTE_PATHS.  It actually doesn't make sense to
> specify the format of an existing image so I left it optional as an
> argument, but it will throw an error if it's not specified for the case
> where we create a new image.
> 
> That make sense?

No, see above, it's important as which format an existing image is
opened.

Kevin
Paolo Bonzini - July 25, 2013, 2:36 p.m.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Il 24/07/2013 22:32, Eric Blake ha scritto:
> On 07/24/2013 04:55 AM, Kevin Wolf wrote:
> 
>>> Unconditionally overriding format for NEW_IMAGE_MODE_EXISTING
>>> is definitely wrong. It's the user's choice which COW format to
>>> use for the backup image. There's no reason why it has to be
>>> the same format as the image that is being backed up.
>>> 
>>> Before, bs->drv->format_name was a default for the case where a
>>> new image had to be created and no format was given; and the
>>> format of existing images could be probed. This is still what
>>> makes most sense to me. What's even the goal with this change?
> Furthermore, I'm proposing that for 1.6, we should make the format 
> argument mandatory for drive-backup.  We made it optional for 
> drive-mirror, to allow for probing, but there have been CVEs in the
> past due to probing of a raw file gone wrong.  We can always relax
> a mandatory argument into an optional one in 1.7, if we decide
> that probing can be done safely, but we can never turn an optional
> argument into a mandatory one once the initial release bakes in the
> option.  It would make the code a lot simpler to just have a
> mandatory format argument, instead of having to bake in and
> document hueristics on which format is picked when the caller
> doesn't provide one.

Probing is unsafe by definition, on the other hand we should allow it
for consistency with the rest of the API.

Making the format mandatory for mode != 'existing' is fine, though.
We can relax it later.

For mode = 'existing' we should allow both probing, and using an
explicit format.

Paolo
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJR8TgEAAoJEBvWZb6bTYbyYcoP/A5whcr0fvd2SBfx71b2xQpZ
/4Ug8c0jjraSSt+IoLMvnwD6EYkHRuXt3qDg1q2R9sIsrjQPaQjZ3PFv2ZFrnj5+
Yrxjzpe6mKQboOWeVEeUBZc1/hvzPrUnsgFf59QnhVjz5piE9xW/c5mRd4/6ij2J
3ngnv1VTB/yoNDbLoUpbMKr4EOMZKTsR0HR96QzEYRMZafL52hihnygyNXtOV84B
sfBrGC68Q9S+3Fj9Yb0qhRWZnEP8xQP8KhA5uO6jlgowUqal2JjIUDFh7/dQn0Mt
b1YlDT0a9KRfZnKc7Q4V37vALkd750kcMgoR3IxcHuuHomKf08UG5tLmYEDoMd/G
+FQol5QL/fnBjOD2uf+oN43DeYXlv3yL4iVChPhlyxBVI9aDZseli90tlJ0sSRtS
4+rEdXhtzhEXavpJMu9Vijfkzi3wYwNWdRViEoBo6ifuh9ZsIsmIMmHaWabn2J2i
rEmpB/VAFhK3XNR6OXegyG0PrYYMqWU92W7INJx0fe91lslX8xzShElL4MHRaRmU
5OiyiVKFjC5XUCTQAwwHaKTxWJ/U8rCQj8Axia5dh6SHunZMmaDLAgpKBK229A1i
X+tuv4PI8vyeWJM3uhIDVSAswqUS41sVmnDYimCzvOj8tI/gudktygS0r1nkTgBW
ePDuM7nPNHm3ifFuaPn9
=fY6P
-----END PGP SIGNATURE-----
Eric Blake - July 25, 2013, 4:57 p.m.
On 07/25/2013 08:36 AM, Paolo Bonzini wrote:

>> Furthermore, I'm proposing that for 1.6, we should make the format 
>> argument mandatory for drive-backup.  We made it optional for 
>> drive-mirror, to allow for probing, but there have been CVEs in the
>> past due to probing of a raw file gone wrong.  We can always relax
>> a mandatory argument into an optional one in 1.7, if we decide
>> that probing can be done safely, but we can never turn an optional
>> argument into a mandatory one once the initial release bakes in the
>> option.  It would make the code a lot simpler to just have a
>> mandatory format argument, instead of having to bake in and
>> document hueristics on which format is picked when the caller
>> doesn't provide one.
> 
> Probing is unsafe by definition, on the other hand we should allow it
> for consistency with the rest of the API.

Shouldn't security trump consistency?  My argument is that if probing
can be abused, it is better to have 1.6 be conservative and mandate a
format argument always; if we can later argue that some forms of probing
are safe, then for 1.7 we can relax the argument to optional in the
qapi-schema.json file and add code checks that still mandate the
argument in the instances where it is needed, but allow probing in the
remaining cases.  But why are we so concerned about allowing probing in
the first place?  Libvirt will ALWAYS be passing a format, because that
is the only way to avoid a security bug; it is easier to design the
format to be mandatory than it is to reason about when probing might be
safe, and there's no need to be consistent with the security holes that
are permanently baked into older commands.

> 
> Making the format mandatory for mode != 'existing' is fine, though.
> We can relax it later.
> 
> For mode = 'existing' we should allow both probing, and using an
> explicit format.

But if you insist on allowing probing for mode='existing', then
qapi-schema.json must leave it as '*format':'str', and we are stuck
implementing the code for when format is mandatory in the C code.
Paolo Bonzini - July 25, 2013, 4:59 p.m.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Il 25/07/2013 18:57, Eric Blake ha scritto:
> On 07/25/2013 08:36 AM, Paolo Bonzini wrote:
> 
>>> Furthermore, I'm proposing that for 1.6, we should make the
>>> format argument mandatory for drive-backup.  We made it
>>> optional for drive-mirror, to allow for probing, but there have
>>> been CVEs in the past due to probing of a raw file gone wrong.
>>> We can always relax a mandatory argument into an optional one
>>> in 1.7, if we decide that probing can be done safely, but we
>>> can never turn an optional argument into a mandatory one once
>>> the initial release bakes in the option.  It would make the
>>> code a lot simpler to just have a mandatory format argument,
>>> instead of having to bake in and document hueristics on which
>>> format is picked when the caller doesn't provide one.
>> 
>> Probing is unsafe by definition, on the other hand we should
>> allow it for consistency with the rest of the API.
> 
> Shouldn't security trump consistency?

If the image can be trusted, disallowing probing makes things more
complex for the user for no reason.

Also it's only on raw images that probing is unsafe.  If all
user-writable images are non-raw, probing is actually safe.

Paolo

  My argument is that if probing
> can be abused, it is better to have 1.6 be conservative and mandate
> a format argument always; if we can later argue that some forms of
> probing are safe, then for 1.7 we can relax the argument to
> optional in the qapi-schema.json file and add code checks that
> still mandate the argument in the instances where it is needed, but
> allow probing in the remaining cases.  But why are we so concerned
> about allowing probing in the first place?  Libvirt will ALWAYS be
> passing a format, because that is the only way to avoid a security
> bug; it is easier to design the format to be mandatory than it is
> to reason about when probing might be safe, and there's no need to
> be consistent with the security holes that are permanently baked
> into older commands.
> 
>> 
>> Making the format mandatory for mode != 'existing' is fine,
>> though. We can relax it later.
>> 
>> For mode = 'existing' we should allow both probing, and using an 
>> explicit format.
> 
> But if you insist on allowing probing for mode='existing', then 
> qapi-schema.json must leave it as '*format':'str', and we are
> stuck implementing the code for when format is mandatory in the C
> code.
> 

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJR8VlaAAoJEBvWZb6bTYbyTcYP/jo6xYvm2kXQ8IdDlTGu8d0I
2h+ySv2TUg7b7KSRPlDc/pAAItbfzkxRl9lrk8m50iMtX+IwFkkFYLPv0hqRBKu2
5QA6DBmXmNkCl+2pABbTaVSwZgyKqxXExuHEMyLczX7Zutx5H9RQeo7hIU3TvhcU
5yuLyO6wgjsiWspvNlax2PZiN0ZMjil5BdQsTG4G2dxP3z9bDDjH9ZFUBStcrUTO
qPuwmWWk1FvQptbK19U+pMybPX1MsDjJRVLd6QwbOTWlSn89zl5MDCXAdqJagAYj
yN4cxuN5mlV4FyKTrnhb1eIfu5Wm+EwCo9YKcSc6EbvmKPCXXNh73xZKbk/1CngT
WOhaHn+QLM0s0rQOIWPZtn+zZU2TqXW9kvRXL0nKF1cXqw/+Aoz6sjP/uJILcEh5
O9vkyeSYL7eM8EmozIRqL//R4puBjaqdKBCxD1yQQYC2dtizIBiLRVU4+w4mbnIk
d561F2MR3UUEAfkGMPMMejL+rNyywffKvle4isT4raEHCppbb5OptK6RtllJ9jPr
mLLJ+758JhMD5H5u1MiKTzE/eQtHaP0MTBrAaQJf4omKvWOfhv/jDD9lsLP0k3Ms
WmivfZdQI+bGwi9vysEhbfAuAYdC6SSeTtZCaY6m8cKsJI/7CF5RhPMXVZElC4Q5
K2Fz6vhiU+HacN6pP2F6
=TyCJ
-----END PGP SIGNATURE-----

Patch

diff --git a/block/backup.c b/block/backup.c
index 16105d4..68abd23 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,69 @@  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)) {
+            /* Yield until the job is cancelled.  We just let our before_write
+             * notify callback service CoW requests. */
+            job->common.busy = false;
+            qemu_coroutine_yield();
+            job->common.busy = true;
         }
+    } 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(bs, start * BACKUP_SECTORS_PER_CLUSTER,
+                                         BACKUP_SECTORS_PER_CLUSTER, &n);
+                /* 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 +330,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 +365,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..482029e 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;
     }
@@ -1465,10 +1461,13 @@  void qmp_drive_backup(const char *device, const char *target,
         return;
     }
 
-    if (!has_format) {
-        format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : bs->drv->format_name;
-    }
-    if (format) {
+    if (mode == NEW_IMAGE_MODE_EXISTING) {
+        format = bs->drv->format_name;
+    } else {
+        if (!has_format) {
+            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
+            return;
+        }
         drv = bdrv_find_format(format);
         if (!drv) {
             error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
@@ -1483,6 +1482,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;
+    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 +1497,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 +1520,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 7b9fef1..8a9bcc5 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1816,8 +1816,8 @@ 
 #          is a device, the existing file/device will be used as the new
 #          destination.  If it does not exist, a new file will be created.
 #
-# @format: #optional the format of the new destination, default is to
-#          probe if @mode is 'existing', else the format of the source
+# @format: #optional If @mode is 'existing' the format will be probed.
+#          If the format requires a new file then format is required.
 #
 # @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..97596da 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -957,6 +957,8 @@  Arguments:
 
 Example:
 -> { "execute": "drive-backup", "arguments": { "device": "drive0",
+                                               "sync": "full",
+                                               "format": "qcow2",
                                                "target": "backup.img" } }
 <- { "return": {} }
 EQMP