On 06/22/2016 11:53 AM, Max Reitz wrote: > On 03.06.2016 06:32, Fam Zheng wrote: >> The added group of operations enables tracking of the changed bits in >> the dirty bitmap. >> >> Signed-off-by: Fam Zheng <f...@redhat.com> >> --- >> block/dirty-bitmap.c | 52 >> ++++++++++++++++++++++++++++++++++++++++++++ >> include/block/dirty-bitmap.h | 9 ++++++++ >> 2 files changed, 61 insertions(+) >> >> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c >> index 628b77c..9c53c56 100644 >> --- a/block/dirty-bitmap.c >> +++ b/block/dirty-bitmap.c >> @@ -38,6 +38,7 @@ >> */ >> struct BdrvDirtyBitmap { >> HBitmap *bitmap; /* Dirty sector bitmap implementation */ >> + HBitmap *meta; /* Meta dirty bitmap */ >> BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */ >> char *name; /* Optional non-empty unique ID */ >> int64_t size; /* Size of the bitmap (Number of sectors) */ >> @@ -103,6 +104,56 @@ BdrvDirtyBitmap >> *bdrv_create_dirty_bitmap(BlockDriverState *bs, >> return bitmap; >> } >> >> +/* bdrv_create_meta_dirty_bitmap >> + * >> + * Create a meta dirty bitmap that tracks the changes of bits in @bitmap. >> I.e. >> + * when a dirty status bit in @bitmap is changed (either from reset to set >> or >> + * the other way around), its respective meta dirty bitmap bit will be >> marked >> + * dirty as well. > > Not wrong, but I'd like a note here that this is not an > when-and-only-when relationship, i.e. that bits in the meta bitmap may > be set even without the tracked bits in the dirty bitmap having changed. >
How? You mean, if the caller manually starts setting things in the meta bitmap object? That's their fault then, isn't it? > Maybe this should be mentioned somewhere in patch 2, too. Or maybe only > in patch 2. > >> + * >> + * @bitmap: the block dirty bitmap for which to create a meta dirty bitmap. >> + * @chunk_size: how many bytes of bitmap data does each bit in the meta >> bitmap >> + * track. >> + */ >> +void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap, >> + int chunk_size) >> +{ >> + assert(!bitmap->meta); >> + bitmap->meta = hbitmap_create_meta(bitmap->bitmap, >> + chunk_size * BITS_PER_BYTE); >> +} >> + >> +void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap) >> +{ >> + assert(bitmap->meta); >> + hbitmap_free_meta(bitmap->bitmap); >> + bitmap->meta = NULL; >> +} >> + >> +int bdrv_dirty_bitmap_get_meta(BlockDriverState *bs, >> + BdrvDirtyBitmap *bitmap, int64_t sector, >> + int nb_sectors) >> +{ >> + uint64_t i; >> + int gran = bdrv_dirty_bitmap_granularity(bitmap) >> BDRV_SECTOR_BITS; >> + >> + /* To optimize: we can make hbitmap to internally check the range in a >> + * coarse level, or at least do it word by word. */ > > We could also multiply gran by the granularity of the meta bitmap. Each > bit of the meta bitmap tracks at least eight bits of the dirty bitmap, > so we're calling hbitmap_get() at least eight times as often as > necessary here. > > Or we just use int gran = hbitmap_granularity(bitmap->meta);. > > Not exactly wrong, though, so: > > Reviewed-by: Max Reitz <mre...@redhat.com> > >> + for (i = sector; i < sector + nb_sectors; i += gran) { >> + if (hbitmap_get(bitmap->meta, i)) { >> + return true; >> + } >> + } >> + return false; >> +} >