Test the reopening of overlay_bs in 'block-commit'

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

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 27.10.2015 um 17:43 hat Alberto Garcia geschrieben:

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

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

Am 28.10.2015 um 12:20 hat Alberto Garcia geschrieben:

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