| Message ID | 1433796555-5608-1-git-send-email-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 06/08/2015 02:49 PM, John Snow wrote: > 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 <jsnow@redhat.com> > --- > block.c | 1 + > 1 file changed, 1 insertion(+) Reviewed-by: Eric Blake <eblake@redhat.com> > > 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; > } > } > >
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 <jsnow@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
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 <jsnow@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
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 <jsnow@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
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; } }
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 <jsnow@redhat.com> --- block.c | 1 + 1 file changed, 1 insertion(+)