From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59814) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YC9ks-0003wm-Bq for qemu-devel@nongnu.org; Fri, 16 Jan 2015 11:28:15 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YC9ko-0000iI-6l for qemu-devel@nongnu.org; Fri, 16 Jan 2015 11:28:14 -0500 Received: from mx1.redhat.com ([209.132.183.28]:39917) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YC9kn-0000gX-UQ for qemu-devel@nongnu.org; Fri, 16 Jan 2015 11:28:10 -0500 Message-ID: <54B93C15.6000804@redhat.com> Date: Fri, 16 Jan 2015 11:28:05 -0500 From: Max Reitz MIME-Version: 1.0 References: <1421080265-2228-1-git-send-email-jsnow@redhat.com> <1421080265-2228-8-git-send-email-jsnow@redhat.com> In-Reply-To: <1421080265-2228-8-git-send-email-jsnow@redhat.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v11 07/13] qmp: Add block-dirty-bitmap-enable and block-dirty-bitmap-disable 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, vsementsov@parallels.com, stefanha@redhat.com On 2015-01-12 at 11:30, John Snow wrote: > From: Fam Zheng > > This allows to put the dirty bitmap into a disabled state where no more > writes will be tracked. > > It will be used before backup or writing to persistent file. > > Signed-off-by: Fam Zheng > Signed-off-by: John Snow > --- > block.c | 24 +++++++++++++++++++++++- > blockdev.c | 40 ++++++++++++++++++++++++++++++++++++++++ > include/block/block.h | 3 +++ > qapi/block-core.json | 28 ++++++++++++++++++++++++++++ > qmp-commands.hx | 10 ++++++++++ > 5 files changed, 104 insertions(+), 1 deletion(-) > > diff --git a/block.c b/block.c > index 7bf7079..bd4b449 100644 > --- a/block.c > +++ b/block.c > @@ -56,6 +56,7 @@ struct BdrvDirtyBitmap { > int64_t size; > int64_t granularity; > char *name; > + bool enabled; > QLIST_ENTRY(BdrvDirtyBitmap) list; > }; > > @@ -5373,10 +5374,16 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, > bitmap->granularity = granularity; > bitmap->bitmap = hbitmap_alloc(bitmap->size, ffs(sector_granularity) - 1); > bitmap->name = g_strdup(name); > + bitmap->enabled = true; > QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list); > return bitmap; > } > > +bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap) > +{ > + return bitmap->enabled; > +} > + > void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap) > { > BdrvDirtyBitmap *bm, *next; > @@ -5391,6 +5398,16 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap) > } > } > > +void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap) > +{ > + bitmap->enabled = false; > +} > + > +void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap) > +{ > + bitmap->enabled = true; > +} > + > BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs) > { > BdrvDirtyBitmap *bm; > @@ -5458,7 +5475,9 @@ void bdrv_dirty_iter_init(BlockDriverState *bs, > void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, > int64_t cur_sector, int nr_sectors) > { > - hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors); > + if (bdrv_dirty_bitmap_enabled(bitmap)) { > + hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors); > + } Why are you checking whether the bitmap is enabled here in bdrv_set_dirty_bitmap(), but neither in bdrv_reset_dirty_bitmap() nor bdrv_clear_dirty_bitmap()? It seems consistent with the commit message which only states that disabled state means that no more writes (i.e. bdrv_set_dirty_bitmap()) will be tracked, but it still seems strange to me. > } > > void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, > @@ -5481,6 +5500,9 @@ static void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, > { > BdrvDirtyBitmap *bitmap; > QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) { > + if (!bdrv_dirty_bitmap_enabled(bitmap)) { > + continue; > + } Same question as above, only this time it's about bdrv_reset_dirty(). In case the answer to the question is "that's intentional": Reviewed-by: Max Reitz