Patchwork [V2,07/12] qmp: add internal snapshot support in qmp_transaction

login
register
mail settings
Submitter Wayne Xia
Date June 14, 2013, 11:39 a.m.
Message ID <1371209999-15579-8-git-send-email-xiawenc@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/251400/
State New
Headers show

Comments

Wayne Xia - June 14, 2013, 11:39 a.m.
Unlike savevm, the qmp_transaction interface will not generate
snapshot name automatically, saving trouble to return information
of the new created snapshot. The snapshot name should not mess up
with snapshot ID, there is a check for it.

Although qcow2 support storing multiple snapshots with same name
but different ID, here it will fail when an snapshot with that name
already exist before the operation. Format such as rbd do not support
ID at all, and in most case, it means trouble to user when he faces
multiple snapshots with same name, so ban that case.

Snapshot ID can't be specified in this interface.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 blockdev.c       |  118 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json |   16 +++++++
 qmp-commands.hx  |   32 +++++++++++---
 3 files changed, 159 insertions(+), 7 deletions(-)
Eric Blake - June 15, 2013, 9:40 a.m.
On 06/14/2013 12:39 PM, Wenchao Xia wrote:
> Unlike savevm, the qmp_transaction interface will not generate
> snapshot name automatically, saving trouble to return information
> of the new created snapshot. The snapshot name should not mess up
> with snapshot ID, there is a check for it.
> 
> Although qcow2 support storing multiple snapshots with same name
> but different ID, here it will fail when an snapshot with that name
> already exist before the operation. Format such as rbd do not support
> ID at all, and in most case, it means trouble to user when he faces
> multiple snapshots with same name, so ban that case.
> 
> Snapshot ID can't be specified in this interface.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  blockdev.c       |  118 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi-schema.json |   16 +++++++
>  qmp-commands.hx  |   32 +++++++++++---
>  3 files changed, 159 insertions(+), 7 deletions(-)

> +++ b/qapi-schema.json
> @@ -1613,6 +1613,21 @@
>  { 'type': 'BlockdevSnapshot',
>    'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str',
>              '*mode': 'NewImageMode' } }
> +##
> +# @BlockdevSnapshotInternal
> +#
> +# @device: the name of the device to generate the snapshot from
> +#
> +# @name: the name of the internal snapshot to be created
> +#
> +# Notes: In transaction, if any snapshot matching @name exists, the operation
> +#        will fail. If the name is a numeric string, it will also fail. Only
> +#        some image format support it, for example, qcow2, rbd, and sheepdog.

s/format/formats/

> +#
> +# Since: 1.6
> +##
> +{ 'type': 'BlockdevSnapshotInternal',
> +  'data': { 'device': 'str', 'name': 'str' } }
>  
>  ##
>  # @TransactionAction
> @@ -1623,6 +1638,7 @@
>  { 'union': 'TransactionAction',
>    'data': {
>         'blockdev-snapshot-sync': 'BlockdevSnapshot'
> +       'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal'

Invalid JSON - you need a comma between name-value pairs. (I _wish_ JSON
had allowed trailing commas, the way C99 does, because it reduces the
size of diffs when adding an element at the end...)

Yet another instance of the introspection problem.  Is there some other
patch in this series that introduces a standalone command that witnesses
whether this transaction is available?  If so, I'm okay; if not, then
libvirt won't know if this transaction is available without
introspection, or by trying it first and detecting the error when it is
not available.  For the purposes of transactions, since any failure
rolls back the entire transaction (including the failure of an unknown
action within the transaction), the try-and-fail approach may be
sufficient, even if it is not the prettiest.

> +++ b/qmp-commands.hx
> @@ -948,13 +948,13 @@ transaction
>  -----------
>  
>  Atomically operate on one or more block devices.  The only supported
> -operation for now is snapshotting.  If there is any failure performing
> -any of the operations, all snapshots for the group are abandoned, and
> -the original disks pre-snapshot attempt are used.
> +operations for now are internal and external snapshotting.  A list of
> +dictionaries is accepted, that contains the actions to be performed.  The
> +sequence of the requests will not affect the result.

Not true.  Taking an internal and then an external snapshot has a
different effect than taking an external and then internal snapshot of
the same disk.  The sequence of requests is significant, because it
controls which of two files involved contains the internal snapshot.

>  
> -A list of dictionaries is accepted, that contains the actions to be performed.
> -For snapshots this is the device, the file to use for the new snapshot,
> -and the format.  The default format, if not specified, is qcow2.
> +For external snapshot, The dictionary is the device, the file to use for the

s/snapshot, The/snapshots, the/
s/dictionary is/dictionary contains/

> +new snapshot, and the format.  The default format, if not specified, is qcow2.
>  
>  Each new snapshot defaults to being created by QEMU (wiping any
>  contents if the file already exists), but it is also possible to reuse
> @@ -963,6 +963,18 @@ the new image file has the same contents as the current one; QEMU cannot
>  perform any meaningful check.  Typically this is achieved by using the
>  current image file as the backing file for the new image.
>  
> +On fail the original disks pre-snapshot attempt will be used.
> +
> +For internal snapshot, The dictionary is the device and the snapshot's name.

s/snapshot, The/snapsohts, the/
s/dictionary is/dictionary contains/

> +If name is a numeric string which will mess up with ID, the request will be
> +rejected.  For example, name "99" is not a valid name.  If an internal snapshot
> +matching name already exists, the request will be also rejected.  Only some
> +image format support it, for example, qcow2, rbd, and sheepdog.
> +
> +On fail, qemu will try delete new created internal snapshot in the

s/fail/failure/

why "try"?  The point of a transaction is that it either completely
happens or is completely aborted.  If you are leaving behind garbage
after a failure later in the transaction, then you didn't design the
transaction correctly.  (And yes, we have an existing bug where external
snapshots that get aborted after creating an empty destination file are
leaving behind garbage, but that's pre-existing and unrelated to your
feature addition.)

> +transaction.  When I/O error make delete fail, user need to fix it later

s/make delete fail/causes deletion failure/
s/user need/the user needs/
Wayne Xia - June 17, 2013, 2:43 a.m.
Will fix the grammar, thanks for checking.


> On 06/14/2013 12:39 PM, Wenchao Xia wrote:
>> Unlike savevm, the qmp_transaction interface will not generate
>> snapshot name automatically, saving trouble to return information
>> of the new created snapshot. The snapshot name should not mess up
>> with snapshot ID, there is a check for it.
>>
>> Although qcow2 support storing multiple snapshots with same name
>> but different ID, here it will fail when an snapshot with that name
>> already exist before the operation. Format such as rbd do not support
>> ID at all, and in most case, it means trouble to user when he faces
>> multiple snapshots with same name, so ban that case.
>>
>> Snapshot ID can't be specified in this interface.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   blockdev.c       |  118 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   qapi-schema.json |   16 +++++++
>>   qmp-commands.hx  |   32 +++++++++++---
>>   3 files changed, 159 insertions(+), 7 deletions(-)
>
>> +++ b/qapi-schema.json
>> @@ -1613,6 +1613,21 @@
>>   { 'type': 'BlockdevSnapshot',
>>     'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str',
>>               '*mode': 'NewImageMode' } }
>> +##
>> +# @BlockdevSnapshotInternal
>> +#
>> +# @device: the name of the device to generate the snapshot from
>> +#
>> +# @name: the name of the internal snapshot to be created
>> +#
>> +# Notes: In transaction, if any snapshot matching @name exists, the operation
>> +#        will fail. If the name is a numeric string, it will also fail. Only
>> +#        some image format support it, for example, qcow2, rbd, and sheepdog.
>
> s/format/formats/
>
>> +#
>> +# Since: 1.6
>> +##
>> +{ 'type': 'BlockdevSnapshotInternal',
>> +  'data': { 'device': 'str', 'name': 'str' } }
>>
>>   ##
>>   # @TransactionAction
>> @@ -1623,6 +1638,7 @@
>>   { 'union': 'TransactionAction',
>>     'data': {
>>          'blockdev-snapshot-sync': 'BlockdevSnapshot'
>> +       'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal'
>
> Invalid JSON - you need a comma between name-value pairs. (I _wish_ JSON
> had allowed trailing commas, the way C99 does, because it reduces the
> size of diffs when adding an element at the end...)
   Will fix it. By the way, why the build/run can succeed without comma
in my test....

>
> Yet another instance of the introspection problem.  Is there some other
> patch in this series that introduces a standalone command that witnesses
> whether this transaction is available?  If so, I'm okay; if not, then
   You mean a command detecting whether
'blockdev-snapshot-internal-sync' is available? No I haven't introduce
it, a standalone command for 'TransactionAction' type detection seems
not so wisely, maybe integration it in qapi capability query.

> libvirt won't know if this transaction is available without
> introspection, or by trying it first and detecting the error when it is
> not available.  For the purposes of transactions, since any failure
> rolls back the entire transaction (including the failure of an unknown
> action within the transaction), the try-and-fail approach may be
> sufficient, even if it is not the prettiest.
>
>> +++ b/qmp-commands.hx
>> @@ -948,13 +948,13 @@ transaction
>>   -----------
>>
>>   Atomically operate on one or more block devices.  The only supported
>> -operation for now is snapshotting.  If there is any failure performing
>> -any of the operations, all snapshots for the group are abandoned, and
>> -the original disks pre-snapshot attempt are used.
>> +operations for now are internal and external snapshotting.  A list of
>> +dictionaries is accepted, that contains the actions to be performed.  The
>> +sequence of the requests will not affect the result.
>
> Not true.  Taking an internal and then an external snapshot has a
> different effect than taking an external and then internal snapshot of
> the same disk.  The sequence of requests is significant, because it
   In external case, the device changes its active image in commit()
stage, so it will not affect internal snapshot. For example:

Taking an internal snapshot sn0 on ide-hd0 with image0.qcow2, then
an external snapshot to create image1.qcow2, result in:
image0.qcow2(sn0)-->image1.qcow2.

Taking an external snapshot to create image1.qcow2 on ide-hd0 with 
image0.qcow2, then an external an internal snapshot sn0 on ide-hd0:
image0.qcow2(sn0)-->image1.qcow2.

   However, the comments reminds me that it maybe different to
take two snapshots sn0, sn1 on same device with different sequence:
the name looks the same, but the actually L1 table contents may
change according to the sequence, although invisible to user.
Maybe I should remove this line in document.

> controls which of two files involved contains the internal snapshot.
>
>>
>> -A list of dictionaries is accepted, that contains the actions to be performed.
>> -For snapshots this is the device, the file to use for the new snapshot,
>> -and the format.  The default format, if not specified, is qcow2.
>> +For external snapshot, The dictionary is the device, the file to use for the
>
> s/snapshot, The/snapshots, the/
> s/dictionary is/dictionary contains/
>
>> +new snapshot, and the format.  The default format, if not specified, is qcow2.
>>
>>   Each new snapshot defaults to being created by QEMU (wiping any
>>   contents if the file already exists), but it is also possible to reuse
>> @@ -963,6 +963,18 @@ the new image file has the same contents as the current one; QEMU cannot
>>   perform any meaningful check.  Typically this is achieved by using the
>>   current image file as the backing file for the new image.
>>
>> +On fail the original disks pre-snapshot attempt will be used.
>> +
>> +For internal snapshot, The dictionary is the device and the snapshot's name.
>
> s/snapshot, The/snapsohts, the/
> s/dictionary is/dictionary contains/
>
>> +If name is a numeric string which will mess up with ID, the request will be
>> +rejected.  For example, name "99" is not a valid name.  If an internal snapshot
>> +matching name already exists, the request will be also rejected.  Only some
>> +image format support it, for example, qcow2, rbd, and sheepdog.
>> +
>> +On fail, qemu will try delete new created internal snapshot in the
>
> s/fail/failure/
>
> why "try"?  The point of a transaction is that it either completely
> happens or is completely aborted.  If you are leaving behind garbage
> after a failure later in the transaction, then you didn't design the
> transaction correctly.  (And yes, we have an existing bug where external
> snapshots that get aborted after creating an empty destination file are
> leaving behind garbage, but that's pre-existing and unrelated to your
> feature addition.)
>
   I can't find a perfect way to rollback disks, since it may I/O error.
The same thing happens to external case, try delete seems the best way.
Consider breaking qcow_snapshot_create() into 3 stage, there would not
be much improvement for same reason. The best thing I can see is make
sure it is fixable by qemu-img.

>> +transaction.  When I/O error make delete fail, user need to fix it later
>
> s/make delete fail/causes deletion failure/
> s/user need/the user needs/
>
Stefan Hajnoczi - June 18, 2013, 2:09 p.m.
On Fri, Jun 14, 2013 at 07:39:54PM +0800, Wenchao Xia wrote:
> +    /* Forbid having a name similar to id, empty name is also forbidden. */
> +    if (!snapshot_name_wellformed(name)) {
> +        error_setg(errp, "Name %s on device %s is not a valid one",

Please use '%s' instead of just %s for arguments in error messages.
This will make the message easier to understand, especially when name is
empty.  It's also consistent with the rest of blockdev.c.

> +                   name, device);
> +        return;
> +    }
> +
> +    /* 3. take the snapshot */
> +    sn1 = &state->sn;
> +    pstrcpy(sn1->name, sizeof(sn1->name), name);
> +    qemu_gettimeofday(&tv);
> +    sn1->date_sec = tv.tv_sec;
> +    sn1->date_nsec = tv.tv_usec * 1000;
> +    /* not good to use vm_clock in block layer, but that is what we can do now,
> +       or drop it later, since it is an emulater concept. */

It's appropriate here and blockdev.c is emulator code, so you can drop
the comment.

Patch

diff --git a/blockdev.c b/blockdev.c
index 4bd6cbc..aaeb0e8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -808,6 +808,119 @@  struct BlkTransactionState {
     QSIMPLEQ_ENTRY(BlkTransactionState) entry;
 };
 
+/* internal snapshot private data */
+typedef struct InternalSnapshotState {
+    BlkTransactionState common;
+    BlockDriverState *bs;
+    QEMUSnapshotInfo sn;
+} InternalSnapshotState;
+
+static void internal_snapshot_prepare(BlkTransactionState *common,
+                                      Error **errp)
+{
+    const char *device;
+    const char *name;
+    BlockDriverState *bs;
+    QEMUSnapshotInfo sn, *sn1;
+    bool ret;
+    qemu_timeval tv;
+    BlockdevSnapshotInternal *internal;
+    InternalSnapshotState *state;
+
+    g_assert(common->action->kind ==
+             TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC);
+    internal = common->action->blockdev_snapshot_internal_sync;
+    state = DO_UPCAST(InternalSnapshotState, common, common);
+
+    /* 1. parse input */
+    device = internal->device;
+    name = internal->name;
+
+    /* 2. check for validation */
+    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_is_read_only(bs)) {
+        error_set(errp, QERR_DEVICE_IS_READ_ONLY, device);
+        return;
+    }
+
+    if (!bdrv_can_snapshot(bs)) {
+        error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
+                  bs->drv->format_name, device, "internal snapshot");
+        return;
+    }
+
+    /* check whether a snapshot with name exist, no need to check id, since
+       name will be checked later to make sure it does not mess up with id. */
+    ret = bdrv_snapshot_find_by_id_and_name(bs, NULL, name, &sn, errp);
+    if (error_is_set(errp)) {
+        return;
+    }
+    if (ret) {
+        error_setg(errp, "Snapshot with name %s already exist on device %s",
+                   name, device);
+        return;
+    }
+
+    /* Forbid having a name similar to id, empty name is also forbidden. */
+    if (!snapshot_name_wellformed(name)) {
+        error_setg(errp, "Name %s on device %s is not a valid one",
+                   name, device);
+        return;
+    }
+
+    /* 3. take the snapshot */
+    sn1 = &state->sn;
+    pstrcpy(sn1->name, sizeof(sn1->name), name);
+    qemu_gettimeofday(&tv);
+    sn1->date_sec = tv.tv_sec;
+    sn1->date_nsec = tv.tv_usec * 1000;
+    /* not good to use vm_clock in block layer, but that is what we can do now,
+       or drop it later, since it is an emulater concept. */
+    sn1->vm_clock_nsec = qemu_get_clock_ns(vm_clock);
+
+    if (bdrv_snapshot_create(bs, sn1) < 0) {
+        error_setg(errp, "Failed to create snapshot %s on device %s",
+                   name, device);
+        return;
+    }
+
+    /* 4. succeed, mark a snapshot is created */
+    state->bs = bs;
+}
+
+static void internal_snapshot_abort(BlkTransactionState *common)
+{
+    InternalSnapshotState *state =
+                             DO_UPCAST(InternalSnapshotState, common, common);
+    BlockDriverState *bs = state->bs;
+    QEMUSnapshotInfo *sn = &state->sn;
+    Error *local_error = NULL;
+
+    if (!bs) {
+        return;
+    }
+
+    if (bdrv_snapshot_delete(bs, sn->id_str, sn->name, &local_error) < 0) {
+        error_report("Failed to delete snapshot with id %s and name %s on "
+                     "device %s in abort, reason is: %s",
+                     sn->id_str,
+                     sn->name,
+                     bdrv_get_device_name(bs),
+                     error_get_pretty(local_error));
+        error_free(local_error);
+    }
+}
+
 /* external snapshot private data */
 typedef struct ExternalSnapshotState {
     BlkTransactionState common;
@@ -926,6 +1039,11 @@  static const BdrvActionOps actions[] = {
         .commit   = external_snapshot_commit,
         .abort = external_snapshot_abort,
     },
+    [TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC] = {
+        .instance_size = sizeof(InternalSnapshotState),
+        .prepare  = internal_snapshot_prepare,
+        .abort = internal_snapshot_abort,
+    },
 };
 
 /*
diff --git a/qapi-schema.json b/qapi-schema.json
index 5ad6894..c53cb31 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1613,6 +1613,21 @@ 
 { 'type': 'BlockdevSnapshot',
   'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str',
             '*mode': 'NewImageMode' } }
+##
+# @BlockdevSnapshotInternal
+#
+# @device: the name of the device to generate the snapshot from
+#
+# @name: the name of the internal snapshot to be created
+#
+# Notes: In transaction, if any snapshot matching @name exists, the operation
+#        will fail. If the name is a numeric string, it will also fail. Only
+#        some image format support it, for example, qcow2, rbd, and sheepdog.
+#
+# Since: 1.6
+##
+{ 'type': 'BlockdevSnapshotInternal',
+  'data': { 'device': 'str', 'name': 'str' } }
 
 ##
 # @TransactionAction
@@ -1623,6 +1638,7 @@ 
 { 'union': 'TransactionAction',
   'data': {
        'blockdev-snapshot-sync': 'BlockdevSnapshot'
+       'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal'
    } }
 
 ##
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 8cea5e5..db5d4e3 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -948,13 +948,13 @@  transaction
 -----------
 
 Atomically operate on one or more block devices.  The only supported
-operation for now is snapshotting.  If there is any failure performing
-any of the operations, all snapshots for the group are abandoned, and
-the original disks pre-snapshot attempt are used.
+operations for now are internal and external snapshotting.  A list of
+dictionaries is accepted, that contains the actions to be performed.  The
+sequence of the requests will not affect the result.  If there is any failure
+performing any of the operations, all operations for the group are abandoned.
 
-A list of dictionaries is accepted, that contains the actions to be performed.
-For snapshots this is the device, the file to use for the new snapshot,
-and the format.  The default format, if not specified, is qcow2.
+For external snapshot, The dictionary is the device, the file to use for the
+new snapshot, and the format.  The default format, if not specified, is qcow2.
 
 Each new snapshot defaults to being created by QEMU (wiping any
 contents if the file already exists), but it is also possible to reuse
@@ -963,6 +963,18 @@  the new image file has the same contents as the current one; QEMU cannot
 perform any meaningful check.  Typically this is achieved by using the
 current image file as the backing file for the new image.
 
+On fail the original disks pre-snapshot attempt will be used.
+
+For internal snapshot, The dictionary is the device and the snapshot's name.
+If name is a numeric string which will mess up with ID, the request will be
+rejected.  For example, name "99" is not a valid name.  If an internal snapshot
+matching name already exists, the request will be also rejected.  Only some
+image format support it, for example, qcow2, rbd, and sheepdog.
+
+On fail, qemu will try delete new created internal snapshot in the
+transaction.  When I/O error make delete fail, user need to fix it later
+with qemu-img or other command.
+
 Arguments:
 
 actions array:
@@ -975,6 +987,9 @@  actions array:
       - "format": format of new image (json-string, optional)
       - "mode": whether and how QEMU should create the snapshot file
         (NewImageMode, optional, default "absolute-paths")
+      When "type" is "blockdev-snapshot-internal-sync":
+      - "device": device name to snapshot (json-string)
+      - "name": name of the new snapshot (json-string)
 
 Example:
 
@@ -986,7 +1001,10 @@  Example:
          { 'type': 'blockdev-snapshot-sync', 'data' : { "device": "ide-hd1",
                                          "snapshot-file": "/some/place/my-image2",
                                          "mode": "existing",
-                                         "format": "qcow2" } } ] } }
+                                         "format": "qcow2" } },
+         { 'type': 'blockdev-snapshot-internal-sync', 'data' : {
+                                         "device": "ide-hd2",
+                                         "name": "snapshot0" } } ] } }
 <- { "return": {} }
 
 EQMP