Re: [Qemu-devel] [PATCH] block: record new size in bdrv_dirty_bitmap_truncate

[John Snow]

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 <address@hidden>
> 
> 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