From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45868) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YLIOT-0002c4-4z for qemu-devel@nongnu.org; Tue, 10 Feb 2015 16:30:54 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YLIOP-0005vg-MK for qemu-devel@nongnu.org; Tue, 10 Feb 2015 16:30:52 -0500 Received: from mx1.redhat.com ([209.132.183.28]:43565) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YLIOO-0005un-VA for qemu-devel@nongnu.org; Tue, 10 Feb 2015 16:30:49 -0500 Message-ID: <54DA7886.8060705@redhat.com> Date: Tue, 10 Feb 2015 16:30:46 -0500 From: John Snow MIME-Version: 1.0 References: <1422356197-5285-1-git-send-email-vsementsov@parallels.com> <1422356197-5285-5-git-send-email-vsementsov@parallels.com> In-Reply-To: <1422356197-5285-5-git-send-email-vsementsov@parallels.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: Vladimir Sementsov-Ogievskiy , qemu-devel@nongnu.org Cc: kwolf@redhat.com, pbonzini@redhat.com, stefanha@redhat.com, den@openvz.org 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. > 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.