[Qemu-devel] [PATCH V3 0/2] Implement sync modes for drive-backup.

[Qemu-devel] [PATCH V3 0/2] 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 with 
'[PATCH] block: add drive_backup HMP command' also applied.

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.

Ian Main (2):
  Implement sync modes for drive-backup.
  Add tests for sync modes 'TOP' and 'NONE'

 block/backup.c                | 91 +++++++++++++++++++++++++++++--------------
 blockdev.c                    | 25 ++++++++----
 include/block/block_int.h     |  4 +-
 qapi-schema.json              |  4 ++
 qmp-commands.hx               |  1 +
 tests/qemu-iotests/055        | 67 +++++++++++++++++++++++++++++--
 tests/qemu-iotests/055.out    |  4 +-
 tests/qemu-iotests/group      |  2 +-
 tests/qemu-iotests/iotests.py |  5 +++
 9 files changed, 157 insertions(+), 46 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH V3 1/2] 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                | 25 ++++++++-----
 include/block/block_int.h |  4 ++-
 qapi-schema.json          |  4 +++
 qmp-commands.hx           |  1 +
 5 files changed, 86 insertions(+), 39 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..000dea6 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1430,17 +1430,13 @@ void qmp_drive_backup(const char *device, const char *target,
                       Error **errp)
 {
     BlockDriverState *bs;
-    BlockDriverState *target_bs;
+    BlockDriverState *target_bs, *source;
     BlockDriver *drv = NULL;
     Error *local_err = NULL;
     int flags;
     int64_t size;
     int ret;
 
-    if (sync != MIRROR_SYNC_MODE_FULL) {
-        error_setg(errp, "only sync mode 'full' is currently supported");
-        return;
-    }
     if (!has_speed) {
         speed = 0;
     }
@@ -1483,6 +1479,13 @@ void qmp_drive_backup(const char *device, const char *target,
 
     flags = bs->open_flags | BDRV_O_RDWR;
 
+    /* See if we have a backing HD we can use to create our new image
+     * on top of. */
+    source = bs->backing_hd;
+    if (!source && sync == MIRROR_SYNC_MODE_TOP) {
+        sync = MIRROR_SYNC_MODE_FULL;
+    }
+
     size = bdrv_getlength(bs);
     if (size < 0) {
         error_setg_errno(errp, -size, "bdrv_getlength failed");
@@ -1491,8 +1494,14 @@ void qmp_drive_backup(const char *device, const char *target,
 
     if (mode != NEW_IMAGE_MODE_EXISTING) {
         assert(format && drv);
-        bdrv_img_create(target, format,
-                        NULL, NULL, NULL, size, flags, &local_err, false);
+        if (sync == MIRROR_SYNC_MODE_TOP) {
+            bdrv_img_create(target, format, source->filename,
+                            source->drv->format_name, NULL,
+                            size, flags, &local_err, false);
+        } else {
+            bdrv_img_create(target, format, NULL, NULL, NULL,
+                            size, flags, &local_err, false);
+        }
     }
 
     if (error_is_set(&local_err)) {
@@ -1508,7 +1517,7 @@ void qmp_drive_backup(const char *device, const char *target,
         return;
     }
 
-    backup_start(bs, target_bs, speed, on_source_error, on_target_error,
+    backup_start(bs, target_bs, speed, sync, on_source_error, on_target_error,
                  block_job_cb, bs, &local_err);
     if (local_err != NULL) {
         bdrv_delete(target_bs);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index c6ac871..e45f2a0 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -404,6 +404,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
  * @bs: Block device to operate on.
  * @target: Block device to write to.
  * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
+ * @sync_mode: What parts of the disk image should be copied to the destination.
  * @on_source_error: The action to take upon error reading from the source.
  * @on_target_error: The action to take upon error writing to the target.
  * @cb: Completion function for the job.
@@ -413,7 +414,8 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
  * until the job is cancelled or manually completed.
  */
 void backup_start(BlockDriverState *bs, BlockDriverState *target,
-                  int64_t speed, BlockdevOnError on_source_error,
+                  int64_t speed, MirrorSyncMode sync_mode,
+                  BlockdevOnError on_source_error,
                   BlockdevOnError on_target_error,
                   BlockDriverCompletionFunc *cb, void *opaque,
                   Error **errp);
diff --git a/qapi-schema.json b/qapi-schema.json
index cdd2d6b..b3f6c2a 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1807,6 +1807,10 @@
 # @format: #optional the format of the new destination, default is to
 #          probe if @mode is 'existing', else the format of the source
 #
+# @sync: what parts of the disk image should be copied to the destination
+#        (all the disk, only the sectors allocated in the topmost image, or
+#        only new I/O).
+#
 # @mode: #optional whether and how QEMU should create a new image, default is
 #        'absolute-paths'.
 #
diff --git a/qmp-commands.hx b/qmp-commands.hx
index e075df4..f3f6b3d 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -957,6 +957,7 @@ Arguments:
 
 Example:
 -> { "execute": "drive-backup", "arguments": { "device": "drive0",
+                                               "sync": "full",
                                                "target": "backup.img" } }
 <- { "return": {} }
 EQMP
-- 
1.8.1.4

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

[Ian Main]

This patch adds tests for sync modes top and none.

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

diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
index c66f8db..06d1f77 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)
@@ -60,6 +61,22 @@ class TestSingleDrive(iotests.QMPTestCase):
         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()
 
@@ -102,6 +119,47 @@ class TestSingleDrive(iotests.QMPTestCase):
                              target=target_img, sync='full')
         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',
+                             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
 
@@ -127,7 +185,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
@@ -285,4 +344,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..96961ed 100644
--- a/tests/qemu-iotests/055.out
+++ b/tests/qemu-iotests/055.out
@@ -1,5 +1,5 @@
-.............
+...............
 ----------------------------------------------------------------------
-Ran 13 tests
+Ran 15 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 V3 0/2] Implement sync modes for drive-backup.

[Paolo Bonzini]

Il 15/07/2013 20:01, Ian Main ha scritto:
> This patch adds sync modes on top of the work that Stefan Hajnoczi has done.
> 
> These patches apply on kevin/block with 
> '[PATCH] block: add drive_backup HMP command' also applied.
> 
> 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.
> 
> Ian Main (2):
>   Implement sync modes for drive-backup.
>   Add tests for sync modes 'TOP' and 'NONE'
> 
>  block/backup.c                | 91 +++++++++++++++++++++++++++++--------------
>  blockdev.c                    | 25 ++++++++----
>  include/block/block_int.h     |  4 +-
>  qapi-schema.json              |  4 ++
>  qmp-commands.hx               |  1 +
>  tests/qemu-iotests/055        | 67 +++++++++++++++++++++++++++++--
>  tests/qemu-iotests/055.out    |  4 +-
>  tests/qemu-iotests/group      |  2 +-
>  tests/qemu-iotests/iotests.py |  5 +++
>  9 files changed, 157 insertions(+), 46 deletions(-)
> 

This still doesn't have a working NONE when used with the NBD server,
which is one of the two cases which we actually care about.

Paolo