Am 09.06.2015 um 17:46 hat John Snow geschrieben:
>
>
> 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.
It's unrelated to this patch (except for touching the same function), so
I'd suggest to make it a separate patch.
Kevin
