From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51705) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YAyBy-0005I0-N5 for qemu-devel@nongnu.org; Tue, 13 Jan 2015 04:55:20 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YAyBv-0007Hv-Fd for qemu-devel@nongnu.org; Tue, 13 Jan 2015 04:55:18 -0500 Received: from mx2.parallels.com ([199.115.105.18]:37272) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YAyBv-0007Hp-6g for qemu-devel@nongnu.org; Tue, 13 Jan 2015 04:55:15 -0500 Message-ID: <54B4EB71.7000807@parallels.com> Date: Tue, 13 Jan 2015 12:54:57 +0300 From: Vladimir Sementsov-Ogievskiy MIME-Version: 1.0 References: <1419297142-24282-1-git-send-email-jsnow@redhat.com> <1419297142-24282-7-git-send-email-jsnow@redhat.com> <54A2ACF7.2020302@parallels.com> <54B09BAF.9040909@redhat.com> In-Reply-To: <54B09BAF.9040909@redhat.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v10 06/13] block: Add bdrv_copy_dirty_bitmap and bdrv_clear_dirty_bitmap List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow , qemu-devel@nongnu.org Cc: kwolf@redhat.com, famz@redhat.com, armbru@redhat.com, stefanha@redhat.com, mreitz@redhat.com Hmm, can't find it in v11 Best regards, Vladimir On 10.01.2015 06:25, John Snow wrote: > > > On 12/30/2014 08:47 AM, Vladimir Sementsov-Ogievskiy wrote: >> I'm sorry if it was already discussed, but I think it is inconsistent to >> have "size" in sectors and "granularity" in bytes in one structure. I've >> misused these fields because of this in my current work. >> >> At least, I think there should be comments about this. >> >> Best regards, >> Vladimir > > Looking at it now, I think it'd be worse to change it, because it > represents the "size" of the bitmap, as in the number of logical bits > for the interface of the bitmap. Since it is a sector bitmap, I think > this should remain in sectors, for now. > > I really want to keep the granularity in bytes in this case, because I > want to match the existing interface, and size makes sense to me in > sectors. > > What I will do instead is just to document this quirk. Look out for > V11 :) > > --John > >> >> On 23.12.2014 04:12, John Snow wrote: >>> Signed-off-by: Fam Zheng >>> Signed-off-by: John Snow >>> --- >>> block.c | 39 +++++++++++++++++++++++++++++++++++---- >>> include/block/block.h | 4 ++++ >>> 2 files changed, 39 insertions(+), 4 deletions(-) >>> >>> diff --git a/block.c b/block.c >>> index a1d9e88..f9e0767 100644 >>> --- a/block.c >>> +++ b/block.c >>> @@ -53,6 +53,8 @@ >>> struct BdrvDirtyBitmap { >>> HBitmap *bitmap; >>> + int64_t size; >>> + int64_t granularity; >>> char *name; >>> QLIST_ENTRY(BdrvDirtyBitmap) list; >>> }; >>> @@ -5343,6 +5345,21 @@ void >>> bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap >>> *bitmap) >>> bitmap->name = NULL; >>> } >>> +BdrvDirtyBitmap *bdrv_copy_dirty_bitmap(BlockDriverState *bs, >>> + BdrvDirtyBitmap *bitmap, >>> + const char *name) >>> +{ >>> + BdrvDirtyBitmap *new_bitmap; >>> + >>> + new_bitmap = g_malloc0(sizeof(BdrvDirtyBitmap)); >>> + new_bitmap->bitmap = hbitmap_copy(bitmap->bitmap); >>> + new_bitmap->size = bitmap->size; >>> + new_bitmap->granularity = bitmap->granularity; >>> + new_bitmap->name = g_strdup(name); >>> + QLIST_INSERT_HEAD(&bs->dirty_bitmaps, new_bitmap, list); >>> + return new_bitmap; >>> +} >>> + >>> BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, >>> int granularity, >>> const char *name, >>> @@ -5350,6 +5367,7 @@ BdrvDirtyBitmap >>> *bdrv_create_dirty_bitmap(BlockDriverState *bs, >>> { >>> int64_t bitmap_size; >>> BdrvDirtyBitmap *bitmap; >>> + int sector_granularity; >>> assert((granularity & (granularity - 1)) == 0); >>> @@ -5357,8 +5375,8 @@ BdrvDirtyBitmap >>> *bdrv_create_dirty_bitmap(BlockDriverState *bs, >>> error_setg(errp, "Bitmap already exists: %s", name); >>> return NULL; >>> } >>> - granularity >>= BDRV_SECTOR_BITS; >>> - assert(granularity); >>> + sector_granularity = granularity >> BDRV_SECTOR_BITS; >>> + assert(sector_granularity); >>> bitmap_size = bdrv_nb_sectors(bs); >>> if (bitmap_size < 0) { >>> error_setg_errno(errp, -bitmap_size, "could not get length >>> of device"); >>> @@ -5366,7 +5384,9 @@ BdrvDirtyBitmap >>> *bdrv_create_dirty_bitmap(BlockDriverState *bs, >>> return NULL; >>> } >>> bitmap = g_new0(BdrvDirtyBitmap, 1); >>> - bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1); >>> + bitmap->size = bitmap_size; >>> + bitmap->granularity = granularity; >>> + bitmap->bitmap = hbitmap_alloc(bitmap->size, >>> ffs(sector_granularity) - 1); >>> bitmap->name = g_strdup(name); >>> QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list); >>> return bitmap; >>> @@ -5439,7 +5459,9 @@ uint64_t >>> bdrv_get_default_bitmap_granularity(BlockDriverState *bs) >>> uint64_t bdrv_dirty_bitmap_granularity(BlockDriverState *bs, >>> BdrvDirtyBitmap *bitmap) >>> { >>> - return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap); >>> + g_assert(BDRV_SECTOR_SIZE << >>> hbitmap_granularity(bitmap->bitmap) == >>> + bitmap->granularity); >>> + return bitmap->granularity; >>> } >>> void bdrv_dirty_iter_init(BlockDriverState *bs, >>> @@ -5460,6 +5482,15 @@ void bdrv_reset_dirty_bitmap(BlockDriverState >>> *bs, BdrvDirtyBitmap *bitmap, >>> hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors); >>> } >>> +/** >>> + * Effectively, reset the hbitmap from bits [0, size) >>> + * Synonymous with bdrv_reset_dirty_bitmap(bs, bitmap, 0, >>> bitmap->size) >>> + */ >>> +void bdrv_clear_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap >>> *bitmap) >>> +{ >>> + hbitmap_reset(bitmap->bitmap, 0, bitmap->size); >>> +} >>> + >>> static void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, >>> int nr_sectors) >>> { >>> diff --git a/include/block/block.h b/include/block/block.h >>> index c7402e7..e964abd 100644 >>> --- a/include/block/block.h >>> +++ b/include/block/block.h >>> @@ -436,6 +436,9 @@ BdrvDirtyBitmap >>> *bdrv_create_dirty_bitmap(BlockDriverState *bs, >>> BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, >>> const char *name); >>> void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, >>> BdrvDirtyBitmap *bitmap); >>> +BdrvDirtyBitmap *bdrv_copy_dirty_bitmap(BlockDriverState *bs, >>> + BdrvDirtyBitmap *bitmap, >>> + const char *name); >>> void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap >>> *bitmap); >>> BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs); >>> uint64_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs); >>> @@ -446,6 +449,7 @@ void bdrv_set_dirty_bitmap(BlockDriverState *bs, >>> BdrvDirtyBitmap *bitmap, >>> int64_t cur_sector, int nr_sectors); >>> void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap >>> *bitmap, >>> int64_t cur_sector, int nr_sectors); >>> +void bdrv_clear_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap >>> *bitmap); >>> void bdrv_dirty_iter_init(BlockDriverState *bs, >>> BdrvDirtyBitmap *bitmap, struct >>> HBitmapIter *hbi); >>> int64_t bdrv_get_dirty_count(BlockDriverState *bs, BdrvDirtyBitmap >>> *bitmap); >> >> >