qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] iotests: test clearing unknown autoclear_features by qcow2
@ 2017-11-10 17:54 Vladimir Sementsov-Ogievskiy
  2017-11-10 18:25 ` no-reply
  2017-11-10 20:02 ` Kevin Wolf
  0 siblings, 2 replies; 7+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-11-10 17:54 UTC (permalink / raw)
  To: qemu-devel, qemu-block
  Cc: mreitz, kwolf, eblake, dgilbert, quintela, den, vsementsov

Test clearing unknown autoclear_features by qcow2 on incoming
migration.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---

Hi all!

This patch shows degradation, added in 2.10 in commit

commit 9c5e6594f15b7364624a3ad40306c396c93a2145
Author: Kevin Wolf <kwolf@redhat.com>
Date:   Thu May 4 18:52:40 2017 +0200

    block: Fix write/resize permissions for inactive images

The problem:
When on incoming migration with shared storage we reopen image to RW mode
we call bdrv_invalidate_cache it firstly call drv->bdrv_invalidate_cache and
only after it, through "parent->role->activate(parent, &local_err);" we set
appropriate RW permission.

Because of this, if drv->bdrv_invalidate_cache wants to write something
(image is RW and BDRV_O_INACTIVE is not set) it goes into
"bdrv_aligned_pwritev: Assertion `child->perm & BLK_PERM_WRITE' failed"

One case, when qcow2_invalidate_cache wants to write
something - when it wants to clear some unknown autoclear features. So,
here is a test for it.

Another case is when we try to migrate persistent dirty bitmaps through
shared storage, on invalidate_cache qcow2 will try to set DIRTY bit in
all loaded bitmaps.

Kevin, what do you think? I understand that operation order should be changed
somehow in bdrv_invalidate_cache, but I'm not sure about how "parent->role->.."
things works and can we safely move this part from function end to the middle.

 tests/qemu-iotests/196     | 63 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/196.out |  5 ++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 69 insertions(+)
 create mode 100755 tests/qemu-iotests/196
 create mode 100644 tests/qemu-iotests/196.out

diff --git a/tests/qemu-iotests/196 b/tests/qemu-iotests/196
new file mode 100755
index 0000000000..9ffbc723c2
--- /dev/null
+++ b/tests/qemu-iotests/196
@@ -0,0 +1,63 @@
+#!/usr/bin/env python
+#
+# Test clearing unknown autoclear_features flag by qcow2 after
+# migration. This test mimics migration to older qemu.
+#
+# Copyright (c) 2017 Parallels International GmbH
+#
+# 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 iotests
+from iotests import qemu_img
+
+disk = os.path.join(iotests.test_dir, 'disk')
+migfile = os.path.join(iotests.test_dir, 'migfile')
+
+class TestInvalidateAutoclear(iotests.QMPTestCase):
+
+    def tearDown(self):
+        self.vm_a.shutdown()
+        self.vm_b.shutdown()
+        os.remove(disk)
+        os.remove(migfile)
+
+    def setUp(self):
+        qemu_img('create', '-f', iotests.imgfmt, disk, '1M')
+
+        self.vm_a = iotests.VM(path_suffix='a').add_drive(disk)
+        self.vm_a.launch()
+
+        self.vm_b = iotests.VM(path_suffix='b').add_drive(disk)
+        self.vm_b.add_incoming("exec: cat '" + migfile + "'")
+
+    def test_migration(self):
+        result = self.vm_a.qmp('migrate', uri='exec:cat>' + migfile)
+        self.assert_qmp(result, 'return', {});
+        self.assertNotEqual(self.vm_a.event_wait("STOP"), None)
+
+        with open(disk, 'r+b') as f:
+            f.seek(88) # first byte of autoclear_features field
+            f.write(b'\xff')
+
+        self.vm_b.launch()
+        self.assertNotEqual(self.vm_b.event_wait("RESUME"), None)
+
+        with open(disk, 'rb') as f:
+            f.seek(88)
+            self.assertEqual(f.read(1), b'\x00')
+
+if __name__ == '__main__':
+    iotests.main(supported_fmts=['qcow2'])
diff --git a/tests/qemu-iotests/196.out b/tests/qemu-iotests/196.out
new file mode 100644
index 0000000000..ae1213e6f8
--- /dev/null
+++ b/tests/qemu-iotests/196.out
@@ -0,0 +1,5 @@
+.
+----------------------------------------------------------------------
+Ran 1 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 24e5ad1b79..78d01d9516 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -193,4 +193,5 @@
 192 rw auto quick
 194 rw auto migration quick
 195 rw auto quick
+196 rw auto quick
 197 rw auto quick
-- 
2.11.1

^ permalink raw reply related	[flat|nested] 7+ messages in thread
* Re: [Qemu-devel] loading bitmaps in invalidate_cache fails
@ 2017-09-12  9:46 Kevin Wolf
  2017-09-28 13:06 ` [Qemu-devel] [PATCH] iotests: test clearing unknown autoclear_features by qcow2 Vladimir Sementsov-Ogievskiy
  0 siblings, 1 reply; 7+ messages in thread
From: Kevin Wolf @ 2017-09-12  9:46 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy; +Cc: qemu-devel, qemu block, Max Reitz

Am 11.09.2017 um 18:51 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Hi Kevin!
> 
> I'm confused with relations of permissions and invalidation, can you please
> help?
> 
> Now dirty bitmaps are loaded in invalidate_cache. Here is a problem with
> migration:
> 
> 1. destination starts (inactive)
> 
> 2. load bitmaps readonly
> 
> ...
> 
> 3. invalidate_cache: here we should make our loaded bitmaps RW, ie set
> BdrvDirtyBitmap->readonly
> 
>   to false and set IN_USE bit in the image. But the latter goes into
> "bdrv_aligned_pwritev: Assertion `child->perm & BLK_PERM_WRITE' failed",
> 
>   because in bdrv_invalidate_cache we call bdrv_set_perm after
> drv->bdrv_invalidate_cache.
> 
> 
> What is the true way of fixing this?

It's all still a bit of a mess. :-(

I think it makes a lot of sense that we should activate the lower layers
first, so the order in bdrv_invalidate_cache() looks wrong. It should be
something like this:

1. invalidate_cache() for the children
2. Update permissions for non-BDRV_O_INACTIVE
3. Call drv->bdrv_invalidate_cache()

I'm currently working on some fixes related to bdrv_reopen() where
things become tricky because the updated permissions shouldn't depend on
the current state, but on the state after the operation has finished.

You get something similar here, but I think just making sure that we
clear BDRV_O_INACTIVE before updating the permissions is enough here.
The only thing to be careful is that in error cases, we need to revert
both the flag and the permissions then.

Kevin

^ permalink raw reply	[flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-11-17  7:51 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-11-10 17:54 [Qemu-devel] [PATCH] iotests: test clearing unknown autoclear_features by qcow2 Vladimir Sementsov-Ogievskiy
2017-11-10 18:25 ` no-reply
2017-11-10 20:02 ` Kevin Wolf
2017-11-16 21:36   ` [Qemu-devel] [Qemu-block] " John Snow
2017-11-17  7:50   ` [Qemu-devel] " Vladimir Sementsov-Ogievskiy
  -- strict thread matches above, loose matches on Subject: below --
2017-09-12  9:46 [Qemu-devel] loading bitmaps in invalidate_cache fails Kevin Wolf
2017-09-28 13:06 ` [Qemu-devel] [PATCH] iotests: test clearing unknown autoclear_features by qcow2 Vladimir Sementsov-Ogievskiy
2017-09-28 13:26   ` no-reply

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).