* [Qemu-devel] [PATCH 1/2] block/mirror: Fix target backing BDS
2016-05-26 13:40 [Qemu-devel] [PATCH 0/2] block/mirror: Fix target backing BDS Max Reitz
@ 2016-05-26 13:40 ` Max Reitz
2016-05-26 13:40 ` [Qemu-devel] [PATCH 2/2] iotests: Add test for post-mirror backing chains Max Reitz
1 sibling, 0 replies; 3+ messages in thread
From: Max Reitz @ 2016-05-26 13:40 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf
Currently, we are trying to move the backing BDS from the source to the
target in bdrv_replace_in_backing_chain(). However, the conditions used
to decide when to move the backing BDS from source to target are not
really correct.
Basically, we do not have to set the target's backing BDS when doing a
commit operation (an active commit, to be specific) but we do have to
set it for every mirror operation (unless the target's backing BDS is
already the one it should have).
The decision when to adjust the target's backing BDS and what it should
be set to is something that the mirror code can do best, so let's do it
there.
Also, we do not need to drop the source's backing BDS. In most cases,
the source will be deleted after the mirroring operation anyway, and if
it is not, we probably want it to keep its own backing chain so it stays
valid.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block.c | 8 --------
block/mirror.c | 13 +++++++++++++
2 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/block.c b/block.c
index 736432f..7ae2766 100644
--- a/block.c
+++ b/block.c
@@ -2275,14 +2275,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..bc65e54 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -482,6 +482,19 @@ static void mirror_exit(BlockJob *job, void *opaque)
/* We just changed the BDS the job BB refers to */
blk_remove_bs(job->blk);
blk_insert_bs(job->blk, src);
+
+ /* Now we need to adjust the target's backing BDS. This is not necessary
+ * when having performed a commit operation.
+ * Note that we cannot do this adjustment before the call to
+ * bdrv_replace_in_backing_chain(), because we may end up using
+ * to_replace as the target's BDS, which makes it impossible to replace
+ * to_replace by the target afterwards */
+ if (!bdrv_chain_contains(backing_bs(to_replace), target_bs)) {
+ BlockDriverState *backing = s->is_none_mode ? to_replace : s->base;
+ if (backing_bs(target_bs) != backing) {
+ bdrv_set_backing_hd(target_bs, backing);
+ }
+ }
}
if (s->to_replace) {
bdrv_op_unblock_all(s->to_replace, s->replace_blocker);
--
2.8.3
^ permalink raw reply related [flat|nested] 3+ messages in thread* [Qemu-devel] [PATCH 2/2] iotests: Add test for post-mirror backing chains
2016-05-26 13:40 [Qemu-devel] [PATCH 0/2] block/mirror: Fix target backing BDS Max Reitz
2016-05-26 13:40 ` [Qemu-devel] [PATCH 1/2] " Max Reitz
@ 2016-05-26 13:40 ` Max Reitz
1 sibling, 0 replies; 3+ messages in thread
From: Max Reitz @ 2016-05-26 13:40 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/155 | 218 +++++++++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/155.out | 5 ++
tests/qemu-iotests/group | 1 +
3 files changed, 224 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..76fdd4f
--- /dev/null
+++ b/tests/qemu-iotests/155
@@ -0,0 +1,218 @@
+#!/usr/bin/env python
+#
+# Test whether the backing BDSs are correct after completion of a
+# mirror block job
+#
+# 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 BaseClass(iotests.QMPTestCase):
+ 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':
+ result = self.vm.qmp('blockdev-add',
+ options={'node-name': 'target',
+ 'driver': iotests.imgfmt,
+ 'file': {'driver': 'file',
+ 'filename': target_img}})
+ 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)
+
+
+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.assert_qmp_absent(node, 'image/backing-image')
+
+ self.assertIntactSourceBackingChain()
+
+ def testTop(self):
+ self.runMirror('top')
+
+ node = self.findBlockNode('target', 'drive0')
+ self.assert_qmp(node, 'image/backing-image/filename', back2_img)
+
+ self.assertIntactSourceBackingChain()
+
+ def testNone(self):
+ self.runMirror('none')
+
+ node = self.findBlockNode('target', 'drive0')
+ self.assert_qmp(node, 'image/backing-image/filename', 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 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..b6f2576
--- /dev/null
+++ b/tests/qemu-iotests/155.out
@@ -0,0 +1,5 @@
+................
+----------------------------------------------------------------------
+Ran 16 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
^ permalink raw reply related [flat|nested] 3+ messages in thread