| Message ID | 1422356197-5285-5-git-send-email-vsementsov@parallels.com |
|---|---|
| State | New |
| Headers | show |
I had hoped it wouldn't come to this :) On 01/27/2015 05:56 AM, Vladimir Sementsov-Ogievskiy wrote: > A dirty-dirty bitmap is a dirty bitmap for BdrvDirtyBitmap. It tracks > set/unset changes of BdrvDirtyBitmap. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com> > --- > block.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ > include/block/block.h | 5 +++++ > 2 files changed, 49 insertions(+) > > diff --git a/block.c b/block.c > index 9860fc1..8ab724b 100644 > --- a/block.c > +++ b/block.c > @@ -53,6 +53,7 @@ > > struct BdrvDirtyBitmap { > HBitmap *bitmap; > + HBitmap *dirty_dirty_bitmap; Just opinions: Maybe we can call it the "meta_bitmap" to help keep the name less confusing, and accompany it with a good structure comment here to explain what the heck it's for. If you feel that's a good idea; s/dirty_dirty_/meta_/g below. Regardless, let's make sure this patch adds documentation for it. > BdrvDirtyBitmap *originator; > int64_t size; > int64_t granularity; > @@ -5373,6 +5374,30 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, > return originator; > } > > +HBitmap *bdrv_create_dirty_dirty_bitmap(BdrvDirtyBitmap *bitmap, > + uint64_t granularity) > +{ > + uint64_t sector_granularity; > + > + assert((granularity & (granularity - 1)) == 0); > + > + granularity *= 8 * bitmap->granularity; > + sector_granularity = granularity >> BDRV_SECTOR_BITS; > + assert(sector_granularity); > + > + bitmap->dirty_dirty_bitmap = > + hbitmap_alloc(bitmap->size, ffsll(sector_granularity) - 1); > + > + return bitmap->dirty_dirty_bitmap; > +} > + > +void bdrv_release_dirty_dirty_bitmap(BdrvDirtyBitmap *bitmap) > +{ > + if (bitmap->dirty_dirty_bitmap) { > + hbitmap_free(bitmap->dirty_dirty_bitmap); > + bitmap->dirty_dirty_bitmap = NULL; > + } > +} > > void bdrv_print_dirty_bitmap(BdrvDirtyBitmap *bitmap) > { > @@ -5447,6 +5472,9 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap) > if (bm == bitmap) { > QLIST_REMOVE(bitmap, list); > hbitmap_free(bitmap->bitmap); > + if (bitmap->dirty_dirty_bitmap) { > + hbitmap_free(bitmap->dirty_dirty_bitmap); > + } > g_free(bitmap->name); > g_free(bitmap); > return; > @@ -5534,6 +5562,10 @@ void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, > { > if (bitmap->enabled) { > hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors); > + > + if (bitmap->dirty_dirty_bitmap) { > + hbitmap_set(bitmap->dirty_dirty_bitmap, cur_sector, nr_sectors); > + } > } > } > > @@ -5541,6 +5573,9 @@ void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, > int64_t cur_sector, int nr_sectors) > { > hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors); > + if (bitmap->dirty_dirty_bitmap) { > + hbitmap_set(bitmap->dirty_dirty_bitmap, cur_sector, nr_sectors); > + } > } > > /** > @@ -5550,6 +5585,9 @@ void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, > void bdrv_clear_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap) > { > hbitmap_reset(bitmap->bitmap, 0, bitmap->size); > + if (bitmap->dirty_dirty_bitmap) { > + hbitmap_set(bitmap->dirty_dirty_bitmap, 0, bitmap->size); > + } > } > > const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap) > @@ -5597,6 +5635,9 @@ static void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, > continue; > } > hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors); > + if (bitmap->dirty_dirty_bitmap) { > + hbitmap_set(bitmap->dirty_dirty_bitmap, cur_sector, nr_sectors); > + } > } > } > > @@ -5606,6 +5647,9 @@ static void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, > BdrvDirtyBitmap *bitmap; > QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) { > hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors); > + if (bitmap->dirty_dirty_bitmap) { > + hbitmap_set(bitmap->dirty_dirty_bitmap, cur_sector, nr_sectors); > + } > } > } > > diff --git a/include/block/block.h b/include/block/block.h > index 0890cd2..648b0a9 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -4,6 +4,7 @@ > #include "block/aio.h" > #include "qemu-common.h" > #include "qemu/option.h" > +#include "qemu/hbitmap.h" > #include "block/coroutine.h" > #include "block/accounting.h" > #include "qapi/qmp/qobject.h" > @@ -473,6 +474,10 @@ void bdrv_dirty_bitmap_deserialize_part0(BdrvDirtyBitmap *bitmap, > uint64_t start, uint64_t count); > void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap); > > +HBitmap *bdrv_create_dirty_dirty_bitmap(BdrvDirtyBitmap *bitmap, > + uint64_t granularity); > +void bdrv_release_dirty_dirty_bitmap(BdrvDirtyBitmap *bitmap); > + > void bdrv_enable_copy_on_read(BlockDriverState *bs); > void bdrv_disable_copy_on_read(BlockDriverState *bs); > > Looks correct, it just needs documentation in my opinion to help explain what this extra bitmap is for. Musings / opinions: I also think that at this point, we may want to make bdrv_reset_dirty and bdrv_set_dirty call the bdrv_reset_dirty_bitmap and bdrv_set_dirty_bitmap functions instead of re-implementing the behavior. I used to think it was fine as-is, but the more conditions we add to how these bitmaps should be accessed, the more I think limiting the interface to as few functions as possible is a good idea. Maybe I'll even do that myself. It might even be nice to split off all the bitmap functions off into something like block/dirty_bitmaps.c as the complexity creeps up and we're trying to improve the maintainability of block.c. Anyway, we can worry about that in a later series.
On 11.02.2015 00:30, John Snow wrote: > I had hoped it wouldn't come to this :) > > On 01/27/2015 05:56 AM, Vladimir Sementsov-Ogievskiy wrote: >> A dirty-dirty bitmap is a dirty bitmap for BdrvDirtyBitmap. It tracks >> set/unset changes of BdrvDirtyBitmap. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com> >> --- >> block.c | 44 >> ++++++++++++++++++++++++++++++++++++++++++++ >> include/block/block.h | 5 +++++ >> 2 files changed, 49 insertions(+) >> >> diff --git a/block.c b/block.c >> index 9860fc1..8ab724b 100644 >> --- a/block.c >> +++ b/block.c >> @@ -53,6 +53,7 @@ >> >> struct BdrvDirtyBitmap { >> HBitmap *bitmap; >> + HBitmap *dirty_dirty_bitmap; > > Just opinions: Maybe we can call it the "meta_bitmap" to help keep the > name less confusing, and accompany it with a good structure comment > here to explain what the heck it's for. > > If you feel that's a good idea; s/dirty_dirty_/meta_/g below. > > Regardless, let's make sure this patch adds documentation for it. No objections. If everyone is happy with meta_bitmaps - I'll use this name. Dirty-dirty bitmaps are just more fanny, I think ;) > >> BdrvDirtyBitmap *originator; >> int64_t size; >> int64_t granularity; >> @@ -5373,6 +5374,30 @@ BdrvDirtyBitmap >> *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, >> return originator; >> } >> >> +HBitmap *bdrv_create_dirty_dirty_bitmap(BdrvDirtyBitmap *bitmap, >> + uint64_t granularity) >> +{ >> + uint64_t sector_granularity; >> + >> + assert((granularity & (granularity - 1)) == 0); >> + >> + granularity *= 8 * bitmap->granularity; >> + sector_granularity = granularity >> BDRV_SECTOR_BITS; >> + assert(sector_granularity); >> + >> + bitmap->dirty_dirty_bitmap = >> + hbitmap_alloc(bitmap->size, ffsll(sector_granularity) - 1); >> + >> + return bitmap->dirty_dirty_bitmap; >> +} >> + >> +void bdrv_release_dirty_dirty_bitmap(BdrvDirtyBitmap *bitmap) >> +{ >> + if (bitmap->dirty_dirty_bitmap) { >> + hbitmap_free(bitmap->dirty_dirty_bitmap); >> + bitmap->dirty_dirty_bitmap = NULL; >> + } >> +} >> >> void bdrv_print_dirty_bitmap(BdrvDirtyBitmap *bitmap) >> { >> @@ -5447,6 +5472,9 @@ void bdrv_release_dirty_bitmap(BlockDriverState >> *bs, BdrvDirtyBitmap *bitmap) >> if (bm == bitmap) { >> QLIST_REMOVE(bitmap, list); >> hbitmap_free(bitmap->bitmap); >> + if (bitmap->dirty_dirty_bitmap) { >> + hbitmap_free(bitmap->dirty_dirty_bitmap); >> + } >> g_free(bitmap->name); >> g_free(bitmap); >> return; >> @@ -5534,6 +5562,10 @@ void bdrv_set_dirty_bitmap(BlockDriverState >> *bs, BdrvDirtyBitmap *bitmap, >> { >> if (bitmap->enabled) { >> hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors); >> + >> + if (bitmap->dirty_dirty_bitmap) { >> + hbitmap_set(bitmap->dirty_dirty_bitmap, cur_sector, >> nr_sectors); >> + } >> } >> } >> >> @@ -5541,6 +5573,9 @@ void bdrv_reset_dirty_bitmap(BlockDriverState >> *bs, BdrvDirtyBitmap *bitmap, >> int64_t cur_sector, int nr_sectors) >> { >> hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors); >> + if (bitmap->dirty_dirty_bitmap) { >> + hbitmap_set(bitmap->dirty_dirty_bitmap, cur_sector, >> nr_sectors); >> + } >> } >> >> /** >> @@ -5550,6 +5585,9 @@ void bdrv_reset_dirty_bitmap(BlockDriverState >> *bs, BdrvDirtyBitmap *bitmap, >> void bdrv_clear_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap >> *bitmap) >> { >> hbitmap_reset(bitmap->bitmap, 0, bitmap->size); >> + if (bitmap->dirty_dirty_bitmap) { >> + hbitmap_set(bitmap->dirty_dirty_bitmap, 0, bitmap->size); >> + } >> } >> >> const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap) >> @@ -5597,6 +5635,9 @@ static void bdrv_set_dirty(BlockDriverState >> *bs, int64_t cur_sector, >> continue; >> } >> hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors); >> + if (bitmap->dirty_dirty_bitmap) { >> + hbitmap_set(bitmap->dirty_dirty_bitmap, cur_sector, >> nr_sectors); >> + } >> } >> } >> >> @@ -5606,6 +5647,9 @@ static void bdrv_reset_dirty(BlockDriverState >> *bs, int64_t cur_sector, >> BdrvDirtyBitmap *bitmap; >> QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) { >> hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors); >> + if (bitmap->dirty_dirty_bitmap) { >> + hbitmap_set(bitmap->dirty_dirty_bitmap, cur_sector, >> nr_sectors); >> + } >> } >> } >> >> diff --git a/include/block/block.h b/include/block/block.h >> index 0890cd2..648b0a9 100644 >> --- a/include/block/block.h >> +++ b/include/block/block.h >> @@ -4,6 +4,7 @@ >> #include "block/aio.h" >> #include "qemu-common.h" >> #include "qemu/option.h" >> +#include "qemu/hbitmap.h" >> #include "block/coroutine.h" >> #include "block/accounting.h" >> #include "qapi/qmp/qobject.h" >> @@ -473,6 +474,10 @@ void >> bdrv_dirty_bitmap_deserialize_part0(BdrvDirtyBitmap *bitmap, >> uint64_t start, uint64_t >> count); >> void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap); >> >> +HBitmap *bdrv_create_dirty_dirty_bitmap(BdrvDirtyBitmap *bitmap, >> + uint64_t granularity); >> +void bdrv_release_dirty_dirty_bitmap(BdrvDirtyBitmap *bitmap); >> + >> void bdrv_enable_copy_on_read(BlockDriverState *bs); >> void bdrv_disable_copy_on_read(BlockDriverState *bs); >> >> > > Looks correct, it just needs documentation in my opinion to help > explain what this extra bitmap is for. > > Musings / opinions: > > I also think that at this point, we may want to make bdrv_reset_dirty > and bdrv_set_dirty call the bdrv_reset_dirty_bitmap and > bdrv_set_dirty_bitmap functions instead of re-implementing the behavior. > > I used to think it was fine as-is, but the more conditions we add to > how these bitmaps should be accessed, the more I think limiting the > interface to as few functions as possible is a good idea. > > Maybe I'll even do that myself. It might even be nice to split off all > the bitmap functions off into something like block/dirty_bitmaps.c as > the complexity creeps up and we're trying to improve the > maintainability of block.c. > > Anyway, we can worry about that in a later series.
diff --git a/block.c b/block.c index 9860fc1..8ab724b 100644 --- a/block.c +++ b/block.c @@ -53,6 +53,7 @@ struct BdrvDirtyBitmap { HBitmap *bitmap; + HBitmap *dirty_dirty_bitmap; BdrvDirtyBitmap *originator; int64_t size; int64_t granularity; @@ -5373,6 +5374,30 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, return originator; } +HBitmap *bdrv_create_dirty_dirty_bitmap(BdrvDirtyBitmap *bitmap, + uint64_t granularity) +{ + uint64_t sector_granularity; + + assert((granularity & (granularity - 1)) == 0); + + granularity *= 8 * bitmap->granularity; + sector_granularity = granularity >> BDRV_SECTOR_BITS; + assert(sector_granularity); + + bitmap->dirty_dirty_bitmap = + hbitmap_alloc(bitmap->size, ffsll(sector_granularity) - 1); + + return bitmap->dirty_dirty_bitmap; +} + +void bdrv_release_dirty_dirty_bitmap(BdrvDirtyBitmap *bitmap) +{ + if (bitmap->dirty_dirty_bitmap) { + hbitmap_free(bitmap->dirty_dirty_bitmap); + bitmap->dirty_dirty_bitmap = NULL; + } +} void bdrv_print_dirty_bitmap(BdrvDirtyBitmap *bitmap) { @@ -5447,6 +5472,9 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap) if (bm == bitmap) { QLIST_REMOVE(bitmap, list); hbitmap_free(bitmap->bitmap); + if (bitmap->dirty_dirty_bitmap) { + hbitmap_free(bitmap->dirty_dirty_bitmap); + } g_free(bitmap->name); g_free(bitmap); return; @@ -5534,6 +5562,10 @@ void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, { if (bitmap->enabled) { hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors); + + if (bitmap->dirty_dirty_bitmap) { + hbitmap_set(bitmap->dirty_dirty_bitmap, cur_sector, nr_sectors); + } } } @@ -5541,6 +5573,9 @@ void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t cur_sector, int nr_sectors) { hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors); + if (bitmap->dirty_dirty_bitmap) { + hbitmap_set(bitmap->dirty_dirty_bitmap, cur_sector, nr_sectors); + } } /** @@ -5550,6 +5585,9 @@ void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, void bdrv_clear_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap) { hbitmap_reset(bitmap->bitmap, 0, bitmap->size); + if (bitmap->dirty_dirty_bitmap) { + hbitmap_set(bitmap->dirty_dirty_bitmap, 0, bitmap->size); + } } const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap) @@ -5597,6 +5635,9 @@ static void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, continue; } hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors); + if (bitmap->dirty_dirty_bitmap) { + hbitmap_set(bitmap->dirty_dirty_bitmap, cur_sector, nr_sectors); + } } } @@ -5606,6 +5647,9 @@ static void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, BdrvDirtyBitmap *bitmap; QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) { hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors); + if (bitmap->dirty_dirty_bitmap) { + hbitmap_set(bitmap->dirty_dirty_bitmap, cur_sector, nr_sectors); + } } } diff --git a/include/block/block.h b/include/block/block.h index 0890cd2..648b0a9 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -4,6 +4,7 @@ #include "block/aio.h" #include "qemu-common.h" #include "qemu/option.h" +#include "qemu/hbitmap.h" #include "block/coroutine.h" #include "block/accounting.h" #include "qapi/qmp/qobject.h" @@ -473,6 +474,10 @@ void bdrv_dirty_bitmap_deserialize_part0(BdrvDirtyBitmap *bitmap, uint64_t start, uint64_t count); void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap); +HBitmap *bdrv_create_dirty_dirty_bitmap(BdrvDirtyBitmap *bitmap, + uint64_t granularity); +void bdrv_release_dirty_dirty_bitmap(BdrvDirtyBitmap *bitmap); + void bdrv_enable_copy_on_read(BlockDriverState *bs); void bdrv_disable_copy_on_read(BlockDriverState *bs);
A dirty-dirty bitmap is a dirty bitmap for BdrvDirtyBitmap. It tracks set/unset changes of BdrvDirtyBitmap. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com> --- block.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ include/block/block.h | 5 +++++ 2 files changed, 49 insertions(+)