On 11/10/2017 03:02 PM, Kevin Wolf wrote: > 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 >
What's the status here, something we need to be paying attention to for 2.11?
