[Qemu-devel] [PATCH 0/2] Allow to repair broken quorum files

[Qemu-devel] [PATCH 0/2] Allow to repair broken quorum files

[Benoît Canet]

This patch allow to build a mirror of a block device and replace an arbitrary
named node of the graph with the mirror.

The obvious first usage is Quorum: with this patch one quand drive-mirror the
block device say quorum0 and then bdrv_swap the result in place of a broken
quorum file (dead filer for example).

The second patch implement drive-mirror tests for this mode.

Benoît Canet (2):
  block: Add node-name and to-replace-node-name arguments to
    drive-mirror.
  qemu-iotests: Add TestRepairQuorum to 041 to test drive-mirror
    node-name mode.

 block/mirror.c             |  39 +++++++--
 blockdev.c                 |  61 ++++++++++++--
 hmp.c                      |   3 +-
 include/block/block_int.h  |   3 +
 qapi-schema.json           |  15 +++-
 qmp-commands.hx            |   9 ++-
 tests/qemu-iotests/041     | 196 ++++++++++++++++++++++++++++++++++++++++++++-
 tests/qemu-iotests/041.out |   4 +-
 8 files changed, 308 insertions(+), 22 deletions(-)

-- 
1.8.3.2

[Qemu-devel] [PATCH 1/2] block: Add node-name and to-replace-node-name arguments to drive-mirror.

[Benoît Canet]

node-name give a name to the created BDS and register it in the node graph.

to-replace-node-name can be used when drive-mirror is called with sync=full.

The purpose of these fields is to be able to reconstruct and replace a broken
quorum file.

drive-mirror will bdrv_swap the new BDS named node-name with the one
pointed by to-replace-node-name when the mirroring is finished.

Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
 block/mirror.c            | 39 ++++++++++++++++++++++++++----
 blockdev.c                | 61 +++++++++++++++++++++++++++++++++++++++++++----
 hmp.c                     |  3 ++-
 include/block/block_int.h |  3 +++
 qapi-schema.json          | 15 +++++++++---
 qmp-commands.hx           |  9 +++++--
 6 files changed, 114 insertions(+), 16 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index e683959..127cfd9 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -32,6 +32,7 @@ typedef struct MirrorBlockJob {
     RateLimit limit;
     BlockDriverState *target;
     BlockDriverState *base;
+    BlockDriverState *to_replace;
     bool is_none_mode;
     BlockdevOnError on_source_error, on_target_error;
     bool synced;
@@ -312,6 +313,10 @@ static void coroutine_fn mirror_run(void *opaque)
     s->common.len = bdrv_getlength(bs);
     if (s->common.len <= 0) {
         block_job_completed(&s->common, s->common.len);
+        /* Fam's new blocker API should be used here. */
+        if (s->to_replace) {
+            bdrv_set_in_use(s->to_replace, 0);
+        }
         return;
     }
 
@@ -478,10 +483,17 @@ immediate_exit:
     bdrv_release_dirty_bitmap(bs, s->dirty_bitmap);
     bdrv_iostatus_disable(s->target);
     if (s->should_complete && ret == 0) {
-        if (bdrv_get_flags(s->target) != bdrv_get_flags(s->common.bs)) {
-            bdrv_reopen(s->target, bdrv_get_flags(s->common.bs), NULL);
+        BlockDriverState *to_replace;
+        /* if a to-replace-node-name was specified use it's bs */
+        if (s->to_replace) {
+            to_replace = s->to_replace;
+        } else {
+            to_replace = s->common.bs;
         }
-        bdrv_swap(s->target, s->common.bs);
+        if (bdrv_get_flags(s->target) != bdrv_get_flags(to_replace)) {
+            bdrv_reopen(s->target, bdrv_get_flags(to_replace), NULL);
+        }
+        bdrv_swap(s->target, to_replace);
         if (s->common.driver->job_type == BLOCK_JOB_TYPE_COMMIT) {
             /* drop the bs loop chain formed by the swap: break the loop then
              * trigger the unref from the top one */
@@ -490,6 +502,10 @@ immediate_exit:
             bdrv_unref(p);
         }
     }
+    /* Fam's new blocker API should be used here. */
+    if (s->to_replace) {
+        bdrv_set_in_use(s->to_replace, 0);
+    }
     bdrv_unref(s->target);
     block_job_completed(&s->common, ret);
 }
@@ -553,6 +569,7 @@ static const BlockJobDriver commit_active_job_driver = {
 };
 
 static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
+                             BlockDriverState *to_replace,
                             int64_t speed, int64_t granularity,
                             int64_t buf_size,
                             BlockdevOnError on_source_error,
@@ -585,6 +602,12 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
         return;
     }
 
+    /* Should be replaced by Fam's blocker API calls */
+    if (to_replace && bdrv_in_use(to_replace)) {
+        error_setg(errp, "node with name %s is already in use",
+                   to_replace->node_name);
+        return;
+    }
 
     s = block_job_create(driver, bs, speed, cb, opaque, errp);
     if (!s) {
@@ -594,6 +617,11 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
     s->on_source_error = on_source_error;
     s->on_target_error = on_target_error;
     s->target = target;
+    s->to_replace = to_replace;
+    /* Should be replaced by Fam's blocker API calls */
+    if (to_replace) {
+        bdrv_set_in_use(to_replace, 1);
+    }
     s->is_none_mode = is_none_mode;
     s->base = base;
     s->granularity = granularity;
@@ -609,6 +637,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
 }
 
 void mirror_start(BlockDriverState *bs, BlockDriverState *target,
+                  BlockDriverState *to_replace,
                   int64_t speed, int64_t granularity, int64_t buf_size,
                   MirrorSyncMode mode, BlockdevOnError on_source_error,
                   BlockdevOnError on_target_error,
@@ -620,7 +649,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
 
     is_none_mode = mode == MIRROR_SYNC_MODE_NONE;
     base = mode == MIRROR_SYNC_MODE_TOP ? bs->backing_hd : NULL;
-    mirror_start_job(bs, target, speed, granularity, buf_size,
+    mirror_start_job(bs, target, to_replace, speed, granularity, buf_size,
                      on_source_error, on_target_error, cb, opaque, errp,
                      &mirror_job_driver, is_none_mode, base);
 }
@@ -668,7 +697,7 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
     }
 
     bdrv_ref(base);
-    mirror_start_job(bs, base, speed, 0, 0,
+    mirror_start_job(bs, base, NULL, speed, 0, 0,
                      on_error, on_error, cb, opaque, &local_err,
                      &commit_active_job_driver, false, base);
     if (error_is_set(&local_err)) {
diff --git a/blockdev.c b/blockdev.c
index 357f760..876f386 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2043,6 +2043,9 @@ BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp)
 
 void qmp_drive_mirror(const char *device, const char *target,
                       bool has_format, const char *format,
+                      bool has_new_node_name, const char *new_node_name,
+                      bool has_to_replace_node_name,
+                      const char *to_replace_node_name,
                       enum MirrorSyncMode sync,
                       bool has_mode, enum NewImageMode mode,
                       bool has_speed, int64_t speed,
@@ -2053,9 +2056,10 @@ void qmp_drive_mirror(const char *device, const char *target,
                       Error **errp)
 {
     BlockDriverState *bs;
-    BlockDriverState *source, *target_bs;
+    BlockDriverState *source, *target_bs, *to_replace_bs = NULL;
     BlockDriver *drv = NULL;
     Error *local_err = NULL;
+    QDict *options = NULL;
     int flags;
     int64_t size;
     int ret;
@@ -2094,6 +2098,33 @@ void qmp_drive_mirror(const char *device, const char *target,
         return;
     }
 
+    /* we are planning to replace a bs graph node once the mirroring is
+     * completed. Look it up now.
+     */
+    if (has_to_replace_node_name) {
+        to_replace_bs = bdrv_find_node(to_replace_node_name);
+        if (!to_replace_bs) {
+            error_setg(errp, "to-replace-node-name=%s not found",
+                       to_replace_node_name);
+            return;
+        }
+
+        /* the code should only be able to replace the top first non filter
+         * node of the graph. For example the first BDS under a quorum.
+         */
+        if (!bdrv_is_first_non_filter(to_replace_bs)) {
+            error_set(errp, QERR_FEATURE_DISABLED,
+                      "drive-mirror and replace node-name");
+            return;
+        }
+    }
+
+    if (has_to_replace_node_name && !has_new_node_name) {
+        error_setg(errp, "a new-node-name must be provided when replacing a "
+                         "named node of the graph");
+        return;
+    }
+
     if (!bdrv_is_inserted(bs)) {
         error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
         return;
@@ -2130,6 +2161,12 @@ void qmp_drive_mirror(const char *device, const char *target,
         return;
     }
 
+    if (has_to_replace_node_name && size < bdrv_getlength(to_replace_bs)) {
+        error_setg(errp, "cannot replace to-replace-node-name image with a "
+                         "mirror image that would be smaller in size");
+        return;
+    }
+
     if ((sync == MIRROR_SYNC_MODE_FULL || !source)
         && mode != NEW_IMAGE_MODE_EXISTING)
     {
@@ -2158,19 +2195,33 @@ void qmp_drive_mirror(const char *device, const char *target,
         return;
     }
 
+    /* if we are planning to replace a graph node name the code should do a full
+     * mirror of the source image
+     */
+    if (has_to_replace_node_name && sync != MIRROR_SYNC_MODE_FULL) {
+        error_setg(errp,
+                   "to-replace-node-name can only be used with sync=full");
+        return;
+    }
+
+    if (has_new_node_name) {
+        options = qdict_new();
+        qdict_put(options, "node-name", qstring_from_str(new_node_name));
+    }
+
     /* Mirroring takes care of copy-on-write using the source's backing
      * file.
      */
     target_bs = NULL;
-    ret = bdrv_open(&target_bs, target, NULL, NULL, flags | BDRV_O_NO_BACKING,
-                    drv, &local_err);
+    ret = bdrv_open(&target_bs, target, NULL, options,
+                    flags | BDRV_O_NO_BACKING, drv, &local_err);
     if (ret < 0) {
         error_propagate(errp, local_err);
         return;
     }
 
-    mirror_start(bs, target_bs, speed, granularity, buf_size, sync,
-                 on_source_error, on_target_error,
+    mirror_start(bs, target_bs, to_replace_bs, speed, granularity, buf_size,
+                 sync, on_source_error, on_target_error,
                  block_job_cb, bs, &local_err);
     if (local_err != NULL) {
         bdrv_unref(target_bs);
diff --git a/hmp.c b/hmp.c
index e3ddd46..8a6145b 100644
--- a/hmp.c
+++ b/hmp.c
@@ -919,7 +919,8 @@ void hmp_drive_mirror(Monitor *mon, const QDict *qdict)
         mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
     }
 
-    qmp_drive_mirror(device, filename, !!format, format,
+    qmp_drive_mirror(device, filename, false, NULL,
+                     false, NULL, !!format, format,
                      full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP,
                      true, mode, false, 0, false, 0, false, 0,
                      false, 0, false, 0, &errp);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 4fc5ea8..ceb7037 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -456,6 +456,8 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
  * mirror_start:
  * @bs: Block device to operate on.
  * @target: Block device to write to.
+ * @to_replace: Block graph node to replace once the mirror is done.
+ *              Can only be used when full mirroring is selected.
  * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
  * @granularity: The chosen granularity for the dirty bitmap.
  * @buf_size: The amount of data that can be in flight at one time.
@@ -472,6 +474,7 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
  * @bs will be switched to read from @target.
  */
 void mirror_start(BlockDriverState *bs, BlockDriverState *target,
+                  BlockDriverState *to_replace,
                   int64_t speed, int64_t granularity, int64_t buf_size,
                   MirrorSyncMode mode, BlockdevOnError on_source_error,
                   BlockdevOnError on_target_error,
diff --git a/qapi-schema.json b/qapi-schema.json
index ac8ad24..8187775 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2140,6 +2140,14 @@
 # @format: #optional the format of the new destination, default is to
 #          probe if @mode is 'existing', else the format of the source
 #
+# @new-node-name: #optional the new block driver state node name in the graph
+#                 (Since 2.1)
+#
+# @to-replace-node-name: #optional with sync=full graph node name to be
+#                        replaced by the new image when a whole image copy is
+#                        done. This can be used to repair broken Quorum files.
+#                        (Since 2.1)
+#
 # @mode: #optional whether and how QEMU should create a new image, default is
 #        'absolute-paths'.
 #
@@ -2172,9 +2180,10 @@
 ##
 { 'command': 'drive-mirror',
   'data': { 'device': 'str', 'target': 'str', '*format': 'str',
-            'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
-            '*speed': 'int', '*granularity': 'uint32',
-            '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
+            '*new-node-name': 'str', '*to-replace-node-name': 'str',
+            'sync': 'MirrorSyncMode', '*mode': 'NewImageMode', '*speed': 'int',
+            '*granularity': 'uint32', '*buf-size': 'int',
+            '*on-source-error': 'BlockdevOnError',
             '*on-target-error': 'BlockdevOnError' } }
 
 ##
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 8a0e832..6324b02 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1270,8 +1270,9 @@ EQMP
     {
         .name       = "drive-mirror",
         .args_type  = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?,"
-                      "on-source-error:s?,on-target-error:s?,"
-                      "granularity:i?,buf-size:i?",
+                      "new-node-name:s?,to-replace-node-name:s?,"
+                      "on-source-error:s?,on-target-error:s?,granularity:i?,"
+                      "buf-size:i?",
         .mhandler.cmd_new = qmp_marshal_input_drive_mirror,
     },
 
@@ -1291,6 +1292,10 @@ Arguments:
 - "device": device name to operate on (json-string)
 - "target": name of new image file (json-string)
 - "format": format of new image (json-string, optional)
+- "new-node-name": the name of the new block driver state in the node graph
+                   (json-string, optional)
+- "to-replace-node-name": the block driver node name to replace when finished
+                          (json-string, optional)
 - "mode": how an image file should be created into the target
   file/device (NewImageMode, optional, default 'absolute-paths')
 - "speed": maximum speed of the streaming job, in bytes per second
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH 1/2] block: Add node-name and to-replace-node-name arguments to drive-mirror.

[Eric Blake]

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

On 03/05/2014 08:18 AM, Benoît Canet wrote:
> node-name give a name to the created BDS and register it in the node graph.

s/give/gives/ s/register/registers/

> 
> to-replace-node-name can be used when drive-mirror is called with sync=full.
> 
> The purpose of these fields is to be able to reconstruct and replace a broken
> quorum file.

There may be other uses possible from this, but the idea makes sense.

> 
> drive-mirror will bdrv_swap the new BDS named node-name with the one
> pointed by to-replace-node-name when the mirroring is finished.
> 
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---

> @@ -312,6 +313,10 @@ static void coroutine_fn mirror_run(void *opaque)
>      s->common.len = bdrv_getlength(bs);
>      if (s->common.len <= 0) {
>          block_job_completed(&s->common, s->common.len);
> +        /* Fam's new blocker API should be used here. */
> +        if (s->to_replace) {

Who is getting merged first?  It seems like this should be fixed before
taking this patch, if Fam's work is indeed closer to inclusion.  At any
rate, the comment seems odd - a year from now, Fam's work won't be new.

> +        BlockDriverState *to_replace;
> +        /* if a to-replace-node-name was specified use it's bs */

s/it's/its/ - the rule is anywhere that you see "it's", re-read the
sentence with "it is" and see if it still makes sense; if not, you meant
"its".


>  
>  static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
> +                             BlockDriverState *to_replace,
>                              int64_t speed, int64_t granularity,

Pre-existing, but as long as you are touching this, you might as well
fix indentation of the other lines in the same signature.

> @@ -2158,19 +2195,33 @@ void qmp_drive_mirror(const char *device, const char *target,
>          return;
>      }
>  
> +    /* if we are planning to replace a graph node name the code should do a full
> +     * mirror of the source image
> +     */
> +    if (has_to_replace_node_name && sync != MIRROR_SYNC_MODE_FULL) {
> +        error_setg(errp,
> +                   "to-replace-node-name can only be used with sync=full");
> +        return;
> +    }

I'm not sure I follow this restriction.  What's to prevent me from doing
a shallow mirror coupled with the mode of reusing an existing file that
already points to a sane backing file, rather than forcing a full sync?
 That is, why not let this command be a fully-generic swap command,
where the semantics are that as long as my old and new image have the
same contents from the guest's perspective (or I'm replacing a broken
file out of a quorum, and the new image has the same contents as the
quorum majority), then we are just updating qemu to point to a new BDS.

On the other hand, back around the 1.5 timeframe, downstream RHEL tried
to add a 'drive-reopen' command that did just that - replaced the
backing file of a guest's disk with an arbitrary other file.  But it was
so powerful and risky that at the time upstream finally added
'transaction' support, we decided to go with the simpler
'drive-mirror/block-job-complete' sequence as the only supported way to
cause qemu to associate a different BDS with a guest image.  Of course,
things have advanced since then, so maybe we finally are at a point
where we want to expose a generic reopen command that can swap out
arbitrary named nodes without interrupting guest services, but now I'm
starting to wonder if it should be a new command instead of adding
optional arguments to the existing drive-mirror.

> +++ b/qapi-schema.json
> @@ -2140,6 +2140,14 @@
>  # @format: #optional the format of the new destination, default is to
>  #          probe if @mode is 'existing', else the format of the source
>  #
> +# @new-node-name: #optional the new block driver state node name in the graph
> +#                 (Since 2.1)

Ah, so you're not trying to get this in before 2.0 freeze - which means
we have more time to think about the implications.

> +#
> +# @to-replace-node-name: #optional with sync=full graph node name to be
> +#                        replaced by the new image when a whole image copy is
> +#                        done. This can be used to repair broken Quorum files.
> +#                        (Since 2.1)

This naming feels long, but I'm not sure if I have a better suggestion.
 It looks like you only allow swapping out one quorum file per
drive-mirror - but what if I have a 3/5 quorum and want to swap out two
files at once?  Also, how does this interact with the 'transaction' command?

>  ##
>  { 'command': 'drive-mirror',
>    'data': { 'device': 'str', 'target': 'str', '*format': 'str',
> -            'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
> -            '*speed': 'int', '*granularity': 'uint32',
> -            '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
> +            '*new-node-name': 'str', '*to-replace-node-name': 'str',
> +            'sync': 'MirrorSyncMode', '*mode': 'NewImageMode', '*speed': 'int',
> +            '*granularity': 'uint32', '*buf-size': 'int',
> +            '*on-source-error': 'BlockdevOnError',

Why the reindent of existing options?

-- 
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: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/2] block: Add node-name and to-replace-node-name arguments to drive-mirror.

[Benoît Canet]

The Wednesday 05 Mar 2014 à 13:54:44 (-0700), Eric Blake wrote :
> On 03/05/2014 08:18 AM, Benoît Canet wrote:
> > node-name give a name to the created BDS and register it in the node graph.
> 
> s/give/gives/ s/register/registers/
> 
> > 
> > to-replace-node-name can be used when drive-mirror is called with sync=full.
> > 
> > The purpose of these fields is to be able to reconstruct and replace a broken
> > quorum file.
> 
> There may be other uses possible from this, but the idea makes sense.
> 
> > 
> > drive-mirror will bdrv_swap the new BDS named node-name with the one
> > pointed by to-replace-node-name when the mirroring is finished.
> > 
> > Signed-off-by: Benoit Canet <benoit@irqsave.net>
> > ---
> 
> > @@ -312,6 +313,10 @@ static void coroutine_fn mirror_run(void *opaque)
> >      s->common.len = bdrv_getlength(bs);
> >      if (s->common.len <= 0) {
> >          block_job_completed(&s->common, s->common.len);
> > +        /* Fam's new blocker API should be used here. */
> > +        if (s->to_replace) {
> 
> Who is getting merged first?  It seems like this should be fixed before
> taking this patch, if Fam's work is indeed closer to inclusion.  At any
> rate, the comment seems odd - a year from now, Fam's work won't be new.

I would really like to get this merged first before 2.0 reach hard freeze
since quorum is not very usable in its current state.

This particular comment was here to inform reviewer of the work I plan to do
once Fam's series is merged.

I would do the work in 2.1.

> 
> > +        BlockDriverState *to_replace;
> > +        /* if a to-replace-node-name was specified use it's bs */
> 
> s/it's/its/ - the rule is anywhere that you see "it's", re-read the
> sentence with "it is" and see if it still makes sense; if not, you meant
> "its".

Thanks for the rule I just used it above :)

> 
> 
> >  
> >  static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
> > +                             BlockDriverState *to_replace,
> >                              int64_t speed, int64_t granularity,
> 
> Pre-existing, but as long as you are touching this, you might as well
> fix indentation of the other lines in the same signature.
> 
> > @@ -2158,19 +2195,33 @@ void qmp_drive_mirror(const char *device, const char *target,
> >          return;
> >      }
> >  
> > +    /* if we are planning to replace a graph node name the code should do a full
> > +     * mirror of the source image
> > +     */
> > +    if (has_to_replace_node_name && sync != MIRROR_SYNC_MODE_FULL) {
> > +        error_setg(errp,
> > +                   "to-replace-node-name can only be used with sync=full");
> > +        return;
> > +    }
> 
> I'm not sure I follow this restriction.  What's to prevent me from doing
> a shallow mirror coupled with the mode of reusing an existing file that
> already points to a sane backing file, rather than forcing a full sync?
>  That is, why not let this command be a fully-generic swap command,
> where the semantics are that as long as my old and new image have the
> same contents from the guest's perspective (or I'm replacing a broken
> file out of a quorum, and the new image has the same contents as the
> quorum majority), then we are just updating qemu to point to a new BDS.
> 
> On the other hand, back around the 1.5 timeframe, downstream RHEL tried
> to add a 'drive-reopen' command that did just that - replaced the
> backing file of a guest's disk with an arbitrary other file.  But it was
> so powerful and risky that at the time upstream finally added
> 'transaction' support, we decided to go with the simpler
> 'drive-mirror/block-job-complete' sequence as the only supported way to
> cause qemu to associate a different BDS with a guest image.  Of course,
> things have advanced since then, so maybe we finally are at a point
> where we want to expose a generic reopen command that can swap out
> arbitrary named nodes without interrupting guest services, but now I'm
> starting to wonder if it should be a new command instead of adding
> optional arguments to the existing drive-mirror.

I choose to hook into drive-mirror because it is supposed to do the swap at the
very moment the two files converge.

I though it would be harder to implement with a separate command because new
writes could obsolete the mirror after drive-mirror complete and before the
swap command is launched.

> 
> > +++ b/qapi-schema.json
> > @@ -2140,6 +2140,14 @@
> >  # @format: #optional the format of the new destination, default is to
> >  #          probe if @mode is 'existing', else the format of the source
> >  #
> > +# @new-node-name: #optional the new block driver state node name in the graph
> > +#                 (Since 2.1)
> 
> Ah, so you're not trying to get this in before 2.0 freeze - which means
> we have more time to think about the implications.

I remembered after sending the series that 2.0 was not in hard freeze yet and
that we have a small chance of shipping quorum in an usable state.

> 
> > +#
> > +# @to-replace-node-name: #optional with sync=full graph node name to be
> > +#                        replaced by the new image when a whole image copy is
> > +#                        done. This can be used to repair broken Quorum files.
> > +#                        (Since 2.1)
> 
> This naming feels long, but I'm not sure if I have a better suggestion.
>  It looks like you only allow swapping out one quorum file per
> drive-mirror - but what if I have a 3/5 quorum and want to swap out two
> files at once?  Also, how does this interact with the 'transaction' command?

I think that we should be able to launch multiple separate drive-mirror
operation. I don't know about the transaction.

> 
> >  ##
> >  { 'command': 'drive-mirror',
> >    'data': { 'device': 'str', 'target': 'str', '*format': 'str',
> > -            'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
> > -            '*speed': 'int', '*granularity': 'uint32',
> > -            '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
> > +            '*new-node-name': 'str', '*to-replace-node-name': 'str',
> > +            'sync': 'MirrorSyncMode', '*mode': 'NewImageMode', '*speed': 'int',
> > +            '*granularity': 'uint32', '*buf-size': 'int',
> > +            '*on-source-error': 'BlockdevOnError',
> 
> Why the reindent of existing options?

The first modified line was exceeding the 80 characters limit.

> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

Re: [Qemu-devel] [PATCH 1/2] block: Add node-name and to-replace-node-name arguments to drive-mirror.

[Benoît Canet]

The Wednesday 05 Mar 2014 à 22:13:13 (+0100), Benoît Canet wrote :
> The Wednesday 05 Mar 2014 à 13:54:44 (-0700), Eric Blake wrote :
> > On 03/05/2014 08:18 AM, Benoît Canet wrote:
> > > node-name give a name to the created BDS and register it in the node graph.
> > 
> > s/give/gives/ s/register/registers/
> > 
> > > 
> > > to-replace-node-name can be used when drive-mirror is called with sync=full.
> > > 
> > > The purpose of these fields is to be able to reconstruct and replace a broken
> > > quorum file.
> > 
> > There may be other uses possible from this, but the idea makes sense.
> > 
> > > 
> > > drive-mirror will bdrv_swap the new BDS named node-name with the one
> > > pointed by to-replace-node-name when the mirroring is finished.
> > > 
> > > Signed-off-by: Benoit Canet <benoit@irqsave.net>
> > > ---
> > 
> > > @@ -312,6 +313,10 @@ static void coroutine_fn mirror_run(void *opaque)
> > >      s->common.len = bdrv_getlength(bs);
> > >      if (s->common.len <= 0) {
> > >          block_job_completed(&s->common, s->common.len);
> > > +        /* Fam's new blocker API should be used here. */
> > > +        if (s->to_replace) {
> > 
> > Who is getting merged first?  It seems like this should be fixed before
> > taking this patch, if Fam's work is indeed closer to inclusion.  At any
> > rate, the comment seems odd - a year from now, Fam's work won't be new.
> 
> I would really like to get this merged first before 2.0 reach hard freeze
> since quorum is not very usable in its current state.
> 
> This particular comment was here to inform reviewer of the work I plan to do
> once Fam's series is merged.
> 
> I would do the work in 2.1.
> 
> > 
> > > +        BlockDriverState *to_replace;
> > > +        /* if a to-replace-node-name was specified use it's bs */
> > 
> > s/it's/its/ - the rule is anywhere that you see "it's", re-read the
> > sentence with "it is" and see if it still makes sense; if not, you meant
> > "its".
> 
> Thanks for the rule I just used it above :)
> 
> > 
> > 
> > >  
> > >  static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
> > > +                             BlockDriverState *to_replace,
> > >                              int64_t speed, int64_t granularity,
> > 
> > Pre-existing, but as long as you are touching this, you might as well
> > fix indentation of the other lines in the same signature.
> > 
> > > @@ -2158,19 +2195,33 @@ void qmp_drive_mirror(const char *device, const char *target,
> > >          return;
> > >      }
> > >  
> > > +    /* if we are planning to replace a graph node name the code should do a full
> > > +     * mirror of the source image
> > > +     */
> > > +    if (has_to_replace_node_name && sync != MIRROR_SYNC_MODE_FULL) {
> > > +        error_setg(errp,
> > > +                   "to-replace-node-name can only be used with sync=full");
> > > +        return;
> > > +    }
> > 
> > I'm not sure I follow this restriction.  What's to prevent me from doing
> > a shallow mirror coupled with the mode of reusing an existing file that
> > already points to a sane backing file, rather than forcing a full sync?
> >  That is, why not let this command be a fully-generic swap command,
> > where the semantics are that as long as my old and new image have the
> > same contents from the guest's perspective (or I'm replacing a broken
> > file out of a quorum, and the new image has the same contents as the
> > quorum majority), then we are just updating qemu to point to a new BDS.
> > 
> > On the other hand, back around the 1.5 timeframe, downstream RHEL tried
> > to add a 'drive-reopen' command that did just that - replaced the
> > backing file of a guest's disk with an arbitrary other file.  But it was
> > so powerful and risky that at the time upstream finally added
> > 'transaction' support, we decided to go with the simpler
> > 'drive-mirror/block-job-complete' sequence as the only supported way to
> > cause qemu to associate a different BDS with a guest image.  Of course,
> > things have advanced since then, so maybe we finally are at a point
> > where we want to expose a generic reopen command that can swap out
> > arbitrary named nodes without interrupting guest services, but now I'm
> > starting to wonder if it should be a new command instead of adding
> > optional arguments to the existing drive-mirror.
> 
> I choose to hook into drive-mirror because it is supposed to do the swap at the
> very moment the two files converge.
> 
> I though it would be harder to implement with a separate command because new
> writes could obsolete the mirror after drive-mirror complete and before the
> swap command is launched.
> 
> > 
> > > +++ b/qapi-schema.json
> > > @@ -2140,6 +2140,14 @@
> > >  # @format: #optional the format of the new destination, default is to
> > >  #          probe if @mode is 'existing', else the format of the source
> > >  #
> > > +# @new-node-name: #optional the new block driver state node name in the graph
> > > +#                 (Since 2.1)
> > 
> > Ah, so you're not trying to get this in before 2.0 freeze - which means
> > we have more time to think about the implications.
> 
> I remembered after sending the series that 2.0 was not in hard freeze yet and
> that we have a small chance of shipping quorum in an usable state.
> 
> > 
> > > +#
> > > +# @to-replace-node-name: #optional with sync=full graph node name to be
> > > +#                        replaced by the new image when a whole image copy is
> > > +#                        done. This can be used to repair broken Quorum files.
> > > +#                        (Since 2.1)
> > 
> > This naming feels long, but I'm not sure if I have a better suggestion.
> >  It looks like you only allow swapping out one quorum file per
> > drive-mirror - but what if I have a 3/5 quorum and want to swap out two
> > files at once?  Also, how does this interact with the 'transaction' command?
> 
> I think that we should be able to launch multiple separate drive-mirror
> operation. I don't know about the transaction.

In fact no. The first drive-mirror will set the quorum device as "in use".
No parallelisation is possible.

Best regards

Benoît

> 
> > 
> > >  ##
> > >  { 'command': 'drive-mirror',
> > >    'data': { 'device': 'str', 'target': 'str', '*format': 'str',
> > > -            'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
> > > -            '*speed': 'int', '*granularity': 'uint32',
> > > -            '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
> > > +            '*new-node-name': 'str', '*to-replace-node-name': 'str',
> > > +            'sync': 'MirrorSyncMode', '*mode': 'NewImageMode', '*speed': 'int',
> > > +            '*granularity': 'uint32', '*buf-size': 'int',
> > > +            '*on-source-error': 'BlockdevOnError',
> > 
> > Why the reindent of existing options?
> 
> The first modified line was exceeding the 80 characters limit.
> 
> > 
> > -- 
> > Eric Blake   eblake redhat com    +1-919-301-3266
> > Libvirt virtualization library http://libvirt.org
> > 
> 
> 
> 

Re: [Qemu-devel] [PATCH 1/2] block: Add node-name and to-replace-node-name arguments to drive-mirror.

[Benoît Canet]

The Wednesday 05 Mar 2014 à 13:54:44 (-0700), Eric Blake wrote :
> On 03/05/2014 08:18 AM, Benoît Canet wrote:
> > node-name give a name to the created BDS and register it in the node graph.
> 
> s/give/gives/ s/register/registers/
> 
> > 
> > to-replace-node-name can be used when drive-mirror is called with sync=full.
> > 
> > The purpose of these fields is to be able to reconstruct and replace a broken
> > quorum file.
> 
> There may be other uses possible from this, but the idea makes sense.
> 
> > 
> > drive-mirror will bdrv_swap the new BDS named node-name with the one
> > pointed by to-replace-node-name when the mirroring is finished.
> > 
> > Signed-off-by: Benoit Canet <benoit@irqsave.net>
> > ---
> 
> > @@ -312,6 +313,10 @@ static void coroutine_fn mirror_run(void *opaque)
> >      s->common.len = bdrv_getlength(bs);
> >      if (s->common.len <= 0) {
> >          block_job_completed(&s->common, s->common.len);
> > +        /* Fam's new blocker API should be used here. */
> > +        if (s->to_replace) {
> 
> Who is getting merged first?  It seems like this should be fixed before
> taking this patch, if Fam's work is indeed closer to inclusion.  At any
> rate, the comment seems odd - a year from now, Fam's work won't be new.
> 
> > +        BlockDriverState *to_replace;
> > +        /* if a to-replace-node-name was specified use it's bs */
> 
> s/it's/its/ - the rule is anywhere that you see "it's", re-read the
> sentence with "it is" and see if it still makes sense; if not, you meant
> "its".
> 
> 
> >  
> >  static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
> > +                             BlockDriverState *to_replace,
> >                              int64_t speed, int64_t granularity,
> 
> Pre-existing, but as long as you are touching this, you might as well
> fix indentation of the other lines in the same signature.
> 
> > @@ -2158,19 +2195,33 @@ void qmp_drive_mirror(const char *device, const char *target,
> >          return;
> >      }
> >  
> > +    /* if we are planning to replace a graph node name the code should do a full
> > +     * mirror of the source image
> > +     */
> > +    if (has_to_replace_node_name && sync != MIRROR_SYNC_MODE_FULL) {
> > +        error_setg(errp,
> > +                   "to-replace-node-name can only be used with sync=full");
> > +        return;
> > +    }
> 
> I'm not sure I follow this restriction.  What's to prevent me from doing
> a shallow mirror coupled with the mode of reusing an existing file that
> already points to a sane backing file, rather than forcing a full sync?
>  That is, why not let this command be a fully-generic swap command,
> where the semantics are that as long as my old and new image have the
> same contents from the guest's perspective (or I'm replacing a broken
> file out of a quorum, and the new image has the same contents as the
> quorum majority), then we are just updating qemu to point to a new BDS.
> 
> On the other hand, back around the 1.5 timeframe, downstream RHEL tried
> to add a 'drive-reopen' command that did just that - replaced the
> backing file of a guest's disk with an arbitrary other file.  But it was
> so powerful and risky that at the time upstream finally added
> 'transaction' support, we decided to go with the simpler
> 'drive-mirror/block-job-complete' sequence as the only supported way to
> cause qemu to associate a different BDS with a guest image.  Of course,
> things have advanced since then, so maybe we finally are at a point
> where we want to expose a generic reopen command that can swap out
> arbitrary named nodes without interrupting guest services, but now I'm
> starting to wonder if it should be a new command instead of adding
> optional arguments to the existing drive-mirror.

Ok this feature won't go in QEMU 2.0.

What about implementing as an additional
drive-mirror-complete target-node-name=no_name_to_replace new-node-name=node_name_of_the_new_bs

The command would do the same work as block-job-complete plus some extras
work and would be used after starting a regular drive-mirror command.

Best regards

Benoît

> 
> > +++ b/qapi-schema.json
> > @@ -2140,6 +2140,14 @@
> >  # @format: #optional the format of the new destination, default is to
> >  #          probe if @mode is 'existing', else the format of the source
> >  #
> > +# @new-node-name: #optional the new block driver state node name in the graph
> > +#                 (Since 2.1)
> 
> Ah, so you're not trying to get this in before 2.0 freeze - which means
> we have more time to think about the implications.
> 
> > +#
> > +# @to-replace-node-name: #optional with sync=full graph node name to be
> > +#                        replaced by the new image when a whole image copy is
> > +#                        done. This can be used to repair broken Quorum files.
> > +#                        (Since 2.1)
> 
> This naming feels long, but I'm not sure if I have a better suggestion.
>  It looks like you only allow swapping out one quorum file per
> drive-mirror - but what if I have a 3/5 quorum and want to swap out two
> files at once?  Also, how does this interact with the 'transaction' command?
> 
> >  ##
> >  { 'command': 'drive-mirror',
> >    'data': { 'device': 'str', 'target': 'str', '*format': 'str',
> > -            'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
> > -            '*speed': 'int', '*granularity': 'uint32',
> > -            '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
> > +            '*new-node-name': 'str', '*to-replace-node-name': 'str',
> > +            'sync': 'MirrorSyncMode', '*mode': 'NewImageMode', '*speed': 'int',
> > +            '*granularity': 'uint32', '*buf-size': 'int',
> > +            '*on-source-error': 'BlockdevOnError',
> 
> Why the reindent of existing options?
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

[Qemu-devel] [PATCH 2/2] qemu-iotests: Add TestRepairQuorum to 041 to test drive-mirror node-name mode.

[Benoît Canet]

The the to-replace-node-name is designed to allow repairing of broken Quorum
file.
This patch introduce a new class TestRepairQuorum testing that the feature
works.
Some further work will be done on QEMU to improve the robutness of the tests.

Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
 tests/qemu-iotests/041     | 196 ++++++++++++++++++++++++++++++++++++++++++++-
 tests/qemu-iotests/041.out |   4 +-
 2 files changed, 194 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index ec470b2..866df87 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -28,6 +28,12 @@ target_backing_img = os.path.join(iotests.test_dir, 'target-backing.img')
 test_img = os.path.join(iotests.test_dir, 'test.img')
 target_img = os.path.join(iotests.test_dir, 'target.img')
 
+quorum_img1 = os.path.join(iotests.test_dir, 'quorum1.img')
+quorum_img2 = os.path.join(iotests.test_dir, 'quorum2.img')
+quorum_img3 = os.path.join(iotests.test_dir, 'quorum3.img')
+quorum_repair_img = os.path.join(iotests.test_dir, 'quorum_repair.img')
+quorum_snapshot_file = os.path.join(iotests.test_dir, 'quorum_snapshot.img')
+
 class ImageMirroringTestCase(iotests.QMPTestCase):
     '''Abstract base class for image mirroring test cases'''
 
@@ -42,8 +48,8 @@ class ImageMirroringTestCase(iotests.QMPTestCase):
                     ready = True
 
     def wait_ready_and_cancel(self, drive='drive0'):
-        self.wait_ready(drive)
-        event = self.cancel_and_wait()
+        self.wait_ready(drive=drive)
+        event = self.cancel_and_wait(drive=drive)
         self.assertEquals(event['event'], 'BLOCK_JOB_COMPLETED')
         self.assert_qmp(event, 'data/type', 'mirror')
         self.assert_qmp(event, 'data/offset', self.image_len)
@@ -52,12 +58,12 @@ class ImageMirroringTestCase(iotests.QMPTestCase):
     def complete_and_wait(self, drive='drive0', wait_ready=True):
         '''Complete a block job and wait for it to finish'''
         if wait_ready:
-            self.wait_ready()
+            self.wait_ready(drive=drive)
 
         result = self.vm.qmp('block-job-complete', device=drive)
         self.assert_qmp(result, 'return', {})
 
-        event = self.wait_until_completed()
+        event = self.wait_until_completed(drive=drive)
         self.assert_qmp(event, 'data/type', 'mirror')
 
 class TestSingleDrive(ImageMirroringTestCase):
@@ -718,5 +724,187 @@ class TestUnbackedSource(ImageMirroringTestCase):
         self.complete_and_wait()
         self.assert_no_active_block_jobs()
 
+class TestRepairQuorum(ImageMirroringTestCase):
+    """ This class test quorum file repair using drive-mirror.
+        It's mostly a fork of TestSingleDrive """
+    image_len = 1 * 1024 * 1024 # MB
+    IMAGES = [ quorum_img1, quorum_img2, quorum_img3 ]
+
+    def setUp(self):
+        self.vm = iotests.VM()
+
+        # Add each individual quorum images
+        for i in self.IMAGES:
+            qemu_img('create', '-f', iotests.imgfmt, i,
+                     str(TestSingleDrive.image_len))
+            # Assign a node name to each quorum image in order to manipulate
+            # them
+            opts = "node-name=img%i" % self.IMAGES.index(i)
+            self.vm = self.vm.add_drive(i, opts)
+
+        self.vm.launch()
+
+        #assemble the quorum block device from the individual files
+        args = { "options" : { "driver": "quorum", "id": "quorum0",
+                 "vote-threshold": 2, "children": [ "img0", "img1", "img2" ] } }
+        result = self.vm.qmp("blockdev-add", **args)
+        self.assert_qmp(result, 'return', {})
+
+
+    def tearDown(self):
+        self.vm.shutdown()
+        for i in self.IMAGES + [ quorum_repair_img ]:
+            # Do a try/except because the test may have deleted some images
+            try:
+                os.remove(i)
+            except OSError:
+                pass
+
+    def test_complete(self):
+        self.assert_no_active_block_jobs()
+
+        result = self.vm.qmp('drive-mirror', device='quorum0', sync='full',
+                             new_node_name="repair0",
+                             to_replace_node_name="img1",
+                             target=quorum_repair_img, format=iotests.imgfmt)
+        self.assert_qmp(result, 'return', {})
+
+        self.complete_and_wait(drive="quorum0")
+        result = self.vm.qmp('query-named-block-nodes')
+        self.assert_qmp(result, 'return[0]/file', quorum_repair_img)
+        # TODO: a better test requiring some QEMU infrastructure will be added
+        #       to check that this file is really driven by quorum
+        self.vm.shutdown()
+        self.assertTrue(iotests.compare_images(quorum_img2, quorum_repair_img),
+                        'target image does not match source after mirroring')
+
+    def test_cancel(self):
+        self.assert_no_active_block_jobs()
+
+        result = self.vm.qmp('drive-mirror', device='quorum0', sync='full',
+                             new_node_name="repair0",
+                             to_replace_node_name="img1",
+                             target=quorum_repair_img, format=iotests.imgfmt)
+        self.assert_qmp(result, 'return', {})
+
+        self.cancel_and_wait(drive="quorum0", force=True)
+        # here we check that the last registered quorum file has not been
+        # swapped out and unref
+        result = self.vm.qmp('query-named-block-nodes')
+        self.assert_qmp(result, 'return[0]/file', quorum_img3)
+        self.vm.shutdown()
+
+    def test_cancel_after_ready(self):
+        self.assert_no_active_block_jobs()
+
+        result = self.vm.qmp('drive-mirror', device='quorum0', sync='full',
+                             new_node_name="repair0",
+                             to_replace_node_name="img1",
+                             target=quorum_repair_img, format=iotests.imgfmt)
+        self.assert_qmp(result, 'return', {})
+
+        self.wait_ready_and_cancel(drive="quorum0")
+        result = self.vm.qmp('query-named-block-nodes')
+        # here we check that the last registered quorum file has not been
+        # swapped out and unref
+        self.assert_qmp(result, 'return[0]/file', quorum_img3)
+        self.vm.shutdown()
+        self.assertTrue(iotests.compare_images(quorum_img2, quorum_repair_img),
+                        'target image does not match source after mirroring')
+
+    def test_pause(self):
+        self.assert_no_active_block_jobs()
+
+        result = self.vm.qmp('drive-mirror', device='quorum0', sync='full',
+                             new_node_name="repair0",
+                             to_replace_node_name="img1",
+                             target=quorum_repair_img, format=iotests.imgfmt)
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('block-job-pause', device='quorum0')
+        self.assert_qmp(result, 'return', {})
+
+        time.sleep(1)
+        result = self.vm.qmp('query-block-jobs')
+        offset = self.dictpath(result, 'return[0]/offset')
+
+        time.sleep(1)
+        result = self.vm.qmp('query-block-jobs')
+        self.assert_qmp(result, 'return[0]/offset', offset)
+
+        result = self.vm.qmp('block-job-resume', device='quorum0')
+        self.assert_qmp(result, 'return', {})
+
+        self.complete_and_wait(drive="quorum0")
+        self.vm.shutdown()
+        self.assertTrue(iotests.compare_images(quorum_img2, quorum_repair_img),
+                        'target image does not match source after mirroring')
+
+    def test_medium_not_found(self):
+        result = self.vm.qmp('drive-mirror', device='ide1-cd0', sync='full',
+                             new_node_name='repair0',
+                             to_replace_node_name='img1',
+                             target=quorum_repair_img, format=iotests.imgfmt)
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+    def test_image_not_found(self):
+        result = self.vm.qmp('drive-mirror', device='quorum0', sync='full',
+                             new_node_name='repair0',
+                             to_replace_node_name='img1',
+                             mode='existing',
+                             target=quorum_repair_img, format=iotests.imgfmt)
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+    def test_device_not_found(self):
+        result = self.vm.qmp('drive-mirror', device='nonexistent', sync='full',
+                             new_node_name='repair0',
+                             to_replace_node_name='img1',
+                             target=quorum_repair_img, format=iotests.imgfmt)
+        self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+
+    def test_wrong_sync_mode(self):
+        result = self.vm.qmp('drive-mirror', device='quorum0',
+                             new_node_name='repair0',
+                             to_replace_node_name='img1',
+                             target=quorum_repair_img, format=iotests.imgfmt)
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+    def test_no_new_node_name(self):
+        result = self.vm.qmp('drive-mirror', device='quorum0', sync='full',
+                             to_replace_node_name='img1',
+                             target=quorum_repair_img, format=iotests.imgfmt)
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+    def test_unexistant_to_replace_node_name(self):
+        result = self.vm.qmp('drive-mirror', device='quorum0', sync='full',
+                             new_node_name='repair0',
+                             to_replace_node_name='img77',
+                             target=quorum_repair_img, format=iotests.imgfmt)
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+    def test_after_a_quorum_snapshot(self):
+        result = self.vm.qmp('blockdev-snapshot-sync', node_name='img1',
+                             snapshot_file=quorum_snapshot_file,
+                             snapshot_node_name="snap1");
+
+        result = self.vm.qmp('drive-mirror', device='quorum0', sync='full',
+                             new_node_name='repair0',
+                             to_replace_node_name="img1",
+                             target=quorum_repair_img, format=iotests.imgfmt)
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+        result = self.vm.qmp('drive-mirror', device='quorum0', sync='full',
+                             new_node_name='repair0',
+                             to_replace_node_name="snap1",
+                             target=quorum_repair_img, format=iotests.imgfmt)
+        self.assert_qmp(result, 'return', {})
+
+        self.complete_and_wait(drive="quorum0")
+        result = self.vm.qmp('query-named-block-nodes')
+        self.assert_qmp(result, 'return[0]/file', quorum_repair_img)
+        # TODO: a better test requiring some QEMU infrastructure will be added
+        #       to check that this file is really driven by quorum
+        self.vm.shutdown()
+
 if __name__ == '__main__':
     iotests.main(supported_fmts=['qcow2', 'qed'])
diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out
index 6d9bee1..73e375a 100644
--- a/tests/qemu-iotests/041.out
+++ b/tests/qemu-iotests/041.out
@@ -1,5 +1,5 @@
-...........................
+......................................
 ----------------------------------------------------------------------
-Ran 27 tests
+Ran 38 tests
 
 OK
-- 
1.8.3.2