[Qemu-devel] [PATCH V6 0/3] Implement sync modes for drive-backup.

[Qemu-devel] [PATCH V6 0/3] Implement sync modes for drive-backup.

[Ian Main]

This patch adds sync modes on top of the work that Stefan Hajnoczi has done.

These patches apply on kevin/block.

Hopefully all is in order as this is my first QEMU patch.  Many thanks to
Stephan and Fam Zheng for their help.

V2:

- No longer poll, instead use qemu_coroutine_yield().
- Use bdrv_co_is_allocated().
- Much better SYNC_MODE_NONE test.

V3:

- A few style fixes.
- Better commit message explaining how TOP and NONE operate.
- Verified using checkpatch.pl.

V4:

- Add patch to use the source as a backing hd during backup.
- Add patch to default sync mode none to qcow2.

V5:

- Fix qcow2 patch.  Forgot to git add final version.

V6:

- Default to requiring 'format' when mode is absolute-paths.
- Removed one bad hunk that was misapplying.
- Fixed docs, examples and tests to match changes.
- Added tests for format bad/missing.
- Added bdrv_set_in_use() to target.
- Default to qcow2 patch not required.

Ian Main (3):
  Implement sync modes for drive-backup.
  Add tests for sync modes 'TOP' and 'NONE'
  Add backing drive while performing backup.

 block/backup.c                | 107 +++++++++++++++++++++++++++++------------
 blockdev.c                    |  36 +++++++++-----
 include/block/block_int.h     |   4 +-
 qapi-schema.json              |   4 +-
 qmp-commands.hx               |   2 +
 tests/qemu-iotests/055        | 108 +++++++++++++++++++++++++++++++++++++-----
 tests/qemu-iotests/055.out    |   4 +-
 tests/qemu-iotests/group      |   2 +-
 tests/qemu-iotests/iotests.py |   5 ++
 9 files changed, 211 insertions(+), 61 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH V6 1/3] Implement sync modes for drive-backup.

[Ian Main]

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;
+                }
+            }
+            /* 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
-- 
1.8.1.4

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

[Kevin Wolf]

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

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

[Ian Main]

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

 

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1353 bytes --]

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

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

[Ian Main]

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
 

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

[Kevin Wolf]

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

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

[Paolo Bonzini]

-----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-----

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2221 bytes --]

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

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

[Paolo Bonzini]

-----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-----

[Qemu-devel] [PATCH V6 2/3] Add tests for sync modes 'TOP' and 'NONE'

[Ian Main]

This patch adds tests for sync modes top and none.  Also added are tests
for invalid and missing formats.

Signed-off-by: Ian Main <imain@redhat.com>
---
 tests/qemu-iotests/055        | 108 +++++++++++++++++++++++++++++++++++++-----
 tests/qemu-iotests/055.out    |   4 +-
 tests/qemu-iotests/group      |   2 +-
 tests/qemu-iotests/iotests.py |   5 ++
 4 files changed, 103 insertions(+), 16 deletions(-)

diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
index c66f8db..5abe0ab 100755
--- a/tests/qemu-iotests/055
+++ b/tests/qemu-iotests/055
@@ -23,8 +23,9 @@
 import time
 import os
 import iotests
-from iotests import qemu_img, qemu_io
+from iotests import qemu_img, qemu_io, create_image
 
+backing_img = os.path.join(iotests.test_dir, 'backing.img')
 test_img = os.path.join(iotests.test_dir, 'test.img')
 target_img = os.path.join(iotests.test_dir, 'target.img')
 
@@ -34,7 +35,7 @@ class TestSingleDrive(iotests.QMPTestCase):
     def setUp(self):
         # Write data to the image so we can compare later
         qemu_img('create', '-f', iotests.imgfmt, test_img, str(TestSingleDrive.image_len))
-        qemu_io('-c', 'write -P0x5d 0 64k', test_img)
+        qemu_io('-c', 'write -P0x41 0 512', test_img)
         qemu_io('-c', 'write -P0xd5 1M 32k', test_img)
         qemu_io('-c', 'write -P0xdc 32M 124k', test_img)
         qemu_io('-c', 'write -P0xdc 67043328 64k', test_img)
@@ -54,17 +55,35 @@ class TestSingleDrive(iotests.QMPTestCase):
         self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('drive-backup', device='drive0',
-                             target=target_img, sync='full')
+                             target=target_img, format=iotests.imgfmt,
+                             sync='full')
         self.assert_qmp(result, 'return', {})
 
         event = self.cancel_and_wait()
         self.assert_qmp(event, 'data/type', 'backup')
 
+    def test_cancel_sync_none(self):
+        self.assert_no_active_block_jobs()
+
+        result = self.vm.qmp('drive-backup', device='drive0',
+                             sync='none', format='raw', target=target_img)
+        self.assert_qmp(result, 'return', {})
+        time.sleep(1)
+        self.vm.hmp_qemu_io('drive0', 'write -P0x5e 0 512')
+        self.vm.hmp_qemu_io('drive0', 'aio_flush')
+        time.sleep(1)
+        # Verify that the original contents exist in the target image.
+        self.assertEqual(-1, qemu_io('-c', 'read -P0x41 0 512', target_img).find("verification failed"))
+
+        event = self.cancel_and_wait()
+        self.assert_qmp(event, 'data/type', 'backup')
+
     def test_pause(self):
         self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('drive-backup', device='drive0',
-                             target=target_img, sync='full')
+                             target=target_img, format=iotests.imgfmt,
+                             sync='full')
         self.assert_qmp(result, 'return', {})
 
         result = self.vm.qmp('block-job-pause', device='drive0')
@@ -89,19 +108,74 @@ class TestSingleDrive(iotests.QMPTestCase):
 
     def test_medium_not_found(self):
         result = self.vm.qmp('drive-backup', device='ide1-cd0',
-                             target=target_img, sync='full')
+                             target=target_img, format=iotests.imgfmt,
+                             sync='full')
         self.assert_qmp(result, 'error/class', 'GenericError')
 
     def test_image_not_found(self):
         result = self.vm.qmp('drive-backup', device='drive0',
-                             target=target_img, sync='full', mode='existing')
+                             target=target_img, sync='full',
+                             format=iotests.imgfmt, mode='existing')
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+    def test_invalid_format(self):
+        result = self.vm.qmp('drive-backup', device='drive0',
+                             target=target_img, sync='full',
+                             format='spaghetti-noodles')
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+    def test_missing_format(self):
+        result = self.vm.qmp('drive-backup', device='drive0',
+                             target=target_img, sync='full')
         self.assert_qmp(result, 'error/class', 'GenericError')
 
     def test_device_not_found(self):
         result = self.vm.qmp('drive-backup', device='nonexistent',
-                             target=target_img, sync='full')
+                             target=target_img, sync='full',
+                             format=iotests.imgfmt)
         self.assert_qmp(result, 'error/class', 'DeviceNotFound')
 
+class TestSyncModeTop(iotests.QMPTestCase):
+    image_len = 2 * 1024 * 1024 # MB
+
+    def setUp(self):
+        create_image(backing_img, TestSyncModeTop.image_len)
+        qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, test_img)
+        qemu_io('-c', 'write -P0x5d 0 64k', test_img)
+        qemu_io('-c', 'write -P0xd5 1M 32k', test_img)
+        qemu_io('-c', 'write -P0xdc 32M 124k', test_img)
+        self.vm = iotests.VM().add_drive(test_img)
+        self.vm.launch()
+
+    def tearDown(self):
+        self.vm.shutdown()
+        os.remove(test_img)
+        os.remove(backing_img)
+        try:
+            os.remove(target_img)
+        except OSError:
+            pass
+
+    def test_complete_top(self):
+        self.assert_no_active_block_jobs()
+        result = self.vm.qmp('drive-backup', device='drive0', sync='top',
+                             format=iotests.imgfmt, target=target_img)
+        self.assert_qmp(result, 'return', {})
+
+        # Custom completed check as we are not copying all data.
+        completed = False
+        while not completed:
+            for event in self.vm.get_qmp_events(wait=True):
+                if event['event'] == 'BLOCK_JOB_COMPLETED':
+                    self.assert_qmp(event, 'data/device', 'drive0')
+                    self.assert_qmp_absent(event, 'data/error')
+                    completed = True
+
+        self.assert_no_active_block_jobs()
+        self.vm.shutdown()
+        self.assertTrue(iotests.compare_images(test_img, target_img),
+                        'target image does not match source after backup')
+
 class TestSetSpeed(iotests.QMPTestCase):
     image_len = 80 * 1024 * 1024 # MB
 
@@ -119,7 +193,8 @@ class TestSetSpeed(iotests.QMPTestCase):
         self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('drive-backup', device='drive0',
-                             target=target_img, sync='full')
+                             format=iotests.imgfmt, target=target_img,
+                             sync='full')
         self.assert_qmp(result, 'return', {})
 
         # Default speed is 0
@@ -127,7 +202,8 @@ class TestSetSpeed(iotests.QMPTestCase):
         self.assert_qmp(result, 'return[0]/device', 'drive0')
         self.assert_qmp(result, 'return[0]/speed', 0)
 
-        result = self.vm.qmp('block-job-set-speed', device='drive0', speed=8 * 1024 * 1024)
+        result = self.vm.qmp('block-job-set-speed', device='drive0',
+                             speed=8 * 1024 * 1024)
         self.assert_qmp(result, 'return', {})
 
         # Ensure the speed we set was accepted
@@ -140,7 +216,8 @@ class TestSetSpeed(iotests.QMPTestCase):
 
         # Check setting speed in drive-backup works
         result = self.vm.qmp('drive-backup', device='drive0',
-                             target=target_img, sync='full', speed=4*1024*1024)
+                             format=iotests.imgfmt, target=target_img,
+                             sync='full', speed=4*1024*1024)
         self.assert_qmp(result, 'return', {})
 
         result = self.vm.qmp('query-block-jobs')
@@ -154,13 +231,15 @@ class TestSetSpeed(iotests.QMPTestCase):
         self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('drive-backup', device='drive0',
-                             target=target_img, sync='full', speed=-1)
+                             format=iotests.imgfmt, target=target_img,
+                             sync='full', speed=-1)
         self.assert_qmp(result, 'error/class', 'GenericError')
 
         self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('drive-backup', device='drive0',
-                             target=target_img, sync='full')
+                             format=iotests.imgfmt, target=target_img,
+                             sync='full')
         self.assert_qmp(result, 'return', {})
 
         result = self.vm.qmp('block-job-set-speed', device='drive0', speed=-1)
@@ -196,6 +275,7 @@ class TestSingleTransaction(iotests.QMPTestCase):
         result = self.vm.qmp('transaction', actions=[{
                 'type': 'drive-backup',
                 'data': { 'device': 'drive0',
+                          'format': iotests.imgfmt,
                           'target': target_img,
                           'sync': 'full' },
             }
@@ -211,6 +291,7 @@ class TestSingleTransaction(iotests.QMPTestCase):
         result = self.vm.qmp('transaction', actions=[{
                 'type': 'drive-backup',
                 'data': { 'device': 'drive0',
+                          'format': iotests.imgfmt,
                           'target': target_img,
                           'sync': 'full' },
             }
@@ -241,6 +322,7 @@ class TestSingleTransaction(iotests.QMPTestCase):
         result = self.vm.qmp('transaction', actions=[{
                 'type': 'drive-backup',
                 'data': { 'device': 'ide1-cd0',
+                          'format': iotests.imgfmt,
                           'target': target_img,
                           'sync': 'full' },
             }
@@ -285,4 +367,4 @@ class TestSingleTransaction(iotests.QMPTestCase):
         self.assert_no_active_block_jobs()
 
 if __name__ == '__main__':
-    iotests.main(supported_fmts=['raw', 'qcow2'])
+    iotests.main(supported_fmts=['qcow2', 'qed'])
diff --git a/tests/qemu-iotests/055.out b/tests/qemu-iotests/055.out
index fa16b5c..52d796e 100644
--- a/tests/qemu-iotests/055.out
+++ b/tests/qemu-iotests/055.out
@@ -1,5 +1,5 @@
-.............
+.................
 ----------------------------------------------------------------------
-Ran 13 tests
+Ran 17 tests
 
 OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index fdc6ed1..d3c3f61 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -61,4 +61,4 @@
 052 rw auto backing
 053 rw auto
 054 rw auto
-055 rw auto
+055 rw auto backing
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index b028a89..33ad0ec 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -95,6 +95,11 @@ class VM(object):
         self._num_drives += 1
         return self
 
+    def hmp_qemu_io(self, drive, cmd):
+        '''Write to a given drive using an HMP command'''
+        return self.qmp('human-monitor-command',
+                        command_line='qemu-io %s "%s"' % (drive, cmd))
+
     def add_fd(self, fd, fdset, opaque, opts=''):
         '''Pass a file descriptor to the VM'''
         options = ['fd=%d' % fd,
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH V6 2/3] Add tests for sync modes 'TOP' and 'NONE'

[Kevin Wolf]

Am 23.07.2013 um 00:09 hat Ian Main geschrieben:
> This patch adds tests for sync modes top and none.  Also added are tests
> for invalid and missing formats.
> 
> Signed-off-by: Ian Main <imain@redhat.com>
> ---
>  tests/qemu-iotests/055        | 108 +++++++++++++++++++++++++++++++++++++-----
>  tests/qemu-iotests/055.out    |   4 +-
>  tests/qemu-iotests/group      |   2 +-
>  tests/qemu-iotests/iotests.py |   5 ++
>  4 files changed, 103 insertions(+), 16 deletions(-)

> @@ -127,7 +202,8 @@ class TestSetSpeed(iotests.QMPTestCase):
>          self.assert_qmp(result, 'return[0]/device', 'drive0')
>          self.assert_qmp(result, 'return[0]/speed', 0)
>  
> -        result = self.vm.qmp('block-job-set-speed', device='drive0', speed=8 * 1024 * 1024)
> +        result = self.vm.qmp('block-job-set-speed', device='drive0',
> +                             speed=8 * 1024 * 1024)

Forgot adding sync?

>          self.assert_qmp(result, 'return', {})
>  
>          # Ensure the speed we set was accepted

> @@ -285,4 +367,4 @@ class TestSingleTransaction(iotests.QMPTestCase):
>          self.assert_no_active_block_jobs()
>  
>  if __name__ == '__main__':
> -    iotests.main(supported_fmts=['raw', 'qcow2'])
> +    iotests.main(supported_fmts=['qcow2', 'qed'])

Not good. Can we split the test in a part that can be run by raw, and a
separate part that uses backing files?

Kevin

Re: [Qemu-devel] [PATCH V6 2/3] Add tests for sync modes 'TOP' and 'NONE'

[Ian Main]

On Wed, Jul 24, 2013 at 01:19:18PM +0200, Kevin Wolf wrote:
> Am 23.07.2013 um 00:09 hat Ian Main geschrieben:
> > This patch adds tests for sync modes top and none.  Also added are tests
> > for invalid and missing formats.
> > 
> > Signed-off-by: Ian Main <imain@redhat.com>
> > ---
> >  tests/qemu-iotests/055        | 108 +++++++++++++++++++++++++++++++++++++-----
> >  tests/qemu-iotests/055.out    |   4 +-
> >  tests/qemu-iotests/group      |   2 +-
> >  tests/qemu-iotests/iotests.py |   5 ++
> >  4 files changed, 103 insertions(+), 16 deletions(-)
> 
> > @@ -127,7 +202,8 @@ class TestSetSpeed(iotests.QMPTestCase):
> >          self.assert_qmp(result, 'return[0]/device', 'drive0')
> >          self.assert_qmp(result, 'return[0]/speed', 0)
> >  
> > -        result = self.vm.qmp('block-job-set-speed', device='drive0', speed=8 * 1024 * 1024)
> > +        result = self.vm.qmp('block-job-set-speed', device='drive0',
> > +                             speed=8 * 1024 * 1024)
> 
> Forgot adding sync?

Sync defaults to FULL which I think is intended here.  IIRC it was just
a long line fix.
 
> >          self.assert_qmp(result, 'return', {})
> >  
> >          # Ensure the speed we set was accepted
> 
> > @@ -285,4 +367,4 @@ class TestSingleTransaction(iotests.QMPTestCase):
> >          self.assert_no_active_block_jobs()
> >  
> >  if __name__ == '__main__':
> > -    iotests.main(supported_fmts=['raw', 'qcow2'])
> > +    iotests.main(supported_fmts=['qcow2', 'qed'])
> 
> Not good. Can we split the test in a part that can be run by raw, and a
> separate part that uses backing files?

If that is what is needed, sure.

	Ian

> Kevin

[Qemu-devel] [PATCH V6 3/3] Add backing drive while performing backup.

[Ian Main]

This patch adds the original source drive as a backing drive to our target
image so that the target image will appear complete during backup.  This
is especially useful for SYNC_MODE_NONE as it allows export via NBD to
have a complete point-in-time snapshot available for export.

Signed-off-by: Ian Main <imain@redhat.com>
---
 block/backup.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/block/backup.c b/block/backup.c
index 68abd23..d32b3b7 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -323,6 +323,11 @@ static void coroutine_fn backup_run(void *opaque)
 
     hbitmap_free(job->bitmap);
 
+    /* Set the target backing drive back to NULL before calling delete or
+     * it will also delete the underlying drive. */
+    target->backing_hd = NULL;
+    bdrv_set_in_use(target, 0);
+
     bdrv_iostatus_disable(target);
     bdrv_delete(target);
 
@@ -362,6 +367,17 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
         return;
     }
 
+    /* Manually set the backing hd to be the backup source drive so
+     * that all reads done while we are backing up will be passed
+     * on to the original source drive.  This allows reading from the
+     * image while the backup is in progress, or in the case of
+     * SYNC_MODE_NONE allows a complete image to be present for export.
+     * Note that we do this for all modes including SYNC_MODE_TOP as
+     * even then it allows on-the-fly reading. */
+    target->backing_hd = bs;
+    /* Set in use so it can only be exported by NBD. */
+    bdrv_set_in_use(target, 1);
+
     job->on_source_error = on_source_error;
     job->on_target_error = on_target_error;
     job->target = target;
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH V6 3/3] Add backing drive while performing backup.

[Kevin Wolf]

Am 23.07.2013 um 00:09 hat Ian Main geschrieben:
> This patch adds the original source drive as a backing drive to our target
> image so that the target image will appear complete during backup.  This
> is especially useful for SYNC_MODE_NONE as it allows export via NBD to
> have a complete point-in-time snapshot available for export.
> 
> Signed-off-by: Ian Main <imain@redhat.com>

This isn't directly usable, right?

Let's complettely leave it out for now, it's incomplete and most likely
wrong, and that's not easy to fix right. I expect that Fam's patches (at
which I have to take a look yet) offer a more complete solution for
this, but I wouldn't consider any change that allows users to access the
backup target for 1.6, because the user can do all sorts of interesting
things with it then, which we probably don't check for in most cases.

Kevin

Re: [Qemu-devel] [PATCH V6 0/3] Implement sync modes for drive-backup.

[Stefan Hajnoczi]

On Mon, Jul 22, 2013 at 03:09:17PM -0700, Ian Main wrote:
> This patch adds sync modes on top of the work that Stefan Hajnoczi has done.
> 
> These patches apply on kevin/block.
> 
> Hopefully all is in order as this is my first QEMU patch.  Many thanks to
> Stephan and Fam Zheng for their help.
> 
> V2:
> 
> - No longer poll, instead use qemu_coroutine_yield().
> - Use bdrv_co_is_allocated().
> - Much better SYNC_MODE_NONE test.
> 
> V3:
> 
> - A few style fixes.
> - Better commit message explaining how TOP and NONE operate.
> - Verified using checkpatch.pl.
> 
> V4:
> 
> - Add patch to use the source as a backing hd during backup.
> - Add patch to default sync mode none to qcow2.
> 
> V5:
> 
> - Fix qcow2 patch.  Forgot to git add final version.
> 
> V6:
> 
> - Default to requiring 'format' when mode is absolute-paths.
> - Removed one bad hunk that was misapplying.
> - Fixed docs, examples and tests to match changes.
> - Added tests for format bad/missing.
> - Added bdrv_set_in_use() to target.
> - Default to qcow2 patch not required.
> 
> Ian Main (3):
>   Implement sync modes for drive-backup.
>   Add tests for sync modes 'TOP' and 'NONE'
>   Add backing drive while performing backup.
> 
>  block/backup.c                | 107 +++++++++++++++++++++++++++++------------
>  blockdev.c                    |  36 +++++++++-----
>  include/block/block_int.h     |   4 +-
>  qapi-schema.json              |   4 +-
>  qmp-commands.hx               |   2 +
>  tests/qemu-iotests/055        | 108 +++++++++++++++++++++++++++++++++++++-----
>  tests/qemu-iotests/055.out    |   4 +-
>  tests/qemu-iotests/group      |   2 +-
>  tests/qemu-iotests/iotests.py |   5 ++
>  9 files changed, 211 insertions(+), 61 deletions(-)

This patch mostly takes care of image fleecing except it does not give
the target a device name which can be used by nbd-server-add.

Fam's series tackles the target device name and some of the overlapping
problems with your series.

The core feature in your series is sync=top|none and that needs to be
merged.

Now we need to figure out which patches to take and what must be
changed.  Please see the sub-threads on Fam's series.  Perhaps we can
reach a consensus there.

Stefan

Re: [Qemu-devel] [PATCH V6 0/3] Implement sync modes for drive-backup.

[Ian Main]

On Tue, Jul 23, 2013 at 01:53:51PM +0200, Stefan Hajnoczi wrote:
> On Mon, Jul 22, 2013 at 03:09:17PM -0700, Ian Main wrote:
> > This patch adds sync modes on top of the work that Stefan Hajnoczi has done.
> > 
> > These patches apply on kevin/block.
> > 
> > Hopefully all is in order as this is my first QEMU patch.  Many thanks to
> > Stephan and Fam Zheng for their help.
> > 
> > V2:
> > 
> > - No longer poll, instead use qemu_coroutine_yield().
> > - Use bdrv_co_is_allocated().
> > - Much better SYNC_MODE_NONE test.
> > 
> > V3:
> > 
> > - A few style fixes.
> > - Better commit message explaining how TOP and NONE operate.
> > - Verified using checkpatch.pl.
> > 
> > V4:
> > 
> > - Add patch to use the source as a backing hd during backup.
> > - Add patch to default sync mode none to qcow2.
> > 
> > V5:
> > 
> > - Fix qcow2 patch.  Forgot to git add final version.
> > 
> > V6:
> > 
> > - Default to requiring 'format' when mode is absolute-paths.
> > - Removed one bad hunk that was misapplying.
> > - Fixed docs, examples and tests to match changes.
> > - Added tests for format bad/missing.
> > - Added bdrv_set_in_use() to target.
> > - Default to qcow2 patch not required.
> > 
> > Ian Main (3):
> >   Implement sync modes for drive-backup.
> >   Add tests for sync modes 'TOP' and 'NONE'
> >   Add backing drive while performing backup.
> > 
> >  block/backup.c                | 107 +++++++++++++++++++++++++++++------------
> >  blockdev.c                    |  36 +++++++++-----
> >  include/block/block_int.h     |   4 +-
> >  qapi-schema.json              |   4 +-
> >  qmp-commands.hx               |   2 +
> >  tests/qemu-iotests/055        | 108 +++++++++++++++++++++++++++++++++++++-----
> >  tests/qemu-iotests/055.out    |   4 +-
> >  tests/qemu-iotests/group      |   2 +-
> >  tests/qemu-iotests/iotests.py |   5 ++
> >  9 files changed, 211 insertions(+), 61 deletions(-)
> 
> This patch mostly takes care of image fleecing except it does not give
> the target a device name which can be used by nbd-server-add.

It's not clear to me how to do that.  I looked through Fams series and
was hoping to find a clue there but I may have missed it.
 
> Fam's series tackles the target device name and some of the overlapping
> problems with your series.

Yes which is great.
 
> The core feature in your series is sync=top|none and that needs to be
> merged.
> 
> Now we need to figure out which patches to take and what must be
> changed.  Please see the sub-threads on Fam's series.  Perhaps we can
> reach a consensus there.

Yes I'm happy to offer whatever help this is.  It does seem like
applying this is the next logical move however.  Fam could then base his
patch on top of this one.  At some point soon we need to reconcile
everything and to me that seems the logical way.  There actually doesn't
seem to be too much overlap in effort from what I can see.

Fam, what do you think?  I'm happy to make changes to make it easier for
your patchset too.

	Ian
 

Re: [Qemu-devel] [PATCH V6 0/3] Implement sync modes for drive-backup.

[Fam Zheng]

On Tue, 07/23 12:55, Ian Main wrote:
> On Tue, Jul 23, 2013 at 01:53:51PM +0200, Stefan Hajnoczi wrote:
> > On Mon, Jul 22, 2013 at 03:09:17PM -0700, Ian Main wrote:
> > > This patch adds sync modes on top of the work that Stefan Hajnoczi has done.
> > > 
> > > These patches apply on kevin/block.
> > > 
> > > Hopefully all is in order as this is my first QEMU patch.  Many thanks to
> > > Stephan and Fam Zheng for their help.
> > > 
> > > V2:
> > > 
> > > - No longer poll, instead use qemu_coroutine_yield().
> > > - Use bdrv_co_is_allocated().
> > > - Much better SYNC_MODE_NONE test.
> > > 
> > > V3:
> > > 
> > > - A few style fixes.
> > > - Better commit message explaining how TOP and NONE operate.
> > > - Verified using checkpatch.pl.
> > > 
> > > V4:
> > > 
> > > - Add patch to use the source as a backing hd during backup.
> > > - Add patch to default sync mode none to qcow2.
> > > 
> > > V5:
> > > 
> > > - Fix qcow2 patch.  Forgot to git add final version.
> > > 
> > > V6:
> > > 
> > > - Default to requiring 'format' when mode is absolute-paths.
> > > - Removed one bad hunk that was misapplying.
> > > - Fixed docs, examples and tests to match changes.
> > > - Added tests for format bad/missing.
> > > - Added bdrv_set_in_use() to target.
> > > - Default to qcow2 patch not required.
> > > 
> > > Ian Main (3):
> > >   Implement sync modes for drive-backup.
> > >   Add tests for sync modes 'TOP' and 'NONE'
> > >   Add backing drive while performing backup.
> > > 
> > >  block/backup.c                | 107 +++++++++++++++++++++++++++++------------
> > >  blockdev.c                    |  36 +++++++++-----
> > >  include/block/block_int.h     |   4 +-
> > >  qapi-schema.json              |   4 +-
> > >  qmp-commands.hx               |   2 +
> > >  tests/qemu-iotests/055        | 108 +++++++++++++++++++++++++++++++++++++-----
> > >  tests/qemu-iotests/055.out    |   4 +-
> > >  tests/qemu-iotests/group      |   2 +-
> > >  tests/qemu-iotests/iotests.py |   5 ++
> > >  9 files changed, 211 insertions(+), 61 deletions(-)
> > 
> > This patch mostly takes care of image fleecing except it does not give
> > the target a device name which can be used by nbd-server-add.
> 
> It's not clear to me how to do that.  I looked through Fams series and
> was hoping to find a clue there but I may have missed it.

In short, my patches added QMP, the device id version of drive-backup:

    blockdev-backup device=source-id target=target-id

where it needs the target already there:

    (HMP) drive_add ...,backing=source-id"

"backing=source-id" is added there too, it's just the same purpose to
override backing_hd, as you added in block/backup.c

Then we have the target-id to use with NBD.

>  
> > Fam's series tackles the target device name and some of the overlapping
> > problems with your series.
> 
> Yes which is great.
>  
> > The core feature in your series is sync=top|none and that needs to be
> > merged.
> > 
> > Now we need to figure out which patches to take and what must be
> > changed.  Please see the sub-threads on Fam's series.  Perhaps we can
> > reach a consensus there.
> 
> Yes I'm happy to offer whatever help this is.  It does seem like
> applying this is the next logical move however.  Fam could then base his
> patch on top of this one.  At some point soon we need to reconcile
> everything and to me that seems the logical way.  There actually doesn't
> seem to be too much overlap in effort from what I can see.

The overlapping I can see is overriding backing_hd, but I'm not sure
which way will end merged, or both.

> 
> Fam, what do you think?  I'm happy to make changes to make it easier for
> your patchset too.
> 
I think it's fine till now, the hard and unresolved parts are adding
bits to control interface, which is independent on your patches.

Thanks.

-- 
Fam