[Qemu-devel] [PATCH v3 0/5] block/mirror: Fix (?) target backing BDS

[Qemu-devel] [PATCH v3 0/5] block/mirror: Fix (?) target backing BDS

[Max Reitz]

Issue #1: If the target image does not have a backing BDS before mirror
completion, qemu tries really hard to give it a backing BDS. If the
source has a backing BDS, it will actually always "succeed".
In some cases, the target is not supposed to have a backing BDS, though
(absolute-paths: because of sync=full; existing: because the target
image does not have a backing file; blockdev-mirror: because of an
explicit "backing": ""). Then, this is pretty bad behavior.

This should generally not change the target's visible data, but it still
is ugly.

Issue #2: Currently the backing chain of the target is basically opened
using bdrv_open_backing_file() (except for sometimes™). This results in
multiple BDSs for a single physical file, which is bad. In most use
cases, this is only temporary, but it still is bad.

If we can reuse the existing backing chain of the source (which is with
drive-mirror in "absolute-paths" mode), we should just do so.


v3:
- Patch 1:
  - More verbose commit message [Kevin]
  - Changed comment to match code [Kevin]
- Patch 2:
  - Do not force use of the source backing chain for the target in
    "existing" mode or with blockdev-mirror [Kevin]
    - Instead keep doing what we've been doing for
      drive-mirror/existing, only that we should still drop the
      bdrv_set_backing_hd() in bdrv_replace_in_backing_chain()
    - And for blockdev-mirror, just do not change the current backing
      chain at all; this is what we've been doing until now, unless the
      target BDS did not have a backing BDS yet
- Patch 3: Added, because it makes the next test a bit nicer
- Patch 4: Adjusted to v3 behavior, and added a new test for
  blockdev-mirror with a target whose backing file has been overridden
  using the "backing" option
- Patch 5: Added [Kevin]


git-backport-diff against v2:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/5:[0005] [FC] 'block: Allow replacement of a BDS by its overlay'
002/5:[0057] [FC] 'block/mirror: Fix target backing BDS'
003/5:[down] 'block/null: Implement bdrv_refresh_filename()'
004/5:[0073] [FC] 'iotests: Add test for post-mirror backing chains'
005/5:[down] 'iotests: Add test for oVirt-like storage migration'


Max Reitz (5):
  block: Allow replacement of a BDS by its overlay
  block/mirror: Fix target backing BDS
  block/null: Implement bdrv_refresh_filename()
  iotests: Add test for post-mirror backing chains
  iotests: Add test for oVirt-like storage migration

 block.c                    |  24 +++--
 block/mirror.c             |  39 +++++--
 block/null.c               |  20 ++++
 blockdev.c                 |  15 ++-
 include/block/block_int.h  |  18 +++-
 tests/qemu-iotests/155     | 263 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/155.out |   5 +
 tests/qemu-iotests/156     | 174 ++++++++++++++++++++++++++++++
 tests/qemu-iotests/156.out |  48 +++++++++
 tests/qemu-iotests/group   |   2 +
 10 files changed, 584 insertions(+), 24 deletions(-)
 create mode 100755 tests/qemu-iotests/155
 create mode 100644 tests/qemu-iotests/155.out
 create mode 100755 tests/qemu-iotests/156
 create mode 100644 tests/qemu-iotests/156.out

-- 
2.8.3

[Qemu-devel] [PATCH v3 1/5] block: Allow replacement of a BDS by its overlay

[Max Reitz]

change_parent_backing_link() asserts that the BDS to be replaced is not
used as a backing file. However, we may want to replace a BDS by its
overlay in which case that very link should not be redirected.

For instance, when doing a sync=none drive-mirror operation, we may have
the following BDS/BB forest before block job completion:

  target

  base <- source <- BlockBackend

During job completion, we want to establish the source BDS as the
target's backing node:

          target
            |
            v
  base <- source <- BlockBackend

This makes the target a valid replacement for the source:

          target <- BlockBackend
            |
            v
  base <- source

Without this modification to change_parent_backing_link() we have to
inject the target into the graph before the source is its backing node,
thus temporarily creating a wrong graph:

  target <- BlockBackend

  base <- source

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index f54bc25..dc76c159 100644
--- a/block.c
+++ b/block.c
@@ -2224,9 +2224,23 @@ void bdrv_close_all(void)
 static void change_parent_backing_link(BlockDriverState *from,
                                        BlockDriverState *to)
 {
-    BdrvChild *c, *next;
+    BdrvChild *c, *next, *to_c;
 
     QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) {
+        if (c->role == &child_backing) {
+            /* @from is generally not allowed to be a backing file, except for
+             * when @to is the overlay. In that case, @from may not be replaced
+             * by @to as @to's backing node. */
+            QLIST_FOREACH(to_c, &to->children, next) {
+                if (to_c == c) {
+                    break;
+                }
+            }
+            if (to_c) {
+                continue;
+            }
+        }
+
         assert(c->role != &child_backing);
         bdrv_ref(to);
         bdrv_replace_child(c, to);
-- 
2.8.3

[Qemu-devel] [PATCH v3 2/5] block/mirror: Fix target backing BDS

[Max Reitz]

Currently, we are trying to move the backing BDS from the source to the
target in bdrv_replace_in_backing_chain() which is called from
mirror_exit(). However, mirror_complete() already tries to open the
target's backing chain with a call to bdrv_open_backing_file().

First, we should only set the target's backing BDS once. Second, the
mirroring block job has a better idea of what to set it to than the
generic code in bdrv_replace_in_backing_chain() (in fact, the latter's
conditions on when to move the backing BDS from source to target are not
really correct).

Therefore, remove that code from bdrv_replace_in_backing_chain() and
leave it to mirror_complete().

Depending on what kind of mirroring is performed, we furthermore want to
use different strategies to open the target's backing chain:

- If blockdev-mirror is used, we can assume the user made sure that the
  target already has the correct backing chain. In particular, we should
  not try to open a backing file if the target does not have any yet.

- If drive-mirror with mode=absolute-paths is used, we can and should
  reuse the already existing chain of nodes that the source BDS is in.
  In case of sync=full, no backing BDS is required; with sync=top, we
  just link the source's backing BDS to the target, and with sync=none,
  we use the source BDS as the target's backing BDS.
  We should not try to open these backing files anew because this would
  lead to two BDSs existing per physical file in the backing chain, and
  we would like to avoid such concurrent access.

- If drive-mirror with mode=existing is used, we have to use the
  information provided in the physical image file which means opening
  the target's backing chain completely anew, just as it has been done
  already.
  If the target's backing chain shares images with the source, this may
  lead to multiple BDSs per physical image file. But since we cannot
  reliably ascertain this case, there is nothing we can do about it.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c                   |  8 --------
 block/mirror.c            | 39 ++++++++++++++++++++++++++++-----------
 blockdev.c                | 15 ++++++++++++---
 include/block/block_int.h | 18 +++++++++++++++++-
 4 files changed, 57 insertions(+), 23 deletions(-)

diff --git a/block.c b/block.c
index dc76c159..2691c2f 100644
--- a/block.c
+++ b/block.c
@@ -2289,14 +2289,6 @@ void bdrv_replace_in_backing_chain(BlockDriverState *old, BlockDriverState *new)
 
     change_parent_backing_link(old, new);
 
-    /* Change backing files if a previously independent node is added to the
-     * chain. For active commit, we replace top by its own (indirect) backing
-     * file and don't do anything here so we don't build a loop. */
-    if (new->backing == NULL && !bdrv_chain_contains(backing_bs(old), new)) {
-        bdrv_set_backing_hd(new, backing_bs(old));
-        bdrv_set_backing_hd(old, NULL);
-    }
-
     bdrv_unref(old);
 }
 
diff --git a/block/mirror.c b/block/mirror.c
index 80fd3c7..13abe8c 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -44,6 +44,7 @@ typedef struct MirrorBlockJob {
     /* Used to block operations on the drive-mirror-replace target */
     Error *replace_blocker;
     bool is_none_mode;
+    BlockMirrorBackingMode backing_mode;
     BlockdevOnError on_source_error, on_target_error;
     bool synced;
     bool should_complete;
@@ -742,20 +743,26 @@ static void mirror_set_speed(BlockJob *job, int64_t speed, Error **errp)
 static void mirror_complete(BlockJob *job, Error **errp)
 {
     MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
-    Error *local_err = NULL;
-    int ret;
+    BlockDriverState *src, *target;
+
+    src = blk_bs(job->blk);
+    target = blk_bs(s->target);
 
-    ret = bdrv_open_backing_file(blk_bs(s->target), NULL, "backing",
-                                 &local_err);
-    if (ret < 0) {
-        error_propagate(errp, local_err);
-        return;
-    }
     if (!s->synced) {
         error_setg(errp, QERR_BLOCK_JOB_NOT_READY, job->id);
         return;
     }
 
+    if (s->backing_mode == MIRROR_OPEN_BACKING_CHAIN) {
+        int ret;
+
+        assert(!target->backing);
+        ret = bdrv_open_backing_file(target, NULL, "backing", errp);
+        if (ret < 0) {
+            return;
+        }
+    }
+
     /* check the target bs is not blocked and block all operations on it */
     if (s->replaces) {
         AioContext *replace_aio_context;
@@ -777,6 +784,13 @@ static void mirror_complete(BlockJob *job, Error **errp)
         aio_context_release(replace_aio_context);
     }
 
+    if (s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
+        BlockDriverState *backing = s->is_none_mode ? src : s->base;
+        if (backing_bs(target) != backing) {
+            bdrv_set_backing_hd(target, backing);
+        }
+    }
+
     s->should_complete = true;
     block_job_enter(&s->common);
 }
@@ -799,6 +813,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
                              const char *replaces,
                              int64_t speed, uint32_t granularity,
                              int64_t buf_size,
+                             BlockMirrorBackingMode backing_mode,
                              BlockdevOnError on_source_error,
                              BlockdevOnError on_target_error,
                              bool unmap,
@@ -836,6 +851,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
     s->on_source_error = on_source_error;
     s->on_target_error = on_target_error;
     s->is_none_mode = is_none_mode;
+    s->backing_mode = backing_mode;
     s->base = base;
     s->granularity = granularity;
     s->buf_size = ROUND_UP(buf_size, granularity);
@@ -859,7 +875,8 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
 void mirror_start(BlockDriverState *bs, BlockDriverState *target,
                   const char *replaces,
                   int64_t speed, uint32_t granularity, int64_t buf_size,
-                  MirrorSyncMode mode, BlockdevOnError on_source_error,
+                  MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
+                  BlockdevOnError on_source_error,
                   BlockdevOnError on_target_error,
                   bool unmap,
                   BlockCompletionFunc *cb,
@@ -875,7 +892,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
     is_none_mode = mode == MIRROR_SYNC_MODE_NONE;
     base = mode == MIRROR_SYNC_MODE_TOP ? backing_bs(bs) : NULL;
     mirror_start_job(bs, target, replaces,
-                     speed, granularity, buf_size,
+                     speed, granularity, buf_size, backing_mode,
                      on_source_error, on_target_error, unmap, cb, opaque, errp,
                      &mirror_job_driver, is_none_mode, base);
 }
@@ -922,7 +939,7 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
         }
     }
 
-    mirror_start_job(bs, base, NULL, speed, 0, 0,
+    mirror_start_job(bs, base, NULL, speed, 0, 0, MIRROR_LEAVE_BACKING_CHAIN,
                      on_error, on_error, false, cb, opaque, &local_err,
                      &commit_active_job_driver, false, base);
     if (local_err) {
diff --git a/blockdev.c b/blockdev.c
index 525645b..b4cd37b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3423,6 +3423,7 @@ static void blockdev_mirror_common(BlockDriverState *bs,
                                    BlockDriverState *target,
                                    bool has_replaces, const char *replaces,
                                    enum MirrorSyncMode sync,
+                                   BlockMirrorBackingMode backing_mode,
                                    bool has_speed, int64_t speed,
                                    bool has_granularity, uint32_t granularity,
                                    bool has_buf_size, int64_t buf_size,
@@ -3480,7 +3481,7 @@ static void blockdev_mirror_common(BlockDriverState *bs,
      */
     mirror_start(bs, target,
                  has_replaces ? replaces : NULL,
-                 speed, granularity, buf_size, sync,
+                 speed, granularity, buf_size, sync, backing_mode,
                  on_source_error, on_target_error, unmap,
                  block_job_cb, bs, errp);
 }
@@ -3503,6 +3504,7 @@ void qmp_drive_mirror(const char *device, const char *target,
     BlockBackend *blk;
     BlockDriverState *source, *target_bs;
     AioContext *aio_context;
+    BlockMirrorBackingMode backing_mode;
     Error *local_err = NULL;
     QDict *options = NULL;
     int flags;
@@ -3576,6 +3578,12 @@ void qmp_drive_mirror(const char *device, const char *target,
         }
     }
 
+    if (mode == NEW_IMAGE_MODE_ABSOLUTE_PATHS) {
+        backing_mode = MIRROR_SOURCE_BACKING_CHAIN;
+    } else {
+        backing_mode = MIRROR_OPEN_BACKING_CHAIN;
+    }
+
     if ((sync == MIRROR_SYNC_MODE_FULL || !source)
         && mode != NEW_IMAGE_MODE_EXISTING)
     {
@@ -3624,7 +3632,7 @@ void qmp_drive_mirror(const char *device, const char *target,
     bdrv_set_aio_context(target_bs, aio_context);
 
     blockdev_mirror_common(bs, target_bs,
-                           has_replaces, replaces, sync,
+                           has_replaces, replaces, sync, backing_mode,
                            has_speed, speed,
                            has_granularity, granularity,
                            has_buf_size, buf_size,
@@ -3656,6 +3664,7 @@ void qmp_blockdev_mirror(const char *device, const char *target,
     BlockBackend *blk;
     BlockDriverState *target_bs;
     AioContext *aio_context;
+    BlockMirrorBackingMode backing_mode = MIRROR_LEAVE_BACKING_CHAIN;
     Error *local_err = NULL;
 
     blk = blk_by_name(device);
@@ -3681,7 +3690,7 @@ void qmp_blockdev_mirror(const char *device, const char *target,
     bdrv_set_aio_context(target_bs, aio_context);
 
     blockdev_mirror_common(bs, target_bs,
-                           has_replaces, replaces, sync,
+                           has_replaces, replaces, sync, backing_mode,
                            has_speed, speed,
                            has_granularity, granularity,
                            has_buf_size, buf_size,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8a4963c..a9249a3 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -510,6 +510,20 @@ struct BlockBackendRootState {
     BlockdevDetectZeroesOptions detect_zeroes;
 };
 
+typedef enum BlockMirrorBackingMode {
+    /* Reuse the existing backing chain from the source for the target.
+     * - sync=full: Set backing BDS to NULL.
+     * - sync=top:  Use source's backing BDS.
+     * - sync=none: Use source as the backing BDS. */
+    MIRROR_SOURCE_BACKING_CHAIN,
+
+    /* Open the target's backing chain completely anew */
+    MIRROR_OPEN_BACKING_CHAIN,
+
+    /* Do not change the target's backing BDS after job completion */
+    MIRROR_LEAVE_BACKING_CHAIN,
+} BlockMirrorBackingMode;
+
 static inline BlockDriverState *backing_bs(BlockDriverState *bs)
 {
     return bs->backing ? bs->backing->bs : NULL;
@@ -672,6 +686,7 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
  * @granularity: The chosen granularity for the dirty bitmap.
  * @buf_size: The amount of data that can be in flight at one time.
  * @mode: Whether to collapse all images in the chain to the target.
+ * @backing_mode: How to establish the target's backing chain after completion.
  * @on_source_error: The action to take upon error reading from the source.
  * @on_target_error: The action to take upon error writing to the target.
  * @unmap: Whether to unmap target where source sectors only contain zeroes.
@@ -687,7 +702,8 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
 void mirror_start(BlockDriverState *bs, BlockDriverState *target,
                   const char *replaces,
                   int64_t speed, uint32_t granularity, int64_t buf_size,
-                  MirrorSyncMode mode, BlockdevOnError on_source_error,
+                  MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
+                  BlockdevOnError on_source_error,
                   BlockdevOnError on_target_error,
                   bool unmap,
                   BlockCompletionFunc *cb,
-- 
2.8.3

[Qemu-devel] [PATCH v3 3/5] block/null: Implement bdrv_refresh_filename()

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/null.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/block/null.c b/block/null.c
index 396500b..b511010 100644
--- a/block/null.c
+++ b/block/null.c
@@ -12,6 +12,8 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qstring.h"
 #include "block/block_int.h"
 
 #define NULL_OPT_LATENCY "latency-ns"
@@ -223,6 +225,20 @@ static int64_t coroutine_fn null_co_get_block_status(BlockDriverState *bs,
     }
 }
 
+static void null_refresh_filename(BlockDriverState *bs, QDict *opts)
+{
+    QINCREF(opts);
+    qdict_del(opts, "filename");
+
+    if (!qdict_size(opts)) {
+        snprintf(bs->exact_filename, sizeof(bs->exact_filename), "%s://",
+                 bs->drv->format_name);
+    }
+
+    qdict_put(opts, "driver", qstring_from_str(bs->drv->format_name));
+    bs->full_open_options = opts;
+}
+
 static BlockDriver bdrv_null_co = {
     .format_name            = "null-co",
     .protocol_name          = "null-co",
@@ -238,6 +254,8 @@ static BlockDriver bdrv_null_co = {
     .bdrv_reopen_prepare    = null_reopen_prepare,
 
     .bdrv_co_get_block_status   = null_co_get_block_status,
+
+    .bdrv_refresh_filename  = null_refresh_filename,
 };
 
 static BlockDriver bdrv_null_aio = {
@@ -255,6 +273,8 @@ static BlockDriver bdrv_null_aio = {
     .bdrv_reopen_prepare    = null_reopen_prepare,
 
     .bdrv_co_get_block_status   = null_co_get_block_status,
+
+    .bdrv_refresh_filename  = null_refresh_filename,
 };
 
 static void bdrv_null_init(void)
-- 
2.8.3

[Qemu-devel] [PATCH v3 4/5] iotests: Add test for post-mirror backing chains

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/155     | 263 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/155.out |   5 +
 tests/qemu-iotests/group   |   1 +
 3 files changed, 269 insertions(+)
 create mode 100755 tests/qemu-iotests/155
 create mode 100644 tests/qemu-iotests/155.out

diff --git a/tests/qemu-iotests/155 b/tests/qemu-iotests/155
new file mode 100755
index 0000000..06ddc5f
--- /dev/null
+++ b/tests/qemu-iotests/155
@@ -0,0 +1,263 @@
+#!/usr/bin/env python
+#
+# Test whether the backing BDSs are correct after completion of a
+# mirror block job; in "existing" modes (drive-mirror with
+# mode=existing and blockdev-mirror) the backing chain should not be
+# overridden.
+#
+# Copyright (C) 2016 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+import os
+import stat
+import time
+import iotests
+from iotests import qemu_img
+
+back0_img = os.path.join(iotests.test_dir, 'back0.' + iotests.imgfmt)
+back1_img = os.path.join(iotests.test_dir, 'back1.' + iotests.imgfmt)
+back2_img = os.path.join(iotests.test_dir, 'back2.' + iotests.imgfmt)
+source_img = os.path.join(iotests.test_dir, 'source.' + iotests.imgfmt)
+target_img = os.path.join(iotests.test_dir, 'target.' + iotests.imgfmt)
+
+
+# Class variables for controlling its behavior:
+#
+# existing: If True, explicitly create the target image and blockdev-add it
+# target_backing: If existing is True: Use this filename as the backing file
+#                 of the target image
+#                 (None: no backing file)
+# target_blockdev_backing: If existing is True: Pass this dict as "backing"
+#                          for the blockdev-add command
+#                          (None: do not pass "backing")
+# target_real_backing: If existing is True: The real filename of the backing
+#                      image during runtime, only makes sense if
+#                      target_blockdev_backing is not None
+#                      (None: same as target_backing)
+
+class BaseClass(iotests.QMPTestCase):
+    target_blockdev_backing = None
+    target_real_backing = None
+
+    def setUp(self):
+        qemu_img('create', '-f', iotests.imgfmt, back0_img, '1M')
+        qemu_img('create', '-f', iotests.imgfmt, '-b', back0_img, back1_img)
+        qemu_img('create', '-f', iotests.imgfmt, '-b', back1_img, back2_img)
+        qemu_img('create', '-f', iotests.imgfmt, '-b', back2_img, source_img)
+
+        self.vm = iotests.VM()
+        self.vm.add_drive(None, '', 'none')
+        self.vm.launch()
+
+        # Add the BDS via blockdev-add so it stays around after the mirror block
+        # job has been completed
+        result = self.vm.qmp('blockdev-add',
+                             options={'node-name': 'source',
+                                      'driver': iotests.imgfmt,
+                                      'file': {'driver': 'file',
+                                               'filename': source_img}})
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('x-blockdev-insert-medium',
+                             device='drive0', node_name='source')
+        self.assert_qmp(result, 'return', {})
+
+        self.assertIntactSourceBackingChain()
+
+        if self.existing:
+            if self.target_backing:
+                qemu_img('create', '-f', iotests.imgfmt,
+                         '-b', self.target_backing, target_img, '1M')
+            else:
+                qemu_img('create', '-f', iotests.imgfmt, target_img, '1M')
+
+            if self.cmd == 'blockdev-mirror':
+                options = { 'node-name': 'target',
+                            'driver': iotests.imgfmt,
+                            'file': { 'driver': 'file',
+                                      'filename': target_img } }
+                if self.target_blockdev_backing:
+                    options['backing'] = self.target_blockdev_backing
+
+                result = self.vm.qmp('blockdev-add', options=options)
+                self.assert_qmp(result, 'return', {})
+
+    def tearDown(self):
+        self.vm.shutdown()
+        os.remove(source_img)
+        os.remove(back2_img)
+        os.remove(back1_img)
+        os.remove(back0_img)
+        try:
+            os.remove(target_img)
+        except OSError:
+            pass
+
+    def findBlockNode(self, node_name, id=None):
+        if id:
+            result = self.vm.qmp('query-block')
+            for device in result['return']:
+                if device['device'] == id:
+                    if node_name:
+                        self.assert_qmp(device, 'inserted/node-name', node_name)
+                    return device['inserted']
+        else:
+            result = self.vm.qmp('query-named-block-nodes')
+            for node in result['return']:
+                if node['node-name'] == node_name:
+                    return node
+
+        self.fail('Cannot find node %s/%s' % (id, node_name))
+
+    def assertIntactSourceBackingChain(self):
+        node = self.findBlockNode('source')
+
+        self.assert_qmp(node, 'image' + '/backing-image' * 0 + '/filename',
+                        source_img)
+        self.assert_qmp(node, 'image' + '/backing-image' * 1 + '/filename',
+                        back2_img)
+        self.assert_qmp(node, 'image' + '/backing-image' * 2 + '/filename',
+                        back1_img)
+        self.assert_qmp(node, 'image' + '/backing-image' * 3 + '/filename',
+                        back0_img)
+        self.assert_qmp_absent(node, 'image' + '/backing-image' * 4)
+
+    def assertCorrectBackingImage(self, node, default_image):
+        if self.existing:
+            if self.target_real_backing:
+                image = self.target_real_backing
+            else:
+                image = self.target_backing
+        else:
+            image = default_image
+
+        if image:
+            self.assert_qmp(node, 'image/backing-image/filename', image)
+        else:
+            self.assert_qmp_absent(node, 'image/backing-image')
+
+
+# Class variables for controlling its behavior:
+#
+# cmd: Mirroring command to execute, either drive-mirror or blockdev-mirror
+
+class MirrorBaseClass(BaseClass):
+    def runMirror(self, sync):
+        if self.cmd == 'blockdev-mirror':
+            result = self.vm.qmp(self.cmd, device='drive0', sync=sync,
+                                 target='target')
+        else:
+            if self.existing:
+                mode = 'existing'
+            else:
+                mode = 'absolute-paths'
+            result = self.vm.qmp(self.cmd, device='drive0', sync=sync,
+                                 target=target_img, format=iotests.imgfmt,
+                                 mode=mode, node_name='target')
+
+        self.assert_qmp(result, 'return', {})
+
+        self.vm.event_wait('BLOCK_JOB_READY')
+
+        result = self.vm.qmp('block-job-complete', device='drive0')
+        self.assert_qmp(result, 'return', {})
+
+        self.vm.event_wait('BLOCK_JOB_COMPLETED')
+
+    def testFull(self):
+        self.runMirror('full')
+
+        node = self.findBlockNode('target', 'drive0')
+        self.assertCorrectBackingImage(node, None)
+        self.assertIntactSourceBackingChain()
+
+    def testTop(self):
+        self.runMirror('top')
+
+        node = self.findBlockNode('target', 'drive0')
+        self.assertCorrectBackingImage(node, back2_img)
+        self.assertIntactSourceBackingChain()
+
+    def testNone(self):
+        self.runMirror('none')
+
+        node = self.findBlockNode('target', 'drive0')
+        self.assertCorrectBackingImage(node, source_img)
+        self.assertIntactSourceBackingChain()
+
+
+class TestDriveMirrorAbsolutePaths(MirrorBaseClass):
+    cmd = 'drive-mirror'
+    existing = False
+
+class TestDriveMirrorExistingNoBacking(MirrorBaseClass):
+    cmd = 'drive-mirror'
+    existing = True
+    target_backing = None
+
+class TestDriveMirrorExistingBacking(MirrorBaseClass):
+    cmd = 'drive-mirror'
+    existing = True
+    target_backing = 'null-co://'
+
+class TestBlockdevMirrorNoBacking(MirrorBaseClass):
+    cmd = 'blockdev-mirror'
+    existing = True
+    target_backing = None
+
+class TestBlockdevMirrorBacking(MirrorBaseClass):
+    cmd = 'blockdev-mirror'
+    existing = True
+    target_backing = 'null-co://'
+
+class TestBlockdevMirrorForcedBacking(MirrorBaseClass):
+    cmd = 'blockdev-mirror'
+    existing = True
+    target_backing = None
+    target_blockdev_backing = { 'driver': 'null-co' }
+    target_real_backing = 'null-co://'
+
+
+class TestCommit(BaseClass):
+    existing = False
+
+    def testCommit(self):
+        result = self.vm.qmp('block-commit', device='drive0', base=back1_img)
+        self.assert_qmp(result, 'return', {})
+
+        self.vm.event_wait('BLOCK_JOB_READY')
+
+        result = self.vm.qmp('block-job-complete', device='drive0')
+        self.assert_qmp(result, 'return', {})
+
+        self.vm.event_wait('BLOCK_JOB_COMPLETED')
+
+        node = self.findBlockNode(None, 'drive0')
+        self.assert_qmp(node, 'image' + '/backing-image' * 0 + '/filename',
+                        back1_img)
+        self.assert_qmp(node, 'image' + '/backing-image' * 1 + '/filename',
+                        back0_img)
+        self.assert_qmp_absent(node, 'image' + '/backing-image' * 2 +
+                               '/filename')
+
+        self.assertIntactSourceBackingChain()
+
+
+BaseClass = None
+MirrorBaseClass = None
+
+if __name__ == '__main__':
+    iotests.main(supported_fmts=['qcow2'])
diff --git a/tests/qemu-iotests/155.out b/tests/qemu-iotests/155.out
new file mode 100644
index 0000000..4176bb9
--- /dev/null
+++ b/tests/qemu-iotests/155.out
@@ -0,0 +1,5 @@
+...................
+----------------------------------------------------------------------
+Ran 19 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index ab1d76e..9f1f2c0 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -154,3 +154,4 @@
 150 rw auto quick
 152 rw auto quick
 154 rw auto backing quick
+155 rw auto
-- 
2.8.3

[Qemu-devel] [PATCH v3 5/5] iotests: Add test for oVirt-like storage migration

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/156     | 174 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/156.out |  48 +++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 223 insertions(+)
 create mode 100755 tests/qemu-iotests/156
 create mode 100644 tests/qemu-iotests/156.out

diff --git a/tests/qemu-iotests/156 b/tests/qemu-iotests/156
new file mode 100755
index 0000000..cc95ff1
--- /dev/null
+++ b/tests/qemu-iotests/156
@@ -0,0 +1,174 @@
+#!/bin/bash
+#
+# Tests oVirt-like storage migration:
+#  - Create snapshot
+#  - Create target image with (not yet existing) target backing chain
+#    (i.e. just write the name of a soon-to-be-copied-over backing file into it)
+#  - drive-mirror the snapshot to the target with mode=existing and sync=top
+#  - In the meantime, copy the original source files to the destination via
+#    conventional means (i.e. outside of qemu)
+#  - Complete the drive-mirror job
+#  - Delete all source images
+#
+# Copyright (C) 2016 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=mreitz@redhat.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+status=1	# failure is the default!
+
+_cleanup()
+{
+    rm -f "$TEST_IMG{,.target}{,.backing,.overlay}"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+
+_supported_fmt qcow2 qed
+_supported_proto generic
+_supported_os Linux
+
+# Create source disk
+TEST_IMG="$TEST_IMG.backing" _make_test_img 1M
+_make_test_img -b "$TEST_IMG.backing" 1M
+
+$QEMU_IO -c 'write -P 1 0 256k' "$TEST_IMG.backing" | _filter_qemu_io
+$QEMU_IO -c 'write -P 2 64k 192k' "$TEST_IMG" | _filter_qemu_io
+
+_launch_qemu -drive if=none,id=source,file="$TEST_IMG"
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'qmp_capabilities' }" \
+    'return'
+
+# Create snapshot
+TEST_IMG="$TEST_IMG.overlay" _make_test_img -b "$TEST_IMG" 1M
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'blockdev-snapshot-sync',
+       'arguments': { 'device': 'source',
+                      'snapshot-file': '$TEST_IMG.overlay',
+                      'format': '$IMGFMT',
+                      'mode': 'existing' } }" \
+    'return'
+
+# Write something to the snapshot
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'human-monitor-command',
+       'arguments': { 'command-line':
+                      'qemu-io source \"write -P 3 128k 128k\"' } }" \
+    'return'
+
+# Create target image
+TEST_IMG="$TEST_IMG.target.overlay" _make_test_img -b "$TEST_IMG.target" 1M
+
+# Mirror snapshot
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'drive-mirror',
+       'arguments': { 'device': 'source',
+                      'target': '$TEST_IMG.target.overlay',
+                      'mode': 'existing',
+                      'sync': 'top' } }" \
+    'return'
+
+# Wait for convergence
+_send_qemu_cmd $QEMU_HANDLE \
+    '' \
+    'BLOCK_JOB_READY'
+
+# Write some more
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'human-monitor-command',
+       'arguments': { 'command-line':
+                      'qemu-io source \"write -P 4 192k 64k\"' } }" \
+    'return'
+
+# Copy source backing chain to the target before completing the job
+cp "$TEST_IMG.backing" "$TEST_IMG.target.backing"
+cp "$TEST_IMG" "$TEST_IMG.target"
+$QEMU_IMG rebase -u -b "$TEST_IMG.target.backing" "$TEST_IMG.target"
+
+# Complete block job
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'block-job-complete',
+       'arguments': { 'device': 'source' } }" \
+    ''
+
+_send_qemu_cmd $QEMU_HANDLE \
+    '' \
+    'BLOCK_JOB_COMPLETED'
+
+# Remove the source images
+rm -f "$TEST_IMG{,.backing,.overlay}"
+
+echo
+
+# Check online disk contents
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'human-monitor-command',
+       'arguments': { 'command-line':
+                      'qemu-io source \"read -P 1 0k 64k\"' } }" \
+    'return'
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'human-monitor-command',
+       'arguments': { 'command-line':
+                      'qemu-io source \"read -P 2 64k 64k\"' } }" \
+    'return'
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'human-monitor-command',
+       'arguments': { 'command-line':
+                      'qemu-io source \"read -P 3 128k 64k\"' } }" \
+    'return'
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'human-monitor-command',
+       'arguments': { 'command-line':
+                      'qemu-io source \"read -P 4 192k 64k\"' } }" \
+    'return'
+
+echo
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'quit' }" \
+    'return'
+
+wait=1 _cleanup_qemu
+
+echo
+
+# Check offline disk contents
+$QEMU_IO -c 'read -P 1 0k 64k' \
+         -c 'read -P 2 64k 64k' \
+         -c 'read -P 3 128k 64k' \
+         -c 'read -P 4 192k 64k' \
+         "$TEST_IMG.target.overlay" | _filter_qemu_io
+
+echo
+
+# success, all done
+echo '*** done'
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/156.out b/tests/qemu-iotests/156.out
new file mode 100644
index 0000000..3af82ae
--- /dev/null
+++ b/tests/qemu-iotests/156.out
@@ -0,0 +1,48 @@
+QA output created by 156
+Formatting 'TEST_DIR/t.IMGFMT.backing', fmt=IMGFMT size=1048576
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.IMGFMT.backing
+wrote 262144/262144 bytes at offset 0
+256 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 196608/196608 bytes at offset 65536
+192 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+{"return": {}}
+Formatting 'TEST_DIR/t.IMGFMT.overlay', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.IMGFMT
+{"return": {}}
+wrote 131072/131072 bytes at offset 131072
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+{"return": ""}
+Formatting 'TEST_DIR/t.IMGFMT.target.overlay', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.IMGFMT.target
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "source", "len": 131072, "offset": 131072, "speed": 0, "type": "mirror"}}
+wrote 65536/65536 bytes at offset 196608
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+{"return": ""}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "source", "len": 196608, "offset": 196608, "speed": 0, "type": "mirror"}}
+
+read 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+{"return": ""}
+read 65536/65536 bytes at offset 65536
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+{"return": ""}
+read 65536/65536 bytes at offset 131072
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+{"return": ""}
+read 65536/65536 bytes at offset 196608
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+{"return": ""}
+
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN"}
+
+read 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 65536/65536 bytes at offset 65536
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 65536/65536 bytes at offset 131072
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 65536/65536 bytes at offset 196608
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 9f1f2c0..1c6fcb6 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -155,3 +155,4 @@
 152 rw auto quick
 154 rw auto backing quick
 155 rw auto
+156 rw auto quick
-- 
2.8.3

Re: [Qemu-devel] [PATCH v3 1/5] block: Allow replacement of a BDS by its overlay

[Fam Zheng]

On Fri, 06/10 20:57, Max Reitz wrote:
> change_parent_backing_link() asserts that the BDS to be replaced is not
> used as a backing file. However, we may want to replace a BDS by its
> overlay in which case that very link should not be redirected.
> 
> For instance, when doing a sync=none drive-mirror operation, we may have
> the following BDS/BB forest before block job completion:
> 
>   target
> 
>   base <- source <- BlockBackend
> 
> During job completion, we want to establish the source BDS as the
> target's backing node:
> 
>           target
>             |
>             v
>   base <- source <- BlockBackend
> 
> This makes the target a valid replacement for the source:
> 
>           target <- BlockBackend
>             |
>             v
>   base <- source
> 
> Without this modification to change_parent_backing_link() we have to
> inject the target into the graph before the source is its backing node,
> thus temporarily creating a wrong graph:
> 
>   target <- BlockBackend
> 
>   base <- source
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index f54bc25..dc76c159 100644
> --- a/block.c
> +++ b/block.c
> @@ -2224,9 +2224,23 @@ void bdrv_close_all(void)
>  static void change_parent_backing_link(BlockDriverState *from,
>                                         BlockDriverState *to)
>  {
> -    BdrvChild *c, *next;
> +    BdrvChild *c, *next, *to_c;
>  
>      QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) {
> +        if (c->role == &child_backing) {
> +            /* @from is generally not allowed to be a backing file, except for
> +             * when @to is the overlay. In that case, @from may not be replaced
> +             * by @to as @to's backing node. */
> +            QLIST_FOREACH(to_c, &to->children, next) {
> +                if (to_c == c) {
> +                    break;
> +                }
> +            }

Why not just test "c->bs == to" here, why is it necessary to iterate through
to->children list?

Fam

> +            if (to_c) {
> +                continue;
> +            }
> +        }
> +
>          assert(c->role != &child_backing);
>          bdrv_ref(to);
>          bdrv_replace_child(c, to);
> -- 
> 2.8.3
> 

Re: [Qemu-devel] [PATCH v3 3/5] block/null: Implement bdrv_refresh_filename()

[Fam Zheng]

On Fri, 06/10 20:57, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>

The commit message could go a little more informative. Seems nothing special in
the added callback, aren't things supposed to just work without this patch?
What is missing?

> ---
>  block/null.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/block/null.c b/block/null.c
> index 396500b..b511010 100644
> --- a/block/null.c
> +++ b/block/null.c
> @@ -12,6 +12,8 @@
>  
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
> +#include "qapi/qmp/qdict.h"
> +#include "qapi/qmp/qstring.h"
>  #include "block/block_int.h"
>  
>  #define NULL_OPT_LATENCY "latency-ns"
> @@ -223,6 +225,20 @@ static int64_t coroutine_fn null_co_get_block_status(BlockDriverState *bs,
>      }
>  }
>  
> +static void null_refresh_filename(BlockDriverState *bs, QDict *opts)
> +{
> +    QINCREF(opts);
> +    qdict_del(opts, "filename");

Why is this qdict_del necessary?

> +
> +    if (!qdict_size(opts)) {
> +        snprintf(bs->exact_filename, sizeof(bs->exact_filename), "%s://",
> +                 bs->drv->format_name);
> +    }
> +
> +    qdict_put(opts, "driver", qstring_from_str(bs->drv->format_name));
> +    bs->full_open_options = opts;
> +}
> +
>  static BlockDriver bdrv_null_co = {
>      .format_name            = "null-co",
>      .protocol_name          = "null-co",
> @@ -238,6 +254,8 @@ static BlockDriver bdrv_null_co = {
>      .bdrv_reopen_prepare    = null_reopen_prepare,
>  
>      .bdrv_co_get_block_status   = null_co_get_block_status,
> +
> +    .bdrv_refresh_filename  = null_refresh_filename,
>  };
>  
>  static BlockDriver bdrv_null_aio = {
> @@ -255,6 +273,8 @@ static BlockDriver bdrv_null_aio = {
>      .bdrv_reopen_prepare    = null_reopen_prepare,
>  
>      .bdrv_co_get_block_status   = null_co_get_block_status,
> +
> +    .bdrv_refresh_filename  = null_refresh_filename,
>  };
>  
>  static void bdrv_null_init(void)
> -- 
> 2.8.3
> 

Re: [Qemu-devel] [PATCH v3 4/5] iotests: Add test for post-mirror backing chains

[Fam Zheng]

On Fri, 06/10 20:57, Max Reitz wrote:
> +import os
> +import stat
> +import time

Unused import 'stat' and 'time'?

> +import iotests
> +from iotests import qemu_img

Re: [Qemu-devel] [PATCH v3 0/5] block/mirror: Fix (?) target backing BDS

[Fam Zheng]

On Fri, 06/10 20:57, Max Reitz wrote:
> Issue #1: If the target image does not have a backing BDS before mirror
> completion, qemu tries really hard to give it a backing BDS. If the
> source has a backing BDS, it will actually always "succeed".
> In some cases, the target is not supposed to have a backing BDS, though
> (absolute-paths: because of sync=full; existing: because the target
> image does not have a backing file; blockdev-mirror: because of an
> explicit "backing": ""). Then, this is pretty bad behavior.
> 
> This should generally not change the target's visible data, but it still
> is ugly.
> 
> Issue #2: Currently the backing chain of the target is basically opened
> using bdrv_open_backing_file() (except for sometimes™). This results in
> multiple BDSs for a single physical file, which is bad. In most use
> cases, this is only temporary, but it still is bad.
> 
> If we can reuse the existing backing chain of the source (which is with
> drive-mirror in "absolute-paths" mode), we should just do so.

Looks good overall. I left a few trivial comments/questions in individual
patches.

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v3 1/5] block: Allow replacement of a BDS by its overlay

[Kevin Wolf]

Am 12.06.2016 um 05:15 hat Fam Zheng geschrieben:
> On Fri, 06/10 20:57, Max Reitz wrote:
> > change_parent_backing_link() asserts that the BDS to be replaced is not
> > used as a backing file. However, we may want to replace a BDS by its
> > overlay in which case that very link should not be redirected.
> > 
> > For instance, when doing a sync=none drive-mirror operation, we may have
> > the following BDS/BB forest before block job completion:
> > 
> >   target
> > 
> >   base <- source <- BlockBackend
> > 
> > During job completion, we want to establish the source BDS as the
> > target's backing node:
> > 
> >           target
> >             |
> >             v
> >   base <- source <- BlockBackend
> > 
> > This makes the target a valid replacement for the source:
> > 
> >           target <- BlockBackend
> >             |
> >             v
> >   base <- source
> > 
> > Without this modification to change_parent_backing_link() we have to
> > inject the target into the graph before the source is its backing node,
> > thus temporarily creating a wrong graph:
> > 
> >   target <- BlockBackend
> > 
> >   base <- source
> > 
> > Signed-off-by: Max Reitz <mreitz@redhat.com>
> > ---
> >  block.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/block.c b/block.c
> > index f54bc25..dc76c159 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -2224,9 +2224,23 @@ void bdrv_close_all(void)
> >  static void change_parent_backing_link(BlockDriverState *from,
> >                                         BlockDriverState *to)
> >  {
> > -    BdrvChild *c, *next;
> > +    BdrvChild *c, *next, *to_c;
> >  
> >      QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) {
> > +        if (c->role == &child_backing) {
> > +            /* @from is generally not allowed to be a backing file, except for
> > +             * when @to is the overlay. In that case, @from may not be replaced
> > +             * by @to as @to's backing node. */
> > +            QLIST_FOREACH(to_c, &to->children, next) {
> > +                if (to_c == c) {
> > +                    break;
> > +                }
> > +            }
> 
> Why not just test "c->bs == to" here, why is it necessary to iterate through
> to->children list?

Because c->bs == from. We want to check whether 'to' is the parent, not
whether it is the child.

Kevin

Re: [Qemu-devel] [PATCH v3 0/5] block/mirror: Fix (?) target backing BDS

[Kevin Wolf]

Am 10.06.2016 um 20:57 hat Max Reitz geschrieben:
> Issue #1: If the target image does not have a backing BDS before mirror
> completion, qemu tries really hard to give it a backing BDS. If the
> source has a backing BDS, it will actually always "succeed".
> In some cases, the target is not supposed to have a backing BDS, though
> (absolute-paths: because of sync=full; existing: because the target
> image does not have a backing file; blockdev-mirror: because of an
> explicit "backing": ""). Then, this is pretty bad behavior.
> 
> This should generally not change the target's visible data, but it still
> is ugly.
> 
> Issue #2: Currently the backing chain of the target is basically opened
> using bdrv_open_backing_file() (except for sometimes™). This results in
> multiple BDSs for a single physical file, which is bad. In most use
> cases, this is only temporary, but it still is bad.
> 
> If we can reuse the existing backing chain of the source (which is with
> drive-mirror in "absolute-paths" mode), we should just do so.

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

I'll still wait to apply the series so that you have a chance to answer
Fam's question before it is in.

Kevin

Re: [Qemu-devel] [PATCH v3 3/5] block/null: Implement bdrv_refresh_filename()

[Max Reitz]

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

On 12.06.2016 06:08, Fam Zheng wrote:
> On Fri, 06/10 20:57, Max Reitz wrote:
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> 
> The commit message could go a little more informative. Seems nothing special in
> the added callback, aren't things supposed to just work without this patch?
> What is missing?

If you pass a filename, it works without this patch. If you don't, it
doesn't.

Compare (before this patch):

$ x86_64-softmmu/qemu-system-x86_64 \
  -drive if=none,file=null-co://,driver=raw -nodefaults -qmp stdio
[...]
{'execute':'query-block'}
[...] "filename": "null-co://", [...]

With:

$ x86_64-softmmu/qemu-system-x86_64 \
  -drive if=none,driver=raw,file.driver=null-co -nodefaults -qmp stdio
[...]
{'execute':'query-block'}
[...] "filename": "json:{\"driver\": \"raw\", \"file\": {\"driver\":
\"null-co\"}}", [...]


So the issue is that you can use the null-co/aio drivers without giving
a filename.

I wouldn't mind including this information in a v4, but I'm not sure it
alone warrants a v4.

> 
>> ---
>>  block/null.c | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/block/null.c b/block/null.c
>> index 396500b..b511010 100644
>> --- a/block/null.c
>> +++ b/block/null.c
>> @@ -12,6 +12,8 @@
>>  
>>  #include "qemu/osdep.h"
>>  #include "qapi/error.h"
>> +#include "qapi/qmp/qdict.h"
>> +#include "qapi/qmp/qstring.h"
>>  #include "block/block_int.h"
>>  
>>  #define NULL_OPT_LATENCY "latency-ns"
>> @@ -223,6 +225,20 @@ static int64_t coroutine_fn null_co_get_block_status(BlockDriverState *bs,
>>      }
>>  }
>>  
>> +static void null_refresh_filename(BlockDriverState *bs, QDict *opts)
>> +{
>> +    QINCREF(opts);
>> +    qdict_del(opts, "filename");
> 
> Why is this qdict_del necessary?

It's not strictly necessary. But the null drivers ignore the filename,
so there's no harm in dropping it from the JSON filename (if we need to
construct one).

Max

>> +
>> +    if (!qdict_size(opts)) {
>> +        snprintf(bs->exact_filename, sizeof(bs->exact_filename), "%s://",
>> +                 bs->drv->format_name);
>> +    }
>> +
>> +    qdict_put(opts, "driver", qstring_from_str(bs->drv->format_name));
>> +    bs->full_open_options = opts;
>> +}
>> +
>>  static BlockDriver bdrv_null_co = {
>>      .format_name            = "null-co",
>>      .protocol_name          = "null-co",
>> @@ -238,6 +254,8 @@ static BlockDriver bdrv_null_co = {
>>      .bdrv_reopen_prepare    = null_reopen_prepare,
>>  
>>      .bdrv_co_get_block_status   = null_co_get_block_status,
>> +
>> +    .bdrv_refresh_filename  = null_refresh_filename,
>>  };
>>  
>>  static BlockDriver bdrv_null_aio = {
>> @@ -255,6 +273,8 @@ static BlockDriver bdrv_null_aio = {
>>      .bdrv_reopen_prepare    = null_reopen_prepare,
>>  
>>      .bdrv_co_get_block_status   = null_co_get_block_status,
>> +
>> +    .bdrv_refresh_filename  = null_refresh_filename,
>>  };
>>  
>>  static void bdrv_null_init(void)
>> -- 
>> 2.8.3
>>



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v3 4/5] iotests: Add test for post-mirror backing chains

[Max Reitz]

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

On 12.06.2016 06:17, Fam Zheng wrote:
> On Fri, 06/10 20:57, Max Reitz wrote:
>> +import os
>> +import stat
>> +import time
> 
> Unused import 'stat' and 'time'?

That happens when you copy old tests and don't check stuff like this...
Well, it won't hurt, but I guess now I may have enough reason to send a
v4 (if I get to it today).

Thanks!

Max

> 
>> +import iotests
>> +from iotests import qemu_img
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v3 0/5] block/mirror: Fix (?) target backing BDS

[Max Reitz]

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

On 10.06.2016 20:57, Max Reitz wrote:
> Issue #1: If the target image does not have a backing BDS before mirror
> completion, qemu tries really hard to give it a backing BDS. If the
> source has a backing BDS, it will actually always "succeed".
> In some cases, the target is not supposed to have a backing BDS, though
> (absolute-paths: because of sync=full; existing: because the target
> image does not have a backing file; blockdev-mirror: because of an
> explicit "backing": ""). Then, this is pretty bad behavior.
> 
> This should generally not change the target's visible data, but it still
> is ugly.
> 
> Issue #2: Currently the backing chain of the target is basically opened
> using bdrv_open_backing_file() (except for sometimes™). This results in
> multiple BDSs for a single physical file, which is bad. In most use
> cases, this is only temporary, but it still is bad.
> 
> If we can reuse the existing backing chain of the source (which is with
> drive-mirror in "absolute-paths" mode), we should just do so.

Following encouragement from Kevin on IRC, and apparently his acceptance
of my commit message proposal for patch 3, I have applied the series to
my block branch with said commit message added to patch 3 and the
superfluous imports removed from patch 4 (thanks, Fam!).

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]