On 06/09/2015 05:24 AM, Kevin Wolf wrote:
> Am 08.06.2015 um 22:49 hat John Snow geschrieben:
>> ce1ffea8 neglected to update the BdrvDirtyBitmap structure
>> itself for internal consistency. It's currently not an issue,
>> but for migration and persistence series this will cause headaches.
>>
>> Signed-off-by: John Snow <js...@redhat.com>
>
> I know nothing about dirty bitmaps, but this one looks obvious enough,
> I'll apply it.
>
>> diff --git a/block.c b/block.c
>> index 2b9ceae..2786e47 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3224,6 +3224,7 @@ static void
>> bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
>> continue;
>> }
>> hbitmap_truncate(bitmap->bitmap, size);
>> + bitmap->size = size;
>> }
>> }
>
> However, I'm left wondering whether that 'continue' in the context of
> that hunk is right. More context:
>
> QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
> if (bdrv_dirty_bitmap_frozen(bitmap)) {
> continue;
> }
> hbitmap_truncate(bitmap->bitmap, size);
> }
>
> If the image just shrunk, the frozen bitmap covers parts of the image
> that don't exist any more. When they are read out for the backup, that
> request would fail.
>
> If the image was extended, the frozen bitmap covers only part of the
> image. There are a few bitmap functions that don't check the size and
> would just work beyond the end of the bitmap if called with a now valid
> sector number that is outside the image.
>
> In practice, I don't think any of these happen because of op blockers
> that prevent resizing while a backup is in progress, but should
> !bdrv_dirty_bitmap_frozen(bitmap) be asserted then rather than just
> skipping the bitmap?
>
> Kevin
>
Yeah, that won't hurt anything and will read cleaner. I'll just v2 this
patch, thanks.
--js
