From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53657) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YLrYF-0000ci-Dk for qemu-devel@nongnu.org; Thu, 12 Feb 2015 06:03:25 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YLrYC-0001b6-6W for qemu-devel@nongnu.org; Thu, 12 Feb 2015 06:03:19 -0500 Received: from mx2.parallels.com ([199.115.105.18]:50973) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YLrYB-0001Jo-U5 for qemu-devel@nongnu.org; Thu, 12 Feb 2015 06:03:16 -0500 Message-ID: <54DC85C1.3040205@parallels.com> Date: Thu, 12 Feb 2015 13:51:45 +0300 From: Vladimir Sementsov-Ogievskiy MIME-Version: 1.0 References: <1422356197-5285-1-git-send-email-vsementsov@parallels.com> <1422356197-5285-5-git-send-email-vsementsov@parallels.com> <54DA7886.8060705@redhat.com> In-Reply-To: <54DA7886.8060705@redhat.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH RFC v2 4/8] block: add dirty-dirty bitmaps List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow , qemu-devel@nongnu.org Cc: kwolf@redhat.com, pbonzini@redhat.com, stefanha@redhat.com, den@openvz.org 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 >> --- >> 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. -- Best regards, Vladimir