diff mbox

[v2,1/2] blockdev: add sync mode to drive-backup QMP command

Message ID 1372248718-27953-2-git-send-email-stefanha@redhat.com
State New
Headers show

Commit Message

Stefan Hajnoczi June 26, 2013, 12:11 p.m. UTC 
The drive-backup command is similar to the drive-mirror command, except
no guest data written after the command executes gets copied.  Add a
sync mode argument which determines whether the entire disk is copied,
just allocated clusters, or only clusters being written to by the guest.

Currently only sync mode 'full' is supported - it copies the entire disk.
For read-only point-in-time snapshots we may only need sync mode 'none'
since the target can be a qcow2 file using the guest's disk as its
backing file (no need to copy the entire disk).  Finally, sync mode
'top' is useful if we wish to preserve the backing chain.

Note that this patch just adds the sync mode argument to drive-backup.
It does not implement sync modes 'top' or 'none'.  This patch is
necessary so we can add a drive-backup HMP command that behaves like the
existing drive-mirror HMP command and takes a sync mode.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 blockdev.c             |  6 ++++++
 qapi-schema.json       | 14 ++++++++++++--
 qmp-commands.hx        |  6 +++++-
 tests/qemu-iotests/055 | 36 +++++++++++++++++++++---------------
 4 files changed, 44 insertions(+), 18 deletions(-)

Comments

Eric Blake July 10, 2013, 2:31 p.m. UTC | #1 
On 06/26/2013 06:11 AM, Stefan Hajnoczi wrote:
> The drive-backup command is similar to the drive-mirror command, except
> no guest data written after the command executes gets copied.  Add a
> sync mode argument which determines whether the entire disk is copied,
> just allocated clusters, or only clusters being written to by the guest.
> 
> Currently only sync mode 'full' is supported - it copies the entire disk.
> For read-only point-in-time snapshots we may only need sync mode 'none'
> since the target can be a qcow2 file using the guest's disk as its
> backing file (no need to copy the entire disk).  Finally, sync mode
> 'top' is useful if we wish to preserve the backing chain.
> 
> Note that this patch just adds the sync mode argument to drive-backup.
> It does not implement sync modes 'top' or 'none'.  This patch is
> necessary so we can add a drive-backup HMP command that behaves like the
> existing drive-mirror HMP command and takes a sync mode.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---

> +++ b/qapi-schema.json
> @@ -1626,6 +1626,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'.
>  #
> @@ -1647,7 +1651,8 @@
>  ##
>  { 'type': 'DriveBackup',
>    'data': { 'device': 'str', 'target': 'str', '*format': 'str',
> -            '*mode': 'NewImageMode', '*speed': 'int',
> +            'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',

Making it mandatory is fine, since the command is new to 1.6 (if this
were after 1.6, we'd have to make it optional).

>  #
> +# @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).

This duplication will conflict with Kevin's patch currently on the qmp
queue for 'qapi-schema: Use existing type for drive-backup arguments';
you may want to coordinate which series gets merged first, and rebase
the other one appropriately.

If you do rebase, feel free to add:
Reviewed-by: Eric Blake <eblake@redhat.com>
Kevin Wolf July 10, 2013, 2:35 p.m. UTC | #2 
Am 10.07.2013 um 16:31 hat Eric Blake geschrieben:
> On 06/26/2013 06:11 AM, Stefan Hajnoczi wrote:
> > The drive-backup command is similar to the drive-mirror command, except
> > no guest data written after the command executes gets copied.  Add a
> > sync mode argument which determines whether the entire disk is copied,
> > just allocated clusters, or only clusters being written to by the guest.
> > 
> > Currently only sync mode 'full' is supported - it copies the entire disk.
> > For read-only point-in-time snapshots we may only need sync mode 'none'
> > since the target can be a qcow2 file using the guest's disk as its
> > backing file (no need to copy the entire disk).  Finally, sync mode
> > 'top' is useful if we wish to preserve the backing chain.
> > 
> > Note that this patch just adds the sync mode argument to drive-backup.
> > It does not implement sync modes 'top' or 'none'.  This patch is
> > necessary so we can add a drive-backup HMP command that behaves like the
> > existing drive-mirror HMP command and takes a sync mode.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> 
> > +++ b/qapi-schema.json
> > @@ -1626,6 +1626,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'.
> >  #
> > @@ -1647,7 +1651,8 @@
> >  ##
> >  { 'type': 'DriveBackup',
> >    'data': { 'device': 'str', 'target': 'str', '*format': 'str',
> > -            '*mode': 'NewImageMode', '*speed': 'int',
> > +            'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
> 
> Making it mandatory is fine, since the command is new to 1.6 (if this
> were after 1.6, we'd have to make it optional).
> 
> >  #
> > +# @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).
> 
> This duplication will conflict with Kevin's patch currently on the qmp
> queue for 'qapi-schema: Use existing type for drive-backup arguments';
> you may want to coordinate which series gets merged first, and rebase
> the other one appropriately.

Luiz said he's going to send his pull request today; then I'll rebase
my tree and fix this conflict tomorrow. (It's trivial enough that we
don't have to go through another version.)

> If you do rebase, feel free to add:
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks, I've added it.

Kevin
diff mbox

Patch

diff --git a/blockdev.c b/blockdev.c
index edca843..fe73278 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -936,6 +936,7 @@  static void drive_backup_prepare(BlkTransactionState *common, Error **errp)
 
     qmp_drive_backup(backup->device, backup->target,
                      backup->has_format, backup->format,
+                     backup->sync,
                      backup->has_mode, backup->mode,
                      backup->has_speed, backup->speed,
                      backup->has_on_source_error, backup->on_source_error,
@@ -1421,6 +1422,7 @@  void qmp_block_commit(const char *device,
 
 void qmp_drive_backup(const char *device, const char *target,
                       bool has_format, const char *format,
+                      enum MirrorSyncMode sync,
                       bool has_mode, enum NewImageMode mode,
                       bool has_speed, int64_t speed,
                       bool has_on_source_error, BlockdevOnError on_source_error,
@@ -1435,6 +1437,10 @@  void qmp_drive_backup(const char *device, const char *target,
     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;
     }
diff --git a/qapi-schema.json b/qapi-schema.json
index d6479e1..1e19e4b 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1626,6 +1626,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'.
 #
@@ -1647,7 +1651,8 @@ 
 ##
 { 'type': 'DriveBackup',
   'data': { 'device': 'str', 'target': 'str', '*format': 'str',
-            '*mode': 'NewImageMode', '*speed': 'int',
+            'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
+            '*speed': 'int',
             '*on-source-error': 'BlockdevOnError',
             '*on-target-error': 'BlockdevOnError' } }
 
@@ -1807,6 +1812,10 @@ 
 #
 # @speed: #optional the maximum speed, in bytes per second
 #
+# @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).
+#
 # @on-source-error: #optional the action to take on an error on the source,
 #                   default 'report'.  'stop' and 'enospc' can only be used
 #                   if the block device supports io-status (see BlockInfo).
@@ -1826,7 +1835,8 @@ 
 ##
 { 'command': 'drive-backup',
   'data': { 'device': 'str', 'target': 'str', '*format': 'str',
-            '*mode': 'NewImageMode', '*speed': 'int',
+            'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
+            '*speed': 'int',
             '*on-source-error': 'BlockdevOnError',
             '*on-target-error': 'BlockdevOnError' } }
 
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 362f0e1..e075df4 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -913,7 +913,7 @@  EQMP
 
     {
         .name       = "drive-backup",
-        .args_type  = "device:B,target:s,speed:i?,mode:s?,format:s?,"
+        .args_type  = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?,"
                       "on-source-error:s?,on-target-error:s?",
         .mhandler.cmd_new = qmp_marshal_input_drive_backup,
     },
@@ -939,6 +939,10 @@  Arguments:
 - "format": the format of the new destination, default is to probe if 'mode' is
             'existing', else the format of the source
             (json-string, optional)
+- "sync": what parts of the disk image should be copied to the destination;
+  possibilities include "full" for all the disk, "top" for only the sectors
+  allocated in the topmost image, or "none" to only replicate new I/O
+  (MirrorSyncMode).
 - "mode": whether and how QEMU should create a new image
           (NewImageMode, optional, default 'absolute-paths')
 - "speed": the maximum speed, in bytes per second (json-int, optional)
diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
index 887c959..c66f8db 100755
--- a/tests/qemu-iotests/055
+++ b/tests/qemu-iotests/055
@@ -54,7 +54,7 @@  class TestSingleDrive(iotests.QMPTestCase):
         self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('drive-backup', device='drive0',
-                             target=target_img)
+                             target=target_img, sync='full')
         self.assert_qmp(result, 'return', {})
 
         event = self.cancel_and_wait()
@@ -64,7 +64,7 @@  class TestSingleDrive(iotests.QMPTestCase):
         self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('drive-backup', device='drive0',
-                             target=target_img)
+                             target=target_img, sync='full')
         self.assert_qmp(result, 'return', {})
 
         result = self.vm.qmp('block-job-pause', device='drive0')
@@ -89,17 +89,17 @@  class TestSingleDrive(iotests.QMPTestCase):
 
     def test_medium_not_found(self):
         result = self.vm.qmp('drive-backup', device='ide1-cd0',
-                             target=target_img)
+                             target=target_img, sync='full')
         self.assert_qmp(result, 'error/class', 'GenericError')
 
     def test_image_not_found(self):
         result = self.vm.qmp('drive-backup', device='drive0',
-                             mode='existing', target=target_img)
+                             target=target_img, sync='full', mode='existing')
         self.assert_qmp(result, 'error/class', 'GenericError')
 
     def test_device_not_found(self):
         result = self.vm.qmp('drive-backup', device='nonexistent',
-                             target=target_img)
+                             target=target_img, sync='full')
         self.assert_qmp(result, 'error/class', 'DeviceNotFound')
 
 class TestSetSpeed(iotests.QMPTestCase):
@@ -119,7 +119,7 @@  class TestSetSpeed(iotests.QMPTestCase):
         self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('drive-backup', device='drive0',
-                             target=target_img)
+                             target=target_img, sync='full')
         self.assert_qmp(result, 'return', {})
 
         # Default speed is 0
@@ -140,7 +140,7 @@  class TestSetSpeed(iotests.QMPTestCase):
 
         # Check setting speed in drive-backup works
         result = self.vm.qmp('drive-backup', device='drive0',
-                             target=target_img, speed=4*1024*1024)
+                             target=target_img, sync='full', speed=4*1024*1024)
         self.assert_qmp(result, 'return', {})
 
         result = self.vm.qmp('query-block-jobs')
@@ -154,13 +154,13 @@  class TestSetSpeed(iotests.QMPTestCase):
         self.assert_no_active_block_jobs()
 
         result = self.vm.qmp('drive-backup', device='drive0',
-                             target=target_img, speed=-1)
+                             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)
+                             target=target_img, sync='full')
         self.assert_qmp(result, 'return', {})
 
         result = self.vm.qmp('block-job-set-speed', device='drive0', speed=-1)
@@ -196,7 +196,8 @@  class TestSingleTransaction(iotests.QMPTestCase):
         result = self.vm.qmp('transaction', actions=[{
                 'type': 'drive-backup',
                 'data': { 'device': 'drive0',
-                          'target': target_img },
+                          'target': target_img,
+                          'sync': 'full' },
             }
         ])
         self.assert_qmp(result, 'return', {})
@@ -210,7 +211,8 @@  class TestSingleTransaction(iotests.QMPTestCase):
         result = self.vm.qmp('transaction', actions=[{
                 'type': 'drive-backup',
                 'data': { 'device': 'drive0',
-                          'target': target_img },
+                          'target': target_img,
+                          'sync': 'full' },
             }
         ])
         self.assert_qmp(result, 'return', {})
@@ -239,7 +241,8 @@  class TestSingleTransaction(iotests.QMPTestCase):
         result = self.vm.qmp('transaction', actions=[{
                 'type': 'drive-backup',
                 'data': { 'device': 'ide1-cd0',
-                          'target': target_img },
+                          'target': target_img,
+                          'sync': 'full' },
             }
         ])
         self.assert_qmp(result, 'error/class', 'GenericError')
@@ -249,7 +252,8 @@  class TestSingleTransaction(iotests.QMPTestCase):
                 'type': 'drive-backup',
                 'data': { 'device': 'drive0',
                           'mode': 'existing',
-                          'target': target_img },
+                          'target': target_img,
+                          'sync': 'full' },
             }
         ])
         self.assert_qmp(result, 'error/class', 'GenericError')
@@ -259,7 +263,8 @@  class TestSingleTransaction(iotests.QMPTestCase):
                 'type': 'drive-backup',
                 'data': { 'device': 'nonexistent',
                           'mode': 'existing',
-                          'target': target_img },
+                          'target': target_img,
+                          'sync': 'full' },
             }
         ])
         self.assert_qmp(result, 'error/class', 'DeviceNotFound')
@@ -269,7 +274,8 @@  class TestSingleTransaction(iotests.QMPTestCase):
                 'type': 'drive-backup',
                 'data': { 'device': 'nonexistent',
                           'mode': 'existing',
-                          'target': target_img },
+                          'target': target_img,
+                          'sync': 'full' },
             }, {
                 'type': 'Abort',
                 'data': {},