[Qemu-devel] [PATCH 0/2] Fix the reopening of images in 'block-commit'

[Qemu-devel] [PATCH 0/2] Fix the reopening of images in 'block-commit'

[Alberto Garcia]

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

[Qemu-devel] [PATCH 1/2] commit: reopen overlay_bs before base

[Alberto Garcia]

'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.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/commit.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index 7312a5b..85a2604 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -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

Re: [Qemu-devel] [PATCH 1/2] commit: reopen overlay_bs before base

[Max Reitz]

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

On 28.10.2015 14:43, Alberto Garcia wrote:
> '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.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/commit.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

[Qemu-devel] [PATCH 2/2] qemu-iotests: Test the reopening of overlay_bs in 'block-commit'

[Alberto Garcia]

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.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 tests/qemu-iotests/040     | 30 ++++++++++++++++++++++++++++++
 tests/qemu-iotests/040.out |  4 ++--
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index ea2f98e..5bdaf3d 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -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'])
diff --git a/tests/qemu-iotests/040.out b/tests/qemu-iotests/040.out
index 42314e9..4fd1c2d 100644
--- a/tests/qemu-iotests/040.out
+++ b/tests/qemu-iotests/040.out
@@ -1,5 +1,5 @@
-........................
+.........................
 ----------------------------------------------------------------------
-Ran 24 tests
+Ran 25 tests
 
 OK
-- 
2.6.1

Re: [Qemu-devel] [PATCH 2/2] qemu-iotests: Test the reopening of overlay_bs in 'block-commit'

[Max Reitz]

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

On 28.10.2015 14:43, Alberto Garcia wrote:
> 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.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  tests/qemu-iotests/040     | 30 ++++++++++++++++++++++++++++++
>  tests/qemu-iotests/040.out |  4 ++--
>  2 files changed, 32 insertions(+), 2 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH 0/2] Fix the reopening of images in 'block-commit'

[Kevin Wolf]

Am 28.10.2015 um 14:43 hat Alberto Garcia geschrieben:
> 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.

Thanks, applied to the block branch.

Kevin