| Message ID | 1426271419-8277-16-git-send-email-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 2015-03-13 at 14:30, John Snow wrote: > Signed-off-by: John Snow <jsnow@redhat.com> > --- > block.c | 18 +++++++++++++++++ > include/qemu/hbitmap.h | 10 ++++++++++ > util/hbitmap.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 80 insertions(+) > > diff --git a/block.c b/block.c > index 1eee394..f40b014 100644 > --- a/block.c > +++ b/block.c > @@ -113,6 +113,7 @@ static void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, > int nr_sectors); > static void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, > int nr_sectors); > +static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs); > /* If non-zero, use only whitelisted block drivers */ > static int use_bdrv_whitelist; > > @@ -3543,6 +3544,7 @@ int bdrv_truncate(BlockDriverState *bs, int64_t offset) > ret = drv->bdrv_truncate(bs, offset); > if (ret == 0) { > ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS); > + bdrv_dirty_bitmap_truncate(bs); > if (bs->blk) { > blk_dev_resize_cb(bs->blk); > } > @@ -5562,6 +5564,22 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, > return parent; > } > > +/** > + * Truncates _all_ bitmaps attached to a BDS. > + */ > +static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs) > +{ > + BdrvDirtyBitmap *bitmap; > + uint64_t size = bdrv_nb_sectors(bs); > + > + QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) { > + if (bdrv_dirty_bitmap_frozen(bitmap)) { > + continue; > + } > + hbitmap_truncate(bitmap->bitmap, size); > + } > +} > + > void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap) > { > BdrvDirtyBitmap *bm, *next; > diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h > index c19c1cb..a75157e 100644 > --- a/include/qemu/hbitmap.h > +++ b/include/qemu/hbitmap.h > @@ -65,6 +65,16 @@ struct HBitmapIter { > HBitmap *hbitmap_alloc(uint64_t size, int granularity); > > /** > + * hbitmap_truncate: > + * @hb: The bitmap to change the size of. > + * @size: The number of elements to change the bitmap to accommodate. > + * > + * truncate or grow an existing bitmap to accommodate a new number of elements. > + * This may invalidate existing HBitmapIterators. > + */ > +void hbitmap_truncate(HBitmap *hb, uint64_t size); > + > +/** > * hbitmap_merge: > * @a: The bitmap to store the result in. > * @b: The bitmap to merge into @a. > diff --git a/util/hbitmap.c b/util/hbitmap.c > index ba11fd3..4505ef7 100644 > --- a/util/hbitmap.c > +++ b/util/hbitmap.c > @@ -400,6 +400,58 @@ HBitmap *hbitmap_alloc(uint64_t size, int granularity) > return hb; > } > > +void hbitmap_truncate(HBitmap *hb, uint64_t size) > +{ > + bool shrink; > + unsigned i; > + uint64_t num_elements = size; > + uint64_t old; > + > + /* Size comes in as logical elements, adjust for granularity. */ > + size = (size + (1ULL << hb->granularity) - 1) >> hb->granularity; > + assert(size <= ((uint64_t)1 << HBITMAP_LOG_MAX_SIZE)); > + shrink = size < hb->size; > + > + /* bit sizes are identical; nothing to do. */ > + if (size == hb->size) { > + return; > + } > + > + /* If we're losing bits, let's clear those bits before we invalidate all of > + * our invariants. This helps keep the bitcount consistent, and will prevent > + * us from carrying around garbage bits beyond the end of the map. > + * > + * Because clearing bits past the end of map might reset bits we care about > + * within the array, record the current value of the last bit we're keeping. > + */ > + if (shrink) { > + bool set = hbitmap_get(hb, num_elements - 1); > + uint64_t fix_count = (hb->size << hb->granularity) - num_elements; > + > + assert(fix_count); > + hbitmap_reset(hb, num_elements, fix_count); > + if (set) { > + hbitmap_set(hb, num_elements - 1, 1); > + } > + } > + > + hb->size = size; > + for (i = HBITMAP_LEVELS; i-- > 0; ) { > + size = MAX(BITS_TO_LONGS(size), 1); Shouldn't this be "size = MAX(BITS_TO_LONGS(size) >> BITS_PER_LEVEL, 1);"? > + if (hb->sizes[i] == size) { > + break; > + } > + old = hb->sizes[i]; > + hb->sizes[i] = size; > + hb->levels[i] = g_realloc(hb->levels[i], size * sizeof(unsigned long)); Any specific reason you got rid of the g_realloc_n()? Apart from these, the changes to v2 look good. Max > + if (!shrink) { > + memset(&hb->levels[i][old], 0x00, > + (size - old) * sizeof(*hb->levels[i])); > + } > + } > +} > + > + > /** > * Given HBitmaps A and B, let A := A (BITOR) B. > * Bitmap B will not be modified.
On 03/17/2015 09:50 AM, Max Reitz wrote: > On 2015-03-13 at 14:30, John Snow wrote: >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> block.c | 18 +++++++++++++++++ >> include/qemu/hbitmap.h | 10 ++++++++++ >> util/hbitmap.c | 52 >> ++++++++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 80 insertions(+) >> >> diff --git a/block.c b/block.c >> index 1eee394..f40b014 100644 >> --- a/block.c >> +++ b/block.c >> @@ -113,6 +113,7 @@ static void bdrv_set_dirty(BlockDriverState *bs, >> int64_t cur_sector, >> int nr_sectors); >> static void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, >> int nr_sectors); >> +static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs); >> /* If non-zero, use only whitelisted block drivers */ >> static int use_bdrv_whitelist; >> @@ -3543,6 +3544,7 @@ int bdrv_truncate(BlockDriverState *bs, int64_t >> offset) >> ret = drv->bdrv_truncate(bs, offset); >> if (ret == 0) { >> ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS); >> + bdrv_dirty_bitmap_truncate(bs); >> if (bs->blk) { >> blk_dev_resize_cb(bs->blk); >> } >> @@ -5562,6 +5564,22 @@ BdrvDirtyBitmap >> *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, >> return parent; >> } >> +/** >> + * Truncates _all_ bitmaps attached to a BDS. >> + */ >> +static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs) >> +{ >> + BdrvDirtyBitmap *bitmap; >> + uint64_t size = bdrv_nb_sectors(bs); >> + >> + QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) { >> + if (bdrv_dirty_bitmap_frozen(bitmap)) { >> + continue; >> + } >> + hbitmap_truncate(bitmap->bitmap, size); >> + } >> +} >> + >> void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap >> *bitmap) >> { >> BdrvDirtyBitmap *bm, *next; >> diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h >> index c19c1cb..a75157e 100644 >> --- a/include/qemu/hbitmap.h >> +++ b/include/qemu/hbitmap.h >> @@ -65,6 +65,16 @@ struct HBitmapIter { >> HBitmap *hbitmap_alloc(uint64_t size, int granularity); >> /** >> + * hbitmap_truncate: >> + * @hb: The bitmap to change the size of. >> + * @size: The number of elements to change the bitmap to accommodate. >> + * >> + * truncate or grow an existing bitmap to accommodate a new number of >> elements. >> + * This may invalidate existing HBitmapIterators. >> + */ >> +void hbitmap_truncate(HBitmap *hb, uint64_t size); >> + >> +/** >> * hbitmap_merge: >> * @a: The bitmap to store the result in. >> * @b: The bitmap to merge into @a. >> diff --git a/util/hbitmap.c b/util/hbitmap.c >> index ba11fd3..4505ef7 100644 >> --- a/util/hbitmap.c >> +++ b/util/hbitmap.c >> @@ -400,6 +400,58 @@ HBitmap *hbitmap_alloc(uint64_t size, int >> granularity) >> return hb; >> } >> +void hbitmap_truncate(HBitmap *hb, uint64_t size) >> +{ >> + bool shrink; >> + unsigned i; >> + uint64_t num_elements = size; >> + uint64_t old; >> + >> + /* Size comes in as logical elements, adjust for granularity. */ >> + size = (size + (1ULL << hb->granularity) - 1) >> hb->granularity; >> + assert(size <= ((uint64_t)1 << HBITMAP_LOG_MAX_SIZE)); >> + shrink = size < hb->size; >> + >> + /* bit sizes are identical; nothing to do. */ >> + if (size == hb->size) { >> + return; >> + } >> + >> + /* If we're losing bits, let's clear those bits before we >> invalidate all of >> + * our invariants. This helps keep the bitcount consistent, and >> will prevent >> + * us from carrying around garbage bits beyond the end of the map. >> + * >> + * Because clearing bits past the end of map might reset bits we >> care about >> + * within the array, record the current value of the last bit >> we're keeping. >> + */ >> + if (shrink) { >> + bool set = hbitmap_get(hb, num_elements - 1); >> + uint64_t fix_count = (hb->size << hb->granularity) - >> num_elements; >> + >> + assert(fix_count); >> + hbitmap_reset(hb, num_elements, fix_count); >> + if (set) { >> + hbitmap_set(hb, num_elements - 1, 1); >> + } >> + } >> + >> + hb->size = size; >> + for (i = HBITMAP_LEVELS; i-- > 0; ) { >> + size = MAX(BITS_TO_LONGS(size), 1); > > Shouldn't this be "size = MAX(BITS_TO_LONGS(size) >> BITS_PER_LEVEL, 1);"? > I don't think so; BITS_TO_LONGS(X) replaces the original construct: (size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL which takes a size, adds <31|63> and then divides by <32|64>. BITS_TO_LONGS performs DIV_ROUND_UP(nr, <32|64>), which will do effectively the same thing. (Actually, a little less efficiently, but I found this macro was nicer to read.) >> + if (hb->sizes[i] == size) { >> + break; >> + } >> + old = hb->sizes[i]; >> + hb->sizes[i] = size; >> + hb->levels[i] = g_realloc(hb->levels[i], size * >> sizeof(unsigned long)); > > Any specific reason you got rid of the g_realloc_n()? > Not available in glib 2.12 (or 2.22.) > Apart from these, the changes to v2 look good. > > Max > >> + if (!shrink) { >> + memset(&hb->levels[i][old], 0x00, >> + (size - old) * sizeof(*hb->levels[i])); >> + } >> + } >> +} >> + >> + >> /** >> * Given HBitmaps A and B, let A := A (BITOR) B. >> * Bitmap B will not be modified. > >
On 2015-03-17 at 13:13, John Snow wrote: > > > On 03/17/2015 09:50 AM, Max Reitz wrote: >> On 2015-03-13 at 14:30, John Snow wrote: >>> Signed-off-by: John Snow <jsnow@redhat.com> >>> --- >>> block.c | 18 +++++++++++++++++ >>> include/qemu/hbitmap.h | 10 ++++++++++ >>> util/hbitmap.c | 52 >>> ++++++++++++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 80 insertions(+) >>> >>> diff --git a/block.c b/block.c >>> index 1eee394..f40b014 100644 >>> --- a/block.c >>> +++ b/block.c >>> @@ -113,6 +113,7 @@ static void bdrv_set_dirty(BlockDriverState *bs, >>> int64_t cur_sector, >>> int nr_sectors); >>> static void bdrv_reset_dirty(BlockDriverState *bs, int64_t >>> cur_sector, >>> int nr_sectors); >>> +static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs); >>> /* If non-zero, use only whitelisted block drivers */ >>> static int use_bdrv_whitelist; >>> @@ -3543,6 +3544,7 @@ int bdrv_truncate(BlockDriverState *bs, int64_t >>> offset) >>> ret = drv->bdrv_truncate(bs, offset); >>> if (ret == 0) { >>> ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS); >>> + bdrv_dirty_bitmap_truncate(bs); >>> if (bs->blk) { >>> blk_dev_resize_cb(bs->blk); >>> } >>> @@ -5562,6 +5564,22 @@ BdrvDirtyBitmap >>> *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, >>> return parent; >>> } >>> +/** >>> + * Truncates _all_ bitmaps attached to a BDS. >>> + */ >>> +static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs) >>> +{ >>> + BdrvDirtyBitmap *bitmap; >>> + uint64_t size = bdrv_nb_sectors(bs); >>> + >>> + QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) { >>> + if (bdrv_dirty_bitmap_frozen(bitmap)) { >>> + continue; >>> + } >>> + hbitmap_truncate(bitmap->bitmap, size); >>> + } >>> +} >>> + >>> void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap >>> *bitmap) >>> { >>> BdrvDirtyBitmap *bm, *next; >>> diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h >>> index c19c1cb..a75157e 100644 >>> --- a/include/qemu/hbitmap.h >>> +++ b/include/qemu/hbitmap.h >>> @@ -65,6 +65,16 @@ struct HBitmapIter { >>> HBitmap *hbitmap_alloc(uint64_t size, int granularity); >>> /** >>> + * hbitmap_truncate: >>> + * @hb: The bitmap to change the size of. >>> + * @size: The number of elements to change the bitmap to accommodate. >>> + * >>> + * truncate or grow an existing bitmap to accommodate a new number of >>> elements. >>> + * This may invalidate existing HBitmapIterators. >>> + */ >>> +void hbitmap_truncate(HBitmap *hb, uint64_t size); >>> + >>> +/** >>> * hbitmap_merge: >>> * @a: The bitmap to store the result in. >>> * @b: The bitmap to merge into @a. >>> diff --git a/util/hbitmap.c b/util/hbitmap.c >>> index ba11fd3..4505ef7 100644 >>> --- a/util/hbitmap.c >>> +++ b/util/hbitmap.c >>> @@ -400,6 +400,58 @@ HBitmap *hbitmap_alloc(uint64_t size, int >>> granularity) >>> return hb; >>> } >>> +void hbitmap_truncate(HBitmap *hb, uint64_t size) >>> +{ >>> + bool shrink; >>> + unsigned i; >>> + uint64_t num_elements = size; >>> + uint64_t old; >>> + >>> + /* Size comes in as logical elements, adjust for granularity. */ >>> + size = (size + (1ULL << hb->granularity) - 1) >> hb->granularity; >>> + assert(size <= ((uint64_t)1 << HBITMAP_LOG_MAX_SIZE)); >>> + shrink = size < hb->size; >>> + >>> + /* bit sizes are identical; nothing to do. */ >>> + if (size == hb->size) { >>> + return; >>> + } >>> + >>> + /* If we're losing bits, let's clear those bits before we >>> invalidate all of >>> + * our invariants. This helps keep the bitcount consistent, and >>> will prevent >>> + * us from carrying around garbage bits beyond the end of the map. >>> + * >>> + * Because clearing bits past the end of map might reset bits we >>> care about >>> + * within the array, record the current value of the last bit >>> we're keeping. >>> + */ >>> + if (shrink) { >>> + bool set = hbitmap_get(hb, num_elements - 1); >>> + uint64_t fix_count = (hb->size << hb->granularity) - >>> num_elements; >>> + >>> + assert(fix_count); >>> + hbitmap_reset(hb, num_elements, fix_count); >>> + if (set) { >>> + hbitmap_set(hb, num_elements - 1, 1); >>> + } >>> + } >>> + >>> + hb->size = size; >>> + for (i = HBITMAP_LEVELS; i-- > 0; ) { >>> + size = MAX(BITS_TO_LONGS(size), 1); >> >> Shouldn't this be "size = MAX(BITS_TO_LONGS(size) >> BITS_PER_LEVEL, >> 1);"? >> > > I don't think so; > > BITS_TO_LONGS(X) replaces the original construct: > (size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL > which takes a size, adds <31|63> and then divides by <32|64>. > > BITS_TO_LONGS performs DIV_ROUND_UP(nr, <32|64>), which will do > effectively the same thing. (Actually, a little less efficiently, but > I found this macro was nicer to read.) You're right (it's probably the same, efficiency-wise, as long as you have a compiler which optimizes x / 64 to x >> 6). I don't think it's nicer to read, though, because apparently it confused me. :-) >>> + if (hb->sizes[i] == size) { >>> + break; >>> + } >>> + old = hb->sizes[i]; >>> + hb->sizes[i] = size; >>> + hb->levels[i] = g_realloc(hb->levels[i], size * >>> sizeof(unsigned long)); >> >> Any specific reason you got rid of the g_realloc_n()? >> > > Not available in glib 2.12 (or 2.22.) Urgh... The RHEL 5 curse? :-/ Anyway: Reviewed-by: Max Reitz <mreitz@redhat.com> > >> Apart from these, the changes to v2 look good. >> >> Max >> >>> + if (!shrink) { >>> + memset(&hb->levels[i][old], 0x00, >>> + (size - old) * sizeof(*hb->levels[i])); >>> + } >>> + } >>> +} >>> + >>> + >>> /** >>> * Given HBitmaps A and B, let A := A (BITOR) B. >>> * Bitmap B will not be modified. >> >>
diff --git a/block.c b/block.c index 1eee394..f40b014 100644 --- a/block.c +++ b/block.c @@ -113,6 +113,7 @@ static void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors); static void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors); +static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs); /* If non-zero, use only whitelisted block drivers */ static int use_bdrv_whitelist; @@ -3543,6 +3544,7 @@ int bdrv_truncate(BlockDriverState *bs, int64_t offset) ret = drv->bdrv_truncate(bs, offset); if (ret == 0) { ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS); + bdrv_dirty_bitmap_truncate(bs); if (bs->blk) { blk_dev_resize_cb(bs->blk); } @@ -5562,6 +5564,22 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, return parent; } +/** + * Truncates _all_ bitmaps attached to a BDS. + */ +static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs) +{ + BdrvDirtyBitmap *bitmap; + uint64_t size = bdrv_nb_sectors(bs); + + QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) { + if (bdrv_dirty_bitmap_frozen(bitmap)) { + continue; + } + hbitmap_truncate(bitmap->bitmap, size); + } +} + void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap) { BdrvDirtyBitmap *bm, *next; diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h index c19c1cb..a75157e 100644 --- a/include/qemu/hbitmap.h +++ b/include/qemu/hbitmap.h @@ -65,6 +65,16 @@ struct HBitmapIter { HBitmap *hbitmap_alloc(uint64_t size, int granularity); /** + * hbitmap_truncate: + * @hb: The bitmap to change the size of. + * @size: The number of elements to change the bitmap to accommodate. + * + * truncate or grow an existing bitmap to accommodate a new number of elements. + * This may invalidate existing HBitmapIterators. + */ +void hbitmap_truncate(HBitmap *hb, uint64_t size); + +/** * hbitmap_merge: * @a: The bitmap to store the result in. * @b: The bitmap to merge into @a. diff --git a/util/hbitmap.c b/util/hbitmap.c index ba11fd3..4505ef7 100644 --- a/util/hbitmap.c +++ b/util/hbitmap.c @@ -400,6 +400,58 @@ HBitmap *hbitmap_alloc(uint64_t size, int granularity) return hb; } +void hbitmap_truncate(HBitmap *hb, uint64_t size) +{ + bool shrink; + unsigned i; + uint64_t num_elements = size; + uint64_t old; + + /* Size comes in as logical elements, adjust for granularity. */ + size = (size + (1ULL << hb->granularity) - 1) >> hb->granularity; + assert(size <= ((uint64_t)1 << HBITMAP_LOG_MAX_SIZE)); + shrink = size < hb->size; + + /* bit sizes are identical; nothing to do. */ + if (size == hb->size) { + return; + } + + /* If we're losing bits, let's clear those bits before we invalidate all of + * our invariants. This helps keep the bitcount consistent, and will prevent + * us from carrying around garbage bits beyond the end of the map. + * + * Because clearing bits past the end of map might reset bits we care about + * within the array, record the current value of the last bit we're keeping. + */ + if (shrink) { + bool set = hbitmap_get(hb, num_elements - 1); + uint64_t fix_count = (hb->size << hb->granularity) - num_elements; + + assert(fix_count); + hbitmap_reset(hb, num_elements, fix_count); + if (set) { + hbitmap_set(hb, num_elements - 1, 1); + } + } + + hb->size = size; + for (i = HBITMAP_LEVELS; i-- > 0; ) { + size = MAX(BITS_TO_LONGS(size), 1); + if (hb->sizes[i] == size) { + break; + } + old = hb->sizes[i]; + hb->sizes[i] = size; + hb->levels[i] = g_realloc(hb->levels[i], size * sizeof(unsigned long)); + if (!shrink) { + memset(&hb->levels[i][old], 0x00, + (size - old) * sizeof(*hb->levels[i])); + } + } +} + + /** * Given HBitmaps A and B, let A := A (BITOR) B. * Bitmap B will not be modified.
Signed-off-by: John Snow <jsnow@redhat.com> --- block.c | 18 +++++++++++++++++ include/qemu/hbitmap.h | 10 ++++++++++ util/hbitmap.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 80 insertions(+)