[Qemu-devel] [PATCH v2 0/6] Mirrored writes using blockdev-transaction

[Qemu-devel] [PATCH v2 0/6] Mirrored writes using blockdev-transaction

[Paolo Bonzini]

This implements all ingredients to establish mirrored writes.
The drive-reopen command that is used to terminate mirrored writes
is not included in this series.

Tested with the following scenarios:

a) mirror only

1) create base.qcow2 and start QEMU with it

2) Execute the following QMP command

{ "execute": "qmp_capabilities" }
{ "execute": "blockdev-transaction", "arguments":
  {'actions': [
    { 'type': 'mirror', 'data' :
      { 'device': 'ide0-hd0', 'target': '/home/pbonzini/mirror.qcow2' } } ] } }
{ "execute": "cont" }

3) hibernate the guest (this requires an IDE disk and -cpu kvm64,-kvmclock)

4) restart the guest with mirror.qcow2


b) atomic snapshot+mirror

1) start QEMU with an existing image test.img

2) Execute the following QMP command

{ "execute": "qmp_capabilities" }
{ "execute": "blockdev-transaction", "arguments":
  {'actions': [
    { 'type': 'snapshot', 'data' :
      { 'device': 'ide0-hd0', 'snapshot-file': '/home/pbonzini/base.qcow2' } },
    { 'type': 'mirror', 'data' :
      { 'device': 'ide0-hd0', 'target': '/home/pbonzini/mirror.qcow2' } } ] } }
{ "execute": "cont" }

3) hibernate the guest (this requires an IDE disk and -cpu kvm64,-kvmclock)

4) check that mirror.qcow2 has test.img as the base

5) restart the guest with base.qcow2

6) restart the guest with mirror.qcow2

v1->v2:
	Removed wrong assertions in patches 3 and 6
	Move new_image_file and other variables inside the while loop (Federico)
	Rename new_source_file to new_source (Federico)
	Fix image name in error message (Federico)
	Fixed documentation in qapi-schema.json (Federico)
	Fixed documentation and example in qmp-commands.hx (Eric)
	Added back AIO (Stefan), with fixes to cancellation


Marcelo Tosatti (1):
  Add blkmirror block driver

Paolo Bonzini (5):
  fix format name for backing file
  qapi: complete implementation of unions
  rename blockdev-group-snapshot-sync
  add reuse field
  add mirroring to blockdev-transaction

 Makefile.objs             |    2 +-
 block/blkmirror.c         |  239 +++++++++++++++++++++++++++++++++++++++++++++
 blockdev.c                |  114 ++++++++++++++--------
 qapi-schema-test.json     |   10 ++
 qapi-schema.json          |   51 ++++++++--
 qmp-commands.hx           |   68 +++++++++----
 scripts/qapi-types.py     |    5 +
 scripts/qapi-visit.py     |   31 ++++++-
 test-qmp-input-visitor.c  |   18 ++++
 test-qmp-output-visitor.c |   34 +++++++
 10 files changed, 496 insertions(+), 76 deletions(-)
 create mode 100644 block/blkmirror.c

-- 
1.7.7.6

[Qemu-devel] [PATCH v2 1/6] fix format name for backing file

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 blockdev.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index d78aa51..96a893b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -800,7 +800,8 @@ void qmp_blockdev_group_snapshot_sync(SnapshotDevList *dev_list,
         /* create new image w/backing file */
         ret = bdrv_img_create(snapshot_file, format,
                               states->old_bs->filename,
-                              drv->format_name, NULL, -1, flags);
+                              states->old_bs->drv->format_name,
+                              NULL, -1, flags);
         if (ret) {
             error_set(errp, QERR_OPEN_FILE_FAILED, snapshot_file);
             goto delete_and_fail;
-- 
1.7.7.6

[Qemu-devel] [PATCH v2 2/6] qapi: complete implementation of unions

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qapi-schema-test.json     |   10 ++++++++++
 scripts/qapi-types.py     |    5 +++++
 scripts/qapi-visit.py     |   31 ++++++++++++++++++++++++++++++-
 test-qmp-input-visitor.c  |   18 ++++++++++++++++++
 test-qmp-output-visitor.c |   34 ++++++++++++++++++++++++++++++++++
 5 files changed, 97 insertions(+), 1 deletions(-)

diff --git a/qapi-schema-test.json b/qapi-schema-test.json
index 2b38919..8c7f9f7 100644
--- a/qapi-schema-test.json
+++ b/qapi-schema-test.json
@@ -22,6 +22,16 @@
                        'dict2': { 'userdef1': 'UserDefOne', 'string2': 'str' },
                        '*dict3': { 'userdef2': 'UserDefOne', 'string3': 'str' } } } }
 
+# for testing unions
+{ 'type': 'UserDefA',
+  'data': { 'boolean': 'bool' } }
+
+{ 'type': 'UserDefB',
+  'data': { 'integer': 'int' } }
+
+{ 'union': 'UserDefUnion',
+  'data': { 'a' : 'UserDefA', 'b' : 'UserDefB' } }
+
 # testing commands
 { 'command': 'user_def_cmd', 'data': {} }
 { 'command': 'user_def_cmd1', 'data': {'ud1a': 'UserDefOne'} }
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index b56225b..6968e7f 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -269,6 +269,7 @@ for expr in exprs:
     elif expr.has_key('union'):
         ret += generate_fwd_struct(expr['union'], expr['data']) + "\n"
         ret += generate_enum('%sKind' % expr['union'], expr['data'].keys())
+        fdef.write(generate_enum_lookup('%sKind' % expr['union'], expr['data'].keys()))
     else:
         continue
     fdecl.write(ret)
@@ -283,6 +284,10 @@ for expr in exprs:
         fdef.write(generate_type_cleanup(expr['type']) + "\n")
     elif expr.has_key('union'):
         ret += generate_union(expr['union'], expr['data'])
+        ret += generate_type_cleanup_decl(expr['union'] + "List")
+        fdef.write(generate_type_cleanup(expr['union'] + "List") + "\n")
+        ret += generate_type_cleanup_decl(expr['union'])
+        fdef.write(generate_type_cleanup(expr['union']) + "\n")
     else:
         continue
     fdecl.write(ret)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 5160d83..54117d4 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -110,10 +110,38 @@ def generate_visit_union(name, members):
 
 void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp)
 {
-}
+    Error *err = NULL;
+
+    visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err);
+    visit_type_%(name)sKind(m, &(*obj)->kind, "type", &err);
+    if (err) {
+        error_propagate(errp, err);
+        goto end;
+    }
+    switch ((*obj)->kind) {
 ''',
                  name=name)
 
+    for key in members:
+        ret += mcgen('''
+    case %(abbrev)s_KIND_%(enum)s:
+        visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "data", errp);
+        break;
+''',
+                abbrev = de_camel_case(name).upper(),
+                enum = de_camel_case(key).upper(),
+                c_type=members[key],
+                c_name=c_var(key))
+
+    ret += mcgen('''
+    default:
+        abort();
+    }
+end:
+    visit_end_struct(m, errp);
+}
+''')
+
     return ret
 
 def generate_declaration(name, members, genlist=True):
@@ -242,6 +270,7 @@ for expr in exprs:
         fdecl.write(ret)
     elif expr.has_key('union'):
         ret = generate_visit_union(expr['union'], expr['data'])
+        ret += generate_visit_list(expr['union'], expr['data'])
         fdef.write(ret)
 
         ret = generate_decl_enum('%sKind' % expr['union'], expr['data'].keys())
diff --git a/test-qmp-input-visitor.c b/test-qmp-input-visitor.c
index 926db5c..1996e49 100644
--- a/test-qmp-input-visitor.c
+++ b/test-qmp-input-visitor.c
@@ -234,6 +234,22 @@ static void test_visitor_in_list(TestInputVisitorData *data,
     qapi_free_UserDefOneList(head);
 }
 
+static void test_visitor_in_union(TestInputVisitorData *data,
+                                  const void *unused)
+{
+    Visitor *v;
+    Error *err = NULL;
+    UserDefUnion *tmp;
+
+    v = visitor_input_test_init(data, "{ 'type': 'b', 'data' : { 'integer': 42 } }");
+
+    visit_type_UserDefUnion(v, &tmp, NULL, &err);
+    g_assert(err == NULL);
+    g_assert_cmpint(tmp->kind, ==, USER_DEF_UNION_KIND_B);
+    g_assert_cmpint(tmp->b->integer, ==, 42);
+    qapi_free_UserDefUnion(tmp);
+}
+
 static void input_visitor_test_add(const char *testpath,
                                    TestInputVisitorData *data,
                                    void (*test_func)(TestInputVisitorData *data, const void *user_data))
@@ -264,6 +280,8 @@ int main(int argc, char **argv)
                             &in_visitor_data, test_visitor_in_struct_nested);
     input_visitor_test_add("/visitor/input/list",
                             &in_visitor_data, test_visitor_in_list);
+    input_visitor_test_add("/visitor/input/union",
+                            &in_visitor_data, test_visitor_in_union);
 
     g_test_run();
 
diff --git a/test-qmp-output-visitor.c b/test-qmp-output-visitor.c
index c94c208..78e5f2d 100644
--- a/test-qmp-output-visitor.c
+++ b/test-qmp-output-visitor.c
@@ -380,6 +380,38 @@ static void test_visitor_out_list_qapi_free(TestOutputVisitorData *data,
     qapi_free_UserDefNestedList(head);
 }
 
+static void test_visitor_out_union(TestOutputVisitorData *data,
+                                   const void *unused)
+{
+    QObject *arg, *qvalue;
+    QDict *qdict, *value;
+
+    Error *err = NULL;
+
+    UserDefUnion *tmp = g_malloc0(sizeof(UserDefUnion));
+    tmp->kind = USER_DEF_UNION_KIND_A;
+    tmp->a = g_malloc0(sizeof(UserDefA));
+    tmp->a->boolean = true;
+
+    visit_type_UserDefUnion(data->ov, &tmp, NULL, &err);
+    g_assert(err == NULL);
+    arg = qmp_output_get_qobject(data->qov);
+
+    g_assert(qobject_type(arg) == QTYPE_QDICT);
+    qdict = qobject_to_qdict(arg);
+
+    g_assert_cmpstr(qdict_get_str(qdict, "type"), ==, "a");
+
+    qvalue = qdict_get(qdict, "data");
+    g_assert(data != NULL);
+    g_assert(qobject_type(qvalue) == QTYPE_QDICT);
+    value = qobject_to_qdict(qvalue);
+    g_assert_cmpint(qdict_get_bool(value, "boolean"), ==, true);
+
+    qapi_free_UserDefUnion(tmp);
+    QDECREF(qdict);
+}
+
 static void output_visitor_test_add(const char *testpath,
                                     TestOutputVisitorData *data,
                                     void (*test_func)(TestOutputVisitorData *data, const void *user_data))
@@ -416,6 +448,8 @@ int main(int argc, char **argv)
                             &out_visitor_data, test_visitor_out_list);
     output_visitor_test_add("/visitor/output/list-qapi-free",
                             &out_visitor_data, test_visitor_out_list_qapi_free);
+    output_visitor_test_add("/visitor/output/union",
+                            &out_visitor_data, test_visitor_out_union);
 
     g_test_run();
 
-- 
1.7.7.6

[Qemu-devel] [PATCH v2 3/6] rename blockdev-group-snapshot-sync

[Paolo Bonzini]

We will add other kinds of operation.  Prepare for this by adjusting
the schema.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 blockdev.c       |   79 ++++++++++++++++++++++++++++-------------------------
 qapi-schema.json |   33 +++++++++++++++-------
 qmp-commands.hx  |   50 ++++++++++++++++++---------------
 3 files changed, 91 insertions(+), 71 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 96a893b..1aa544a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -716,31 +716,25 @@ void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
 
 
 /* New and old BlockDriverState structs for group snapshots */
-typedef struct BlkGroupSnapshotStates {
+typedef struct BlkTransactionStates {
     BlockDriverState *old_bs;
     BlockDriverState *new_bs;
-    QSIMPLEQ_ENTRY(BlkGroupSnapshotStates) entry;
-} BlkGroupSnapshotStates;
+    QSIMPLEQ_ENTRY(BlkTransactionStates) entry;
+} BlkTransactionStates;
 
 /*
  * 'Atomic' group snapshots.  The snapshots are taken as a set, and if any fail
  *  then we do not pivot any of the devices in the group, and abandon the
  *  snapshots
  */
-void qmp_blockdev_group_snapshot_sync(SnapshotDevList *dev_list,
-                                      Error **errp)
+void qmp_blockdev_transaction(BlockdevActionList *dev_list,
+                              Error **errp)
 {
     int ret = 0;
-    SnapshotDevList *dev_entry = dev_list;
-    SnapshotDev *dev_info = NULL;
-    BlkGroupSnapshotStates *states;
-    BlockDriver *proto_drv;
-    BlockDriver *drv;
-    int flags;
-    const char *format;
-    const char *snapshot_file;
+    BlockdevActionList *dev_entry = dev_list;
+    BlkTransactionStates *states;
 
-    QSIMPLEQ_HEAD(snap_bdrv_states, BlkGroupSnapshotStates) snap_bdrv_states;
+    QSIMPLEQ_HEAD(snap_bdrv_states, BlkTransactionStates) snap_bdrv_states;
     QSIMPLEQ_INIT(&snap_bdrv_states);
 
     /* drain all i/o before any snapshots */
@@ -748,21 +742,46 @@ void qmp_blockdev_group_snapshot_sync(SnapshotDevList *dev_list,
 
     /* We don't do anything in this loop that commits us to the snapshot */
     while (NULL != dev_entry) {
+        BlockdevAction *dev_info = NULL;
+        BlockDriver *proto_drv;
+        BlockDriver *drv;
+        int flags;
+        const char *device;
+        const char *format = "qcow2";
+        const char *new_image_file = NULL;
+
         dev_info = dev_entry->value;
         dev_entry = dev_entry->next;
 
-        states = g_malloc0(sizeof(BlkGroupSnapshotStates));
+        states = g_malloc0(sizeof(BlkTransactionStates));
         QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, states, entry);
 
-        states->old_bs = bdrv_find(dev_info->device);
+        switch (dev_info->kind) {
+        case BLOCKDEV_ACTION_KIND_SNAPSHOT:
+            device = dev_info->snapshot->device;
+            if (dev_info->snapshot->has_format) {
+                format = dev_info->snapshot->format;
+            }
+            new_image_file = dev_info->snapshot->snapshot_file;
+            break;
+        default:
+            abort();
+        }
+
+        drv = bdrv_find_format(format);
+        if (!drv) {
+            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
+            goto delete_and_fail;
+        }
 
+        states->old_bs = bdrv_find(device);
         if (!states->old_bs) {
-            error_set(errp, QERR_DEVICE_NOT_FOUND, dev_info->device);
+            error_set(errp, QERR_DEVICE_NOT_FOUND, device);
             goto delete_and_fail;
         }
 
         if (bdrv_in_use(states->old_bs)) {
-            error_set(errp, QERR_DEVICE_IN_USE, dev_info->device);
+            error_set(errp, QERR_DEVICE_IN_USE, device);
             goto delete_and_fail;
         }
 
@@ -775,44 +794,30 @@ void qmp_blockdev_group_snapshot_sync(SnapshotDevList *dev_list,
             }
         }
 
-        snapshot_file = dev_info->snapshot_file;
-
         flags = states->old_bs->open_flags;
 
-        if (!dev_info->has_format) {
-            format = "qcow2";
-        } else {
-            format = dev_info->format;
-        }
-
-        drv = bdrv_find_format(format);
-        if (!drv) {
-            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
-            goto delete_and_fail;
-        }
-
-        proto_drv = bdrv_find_protocol(snapshot_file);
+        proto_drv = bdrv_find_protocol(new_image_file);
         if (!proto_drv) {
             error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
             goto delete_and_fail;
         }
 
         /* create new image w/backing file */
-        ret = bdrv_img_create(snapshot_file, format,
+        ret = bdrv_img_create(new_image_file, format,
                               states->old_bs->filename,
                               states->old_bs->drv->format_name,
                               NULL, -1, flags);
         if (ret) {
-            error_set(errp, QERR_OPEN_FILE_FAILED, snapshot_file);
+            error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
             goto delete_and_fail;
         }
 
         /* We will manually add the backing_hd field to the bs later */
         states->new_bs = bdrv_new("");
-        ret = bdrv_open(states->new_bs, snapshot_file,
+        ret = bdrv_open(states->new_bs, new_image_file,
                         flags | BDRV_O_NO_BACKING, drv);
         if (ret != 0) {
-            error_set(errp, QERR_OPEN_FILE_FAILED, snapshot_file);
+            error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
             goto delete_and_fail;
         }
     }
diff --git a/qapi-schema.json b/qapi-schema.json
index 5f293c4..d2fbee3 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1118,7 +1118,7 @@
 { 'command': 'block_resize', 'data': { 'device': 'str', 'size': 'int' }}
 
 ##
-# @SnapshotDev
+# @BlockdevSnapshot
 #
 # @device:  the name of the device to generate the snapshot from.
 #
@@ -1126,19 +1126,30 @@
 #
 # @format: #optional the format of the snapshot image, default is 'qcow2'.
 ##
-{ 'type': 'SnapshotDev',
-  'data': {'device': 'str', 'snapshot-file': 'str', '*format': 'str' } }
+{ 'type': 'BlockdevSnapshot',
+  'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str' } }
+
+##
+# @BlockdevAction
+#
+# A discriminated record of operations that can be performed with
+# @blockdev-transaction.
+##
+{ 'union': 'BlockdevAction',
+  'data': {
+       'snapshot': 'BlockdevSnapshot'
+   } }
 
 ##
-# @blockdev-group-snapshot-sync
+# @blockdev-transaction
 #
-# Generates a synchronous snapshot of a group of one or more block devices,
-# as atomically as possible.  If the snapshot of any device in the group
-# fails, then the entire group snapshot will be abandoned and the
-# appropriate error returned.
+# Atomically operate on a group of one or more block devices.  If
+# any operation fails, then the entire set of actions will be
+# abandoned and the appropriate error returned.  The only operation
+# supported is currently snapshot.
 #
 #  List of:
-#  @SnapshotDev: information needed for the device snapshot
+#  @BlockdevAction: information needed for the device snapshot
 #
 # Returns: nothing on success
 #          If @device is not a valid block device, DeviceNotFound
@@ -1152,8 +1163,8 @@
 # returned in an error condition, and subsequent devices will not have been
 # attempted.
 ##
-{ 'command': 'blockdev-group-snapshot-sync',
-  'data': { 'devlist': [ 'SnapshotDev' ] } }
+{ 'command': 'blockdev-transaction',
+  'data': { 'actions': [ 'BlockdevAction' ] } }
 
 ##
 # @blockdev-snapshot-sync
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 0c9bfac..71d4825 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -687,41 +687,45 @@ EQMP
         .mhandler.cmd_new = qmp_marshal_input_block_job_cancel,
     },
     {
-        .name       = "blockdev-group-snapshot-sync",
-        .args_type  = "devlist:O",
-        .params  = "device:B,snapshot-file:s,format:s?",
-        .mhandler.cmd_new = qmp_marshal_input_blockdev_group_snapshot_sync,
+        .name       = "blockdev-transaction",
+        .args_type  = "actions:O",
+        .mhandler.cmd_new = qmp_marshal_input_blockdev_transaction,
     },
 
 SQMP
-blockdev-group-snapshot-sync
+blockdev-transaction
 ----------------------
 
-Synchronous snapshot of one or more block devices.  A list array input
-is accepted, that contains the device and snapshot file information for
-each device in group. The default format, if not specified, is qcow2.
-
-If there is any failure creating or opening a new snapshot, all snapshots
-for the group are abandoned, and the original disks pre-snapshot attempt
-are used.
+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.
 
+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.
 
 Arguments:
 
-devlist array:
-    - "device": device name to snapshot (json-string)
-    - "snapshot-file": name of new image file (json-string)
-    - "format": format of new image (json-string, optional)
+actions array:
+    - "type": the operation to perform.  The only supported
+      value is "snapshot". (json-string)
+    - "data": a dictionary.  The contents depend on the value
+      of "type".  When "type" is "snapshot":
+      - "device": device name to snapshot (json-string)
+      - "snapshot-file": name of new image file (json-string)
+      - "format": format of new image (json-string, optional)
 
 Example:
 
--> { "execute": "blockdev-group-snapshot-sync", "arguments":
-                      { "devlist": [{ "device": "ide-hd0",
-                                      "snapshot-file": "/some/place/my-image",
-                                      "format": "qcow2" },
-                                    { "device": "ide-hd1",
-                                      "snapshot-file": "/some/place/my-image2",
-                                      "format": "qcow2" }] } }
+-> { "execute": "blockdev-transaction",
+     "arguments": { "actions": [
+         { 'type': 'snapshot, 'data' : { "device": "ide-hd0",
+                                         "snapshot-file": "/some/place/my-image",
+                                         "format": "qcow2" } },
+         { 'type': 'snapshot, 'data' : { "device": "ide-hd1",
+                                         "snapshot-file": "/some/place/my-image2",
+                                         "format": "qcow2" } } ] } }
 <- { "return": {} }
 
 EQMP
-- 
1.7.7.6

[Qemu-devel] [PATCH v2 4/6] add reuse field

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 blockdev.c       |   18 +++++++++++-------
 qapi-schema.json |    3 ++-
 qmp-commands.hx  |   10 ++++++++++
 3 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 1aa544a..8fcdf0e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -749,6 +749,7 @@ void qmp_blockdev_transaction(BlockdevActionList *dev_list,
         const char *device;
         const char *format = "qcow2";
         const char *new_image_file = NULL;
+        bool do_snapshot;
 
         dev_info = dev_entry->value;
         dev_entry = dev_entry->next;
@@ -760,6 +761,7 @@ void qmp_blockdev_transaction(BlockdevActionList *dev_list,
         switch (dev_info->kind) {
         case BLOCKDEV_ACTION_KIND_SNAPSHOT:
             device = dev_info->snapshot->device;
+            do_snapshot = !dev_info->snapshot->has_reuse || !dev_info->snapshot->reuse;
             if (dev_info->snapshot->has_format) {
                 format = dev_info->snapshot->format;
             }
@@ -803,13 +805,15 @@ void qmp_blockdev_transaction(BlockdevActionList *dev_list,
         }
 
         /* create new image w/backing file */
-        ret = bdrv_img_create(new_image_file, format,
-                              states->old_bs->filename,
-                              states->old_bs->drv->format_name,
-                              NULL, -1, flags);
-        if (ret) {
-            error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
-            goto delete_and_fail;
+        if (do_snapshot) {
+            ret = bdrv_img_create(new_image_file, format,
+                                  states->old_bs->filename,
+                                  states->old_bs->drv->format_name,
+                                  NULL, -1, flags);
+            if (ret) {
+                error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
+                goto delete_and_fail;
+            }
         }
 
         /* We will manually add the backing_hd field to the bs later */
diff --git a/qapi-schema.json b/qapi-schema.json
index d2fbee3..78df122 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1127,7 +1127,8 @@
 # @format: #optional the format of the snapshot image, default is 'qcow2'.
 ##
 { 'type': 'BlockdevSnapshot',
-  'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str' } }
+  'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str',
+            '*reuse': 'bool' } }
 
 ##
 # @BlockdevAction
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 71d4825..817b689 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -705,6 +705,13 @@ 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.
 
+Each new snapshot defaults to being created by QEMU (wiping any
+contents if the file already exists), but it is also possible to reuse
+an externally-created file.  In the latter case, you should ensure that
+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.
+
 Arguments:
 
 actions array:
@@ -715,6 +722,8 @@ actions array:
       - "device": device name to snapshot (json-string)
       - "snapshot-file": name of new image file (json-string)
       - "format": format of new image (json-string, optional)
+      - "reuse": whether QEMU should look for an existing image file
+        (json-bool, optional, default false)
 
 Example:
 
@@ -725,6 +734,7 @@ Example:
                                          "format": "qcow2" } },
          { 'type': 'snapshot, 'data' : { "device": "ide-hd1",
                                          "snapshot-file": "/some/place/my-image2",
+                                         "reuse": true,
                                          "format": "qcow2" } } ] } }
 <- { "return": {} }
 
-- 
1.7.7.6

[Qemu-devel] [PATCH v2 5/6] Add blkmirror block driver

[Paolo Bonzini]

From: Marcelo Tosatti <mtosatti@redhat.com>

Mirrored writes are used by live block copy.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
Signed-off-by: Federico Simoncelli <fsimonce@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 Makefile.objs     |    2 +-
 block/blkmirror.c |  239 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 240 insertions(+), 1 deletions(-)
 create mode 100644 block/blkmirror.c

diff --git a/Makefile.objs b/Makefile.objs
index 808de6a..982f44b 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -74,7 +74,7 @@ fsdev-obj-$(CONFIG_VIRTFS) += $(addprefix fsdev/, $(fsdev-nested-y))
 # suppress *all* target specific code in case of system emulation, i.e. a
 # single QEMU executable should support all CPUs and machines.
 
-common-obj-y = $(block-obj-y) blockdev.o
+common-obj-y = $(block-obj-y) blockdev.o block/blkmirror.o
 common-obj-y += $(net-obj-y)
 common-obj-y += $(qobject-obj-y)
 common-obj-$(CONFIG_LINUX) += $(fsdev-obj-$(CONFIG_LINUX))
diff --git a/block/blkmirror.c b/block/blkmirror.c
new file mode 100644
index 0000000..0ee2ca6
--- /dev/null
+++ b/block/blkmirror.c
@@ -0,0 +1,239 @@
+/*
+ * Block driver for mirrored writes.
+ *
+ * Copyright (C) 2011-2012 Red Hat, Inc.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include <stdarg.h>
+#include "block_int.h"
+
+typedef struct DupAIOCB DupAIOCB;
+
+typedef struct SingleAIOCB {
+    BlockDriverAIOCB *aiocb;
+    DupAIOCB *parent;
+} SingleAIOCB;
+
+struct DupAIOCB {
+    BlockDriverAIOCB common;
+    int count;
+    int canceled;
+
+    BlockDriverCompletionFunc *cb;
+    SingleAIOCB aios[2];
+    int ret;
+};
+
+/* Valid blkmirror filenames look like blkmirror:format:path/to/target.
+ *
+ * The driver is not intended for general usage.  It expects bdrv_append
+ * to tack it onto an existing image, which is used as the primary
+ * source and hardcoded to be the backing file for the target.
+ */
+static int blkmirror_open(BlockDriverState *bs, const char *filename, int flags)
+{
+    int ret, n;
+    const char *filename2;
+    char *format;
+    BlockDriver *drv;
+
+    /* Parse the blkmirror: prefix */
+    if (strncmp(filename, "blkmirror:", strlen("blkmirror:"))) {
+        return -EINVAL;
+    }
+    filename += strlen("blkmirror:");
+
+    /* The source image filename is added by bdrv_append.  We only need
+     * to parse and open the destination image and format.  */
+    n = strcspn(filename, ":");
+    if (filename[n] == 0) {
+        format = NULL;
+        filename2 = filename;
+    } else {
+        format = g_strdup(filename);
+        format[n] = 0;
+        filename2 = format + n + 1;
+    }
+
+    drv = bdrv_find_whitelisted_format(format);
+    if (!drv) {
+        ret = -ENOENT;
+        goto out;
+    }
+
+    bs->file = bdrv_new("");
+    if (bs->file == NULL) {
+        ret = -ENOMEM;
+        goto out;
+    }
+
+    /* If we crash, we cannot assume that the destination is a
+     * valid mirror and we have to start over.  So speed up things
+     * by effectively operating on the destination in cache=unsafe
+     * mode.
+     */
+    ret = bdrv_open(bs->file, filename2,
+                    flags | BDRV_O_NO_BACKING | BDRV_O_NO_FLUSH | BDRV_O_CACHE_WB,
+                    drv);
+    if (ret < 0) {
+        goto out;
+    }
+
+out:
+    g_free(format);
+    return ret;
+}
+
+static void blkmirror_close(BlockDriverState *bs)
+{
+    bs->file->backing_hd = NULL;
+
+    /* backing_hd and file closed by the caller.  */
+}
+
+static coroutine_fn int blkmirror_co_flush(BlockDriverState *bs)
+{
+    return bdrv_co_flush(bs->backing_hd);
+}
+
+static int64_t blkmirror_getlength(BlockDriverState *bs)
+{
+    return bdrv_getlength(bs->file);
+}
+
+static int coroutine_fn blkmirror_co_is_allocated(BlockDriverState *bs,
+                                                  int64_t sector_num,
+                                                  int nb_sectors, int *pnum)
+{
+    return bdrv_is_allocated(bs->file, sector_num, nb_sectors, pnum);
+}
+
+static BlockDriverAIOCB *blkmirror_aio_readv(BlockDriverState *bs,
+                                             int64_t sector_num,
+                                             QEMUIOVector *qiov, int nb_sectors,
+                                             BlockDriverCompletionFunc *cb,
+                                             void *opaque)
+{
+    return bdrv_aio_readv(bs->backing_hd, sector_num, qiov, nb_sectors,
+                          cb, opaque);
+}
+
+static void dup_aio_cancel(BlockDriverAIOCB *acb)
+{
+    DupAIOCB *dcb = container_of(acb, DupAIOCB, common);
+    int i;
+
+    dcb->canceled = true;
+    for (i = 0 ; i < 2; i++) {
+        if (dcb->aios[i].aiocb) {
+            bdrv_aio_cancel(dcb->aios[i].aiocb);
+        }
+    }
+    qemu_aio_release(dcb);
+}
+
+static AIOPool dup_aio_pool = {
+    .aiocb_size         = sizeof(DupAIOCB),
+    .cancel             = dup_aio_cancel,
+};
+
+static void blkmirror_aio_cb(void *opaque, int ret)
+{
+    SingleAIOCB *scb = opaque;
+    DupAIOCB *dcb = scb->parent;
+
+    scb->aiocb = NULL;
+
+    assert(dcb->count > 0);
+    if (ret < 0 && dcb->ret == 0) {
+        dcb->ret = ret;
+    }
+    if (--dcb->count == 0) {
+        dcb->common.cb(dcb->common.opaque, dcb->ret);
+        if (!dcb->canceled) {
+            qemu_aio_release(dcb);
+        }
+    }
+}
+
+static DupAIOCB *dup_aio_get(BlockDriverState *bs,
+                             BlockDriverCompletionFunc *cb,
+                             void *opaque)
+{
+    DupAIOCB *dcb = qemu_aio_get(&dup_aio_pool, bs, cb, opaque);
+
+    dcb->canceled = false;
+    dcb->aios[0].parent = dcb;
+    dcb->aios[1].parent = dcb;
+    dcb->count = 2;
+    dcb->ret = 0;
+    return dcb;
+}
+
+static BlockDriverAIOCB *blkmirror_aio_writev(BlockDriverState *bs,
+                                              int64_t sector_num,
+                                              QEMUIOVector *qiov,
+                                              int nb_sectors,
+                                              BlockDriverCompletionFunc *cb,
+                                              void *opaque)
+{
+    DupAIOCB *dcb = dup_aio_get(bs, cb, opaque);
+
+    /* bs->backing_hd is set after initialization.  */
+    bs->file->backing_hd = bs->backing_hd;
+
+    dcb->aios[0].aiocb = bdrv_aio_writev(bs->backing_hd, sector_num, qiov,
+                                         nb_sectors, blkmirror_aio_cb,
+                                         &dcb->aios[0]);
+    dcb->aios[1].aiocb = bdrv_aio_writev(bs->file, sector_num, qiov,
+                                         nb_sectors, blkmirror_aio_cb,
+                                         &dcb->aios[1]);
+
+    return &dcb->common;
+}
+
+static BlockDriverAIOCB *blkmirror_aio_discard(BlockDriverState *bs,
+                                               int64_t sector_num,
+                                               int nb_sectors,
+                                               BlockDriverCompletionFunc *cb,
+                                               void *opaque)
+{
+    DupAIOCB *dcb = dup_aio_get(bs, cb, opaque);
+
+    dcb->aios[0].aiocb = bdrv_aio_discard(bs->backing_hd, sector_num,
+                                          nb_sectors, blkmirror_aio_cb,
+                                          &dcb->aios[0]);
+    dcb->aios[1].aiocb = bdrv_aio_discard(bs->file, sector_num,
+                                          nb_sectors, blkmirror_aio_cb,
+                                          &dcb->aios[1]);
+
+    return &dcb->common;
+}
+
+
+static BlockDriver bdrv_blkmirror = {
+    .format_name        = "blkmirror",
+    .protocol_name      = "blkmirror",
+    .instance_size      = 0,
+
+    .bdrv_getlength     = blkmirror_getlength,
+
+    .bdrv_file_open     = blkmirror_open,
+    .bdrv_close         = blkmirror_close,
+    .bdrv_co_flush_to_disk = blkmirror_co_flush,
+    .bdrv_co_is_allocated = blkmirror_co_is_allocated,
+
+    .bdrv_aio_readv      = blkmirror_aio_readv,
+    .bdrv_aio_writev     = blkmirror_aio_writev,
+    .bdrv_aio_discard    = blkmirror_aio_discard,
+};
+
+static void bdrv_blkmirror_init(void)
+{
+    bdrv_register(&bdrv_blkmirror);
+}
+
+block_init(bdrv_blkmirror_init);
-- 
1.7.7.6

[Qemu-devel] [PATCH v2 6/6] add mirroring to blockdev-transaction

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 blockdev.c       |   42 ++++++++++++++++++++++++++++++++----------
 qapi-schema.json |   19 ++++++++++++++++++-
 qmp-commands.hx  |   12 +++++++++++-
 3 files changed, 61 insertions(+), 12 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 8fcdf0e..f181c6f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -733,6 +733,7 @@ void qmp_blockdev_transaction(BlockdevActionList *dev_list,
     int ret = 0;
     BlockdevActionList *dev_entry = dev_list;
     BlkTransactionStates *states;
+    char *new_source = NULL;
 
     QSIMPLEQ_HEAD(snap_bdrv_states, BlkTransactionStates) snap_bdrv_states;
     QSIMPLEQ_INIT(&snap_bdrv_states);
@@ -744,12 +745,12 @@ void qmp_blockdev_transaction(BlockdevActionList *dev_list,
     while (NULL != dev_entry) {
         BlockdevAction *dev_info = NULL;
         BlockDriver *proto_drv;
-        BlockDriver *drv;
+        BlockDriver *target_drv;
+        BlockDriver *drv = NULL;
         int flags;
         const char *device;
         const char *format = "qcow2";
         const char *new_image_file = NULL;
-        bool do_snapshot;
 
         dev_info = dev_entry->value;
         dev_entry = dev_entry->next;
@@ -760,21 +761,40 @@ void qmp_blockdev_transaction(BlockdevActionList *dev_list,
         switch (dev_info->kind) {
         case BLOCKDEV_ACTION_KIND_SNAPSHOT:
             device = dev_info->snapshot->device;
-            do_snapshot = !dev_info->snapshot->has_reuse || !dev_info->snapshot->reuse;
+            if (!dev_info->snapshot->has_reuse || !dev_info->snapshot->reuse) {
+                new_image_file = dev_info->snapshot->snapshot_file;
+            }
             if (dev_info->snapshot->has_format) {
                 format = dev_info->snapshot->format;
             }
-            new_image_file = dev_info->snapshot->snapshot_file;
+            new_source = g_strdup(dev_info->snapshot->snapshot_file);
             break;
+
+        case BLOCKDEV_ACTION_KIND_MIRROR:
+            device = dev_info->mirror->device;
+            drv = bdrv_find_format("blkmirror");
+            if (!dev_info->mirror->has_reuse || !dev_info->mirror->reuse) {
+                new_image_file = dev_info->mirror->target;
+            }
+            if (dev_info->mirror->has_format) {
+                format = dev_info->mirror->format;
+            }
+            new_source = g_strdup_printf("blkmirror:%s:%s", format,
+                                         dev_info->mirror->target);
+            break;
+
         default:
             abort();
         }
 
-        drv = bdrv_find_format(format);
-        if (!drv) {
+        target_drv = bdrv_find_format(format);
+        if (!target_drv) {
             error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
             goto delete_and_fail;
         }
+        if (!drv) {
+            drv = target_drv;
+        }
 
         states->old_bs = bdrv_find(device);
         if (!states->old_bs) {
@@ -798,14 +817,14 @@ void qmp_blockdev_transaction(BlockdevActionList *dev_list,
 
         flags = states->old_bs->open_flags;
 
-        proto_drv = bdrv_find_protocol(new_image_file);
+        proto_drv = bdrv_find_protocol(new_source);
         if (!proto_drv) {
             error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
             goto delete_and_fail;
         }
 
         /* create new image w/backing file */
-        if (do_snapshot) {
+        if (new_image_file) {
             ret = bdrv_img_create(new_image_file, format,
                                   states->old_bs->filename,
                                   states->old_bs->drv->format_name,
@@ -818,12 +837,14 @@ void qmp_blockdev_transaction(BlockdevActionList *dev_list,
 
         /* We will manually add the backing_hd field to the bs later */
         states->new_bs = bdrv_new("");
-        ret = bdrv_open(states->new_bs, new_image_file,
+        ret = bdrv_open(states->new_bs, new_source,
                         flags | BDRV_O_NO_BACKING, drv);
         if (ret != 0) {
-            error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
+            error_set(errp, QERR_OPEN_FILE_FAILED, new_source);
             goto delete_and_fail;
         }
+        g_free(new_source);
+        new_source = NULL;
     }
 
 
@@ -851,6 +872,7 @@ exit:
     QSIMPLEQ_FOREACH(states, &snap_bdrv_states, entry) {
         g_free(states);
     }
+    g_free(new_source);
     return;
 }
 
diff --git a/qapi-schema.json b/qapi-schema.json
index 78df122..fd5973d 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1131,6 +1131,22 @@
             '*reuse': 'bool' } }
 
 ##
+# @BlockdevMirror
+#
+# @device:  the name of the device to start mirroring.
+#
+# @target: the image that will start receiving writes for @device. A new
+#          file will be created if @reuse is absent or false.
+#
+# @format: #optional the format of the target image, default is 'qcow2'.
+#
+# @reuse: #optional whether QEMU should create a new image.
+##
+{ 'type': 'BlockdevMirror',
+  'data': { 'device': 'str', 'target': 'str', '*format': 'str',
+            '*reuse': 'bool' } }
+
+##
 # @BlockdevAction
 #
 # A discriminated record of operations that can be performed with
@@ -1138,7 +1154,8 @@
 ##
 { 'union': 'BlockdevAction',
   'data': {
-       'snapshot': 'BlockdevSnapshot'
+       'snapshot': 'BlockdevSnapshot',
+       'mirror': 'BlockdevMirror',
    } }
 
 ##
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 817b689..50ac5a0 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -701,7 +701,11 @@ 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.
 
+Mirrored writes keep the previous image file open, and start writing
+data also to the new file specified in the command.
+
 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.
 
@@ -716,7 +720,7 @@ Arguments:
 
 actions array:
     - "type": the operation to perform.  The only supported
-      value is "snapshot". (json-string)
+      values are "snapshot" and "mirror". (json-string)
     - "data": a dictionary.  The contents depend on the value
       of "type".  When "type" is "snapshot":
       - "device": device name to snapshot (json-string)
@@ -724,6 +728,12 @@ actions array:
       - "format": format of new image (json-string, optional)
       - "reuse": whether QEMU should look for an existing image file
         (json-bool, optional, default false)
+      When "type" is "mirror":
+      - "device": device name to snapshot (json-string)
+      - "target": name of destination image file (json-string)
+      - "format": format of new image (json-string, optional)
+      - "reuse": whether QEMU should look for an existing image file
+        (json-bool, optional, default false)
 
 Example:
 
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH v2 2/6] qapi: complete implementation of unions

[Kevin Wolf]

Am 01.03.2012 12:21, schrieb Paolo Bonzini:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qapi-schema-test.json     |   10 ++++++++++
>  scripts/qapi-types.py     |    5 +++++
>  scripts/qapi-visit.py     |   31 ++++++++++++++++++++++++++++++-
>  test-qmp-input-visitor.c  |   18 ++++++++++++++++++
>  test-qmp-output-visitor.c |   34 ++++++++++++++++++++++++++++++++++
>  5 files changed, 97 insertions(+), 1 deletions(-)

Makes sense to me, but before applying this patch I'd like to get an
Acked-by from Luiz.

Kevin

Re: [Qemu-devel] [PATCH v2 2/6] qapi: complete implementation of unions

[Luiz Capitulino]

On Thu, 01 Mar 2012 14:52:02 +0100
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 01.03.2012 12:21, schrieb Paolo Bonzini:
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  qapi-schema-test.json     |   10 ++++++++++
> >  scripts/qapi-types.py     |    5 +++++
> >  scripts/qapi-visit.py     |   31 ++++++++++++++++++++++++++++++-
> >  test-qmp-input-visitor.c  |   18 ++++++++++++++++++
> >  test-qmp-output-visitor.c |   34 ++++++++++++++++++++++++++++++++++
> >  5 files changed, 97 insertions(+), 1 deletions(-)
> 
> Makes sense to me, but before applying this patch I'd like to get an
> Acked-by from Luiz.

Acked-by: Luiz Capitulino <lcapitulino@redhat.com>

Re: [Qemu-devel] [PATCH v2 0/6] Mirrored writes using blockdev-transaction

[Luiz Capitulino]

On Thu,  1 Mar 2012 12:21:42 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> This implements all ingredients to establish mirrored writes.

This looks good to me. I just have two comments:

 1. I'm wondering if it would make more sense to have this transaction
    operation in qmp instead of the block layer. Looks more complex to do though,
    so I'm ok with this implementation

 2. Anthony had a different proposal for this and would be good to get input
    from him. I pinged him on irc and he said he will try to review this series
    later today

Re: [Qemu-devel] [PATCH v2 0/6] Mirrored writes using blockdev-transaction

[Kevin Wolf]

Am 01.03.2012 17:02, schrieb Luiz Capitulino:
> On Thu,  1 Mar 2012 12:21:42 +0100
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> This implements all ingredients to establish mirrored writes.
> 
> This looks good to me. I just have two comments:
> 
>  1. I'm wondering if it would make more sense to have this transaction
>     operation in qmp instead of the block layer. Looks more complex to do though,
>     so I'm ok with this implementation

Depends on what you're thinking of. Renaming the command to just
'transaction' and allowing anything to be added to the union wouldn't be
very complex.

The one thing we would need to change in order to make it generally
useful is to move the actual logic into prepare/commit/abort handlers. I
discussed this with Paolo on IRC and I think the conclusion was that for
now the approach in the patches is good enough, but in the long run
we'll switch. It doesn't affect external interfaces, so we can do it
whenever we like.

Kevin

Re: [Qemu-devel] [PATCH v2 0/6] Mirrored writes using blockdev-transaction

[Luiz Capitulino]

On Thu, 01 Mar 2012 17:18:04 +0100
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 01.03.2012 17:02, schrieb Luiz Capitulino:
> > On Thu,  1 Mar 2012 12:21:42 +0100
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> > 
> >> This implements all ingredients to establish mirrored writes.
> > 
> > This looks good to me. I just have two comments:
> > 
> >  1. I'm wondering if it would make more sense to have this transaction
> >     operation in qmp instead of the block layer. Looks more complex to do though,
> >     so I'm ok with this implementation
> 
> Depends on what you're thinking of. Renaming the command to just
> 'transaction' and allowing anything to be added to the union wouldn't be
> very complex.
> 
> The one thing we would need to change in order to make it generally
> useful is to move the actual logic into prepare/commit/abort handlers. I
> discussed this with Paolo on IRC and I think the conclusion was that for
> now the approach in the patches is good enough, but in the long run
> we'll switch. It doesn't affect external interfaces, so we can do it
> whenever we like.

Yes, that's what I called "transaction operation in qmp", but I don't mind
accepting this one and deferring the more complex idea to the future.

Re: [Qemu-devel] [PATCH v2 0/6] Mirrored writes using blockdev-transaction

[Anthony Liguori]

On 03/01/2012 05:21 AM, Paolo Bonzini wrote:
> This implements all ingredients to establish mirrored writes.
> The drive-reopen command that is used to terminate mirrored writes
> is not included in this series.
>
> Tested with the following scenarios:
>
> a) mirror only
>
> 1) create base.qcow2 and start QEMU with it
>
> 2) Execute the following QMP command
>
> { "execute": "qmp_capabilities" }
> { "execute": "blockdev-transaction", "arguments":
>    {'actions': [
>      { 'type': 'mirror', 'data' :
>        { 'device': 'ide0-hd0', 'target': '/home/pbonzini/mirror.qcow2' } } ] } }
> { "execute": "cont" }
>
> 3) hibernate the guest (this requires an IDE disk and -cpu kvm64,-kvmclock)
>
> 4) restart the guest with mirror.qcow2
>
>
> b) atomic snapshot+mirror
>
> 1) start QEMU with an existing image test.img
>
> 2) Execute the following QMP command
>
> { "execute": "qmp_capabilities" }
> { "execute": "blockdev-transaction", "arguments":
>    {'actions': [
>      { 'type': 'snapshot', 'data' :
>        { 'device': 'ide0-hd0', 'snapshot-file': '/home/pbonzini/base.qcow2' } },
>      { 'type': 'mirror', 'data' :
>        { 'device': 'ide0-hd0', 'target': '/home/pbonzini/mirror.qcow2' } } ] } }
> { "execute": "cont" }

We don't have schema introspection today.  How would one determine when new 
transaction types are available?

I think we need some sort of introspection method too in order for clients to 
figure out when the command is extended.

Regards,

Anthony Liguori

>
> 3) hibernate the guest (this requires an IDE disk and -cpu kvm64,-kvmclock)
>
> 4) check that mirror.qcow2 has test.img as the base
>
> 5) restart the guest with base.qcow2
>
> 6) restart the guest with mirror.qcow2
>
> v1->v2:
> 	Removed wrong assertions in patches 3 and 6
> 	Move new_image_file and other variables inside the while loop (Federico)
> 	Rename new_source_file to new_source (Federico)
> 	Fix image name in error message (Federico)
> 	Fixed documentation in qapi-schema.json (Federico)
> 	Fixed documentation and example in qmp-commands.hx (Eric)
> 	Added back AIO (Stefan), with fixes to cancellation
>
>
> Marcelo Tosatti (1):
>    Add blkmirror block driver
>
> Paolo Bonzini (5):
>    fix format name for backing file
>    qapi: complete implementation of unions
>    rename blockdev-group-snapshot-sync
>    add reuse field
>    add mirroring to blockdev-transaction
>
>   Makefile.objs             |    2 +-
>   block/blkmirror.c         |  239 +++++++++++++++++++++++++++++++++++++++++++++
>   blockdev.c                |  114 ++++++++++++++--------
>   qapi-schema-test.json     |   10 ++
>   qapi-schema.json          |   51 ++++++++--
>   qmp-commands.hx           |   68 +++++++++----
>   scripts/qapi-types.py     |    5 +
>   scripts/qapi-visit.py     |   31 ++++++-
>   test-qmp-input-visitor.c  |   18 ++++
>   test-qmp-output-visitor.c |   34 +++++++
>   10 files changed, 496 insertions(+), 76 deletions(-)
>   create mode 100644 block/blkmirror.c
>

Re: [Qemu-devel] [PATCH v2 0/6] Mirrored writes using blockdev-transaction

[Eric Blake]

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

On 03/01/2012 02:10 PM, Anthony Liguori wrote:
>> 2) Execute the following QMP command
>>
>> { "execute": "qmp_capabilities" }
>> { "execute": "blockdev-transaction", "arguments":
>>    {'actions': [
>>      { 'type': 'snapshot', 'data' :
>>        { 'device': 'ide0-hd0', 'snapshot-file':
>> '/home/pbonzini/base.qcow2' } },
>>      { 'type': 'mirror', 'data' :
>>        { 'device': 'ide0-hd0', 'target': '/home/pbonzini/mirror.qcow2'
>> } } ] } }
>> { "execute": "cont" }
> 
> We don't have schema introspection today.  How would one determine when
> new transaction types are available?
> 
> I think we need some sort of introspection method too in order for
> clients to figure out when the command is extended.
> 

I agree that introspection is necessary.  Up till now, libvirt could get
by with query-commands (either a command exists or it doesn't).  But now
we have the case where blockdev-transaction might exist, but doesn't
support the particular union action such as 'mirror' that libvirt wants
to use.

Could this be something we wire up to the query-commands command?

Something like:

{ 'type': 'CommandInfo', 'data': {'name': 'str', '*syntax': 'str'} }
{ 'command': 'query-commands,
  'data': { '*syntax': 'bool', '*names': ['str'] },
  'returns': ['CommandInfo'] }

where the normal {"execute":"qemu-commands"} just returns the list of
command names, but {"execute":"qemu-commands", "arguments": { "syntax":
"true", "names" : [ "blockdev-transaction" ] } } then returns:

{"return":[{"name":"blockdev-transaction", "syntax":"{ 'command' ...
}"}], "id":"..."}

that is, return back the qapi-schema.json description of the command.
Actually, I'd guess you'd also want to be able to query 'type' listings,
not just 'command's, so maybe this deserves a new monitor command rather
than shoe-horning it onto an existing 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: 620 bytes --]

Re: [Qemu-devel] [PATCH v2 0/6] Mirrored writes using blockdev-transaction

[Anthony Liguori]

On 03/01/2012 03:30 PM, Eric Blake wrote:
> On 03/01/2012 02:10 PM, Anthony Liguori wrote:
>>> 2) Execute the following QMP command
>>>
>>> { "execute": "qmp_capabilities" }
>>> { "execute": "blockdev-transaction", "arguments":
>>>     {'actions': [
>>>       { 'type': 'snapshot', 'data' :
>>>         { 'device': 'ide0-hd0', 'snapshot-file':
>>> '/home/pbonzini/base.qcow2' } },
>>>       { 'type': 'mirror', 'data' :
>>>         { 'device': 'ide0-hd0', 'target': '/home/pbonzini/mirror.qcow2'
>>> } } ] } }
>>> { "execute": "cont" }
>>
>> We don't have schema introspection today.  How would one determine when
>> new transaction types are available?
>>
>> I think we need some sort of introspection method too in order for
>> clients to figure out when the command is extended.
>>
>
> I agree that introspection is necessary.  Up till now, libvirt could get
> by with query-commands (either a command exists or it doesn't).  But now
> we have the case where blockdev-transaction might exist, but doesn't
> support the particular union action such as 'mirror' that libvirt wants
> to use.
>
> Could this be something we wire up to the query-commands command?
>
> Something like:
>
> { 'type': 'CommandInfo', 'data': {'name': 'str', '*syntax': 'str'} }
> { 'command': 'query-commands,
>    'data': { '*syntax': 'bool', '*names': ['str'] },
>    'returns': ['CommandInfo'] }
>
> where the normal {"execute":"qemu-commands"} just returns the list of
> command names, but {"execute":"qemu-commands", "arguments": { "syntax":
> "true", "names" : [ "blockdev-transaction" ] } } then returns:
>
> {"return":[{"name":"blockdev-transaction", "syntax":"{ 'command' ...
> }"}], "id":"..."}
>
> that is, return back the qapi-schema.json description of the command.
> Actually, I'd guess you'd also want to be able to query 'type' listings,
> not just 'command's, so maybe this deserves a new monitor command rather
> than shoe-horning it onto an existing one.

Yes, this is what I would like to do, but it's a requires a bit of work to 
implement.  You'd have to either install qapi-schema.json and read it at run 
time or serialize it to a C data structure that can then be used to initialize a 
QObject.

Nothing Earth shattering, just a fair amount of bit moving.

Regards,

Anthony Liguori

>

Re: [Qemu-devel] [PATCH v2 0/6] Mirrored writes using blockdev-transaction

[Anthony Liguori]

On 03/01/2012 03:36 PM, Anthony Liguori wrote:
> On 03/01/2012 03:30 PM, Eric Blake wrote:
>> On 03/01/2012 02:10 PM, Anthony Liguori wrote:
>>>> 2) Execute the following QMP command
>>>>
>>>> { "execute": "qmp_capabilities" }
>>>> { "execute": "blockdev-transaction", "arguments":
>>>> {'actions': [
>>>> { 'type': 'snapshot', 'data' :
>>>> { 'device': 'ide0-hd0', 'snapshot-file':
>>>> '/home/pbonzini/base.qcow2' } },
>>>> { 'type': 'mirror', 'data' :
>>>> { 'device': 'ide0-hd0', 'target': '/home/pbonzini/mirror.qcow2'
>>>> } } ] } }
>>>> { "execute": "cont" }
>>>
>>> We don't have schema introspection today. How would one determine when
>>> new transaction types are available?
>>>
>>> I think we need some sort of introspection method too in order for
>>> clients to figure out when the command is extended.
>>>
>>
>> I agree that introspection is necessary. Up till now, libvirt could get
>> by with query-commands (either a command exists or it doesn't). But now
>> we have the case where blockdev-transaction might exist, but doesn't
>> support the particular union action such as 'mirror' that libvirt wants
>> to use.
>>
>> Could this be something we wire up to the query-commands command?
>>
>> Something like:
>>
>> { 'type': 'CommandInfo', 'data': {'name': 'str', '*syntax': 'str'} }
>> { 'command': 'query-commands,
>> 'data': { '*syntax': 'bool', '*names': ['str'] },
>> 'returns': ['CommandInfo'] }
>>
>> where the normal {"execute":"qemu-commands"} just returns the list of
>> command names, but {"execute":"qemu-commands", "arguments": { "syntax":
>> "true", "names" : [ "blockdev-transaction" ] } } then returns:
>>
>> {"return":[{"name":"blockdev-transaction", "syntax":"{ 'command' ...
>> }"}], "id":"..."}
>>
>> that is, return back the qapi-schema.json description of the command.
>> Actually, I'd guess you'd also want to be able to query 'type' listings,
>> not just 'command's, so maybe this deserves a new monitor command rather
>> than shoe-horning it onto an existing one.
>
> Yes, this is what I would like to do, but it's a requires a bit of work to
> implement. You'd have to either install qapi-schema.json and read it at run time
> or serialize it to a C data structure that can then be used to initialize a
> QObject.
>
> Nothing Earth shattering, just a fair amount of bit moving.

BTW, in the short term, this can be addressed with careful documentation.  Just 
make it clear that in the absence of introspection, the only two supported 
operations are what's currently here.

And realize that to add new commands in the future, we need introspection first.

Regards,

Anthony Liguori

>
> Regards,
>
> Anthony Liguori
>
>>
>
>

Re: [Qemu-devel] [PATCH v2 0/6] Mirrored writes using blockdev-transaction

[Kevin Wolf]

Am 01.03.2012 22:10, schrieb Anthony Liguori:
> On 03/01/2012 05:21 AM, Paolo Bonzini wrote:
>> This implements all ingredients to establish mirrored writes.
>> The drive-reopen command that is used to terminate mirrored writes
>> is not included in this series.
>>
>> Tested with the following scenarios:
>>
>> a) mirror only
>>
>> 1) create base.qcow2 and start QEMU with it
>>
>> 2) Execute the following QMP command
>>
>> { "execute": "qmp_capabilities" }
>> { "execute": "blockdev-transaction", "arguments":
>>    {'actions': [
>>      { 'type': 'mirror', 'data' :
>>        { 'device': 'ide0-hd0', 'target': '/home/pbonzini/mirror.qcow2' } } ] } }
>> { "execute": "cont" }
>>
>> 3) hibernate the guest (this requires an IDE disk and -cpu kvm64,-kvmclock)
>>
>> 4) restart the guest with mirror.qcow2
>>
>>
>> b) atomic snapshot+mirror
>>
>> 1) start QEMU with an existing image test.img
>>
>> 2) Execute the following QMP command
>>
>> { "execute": "qmp_capabilities" }
>> { "execute": "blockdev-transaction", "arguments":
>>    {'actions': [
>>      { 'type': 'snapshot', 'data' :
>>        { 'device': 'ide0-hd0', 'snapshot-file': '/home/pbonzini/base.qcow2' } },
>>      { 'type': 'mirror', 'data' :
>>        { 'device': 'ide0-hd0', 'target': '/home/pbonzini/mirror.qcow2' } } ] } }
>> { "execute": "cont" }
> 
> We don't have schema introspection today.  How would one determine when new 
> transaction types are available?
> 
> I think we need some sort of introspection method too in order for clients to 
> figure out when the command is extended.

How about coupling the types with independently available commands for
now? We would rename 'snapshot' to 'blockdev-snapshot-sync', which does
the same thing outside of transactions. The mirror patches would then
introduce a 'drive-mirror' top-level command at the same time as they
introduce a 'drive-mirror' transaction type.

Kevin

Re: [Qemu-devel] [PATCH v2 0/6] Mirrored writes using blockdev-transaction

[Paolo Bonzini]

Il 05/03/2012 09:53, Kevin Wolf ha scritto:
>> > 
>> > I think we need some sort of introspection method too in order for clients to 
>> > figure out when the command is extended.
> How about coupling the types with independently available commands for
> now? We would rename 'snapshot' to 'blockdev-snapshot-sync', which does
> the same thing outside of transactions. The mirror patches would then
> introduce a 'drive-mirror' top-level command at the same time as they
> introduce a 'drive-mirror' transaction type.

Makes sense.  It would also be a good excuse to port
blockdev-snapshot-sync to the new fail-safe way.  I'll refresh my
patches if there's consensus.

Paolo

Re: [Qemu-devel] [PATCH v2 0/6] Mirrored writes using blockdev-transaction

[Kevin Wolf]

Am 05.03.2012 10:28, schrieb Paolo Bonzini:
> Il 05/03/2012 09:53, Kevin Wolf ha scritto:
>>>>
>>>> I think we need some sort of introspection method too in order for clients to 
>>>> figure out when the command is extended.
>> How about coupling the types with independently available commands for
>> now? We would rename 'snapshot' to 'blockdev-snapshot-sync', which does
>> the same thing outside of transactions. The mirror patches would then
>> introduce a 'drive-mirror' top-level command at the same time as they
>> introduce a 'drive-mirror' transaction type.
> 
> Makes sense.  It would also be a good excuse to port
> blockdev-snapshot-sync to the new fail-safe way.  I'll refresh my
> patches if there's consensus.

Will you also replace 'blockdev-transaction' by 'transaction' to keep
things generic at the interface level? The necessary refactoring to
actually make it useful outside the block layer can come later, but we
can leave the external API stable then.

Kevin

Re: [Qemu-devel] [PATCH v2 0/6] Mirrored writes using blockdev-transaction

[Paolo Bonzini]

Il 05/03/2012 13:13, Kevin Wolf ha scritto:
>> Makes sense.  It would also be a good excuse to port
>> blockdev-snapshot-sync to the new fail-safe way.  I'll refresh my
>> patches if there's consensus.
> 
> Will you also replace 'blockdev-transaction' by 'transaction' to keep
> things generic at the interface level? The necessary refactoring to
> actually make it useful outside the block layer can come later, but we
> can leave the external API stable then.

Ok.

Paolo

Re: [Qemu-devel] [PATCH v2 0/6] Mirrored writes using blockdev-transaction

[Anthony Liguori]

On 03/05/2012 02:53 AM, Kevin Wolf wrote:
> Am 01.03.2012 22:10, schrieb Anthony Liguori:
>> On 03/01/2012 05:21 AM, Paolo Bonzini wrote:
>>> This implements all ingredients to establish mirrored writes.
>>> The drive-reopen command that is used to terminate mirrored writes
>>> is not included in this series.
>>>
>>> Tested with the following scenarios:
>>>
>>> a) mirror only
>>>
>>> 1) create base.qcow2 and start QEMU with it
>>>
>>> 2) Execute the following QMP command
>>>
>>> { "execute": "qmp_capabilities" }
>>> { "execute": "blockdev-transaction", "arguments":
>>>     {'actions': [
>>>       { 'type': 'mirror', 'data' :
>>>         { 'device': 'ide0-hd0', 'target': '/home/pbonzini/mirror.qcow2' } } ] } }
>>> { "execute": "cont" }
>>>
>>> 3) hibernate the guest (this requires an IDE disk and -cpu kvm64,-kvmclock)
>>>
>>> 4) restart the guest with mirror.qcow2
>>>
>>>
>>> b) atomic snapshot+mirror
>>>
>>> 1) start QEMU with an existing image test.img
>>>
>>> 2) Execute the following QMP command
>>>
>>> { "execute": "qmp_capabilities" }
>>> { "execute": "blockdev-transaction", "arguments":
>>>     {'actions': [
>>>       { 'type': 'snapshot', 'data' :
>>>         { 'device': 'ide0-hd0', 'snapshot-file': '/home/pbonzini/base.qcow2' } },
>>>       { 'type': 'mirror', 'data' :
>>>         { 'device': 'ide0-hd0', 'target': '/home/pbonzini/mirror.qcow2' } } ] } }
>>> { "execute": "cont" }
>>
>> We don't have schema introspection today.  How would one determine when new
>> transaction types are available?
>>
>> I think we need some sort of introspection method too in order for clients to
>> figure out when the command is extended.
>
> How about coupling the types with independently available commands for
> now? We would rename 'snapshot' to 'blockdev-snapshot-sync', which does
> the same thing outside of transactions. The mirror patches would then
> introduce a 'drive-mirror' top-level command at the same time as they
> introduce a 'drive-mirror' transaction type.

I think it's reasonable but we need to make sure to document the relationship 
here explicitly in the schema.

Regards,

Anthony Liguori

>
> Kevin

Re: [Qemu-devel] [PATCH v2 0/6] Mirrored writes using blockdev-transaction

[Paolo Bonzini]

Il 05/03/2012 15:47, Anthony Liguori ha scritto:
>> How about coupling the types with independently available commands for
>> now? We would rename 'snapshot' to 'blockdev-snapshot-sync', which does
>> the same thing outside of transactions. The mirror patches would then
>> introduce a 'drive-mirror' top-level command at the same time as they
>> introduce a 'drive-mirror' transaction type.
> 
> I think it's reasonable but we need to make sure to document the
> relationship here explicitly in the schema.

We can easily add a query-commands-transaction that returns the commands
that can be placed in a transaction.

Paolo