[Qemu-devel] ping Re: loading bitmaps in invalidate_cache fails

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

Hi!

Any ideas?

05.10.2017 12:41, Vladimir Sementsov-Ogievskiy wrote:
> 12.09.2017 12:46, Kevin Wolf wrote:
>> 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
>
> I have experimented a bit. Permission for qcow2 bs is actually set in 
> "parent->role->activate(parent, &local_err);",
>
> so a patch like this fixes the bug (of course, some roll back of 
> set_perm and activate should be added to error path)
>
> @@ -4126,21 +4129,6 @@ void bdrv_invalidate_cache(BlockDriverState 
> *bs, Error **errp)
>      }
>
>      bs->open_flags &= ~BDRV_O_INACTIVE;
> -    if (bs->drv->bdrv_invalidate_cache) {
> -        bs->drv->bdrv_invalidate_cache(bs, &local_err);
> -        if (local_err) {
> -            bs->open_flags |= BDRV_O_INACTIVE;
> -            error_propagate(errp, local_err);
> -            return;
> -        }
> -    }
> -
> -    ret = refresh_total_sectors(bs, bs->total_sectors);
> -    if (ret < 0) {
> -        bs->open_flags |= BDRV_O_INACTIVE;
> -        error_setg_errno(errp, -ret, "Could not refresh total sector 
> count");
> -        return;
> -    }
>
>      /* Update permissions, they may differ for inactive nodes */
>      bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
> @@ -4161,6 +4149,22 @@ void bdrv_invalidate_cache(BlockDriverState 
> *bs, Error **errp)
>              }
>          }
>      }
> +
> +    if (bs->drv->bdrv_invalidate_cache) {
> +        bs->drv->bdrv_invalidate_cache(bs, &local_err);
> +        if (local_err) {
> +            bs->open_flags |= BDRV_O_INACTIVE;
> +            error_propagate(errp, local_err);
> +            return;
> +        }
> +    }
> +
> +    ret = refresh_total_sectors(bs, bs->total_sectors);
> +    if (ret < 0) {
> +        bs->open_flags |= BDRV_O_INACTIVE;
> +        error_setg_errno(errp, -ret, "Could not refresh total sector 
> count");
> +        return;
> +    }
>  }
>
>
> however, I doubt that it is safe to call ->activate before 
> invalidate_cache... I have no more ideas, just a patch that shows a 
> regression :(
>


-- 
Best regards,
Vladimir