[v2,11/11] qmp: add command 'blockdev-backup'
diff mbox

Message ID 1374054136-28741-12-git-send-email-famz@redhat.com
State New
Headers show

Commit Message

Fam Zheng July 17, 2013, 9:42 a.m. UTC
Similar to drive-backup, but this command uses a device id as target
instead of creating/opening an image file.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev.c       | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json | 49 ++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx  | 22 ++++++++++++++++++
 3 files changed, 142 insertions(+)

Comments

Eric Blake July 17, 2013, 12:44 p.m. UTC | #1
On 07/17/2013 03:42 AM, Fam Zheng wrote:
> Similar to drive-backup, but this command uses a device id as target
> instead of creating/opening an image file.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  blockdev.c       | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi-schema.json | 49 ++++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx  | 22 ++++++++++++++++++
>  3 files changed, 142 insertions(+)
> 

> +++ b/qapi-schema.json
> @@ -1665,6 +1665,40 @@
>              '*on-target-error': 'BlockdevOnError' } }
>  
>  ##
> +# @BlockdevBackup
> +#

> +{ 'type': 'BlockdevBackup',
> +  'data': { 'device': 'str', 'target': 'str',
> +            'sync': 'MirrorSyncMode',
> +            '*speed': 'int',
> +            '*on-source-error': 'BlockdevOnError',
> +            '*on-target-error': 'BlockdevOnError' } }

Seems okay.  But what is missing is the addition of this type into the
union used for 'transaction' - shouldn't it be possible to mix this with
other transaction capabilities?
Fam Zheng July 18, 2013, 4:41 a.m. UTC | #2
On Wed, 07/17 06:44, Eric Blake wrote:
> On 07/17/2013 03:42 AM, Fam Zheng wrote:
> > Similar to drive-backup, but this command uses a device id as target
> > instead of creating/opening an image file.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  blockdev.c       | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  qapi-schema.json | 49 ++++++++++++++++++++++++++++++++++++++
> >  qmp-commands.hx  | 22 ++++++++++++++++++
> >  3 files changed, 142 insertions(+)
> > 
> 
> > +++ b/qapi-schema.json
> > @@ -1665,6 +1665,40 @@
> >              '*on-target-error': 'BlockdevOnError' } }
> >  
> >  ##
> > +# @BlockdevBackup
> > +#
> 
> > +{ 'type': 'BlockdevBackup',
> > +  'data': { 'device': 'str', 'target': 'str',
> > +            'sync': 'MirrorSyncMode',
> > +            '*speed': 'int',
> > +            '*on-source-error': 'BlockdevOnError',
> > +            '*on-target-error': 'BlockdevOnError' } }
> 
> Seems okay.  But what is missing is the addition of this type into the
> union used for 'transaction' - shouldn't it be possible to mix this with
> other transaction capabilities?
> 

Should be possible, as users may want point-in-time snapshot of multiple
disks. If this API looks OK, I will include it into transaction in the
next version.
Wayne Xia July 19, 2013, 10:16 a.m. UTC | #3
于 2013-7-18 12:41, Fam Zheng 写道:
> On Wed, 07/17 06:44, Eric Blake wrote:
>> On 07/17/2013 03:42 AM, Fam Zheng wrote:
>>> Similar to drive-backup, but this command uses a device id as target
>>> instead of creating/opening an image file.
>>>
>>> Signed-off-by: Fam Zheng <famz@redhat.com>
>>> ---
>>>   blockdev.c       | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   qapi-schema.json | 49 ++++++++++++++++++++++++++++++++++++++
>>>   qmp-commands.hx  | 22 ++++++++++++++++++
>>>   3 files changed, 142 insertions(+)
>>>
>>
>>> +++ b/qapi-schema.json
>>> @@ -1665,6 +1665,40 @@
>>>               '*on-target-error': 'BlockdevOnError' } }
>>>
>>>   ##
>>> +# @BlockdevBackup
>>> +#
>>
>>> +{ 'type': 'BlockdevBackup',
>>> +  'data': { 'device': 'str', 'target': 'str',
>>> +            'sync': 'MirrorSyncMode',
>>> +            '*speed': 'int',
>>> +            '*on-source-error': 'BlockdevOnError',
>>> +            '*on-target-error': 'BlockdevOnError' } }
>>
>> Seems okay.  But what is missing is the addition of this type into the
>> union used for 'transaction' - shouldn't it be possible to mix this with
>> other transaction capabilities?
>>
>
> Should be possible, as users may want point-in-time snapshot of multiple
> disks. If this API looks OK, I will include it into transaction in the
> next version.
>
   Instead of add this new API, how about extend Driver-backup API? This
patch is basically doing similar things with driver-backup, extension
of old API will save trouble to do same things driver-backup already
does, such as support qmp_transaction.
Stefan Hajnoczi July 23, 2013, 10:10 a.m. UTC | #4
On Fri, Jul 19, 2013 at 06:16:55PM +0800, Wenchao Xia wrote:
> 于 2013-7-18 12:41, Fam Zheng 写道:
> >On Wed, 07/17 06:44, Eric Blake wrote:
> >>On 07/17/2013 03:42 AM, Fam Zheng wrote:
> >>>Similar to drive-backup, but this command uses a device id as target
> >>>instead of creating/opening an image file.
> >>>
> >>>Signed-off-by: Fam Zheng <famz@redhat.com>
> >>>---
> >>>  blockdev.c       | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>  qapi-schema.json | 49 ++++++++++++++++++++++++++++++++++++++
> >>>  qmp-commands.hx  | 22 ++++++++++++++++++
> >>>  3 files changed, 142 insertions(+)
> >>>
> >>
> >>>+++ b/qapi-schema.json
> >>>@@ -1665,6 +1665,40 @@
> >>>              '*on-target-error': 'BlockdevOnError' } }
> >>>
> >>>  ##
> >>>+# @BlockdevBackup
> >>>+#
> >>
> >>>+{ 'type': 'BlockdevBackup',
> >>>+  'data': { 'device': 'str', 'target': 'str',
> >>>+            'sync': 'MirrorSyncMode',
> >>>+            '*speed': 'int',
> >>>+            '*on-source-error': 'BlockdevOnError',
> >>>+            '*on-target-error': 'BlockdevOnError' } }
> >>
> >>Seems okay.  But what is missing is the addition of this type into the
> >>union used for 'transaction' - shouldn't it be possible to mix this with
> >>other transaction capabilities?
> >>
> >
> >Should be possible, as users may want point-in-time snapshot of multiple
> >disks. If this API looks OK, I will include it into transaction in the
> >next version.
> >
>   Instead of add this new API, how about extend Driver-backup API? This
> patch is basically doing similar things with driver-backup, extension
> of old API will save trouble to do same things driver-backup already
> does, such as support qmp_transaction.

The rationale was that drive-* commands should be high-level and can
create image files.  blockdev-* commands should be low-level and work on
an existing -drive.

The reason for keeping two separate commands is to avoid adding in
parameters that work at different levels (filename vs drive name).

In terms of API design I think drive- + blockdev- really is cleaner.
Kevin can explain in more detail if I got something wrong.

Stefan

Patch
diff mbox

diff --git a/blockdev.c b/blockdev.c
index bb986a1..a3fa5d1 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1517,7 +1517,78 @@  void qmp_drive_backup(const char *device, const char *target,
         error_propagate(errp, local_err);
         return;
     }
+    /* Grab a reference so hotplug does not delete the BlockDriverState from
+     * underneath us.
+     */
+    drive_get_ref(drive_get_by_blockdev(bs));
+}
+
+void qmp_blockdev_backup(const char *device, const char *target,
+                       enum MirrorSyncMode sync,
+                       bool has_speed, int64_t speed,
+                       bool has_on_source_error,
+                       BlockdevOnError on_source_error,
+                       bool has_on_target_error,
+                       BlockdevOnError on_target_error,
+                       Error **errp)
+{
+    BlockDriverState *bs;
+    BlockDriverState *target_bs;
+    Error *local_err = NULL;
+
+    if (sync != MIRROR_SYNC_MODE_FULL) {
+        error_setg(errp, "only sync mode 'full' is currently supported");
+        return;
+    }
+    if (!has_speed) {
+        speed = 0;
+    }
+    if (!has_on_source_error) {
+        on_source_error = BLOCKDEV_ON_ERROR_REPORT;
+    }
+    if (!has_on_target_error) {
+        on_target_error = BLOCKDEV_ON_ERROR_REPORT;
+    }
+
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    if (!bdrv_is_inserted(bs)) {
+        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
+        return;
+    }
+
+    if (bdrv_in_use(bs)) {
+        error_set(errp, QERR_DEVICE_IN_USE, device);
+        return;
+    }
+
+    target_bs = bdrv_find(target);
+    if (!target_bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, target);
+        return;
+    }
+
+    if (!bdrv_is_inserted(target_bs)) {
+        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, target);
+        return;
+    }
+
+    if (bdrv_in_use(target_bs)) {
+        error_set(errp, QERR_DEVICE_IN_USE, target);
+        return;
+    }
 
+    backup_start(bs, target_bs, speed, on_source_error, on_target_error,
+                 block_job_cb, bs, &local_err);
+    if (local_err != NULL) {
+        bdrv_delete(target_bs);
+        error_propagate(errp, local_err);
+        return;
+    }
     /* Grab a reference so hotplug does not delete the BlockDriverState from
      * underneath us.
      */
diff --git a/qapi-schema.json b/qapi-schema.json
index 7b9fef1..3b7cfcc 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1665,6 +1665,40 @@ 
             '*on-target-error': 'BlockdevOnError' } }
 
 ##
+# @BlockdevBackup
+#
+# @device: the name of the device which should be copied.
+#
+# @target: the name of the backup target device
+#
+# @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).
+#
+# @speed: #optional the maximum speed, in bytes per second
+#
+# @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).
+#
+# @on-target-error: #optional the action to take on an error on the target,
+#                   default 'report' (no limitations, since this applies to
+#                   a different block device than @device).
+#
+# Note that @on-source-error and @on-target-error only affect background I/O.
+# If an error occurs during a guest write request, the device's rerror/werror
+# actions will be used.
+#
+# Since: 1.6
+##
+{ 'type': 'BlockdevBackup',
+  'data': { 'device': 'str', 'target': 'str',
+            'sync': 'MirrorSyncMode',
+            '*speed': 'int',
+            '*on-source-error': 'BlockdevOnError',
+            '*on-target-error': 'BlockdevOnError' } }
+
+##
 # @Abort
 #
 # This action can be used to test transaction failure.
@@ -1806,6 +1840,21 @@ 
 { 'command': 'drive-backup', 'data': 'DriveBackup' }
 
 ##
+# @blockdev-backup
+#
+# Block device version for drive-backup command. Use existing device as target
+# of backup.
+#
+# For the arguments, see the documentation of BlockdevBackup.
+#
+# Returns: nothing on success
+#          If @device or @target is not a valid block device, DeviceNotFound
+#
+# Since 1.6
+##
+{ 'command': 'blockdev-backup', 'data': 'BlockdevBackup' }
+
+##
 # @drive-mirror
 #
 # Start mirroring a block device's writes to a new destination.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index e075df4..ecd9f64 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -959,6 +959,28 @@  Example:
 -> { "execute": "drive-backup", "arguments": { "device": "drive0",
                                                "target": "backup.img" } }
 <- { "return": {} }
+
+EQMP
+
+    {
+        .name       = "blockdev-backup",
+        .args_type  = "sync:s,device:B,target:B,speed:i?,"
+                      "on-source-error:s?,on-target-error:s?",
+        .mhandler.cmd_new = qmp_marshal_input_blockdev_backup,
+    },
+
+SQMP
+blockdev-backup
+------------
+
+The device version of drive-backup: this command doesn't create new image for
+target, instead it uses a existing named device as target.
+
+Example:
+-> { "execute": "blockdev-backup", "arguments": { "device": "drive0",
+                                                  "target": "drive1" } }
+<- { "return": {} }
+
 EQMP
 
     {