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

[Kevin Wolf]

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 <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.

It's unrelated to this patch (except for touching the same function), so
I'd suggest to make it a separate patch.

Kevin