Am 10.11.2017 um 18:54 hat Vladimir Sementsov-Ogievskiy geschrieben:
Test clearing unknown autoclear_features by qcow2 on incoming
migration.
Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
---
Hi all!
This patch shows degradation, added in 2.10 in commit
commit 9c5e6594f15b7364624a3ad40306c396c93a2145
Author: Kevin Wolf <address@hidden>
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.
I don't think you need to move the parent->role->activate() callback,
but just the bdrv_set_perm() so that we request the correct permissions
for operation without the BDRV_O_INACTIVE flag.
The following seems to work for me (including a fix for the test case,
we don't seem to get a RESUME event).
I'm not sure about the error paths
yet. We should probably try do undo the permission changes there. I can
have a closer look into this next week.
Kevin
diff --git a/block.c b/block.c
index edc5bb9f9b..f530b630b6 100644
--- a/block.c
+++ b/block.c
@@ -4178,7 +4178,17 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error
**errp)
}
}
+ /* Update permissions, they may differ for inactive nodes */
bs->open_flags &= ~BDRV_O_INACTIVE;
+ bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
+ ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, &local_err);
+ if (ret < 0) {
+ bs->open_flags |= BDRV_O_INACTIVE;
+ error_propagate(errp, local_err);
+ return;
+ }
+ bdrv_set_perm(bs, perm, shared_perm);
+
if (bs->drv->bdrv_invalidate_cache) {
bs->drv->bdrv_invalidate_cache(bs, &local_err);
if (local_err) {
@@ -4195,16 +4205,6 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error
**errp)
return;
}
- /* Update permissions, they may differ for inactive nodes */
- bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
- ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, &local_err);
- if (ret < 0) {
- bs->open_flags |= BDRV_O_INACTIVE;
- error_propagate(errp, local_err);
- return;
- }
- bdrv_set_perm(bs, perm, shared_perm);
-
QLIST_FOREACH(parent, &bs->parents, next_parent) {
if (parent->role->activate) {
parent->role->activate(parent, &local_err);
diff --git a/tests/qemu-iotests/196 b/tests/qemu-iotests/196
index 9ffbc723c2..4116ebc92b 100755
--- a/tests/qemu-iotests/196
+++ b/tests/qemu-iotests/196
@@ -53,7 +53,10 @@ class TestInvalidateAutoclear(iotests.QMPTestCase):
f.write(b'\xff')
self.vm_b.launch()
- self.assertNotEqual(self.vm_b.event_wait("RESUME"), None)
+ while True:
+ result = self.vm_b.qmp('query-status')
+ if result['return']['status'] == 'running':
+ break
with open(disk, 'rb') as f:
f.seek(88)