From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43138) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y9mgK-0007MK-0D for qemu-devel@nongnu.org; Fri, 09 Jan 2015 22:25:45 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y9mgG-0001Nu-Qo for qemu-devel@nongnu.org; Fri, 09 Jan 2015 22:25:43 -0500 Received: from mx1.redhat.com ([209.132.183.28]:53851) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y9mgG-0001Nl-J6 for qemu-devel@nongnu.org; Fri, 09 Jan 2015 22:25:40 -0500 Message-ID: <54B09BAF.9040909@redhat.com> Date: Fri, 09 Jan 2015 22:25:35 -0500 From: John Snow 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> In-Reply-To: <54A2ACF7.2020302@parallels.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable 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: Vladimir Sementsov-Ogievskiy , qemu-devel@nongnu.org Cc: kwolf@redhat.com, famz@redhat.com, armbru@redhat.com, stefanha@redhat.com, mreitz@redhat.com 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 t= o > have "size" in sectors and "granularity" in bytes in one structure. I'v= e > 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=20 represents the "size" of the bitmap, as in the number of logical bits=20 for the interface of the bitmap. Since it is a sector bitmap, I think=20 this should remain in sectors, for now. I really want to keep the granularity in bytes in this case, because I=20 want to match the existing interface, and size makes sense to me in secto= rs. 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 =3D NULL; >> } >> +BdrvDirtyBitmap *bdrv_copy_dirty_bitmap(BlockDriverState *bs, >> + BdrvDirtyBitmap *bitmap, >> + const char *name) >> +{ >> + BdrvDirtyBitmap *new_bitmap; >> + >> + new_bitmap =3D g_malloc0(sizeof(BdrvDirtyBitmap)); >> + new_bitmap->bitmap =3D hbitmap_copy(bitmap->bitmap); >> + new_bitmap->size =3D bitmap->size; >> + new_bitmap->granularity =3D bitmap->granularity; >> + new_bitmap->name =3D 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)) =3D=3D 0); >> @@ -5357,8 +5375,8 @@ BdrvDirtyBitmap >> *bdrv_create_dirty_bitmap(BlockDriverState *bs, >> error_setg(errp, "Bitmap already exists: %s", name); >> return NULL; >> } >> - granularity >>=3D BDRV_SECTOR_BITS; >> - assert(granularity); >> + sector_granularity =3D granularity >> BDRV_SECTOR_BITS; >> + assert(sector_granularity); >> bitmap_size =3D 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 =3D g_new0(BdrvDirtyBitmap, 1); >> - bitmap->bitmap =3D hbitmap_alloc(bitmap_size, ffs(granularity) - = 1); >> + bitmap->size =3D bitmap_size; >> + bitmap->granularity =3D granularity; >> + bitmap->bitmap =3D hbitmap_alloc(bitmap->size, >> ffs(sector_granularity) - 1); >> bitmap->name =3D 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) = =3D=3D >> + 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->siz= e) >> + */ >> +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); > > --=20 =E2=80=94js