Fix the reopening of images in 'block-commit'

This series fixes a bug in the 'block-commit' operation under the following scenario:

[A] <- [B] <- [C] <- [D]

If we do block-commit top=B base=A, the contents of [B] will be written into [A] resulting in this chain:

[A] <- [C] <- [D]

In order to perform this operation, [A] must be reopened in read-write mode but so does [C] because its backing file string needs to be updated to point at [A].

There's a bug in the current code that makes [A] read-only again when
[C] is reopened. This series includes a fix for that bug plus a test case for the scenario.

This affects both master and the 2.4 branch.

Berto

Alberto Garcia (2):
commit: reopen overlay_bs before base
qemu-iotests: Test the reopening of overlay_bs in 'block-commit'

 block/commit.c             |  8 ++++----
 tests/qemu-iotests/040     | 30 ++++++++++++++++++++++++++++++
 tests/qemu-iotests/040.out |  4 ++--
 3 files changed, 36 insertions(+), 6 deletions(-)

2.6.1

'block-commit' needs write access to two different nodes of the chain:

- 'base', because that's where the data is written to.
- the overlay of 'top', because it needs to update the backing file string to point to 'base' after the operation.

Both images have to be opened in read-write mode, and commit_start() takes care of reopening them if necessary.

With the current implementation, however, when overlay_bs is reopened in read-write mode it has the side effect of making 'base' read-only again, eventually making 'block-commit' fail.

This needs to be fixed in bdrv_reopen(), but until we get to that it can be worked around simply by swapping the order of base and overlay_bs in the reopen queue.

In order to reproduce this bug, overlay_bs needs to be initially in read-only mode. That is: the 'top' parameter of 'block-commit' cannot be the active layer nor its immediate backing chain.

---
 block/commit.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

index 7312a5b..85a2604 100644
@@ -235,14 +235,14 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
     orig_overlay_flags = bdrv_get_flags(overlay_bs);
 
     /* convert base & overlay_bs to r/w, if necessary */
-    if (!(orig_base_flags & BDRV_O_RDWR)) {
-        reopen_queue = bdrv_reopen_queue(reopen_queue, base,
-                                         orig_base_flags | BDRV_O_RDWR);
-    }
     if (!(orig_overlay_flags & BDRV_O_RDWR)) {
         reopen_queue = bdrv_reopen_queue(reopen_queue, overlay_bs,
                                          orig_overlay_flags | BDRV_O_RDWR);
     }
+    if (!(orig_base_flags & BDRV_O_RDWR)) {
+        reopen_queue = bdrv_reopen_queue(reopen_queue, base,
+                                         orig_base_flags | BDRV_O_RDWR);
+    }
     if (reopen_queue) {
         bdrv_reopen_multiple(reopen_queue, &local_err);
         if (local_err != NULL) {

2.6.1

The 'block-commit' command needs the overlay image of 'top' to be opened in read-write mode in order to update the backing file string. If 'top' is not the active layer or its backing file then its overlay needs to be reopened during the block job.

This is a test case for that scenario.

---
 tests/qemu-iotests/040     | 30 ++++++++++++++++++++++++++++++
 tests/qemu-iotests/040.out |  4 ++--
 2 files changed, 32 insertions(+), 2 deletions(-)

index ea2f98e..5bdaf3d 100755
@@ -41,6 +41,7 @@ class ImageCommitTestCase(iotests.QMPTestCase):
         while not completed:
             for event in self.vm.get_qmp_events(wait=True):
                 if event['event'] == 'BLOCK_JOB_COMPLETED':
+                    self.assert_qmp_absent(event, 'data/error')
                     self.assert_qmp(event, 'data/type', 'commit')
                     self.assert_qmp(event, 'data/device', 'drive0')
                     self.assert_qmp(event, 'data/offset', event['data']['len'])

@@ -251,5 +252,34 @@ class TestSetSpeed(ImageCommitTestCase):
 class TestActiveZeroLengthImage(TestSingleDrive):
     image_len = 0
 
+class TestReopenOverlay(ImageCommitTestCase):
+    image_len = 1024 * 1024
+    img0 = os.path.join(iotests.test_dir, '0.img')
+    img1 = os.path.join(iotests.test_dir, '1.img')
+    img2 = os.path.join(iotests.test_dir, '2.img')
+    img3 = os.path.join(iotests.test_dir, '3.img')
+
+    def setUp(self):
+        iotests.create_image(self.img0, self.image_len)
+        qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % self.img0, self.img1)
+        qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % self.img1, self.img2)
+        qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % self.img2, self.img3)
+        qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0xab 0 128K', self.img1)
+        self.vm = iotests.VM().add_drive(self.img3)
+        self.vm.launch()
+
+    def tearDown(self):
+        self.vm.shutdown()
+        os.remove(self.img0)
+        os.remove(self.img1)
+        os.remove(self.img2)
+        os.remove(self.img3)
+
+    # This tests what happens when the overlay image of the 'top' node
+    # needs to be reopened in read-write mode in order to update the
+    # backing image string.
+    def test_reopen_overlay(self):
+        self.run_commit_test(self.img1, self.img0)
+
 if __name__ == '__main__':
     iotests.main(supported_fmts=['qcow2', 'qed'])

index 42314e9..4fd1c2d 100644
@@ -1,5 +1,5 @@
-........................
+.........................
 ----------------------------------------------------------------------
-Ran 24 tests
+Ran 25 tests
 
 OK

2.6.1

Am 28.10.2015 um 14:43 hat Alberto Garcia geschrieben:

Thanks, applied to the block branch.

Kevin