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 <vsement...@virtuozzo.com> > --- > > Hi all! > > This patch shows degradation, added in 2.10 in commit > > commit 9c5e6594f15b7364624a3ad40306c396c93a2145 > Author: Kevin Wolf <kw...@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.
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)