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

[Qemu-devel] [PATCH 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



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         |  153 +++++++++++++++++++++++++++++++++++++++++++++
 blockdev.c                |  111 +++++++++++++++++++++-----------
 docs/blkmirror.txt        |   16 +++++
 qapi-schema-test.json     |   10 +++
 qapi-schema.json          |   51 ++++++++++++---
 qmp-commands.hx           |   66 +++++++++++++-------
 scripts/qapi-types.py     |    5 ++
 scripts/qapi-visit.py     |   31 +++++++++-
 test-qmp-input-visitor.c  |   19 ++++++
 test-qmp-output-visitor.c |   35 ++++++++++
 11 files changed, 426 insertions(+), 73 deletions(-)
 create mode 100644 block/blkmirror.c
 create mode 100644 docs/blkmirror.txt

-- 
1.7.7.6

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

[Paolo Bonzini]

The new image's backing file is the existing image, so we need to take
the format from there.

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 2/6] qapi: complete implementation of unions

[Paolo Bonzini]

Union types are very cool, but nobody bothered to implement the visitors
so far. :)  Fill the gap.

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  |   19 +++++++++++++++++++
 test-qmp-output-visitor.c |   35 +++++++++++++++++++++++++++++++++++
 5 files changed, 99 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..c947456 100644
--- a/test-qmp-input-visitor.c
+++ b/test-qmp-input-visitor.c
@@ -234,6 +234,23 @@ static void test_visitor_in_list(TestInputVisitorData *data,
     qapi_free_UserDefOneList(head);
 }
 
+/* test commands with no input and no return value */
+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 +281,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..7543338 100644
--- a/test-qmp-output-visitor.c
+++ b/test-qmp-output-visitor.c
@@ -380,6 +380,39 @@ static void test_visitor_out_list_qapi_free(TestOutputVisitorData *data,
     qapi_free_UserDefNestedList(head);
 }
 
+/* test commands with no input and no return value */
+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 +449,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 3/6] rename blockdev-group-snapshot-sync

[Paolo Bonzini]

We will add other kinds of operation.  Prepare for this by adjusting
the schema and renaming some types/variables.

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

diff --git a/blockdev.c b/blockdev.c
index 96a893b..3c35553 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -716,31 +716,32 @@ 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;
+    BlockdevActionList *dev_entry = dev_list;
+    BlockdevAction *dev_info = NULL;
+    BlkTransactionStates *states;
     BlockDriver *proto_drv;
     BlockDriver *drv;
     int flags;
+    const char *device;
     const char *format;
-    const char *snapshot_file;
+    const char *new_image_file = NULL;
 
-    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 */
@@ -751,18 +752,38 @@ void qmp_blockdev_group_snapshot_sync(SnapshotDevList *dev_list,
         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;
+            new_image_file = dev_info->snapshot->snapshot_file;
+            format = dev_info->snapshot->format;
+            assert(!(dev_info->snapshot->has_format && format));
+            break;
+        default:
+            abort();
+        }
 
+        if (!format) {
+            format = "qcow2";
+        }
+
+        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 +796,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..ee05ec2 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -687,41 +687,46 @@ 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",
+                                         "op": "snapshot" } } ] } }
 <- { "return": {} }
 
 EQMP
-- 
1.7.7.6

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

[Paolo Bonzini]

Il 29/02/2012 14:37, Paolo Bonzini ha scritto:
> +            assert(!(dev_info->snapshot->has_format && format));

Oops, this wanted to be

assert(dev_info->snapshot->has_format == (format != NULL));

but it can just be omitted, I wasn't sure of how QAPI handled optionals.

Paolo

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

[Luiz Capitulino]

On Wed, 29 Feb 2012 15:25:29 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 29/02/2012 14:37, Paolo Bonzini ha scritto:
> > +            assert(!(dev_info->snapshot->has_format && format));
> 
> Oops, this wanted to be
> 
> assert(dev_info->snapshot->has_format == (format != NULL));
> 
> but it can just be omitted, I wasn't sure of how QAPI handled optionals.

Do you now?

All optionals will be accompanied of a 'bool has_OPTIONAL_NAME', this bool
will be true if the optional has been passed by the caller/client or false
otherwise (in which case you shouldn't trust it).

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

[Paolo Bonzini]

Il 29/02/2012 16:08, Luiz Capitulino ha scritto:
>> > 
>> > but it can just be omitted, I wasn't sure of how QAPI handled optionals.
> Do you now?

Perhaps not. :)

> All optionals will be accompanied of a 'bool has_OPTIONAL_NAME', this bool
> will be true if the optional has been passed by the caller/client or false
> otherwise (in which case you shouldn't trust it).

My understanding was that in this case I can trust the value to be
all-zeros (zero, false, 0.0, NULL), at least in the context of QAPI.
The QmpInputVisitor uses g_malloc0.

Paolo

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

[Luiz Capitulino]

On Wed, 29 Feb 2012 16:23:20 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 29/02/2012 16:08, Luiz Capitulino ha scritto:
> >> > 
> >> > but it can just be omitted, I wasn't sure of how QAPI handled optionals.
> > Do you now?
> 
> Perhaps not. :)
> 
> > All optionals will be accompanied of a 'bool has_OPTIONAL_NAME', this bool
> > will be true if the optional has been passed by the caller/client or false
> > otherwise (in which case you shouldn't trust it).
> 
> My understanding was that in this case I can trust the value to be
> all-zeros (zero, false, 0.0, NULL), at least in the context of QAPI.
> The QmpInputVisitor uses g_malloc0.

Yes, as a side effect :) I mean, I don't think the reason for using g_malloc0()
was to have optionals zeroed. If we're going to count on this, then it's
better to make it explicit and/or document it.

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

[Paolo Bonzini]

Il 01/03/2012 14:30, Luiz Capitulino ha scritto:
>> > My understanding was that in this case I can trust the value to be
>> > all-zeros (zero, false, 0.0, NULL), at least in the context of QAPI.
>> > The QmpInputVisitor uses g_malloc0.
> Yes, as a side effect :) I mean, I don't think the reason for using g_malloc0()
> was to have optionals zeroed. If we're going to count on this, then it's
> better to make it explicit and/or document it.

I removed the assumption from v2 anyway.

Paolo

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

[Eric Blake]

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

On 02/29/2012 06:37 AM, Paolo Bonzini wrote:
> We will add other kinds of operation.  Prepare for this by adjusting
> the schema and renaming some types/variables.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  blockdev.c       |   73 +++++++++++++++++++++++++++++------------------------
>  qapi-schema.json |   33 ++++++++++++++++--------
>  qmp-commands.hx  |   51 ++++++++++++++++++++-----------------
>  3 files changed, 90 insertions(+), 67 deletions(-)
> 

>  ##
> -# @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

>  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)

I like it.  Of course, this means I still have a moving target for
implementing the libvirt side of things, so I'd like consensus sooner
rather than later.  More importantly, as long as
blockdev-group-snapshot-sync is not part of a release, renaming it is
fine; but if we get to the qemu 1.1 release with Jeff's patches but not
Paolo's rename, then libvirt has a harder job to cope with both names.


>  
>  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",
> +                                         "op": "snapshot" } } ] } }

Drop the "op":"snapshot".  It's a leftover from before your conversion
to a union type.

Question - when reading/writing these examples, are 'type' and "type"
interchangeable (like in XML)?

-- 
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 3/6] rename blockdev-group-snapshot-sync

[Paolo Bonzini]

Il 29/02/2012 19:41, Eric Blake ha scritto:
> Drop the "op":"snapshot".  It's a leftover from before your conversion
> to a union type.
> 
> Question - when reading/writing these examples, are 'type' and "type"
> interchangeable (like in XML)?

IIRC, in JSON only "type" is allowed, but QEMU allows 'type' too because
then it's so less painful to include JSON in C.  So as far as QEMU is
concerned they are interchangeable.

Paolo

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

[Paolo Bonzini]

In some cases it can be useful to use an existing file as the new image
in a snapshot.  Add this capability to blockdev-transaction.

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

diff --git a/blockdev.c b/blockdev.c
index 3c35553..61e20f7 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -740,6 +740,7 @@ void qmp_blockdev_transaction(BlockdevActionList *dev_list,
     const char *device;
     const char *format;
     const char *new_image_file = NULL;
+    bool do_snapshot;
 
     QSIMPLEQ_HEAD(snap_bdrv_states, BlkTransactionStates) snap_bdrv_states;
     QSIMPLEQ_INIT(&snap_bdrv_states);
@@ -759,6 +760,7 @@ void qmp_blockdev_transaction(BlockdevActionList *dev_list,
         case BLOCKDEV_ACTION_KIND_SNAPSHOT:
             device = dev_info->snapshot->device;
             new_image_file = dev_info->snapshot->snapshot_file;
+            do_snapshot = !dev_info->snapshot->has_reuse || !dev_info->snapshot->reuse;
             format = dev_info->snapshot->format;
             assert(!(dev_info->snapshot->has_format && format));
             break;
@@ -805,13 +807,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 ee05ec2..6728495 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -704,6 +704,10 @@ 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.
+Image files can be created by QEMU, or it can be created externally.
+In the latter case, the contents of the image must be the same as the
+current contents of the device.  Typically this is achieved by using
+the current image file as the backing file for the image.
 
 Arguments:
 
@@ -715,6 +719,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 +731,7 @@ Example:
                                          "format": "qcow2" } },
          { 'type': 'snapshot, 'data' : { "device": "ide-hd1",
                                          "snapshot-file": "/some/place/my-image2",
+                                         "reuse": true,
                                          "format": "qcow2",
                                          "op": "snapshot" } } ] } }
 <- { "return": {} }
-- 
1.7.7.6

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

[Eric Blake]

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

On 02/29/2012 06:37 AM, Paolo Bonzini wrote:
> In some cases it can be useful to use an existing file as the new image
> in a snapshot.  Add this capability to blockdev-transaction.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  blockdev.c       |   18 +++++++++++-------
>  qapi-schema.json |    3 ++-
>  qmp-commands.hx  |    7 +++++++
>  3 files changed, 20 insertions(+), 8 deletions(-)
> @@ -805,13 +807,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;
> +            }

Is there any sanity checking that we should be doing if the 'reuse' flag
was provided, or is this a case of 'the user better be giving us the
right information, and it's their fault if things break'?

> +++ 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 ee05ec2..6728495 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -704,6 +704,10 @@ 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.
> +Image files can be created by QEMU, or it can be created externally.

'files' is plural, 'it' is singular.  Perhaps this wording is better?

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.

-- 
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 4/6] add reuse field

[Kevin Wolf]

Am 29.02.2012 19:50, schrieb Eric Blake:
> On 02/29/2012 06:37 AM, Paolo Bonzini wrote:
>> In some cases it can be useful to use an existing file as the new image
>> in a snapshot.  Add this capability to blockdev-transaction.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  blockdev.c       |   18 +++++++++++-------
>>  qapi-schema.json |    3 ++-
>>  qmp-commands.hx  |    7 +++++++
>>  3 files changed, 20 insertions(+), 8 deletions(-)
>> @@ -805,13 +807,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;
>> +            }
> 
> Is there any sanity checking that we should be doing if the 'reuse' flag
> was provided, or is this a case of 'the user better be giving us the
> right information, and it's their fault if things break'?

The latter, qemu can't really perform meaningful checks here, especially
once we allow using file descriptors instead of file names.

(Would this command actually provide a hackish way for using FDs with
backing files even without -blockdev? Hm... Better forget it right now,
I never told anyone about this! :-))

>> +++ 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 ee05ec2..6728495 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -704,6 +704,10 @@ 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.
>> +Image files can be created by QEMU, or it can be created externally.
> 
> 'files' is plural, 'it' is singular.  Perhaps this wording is better?
> 
> 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.

And let's add a warning that you're on your own if you override qemu's
snapshot creation.

Kevin

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

[Paolo Bonzini]

From: Marcelo Tosatti <mtosatti@redhat.com>

Mirrored writes are used by live block copy.

The blkmirror driver is for internal use only, because it requires
bdrv_append to set up a backing_hd for it.  It relies on a quirk
of bdrv_append, which leaves the old image open for writes.

The source is hardcoded as the backing_hd for the destination, so that
copy-on-write functions properly.  Since the source is not yet available
at the time blkmirror_open is called, the backing_hd is set later.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
Signed-off-by: Federico Simoncelli <fsimonce@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
	This version of the driver is almost entirely rewritten to
	use bs->backing_hd and bs->file.  This is necessary in order
	to share as much code as possible with group snapshots.

 Makefile.objs      |    2 +-
 block/blkmirror.c  |  153 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 docs/blkmirror.txt |   16 ++++++
 3 files changed, 170 insertions(+), 1 deletions(-)
 create mode 100644 block/blkmirror.c
 create mode 100644 docs/blkmirror.txt

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..4862364
--- /dev/null
+++ b/block/blkmirror.c
@@ -0,0 +1,153 @@
+/*
+ * Block driver for mirrored writes.
+ *
+ * Copyright (C) 2011 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"
+
+/* Valid blkmirror filenames look like
+ * blkmirror:path/to/image1:path/to/image2 */
+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 int blkmirror_co_readv(BlockDriverState *bs,
+                              int64_t sector_num, int nb_sectors,
+                              QEMUIOVector *qiov)
+{
+    return bdrv_co_readv(bs->backing_hd, sector_num, nb_sectors, qiov);
+}
+
+static int blkmirror_co_writev(BlockDriverState *bs,
+                               int64_t sector_num, int nb_sectors,
+                               QEMUIOVector *qiov)
+{
+    int ret;
+
+    /* bs->backing_hd is set after initialization.  */
+    bs->file->backing_hd = bs->backing_hd;
+
+    ret = bdrv_co_writev(bs->backing_hd, sector_num, nb_sectors, qiov);
+    if (ret >= 0) {
+        ret = bdrv_co_writev(bs->file, sector_num, nb_sectors, qiov);
+    }
+
+    return ret;
+}
+
+static coroutine_fn int blkmirror_co_discard(BlockDriverState *bs,
+                             int64_t sector_num, int nb_sectors)
+{
+    int ret;
+
+    ret = bdrv_co_discard(bs->backing_hd, sector_num, nb_sectors);
+    if (ret >= 0) {
+        ret = bdrv_co_discard(bs->file, sector_num, nb_sectors);
+    }
+
+    return ret;
+}
+
+
+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_discard    = blkmirror_co_discard,
+
+    .bdrv_co_is_allocated = blkmirror_co_is_allocated,
+    .bdrv_co_readv      = blkmirror_co_readv,
+    .bdrv_co_writev     = blkmirror_co_writev,
+};
+
+static void bdrv_blkmirror_init(void)
+{
+    bdrv_register(&bdrv_blkmirror);
+}
+
+block_init(bdrv_blkmirror_init);
diff --git a/docs/blkmirror.txt b/docs/blkmirror.txt
new file mode 100644
index 0000000..cf73f3f
--- /dev/null
+++ b/docs/blkmirror.txt
@@ -0,0 +1,16 @@
+Block mirror driver
+-------------------
+
+This driver will mirror writes to two distinct images.
+It's used internally by live block copy.
+
+Format
+------
+
+blkmirror:/image1.img:/image2.img
+
+'\' (backslash) can be used to escape colon processing
+as a separator, in the first image filename.
+Backslashes themselves also can be escaped as '\\'.
+
+
-- 
1.7.7.6

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

[Stefan Hajnoczi]

On Wed, Feb 29, 2012 at 1:37 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> From: Marcelo Tosatti <mtosatti@redhat.com>
>
> Mirrored writes are used by live block copy.
>
> The blkmirror driver is for internal use only, because it requires
> bdrv_append to set up a backing_hd for it.  It relies on a quirk
> of bdrv_append, which leaves the old image open for writes.
>
> The source is hardcoded as the backing_hd for the destination, so that
> copy-on-write functions properly.  Since the source is not yet available
> at the time blkmirror_open is called, the backing_hd is set later.
>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> Signed-off-by: Federico Simoncelli <fsimonce@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>        This version of the driver is almost entirely rewritten to
>        use bs->backing_hd and bs->file.  This is necessary in order
>        to share as much code as possible with group snapshots.
>
>  Makefile.objs      |    2 +-
>  block/blkmirror.c  |  153 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  docs/blkmirror.txt |   16 ++++++
>  3 files changed, 170 insertions(+), 1 deletions(-)
>  create mode 100644 block/blkmirror.c
>  create mode 100644 docs/blkmirror.txt

Mostly happy here, I just recommend tweaking the name of this block
driver and documenting clearly that this is not a general-purpose
mirroring driver, given that it points image B's backing file at image
A's backing file.  I see this driver as internal functionality and
it's fairly easy for users to misuse it and be surprised by the
results.

> +static int blkmirror_co_writev(BlockDriverState *bs,
> +                               int64_t sector_num, int nb_sectors,
> +                               QEMUIOVector *qiov)
> +{
> +    int ret;
> +
> +    /* bs->backing_hd is set after initialization.  */
> +    bs->file->backing_hd = bs->backing_hd;
> +
> +    ret = bdrv_co_writev(bs->backing_hd, sector_num, nb_sectors, qiov);
> +    if (ret >= 0) {
> +        ret = bdrv_co_writev(bs->file, sector_num, nb_sectors, qiov);
> +    }
> +
> +    return ret;
> +}

Have you done performance tests?  It seems suboptimal to use
.bdrv_co_writev() and perform writes sequentially, even with
cache=unsafe.

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

[Paolo Bonzini]

Il 29/02/2012 15:37, Stefan Hajnoczi ha scritto:
>>  Makefile.objs      |    2 +-
>>  block/blkmirror.c  |  153 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  docs/blkmirror.txt |   16 ++++++
>>  3 files changed, 170 insertions(+), 1 deletions(-)
>>  create mode 100644 block/blkmirror.c
>>  create mode 100644 docs/blkmirror.txt
> 
> Mostly happy here, I just recommend tweaking the name of this block
> driver and documenting clearly that this is not a general-purpose
> mirroring driver, given that it points image B's backing file at image
> A's backing file.  I see this driver as internal functionality and
> it's fairly easy for users to misuse it and be surprised by the
> results.

I was thinking of adding something like no_user later.

And of course docs/blkmirror.txt is totally obsolete, I'll merge it into
block/blkmirror.c.

>> +static int blkmirror_co_writev(BlockDriverState *bs,
>> +                               int64_t sector_num, int nb_sectors,
>> +                               QEMUIOVector *qiov)
>> +{
>> +    int ret;
>> +
>> +    /* bs->backing_hd is set after initialization.  */
>> +    bs->file->backing_hd = bs->backing_hd;
>> +
>> +    ret = bdrv_co_writev(bs->backing_hd, sector_num, nb_sectors, qiov);
>> +    if (ret >= 0) {
>> +        ret = bdrv_co_writev(bs->file, sector_num, nb_sectors, qiov);
>> +    }
>> +
>> +    return ret;
>> +}
> 
> Have you done performance tests?  It seems suboptimal to use
> .bdrv_co_writev() and perform writes sequentially, even with
> cache=unsafe.

No, not yet.  I can add back AIO here.

Paolo

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

[Eric Blake]

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

On 02/29/2012 06:37 AM, Paolo Bonzini wrote:
> From: Marcelo Tosatti <mtosatti@redhat.com>
> 
> Mirrored writes are used by live block copy.
> 
> The blkmirror driver is for internal use only, because it requires
> bdrv_append to set up a backing_hd for it.  It relies on a quirk
> of bdrv_append, which leaves the old image open for writes.
> 
> The source is hardcoded as the backing_hd for the destination, so that
> copy-on-write functions properly.  Since the source is not yet available
> at the time blkmirror_open is called, the backing_hd is set later.
> 

> +++ b/block/blkmirror.c
> @@ -0,0 +1,153 @@
> +/*
> + * Block driver for mirrored writes.
> + *
> + * Copyright (C) 2011 Red Hat, Inc.

It's now 2012; should this be expanded?

-- 
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 --]

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

[Paolo Bonzini]

Add a new transaction type, "mirror".  It stacks a new blkmirror
file (instead of a snapshot) on top of the existing image.

It is possible to combine snapshot and mirror as two actions in the
same transaction.  Because of atomicity ensured by blockdev-transaction,
this will create a snapshot *and* ensure that _all_ operations that are
sent to it are also mirrored.

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

diff --git a/blockdev.c b/blockdev.c
index 61e20f7..eef93f0 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -735,12 +735,13 @@ void qmp_blockdev_transaction(BlockdevActionList *dev_list,
     BlockdevAction *dev_info = NULL;
     BlkTransactionStates *states;
     BlockDriver *proto_drv;
-    BlockDriver *drv;
+    BlockDriver *target_drv;
+    BlockDriver *drv = NULL;
     int flags;
     const char *device;
     const char *format;
     const char *new_image_file = NULL;
-    bool do_snapshot;
+    char *new_source_file = NULL;
 
     QSIMPLEQ_HEAD(snap_bdrv_states, BlkTransactionStates) snap_bdrv_states;
     QSIMPLEQ_INIT(&snap_bdrv_states);
@@ -759,11 +760,23 @@ void qmp_blockdev_transaction(BlockdevActionList *dev_list,
         switch (dev_info->kind) {
         case BLOCKDEV_ACTION_KIND_SNAPSHOT:
             device = dev_info->snapshot->device;
-            new_image_file = dev_info->snapshot->snapshot_file;
-            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;
+            }
+            new_source_file = g_strdup(dev_info->snapshot->snapshot_file);
             format = dev_info->snapshot->format;
             assert(!(dev_info->snapshot->has_format && format));
             break;
+
+        case BLOCKDEV_ACTION_KIND_MIRROR:
+            device = dev_info->mirror->device;
+            if (!dev_info->mirror->has_reuse || !dev_info->mirror->reuse) {
+                new_image_file = dev_info->mirror->target;
+            }
+            format = dev_info->mirror->format;
+            assert(!(dev_info->mirror->has_format && format));
+            drv = bdrv_find_format("blkmirror");
+            break;
         default:
             abort();
         }
@@ -771,12 +784,19 @@ void qmp_blockdev_transaction(BlockdevActionList *dev_list,
         if (!format) {
             format = "qcow2";
         }
-
-        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;
+        }
+
+        if (dev_info->kind == BLOCKDEV_ACTION_KIND_MIRROR) {
+            new_source_file = g_strdup_printf("blkmirror:%s:%s", format,
+                                              dev_info->mirror->target);
+        }
 
         states->old_bs = bdrv_find(device);
         if (!states->old_bs) {
@@ -800,32 +820,34 @@ 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_file);
         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,
                                   NULL, -1, flags);
             if (ret) {
-                error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
+                error_set(errp, QERR_OPEN_FILE_FAILED, new_source_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, new_image_file,
+        ret = bdrv_open(states->new_bs, new_source_file,
                         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_file);
             goto delete_and_fail;
         }
+        g_free(new_source_file);
+        new_source_file = NULL;
     }
 
 
@@ -853,6 +875,7 @@ exit:
     QSIMPLEQ_FOREACH(states, &snap_bdrv_states, entry) {
         g_free(states);
     }
+    g_free(new_source_file);
     return;
 }
 
diff --git a/qapi-schema.json b/qapi-schema.json
index 78df122..5f0fcee 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 target of the new image. A new file will be created if
+#          @reuse is absent or false.
+#
+# @format: #optional the format of the snapshot 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 6728495..b7a9ada 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.
 Image files can be created by QEMU, or it can be created externally.
@@ -713,7 +717,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)
@@ -721,6 +725,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 6/6] add mirroring to blockdev-transaction

[Eric Blake]

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

On 02/29/2012 06:37 AM, Paolo Bonzini wrote:
> Add a new transaction type, "mirror".  It stacks a new blkmirror
> file (instead of a snapshot) on top of the existing image.
> 
> It is possible to combine snapshot and mirror as two actions in the
> same transaction.  Because of atomicity ensured by blockdev-transaction,
> this will create a snapshot *and* ensure that _all_ operations that are
> sent to it are also mirrored.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  blockdev.c       |   47 +++++++++++++++++++++++++++++++++++------------
>  qapi-schema.json |   19 ++++++++++++++++++-
>  qmp-commands.hx  |   12 +++++++++++-
>  3 files changed, 64 insertions(+), 14 deletions(-)
> 

> @@ -721,6 +725,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)

This falls out very nicely.

Do we want to also add a 'reopen' operation to the union, for the
remaining action needed by oVirt live migration?

-- 
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 6/6] add mirroring to blockdev-transaction

[Paolo Bonzini]

Il 29/02/2012 20:47, Eric Blake ha scritto:
> This falls out very nicely.
> 
> Do we want to also add a 'reopen' operation to the union, for the
> remaining action needed by oVirt live migration?

We can add it later, I think, if need arises.  Switching to the
destination need not be done atomically for all disks.  (In fact,
neither does the start-mirroring operation if you want to migrate
multiple disks; it's just the snapshot+mirror pair that needs it).

Paolo