| Message ID | 20170703151051.31327-5-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/03/2017 11:10 AM, Eric Blake wrote: > We had several functions that no one is currently using, and which > use sector-based interfaces. I'm trying to convert towards byte-based > interfaces, so it's easier to just drop the unused functions: > > bdrv_dirty_bitmap_get_meta > bdrv_dirty_bitmap_get_meta_locked > bdrv_dirty_bitmap_reset_meta > bdrv_dirty_bitmap_meta_granularity > > Signed-off-by: Eric Blake <eblake@redhat.com> > Admittedly I forget entirely why some of these are here, or to be more precise I forget entirely why they are unused as I remember exactly why we needed them -- but I forget why we merged them without a caller. Oh well. If they're unused, they're unused, and can be re-added, especially if we're changing the underpinnings. I bet my last review on this email used nearly identical wordings. I'm CCing Vladimir so he has a chance to see that we're removing functions that he may have wanted to use for his migration series. Reviewed-by: John Snow <jsnow@redhat.com>
07.07.2017 02:43, John Snow wrote: > > On 07/03/2017 11:10 AM, Eric Blake wrote: >> We had several functions that no one is currently using, and which >> use sector-based interfaces. I'm trying to convert towards byte-based >> interfaces, so it's easier to just drop the unused functions: >> >> bdrv_dirty_bitmap_get_meta >> bdrv_dirty_bitmap_get_meta_locked >> bdrv_dirty_bitmap_reset_meta >> bdrv_dirty_bitmap_meta_granularity >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> > Admittedly I forget entirely why some of these are here, or to be more > precise I forget entirely why they are unused as I remember exactly why > we needed them -- but I forget why we merged them without a caller. > > Oh well. If they're unused, they're unused, and can be re-added, > especially if we're changing the underpinnings. > > I bet my last review on this email used nearly identical wordings. > > I'm CCing Vladimir so he has a chance to see that we're removing > functions that he may have wanted to use for his migration series. > > Reviewed-by: John Snow <jsnow@redhat.com> > Meta bitmaps was first approach of my dirty-bitmap migration realization, and their first realization was in early bitmap-migration series. However, now my series are based on postcopy approach and meta bitmaps are not needed. Current realization of meta bitmaps was started by Fam, he wanted to use them not only for my migration, but also for persistent bitmap optimization (to reduce amount of dirt-bitmap data to be saved, to save only changed parts). This is still true: we can try to make persistent bitmaps storing more efficient, using meta bitmaps, but I'm not sure that it is worth doing and I have no plans on it for now.
07.07.2017 11:44, Vladimir Sementsov-Ogievskiy wrote: > 07.07.2017 02:43, John Snow wrote: >> >> On 07/03/2017 11:10 AM, Eric Blake wrote: >>> We had several functions that no one is currently using, and which >>> use sector-based interfaces. I'm trying to convert towards byte-based >>> interfaces, so it's easier to just drop the unused functions: >>> >>> bdrv_dirty_bitmap_get_meta >>> bdrv_dirty_bitmap_get_meta_locked >>> bdrv_dirty_bitmap_reset_meta >>> bdrv_dirty_bitmap_meta_granularity >>> >>> Signed-off-by: Eric Blake <eblake@redhat.com> >>> >> Admittedly I forget entirely why some of these are here, or to be more >> precise I forget entirely why they are unused as I remember exactly why >> we needed them -- but I forget why we merged them without a caller. >> >> Oh well. If they're unused, they're unused, and can be re-added, >> especially if we're changing the underpinnings. >> >> I bet my last review on this email used nearly identical wordings. >> >> I'm CCing Vladimir so he has a chance to see that we're removing >> functions that he may have wanted to use for his migration series. >> >> Reviewed-by: John Snow <jsnow@redhat.com> >> > > Meta bitmaps was first approach of my dirty-bitmap migration > realization, and their first realization was in early bitmap-migration > series. However, now my series are based on postcopy approach and meta > bitmaps are not needed. > > Current realization of meta bitmaps was started by Fam, he wanted to > use them not only for my migration, but also for persistent bitmap > optimization (to reduce amount of dirt-bitmap data to be saved, to > save only changed parts). > > This is still true: we can try to make persistent bitmaps storing more > efficient, using meta bitmaps, but I'm not sure that it is worth doing > and I have no plans on it for now. > > > Also, I think true way of optimization would be move from hbitmap to some kind of sparse bitmap, where bitamp is devided into several blocks, and for each block we can store additional info like - offset (link to data block in ram) - present (offset is valid link to corresponding data block, otherwise, block is not allocated in ram) - all zeros (data block is not needed, offset is ignored) - all ones - not loaded (for lazy persistent bitmap loading, we can load bitmap blocks on demand) - changed (for persistent bitmaps - this block should be saved) - etc..
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h index a79a58d..8fd842e 100644 --- a/include/block/dirty-bitmap.h +++ b/include/block/dirty-bitmap.h @@ -34,7 +34,6 @@ void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap); BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs); uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs); uint32_t bdrv_dirty_bitmap_granularity(const BdrvDirtyBitmap *bitmap); -uint32_t bdrv_dirty_bitmap_meta_granularity(BdrvDirtyBitmap *bitmap); bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap); bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap); const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap); @@ -44,15 +43,6 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap, int64_t cur_sector, int64_t nr_sectors); void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap, int64_t cur_sector, int64_t nr_sectors); -int bdrv_dirty_bitmap_get_meta(BlockDriverState *bs, - BdrvDirtyBitmap *bitmap, int64_t sector, - int nb_sectors); -int bdrv_dirty_bitmap_get_meta_locked(BlockDriverState *bs, - BdrvDirtyBitmap *bitmap, int64_t sector, - int nb_sectors); -void bdrv_dirty_bitmap_reset_meta(BlockDriverState *bs, - BdrvDirtyBitmap *bitmap, int64_t sector, - int nb_sectors); BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap); BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap, uint64_t first_sector); diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 0490ca3..42a55e4 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -173,45 +173,6 @@ void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap) qemu_mutex_unlock(bitmap->mutex); } -int bdrv_dirty_bitmap_get_meta_locked(BlockDriverState *bs, - BdrvDirtyBitmap *bitmap, int64_t sector, - int nb_sectors) -{ - uint64_t i; - int sectors_per_bit = 1 << hbitmap_granularity(bitmap->meta); - - /* To optimize: we can make hbitmap to internally check the range in a - * coarse level, or at least do it word by word. */ - for (i = sector; i < sector + nb_sectors; i += sectors_per_bit) { - if (hbitmap_get(bitmap->meta, i)) { - return true; - } - } - return false; -} - -int bdrv_dirty_bitmap_get_meta(BlockDriverState *bs, - BdrvDirtyBitmap *bitmap, int64_t sector, - int nb_sectors) -{ - bool dirty; - - qemu_mutex_lock(bitmap->mutex); - dirty = bdrv_dirty_bitmap_get_meta_locked(bs, bitmap, sector, nb_sectors); - qemu_mutex_unlock(bitmap->mutex); - - return dirty; -} - -void bdrv_dirty_bitmap_reset_meta(BlockDriverState *bs, - BdrvDirtyBitmap *bitmap, int64_t sector, - int nb_sectors) -{ - qemu_mutex_lock(bitmap->mutex); - hbitmap_reset(bitmap->meta, sector, nb_sectors); - qemu_mutex_unlock(bitmap->mutex); -} - int64_t bdrv_dirty_bitmap_size(const BdrvDirtyBitmap *bitmap) { return bitmap->size; @@ -511,11 +472,6 @@ uint32_t bdrv_dirty_bitmap_granularity(const BdrvDirtyBitmap *bitmap) return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap); } -uint32_t bdrv_dirty_bitmap_meta_granularity(BdrvDirtyBitmap *bitmap) -{ - return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->meta); -} - BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap, uint64_t first_sector) {
We had several functions that no one is currently using, and which use sector-based interfaces. I'm trying to convert towards byte-based interfaces, so it's easier to just drop the unused functions: bdrv_dirty_bitmap_get_meta bdrv_dirty_bitmap_get_meta_locked bdrv_dirty_bitmap_reset_meta bdrv_dirty_bitmap_meta_granularity Signed-off-by: Eric Blake <eblake@redhat.com> --- v4: rebase to Vladimir's persistent bitmaps (bdrv_dirty_bitmap_size now in use), dropped R-b v3: rebase to upstream changes (bdrv_dirty_bitmap_get_meta_locked was added in b64bd51e with no clients), kept R-b v2: tweak commit message based on review, no code change --- include/block/dirty-bitmap.h | 10 ---------- block/dirty-bitmap.c | 44 -------------------------------------------- 2 files changed, 54 deletions(-)