[Qemu-devel] [PATCH 0/1] Test the reopening of overlay_bs in 'block-commit'

[Qemu-devel] [PATCH 0/1] Test the reopening of overlay_bs in 'block-commit'

[Alberto Garcia]

Hi, looks like we have a bug in the bdrv_reopen() code.

It turns out that 'block-commit' fails if the 'top' node is not the
active layer or its immediate backing file, and none of our test cases
has detected that. I'm attaching one that reproduces the problem.

What happens is that 'block-commit' reopens the overlay of the top
node in read-write mode in order to update the backing file string. In
addition to that, the 'base' image also needs to be reopened in r/w.

Here's the relevant code from commit_start():

    if (!(orig_base_flags & BDRV_O_RDWR)) {
        reopen_queue = bdrv_reopen_queue(reopen_queue, base, NULL,
                                         orig_base_flags | BDRV_O_RDWR);
    }
    if (!(orig_overlay_flags & BDRV_O_RDWR)) {
        reopen_queue = bdrv_reopen_queue(reopen_queue, overlay_bs, NULL,
                                         orig_overlay_flags | BDRV_O_RDWR);
    }
    if (reopen_queue) {
        bdrv_reopen_multiple(reopen_queue, &local_err);
        /*...*/
    }

'base' is reopened first in r/w mode, then 'overlay_bs'. However it
seems that the latter has the side effect or reopening 'base' again in
read-only mode, therefore the job ends up failing with -EPERM.

Just swapping the order of the bdrv_reopen_queue() calls is enough to
fix the problem, but I'm sure this needs deeper changes in the
bdrv_reopen() code instead.

Berto

Alberto Garcia (1):
  qemu-iotests: Test the reopening of overlay_bs in 'block-commit'

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

-- 
2.6.1

[Qemu-devel] [PATCH 1/1] 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 0/1] Test the reopening of overlay_bs in 'block-commit'

[Kevin Wolf]

Am 27.10.2015 um 17:43 hat Alberto Garcia geschrieben:
> Hi, looks like we have a bug in the bdrv_reopen() code.
> 
> It turns out that 'block-commit' fails if the 'top' node is not the
> active layer or its immediate backing file, and none of our test cases
> has detected that. I'm attaching one that reproduces the problem.
> 
> What happens is that 'block-commit' reopens the overlay of the top
> node in read-write mode in order to update the backing file string. In
> addition to that, the 'base' image also needs to be reopened in r/w.
> 
> Here's the relevant code from commit_start():
> 
>     if (!(orig_base_flags & BDRV_O_RDWR)) {
>         reopen_queue = bdrv_reopen_queue(reopen_queue, base, NULL,
>                                          orig_base_flags | BDRV_O_RDWR);
>     }
>     if (!(orig_overlay_flags & BDRV_O_RDWR)) {
>         reopen_queue = bdrv_reopen_queue(reopen_queue, overlay_bs, NULL,
>                                          orig_overlay_flags | BDRV_O_RDWR);
>     }
>     if (reopen_queue) {
>         bdrv_reopen_multiple(reopen_queue, &local_err);
>         /*...*/
>     }
> 
> 'base' is reopened first in r/w mode, then 'overlay_bs'. However it
> seems that the latter has the side effect or reopening 'base' again in
> read-only mode, therefore the job ends up failing with -EPERM.
> 
> Just swapping the order of the bdrv_reopen_queue() calls is enough to
> fix the problem, but I'm sure this needs deeper changes in the
> bdrv_reopen() code instead.

I think this might be fixed by applying the rest of my bdrv_reopen()
patches and then converting read-only from a flag to an option. The
reason is that we really have three values here: ro, rw and inherit from
parent. This can be represented by options (false, true, missing QDict
entry), but not by flags.

I've applied your test case to my working branch so I won't forget about
this. Maybe I should really try to get the series into 2.5 then.

All that being said, having the same BDS twice in a reopen queue is
calling for trouble. I guess currently the last one wins, with all
changes made in the first call reverted even if the respective options
aren't touched by the second one. I wonder if we need to merge multiple
entries for the same BDS.

Kevin

Re: [Qemu-devel] [PATCH 0/1] Test the reopening of overlay_bs in 'block-commit'

[Alberto Garcia]

On Wed 28 Oct 2015 10:33:01 AM CET, Kevin Wolf <kwolf@redhat.com> wrote:

> I've applied your test case to my working branch so I won't forget
> about this. Maybe I should really try to get the series into 2.5 then.

Note that 2.4 is also affected by this. I guess for that version we can
simply swap the order of the bdrv_reopen_queue() calls. I can prepare a
patch if you're ok with that solution.

Berto

Re: [Qemu-devel] [PATCH 0/1] Test the reopening of overlay_bs in 'block-commit'

[Kevin Wolf]

Am 28.10.2015 um 12:20 hat Alberto Garcia geschrieben:
> On Wed 28 Oct 2015 10:33:01 AM CET, Kevin Wolf <kwolf@redhat.com> wrote:
> 
> > I've applied your test case to my working branch so I won't forget
> > about this. Maybe I should really try to get the series into 2.5 then.
> 
> Note that 2.4 is also affected by this. I guess for that version we can
> simply swap the order of the bdrv_reopen_queue() calls. I can prepare a
> patch if you're ok with that solution.

Okay. Then I'd suggest that you send a series with the swap and the test
case, CC qemu-stable, and we'll just apply that for now. And I'll keep a
note to look into the bdrv_reopen() part.

Kevin