From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40327) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YCAP9-0006Fd-BI for qemu-devel@nongnu.org; Fri, 16 Jan 2015 12:09:52 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YCAP4-0008Lp-BY for qemu-devel@nongnu.org; Fri, 16 Jan 2015 12:09:51 -0500 Received: from mx1.redhat.com ([209.132.183.28]:57828) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YCAP4-0008Lf-2u for qemu-devel@nongnu.org; Fri, 16 Jan 2015 12:09:46 -0500 Message-ID: <54B945D4.2090603@redhat.com> Date: Fri, 16 Jan 2015 12:09:40 -0500 From: John Snow MIME-Version: 1.0 References: <1421080265-2228-1-git-send-email-jsnow@redhat.com> <1421080265-2228-8-git-send-email-jsnow@redhat.com> <54B93C15.6000804@redhat.com> In-Reply-To: <54B93C15.6000804@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: Max Reitz , qemu-devel@nongnu.org Cc: kwolf@redhat.com, vsementsov@parallels.com, famz@redhat.com, armbru@redhat.com, stefanha@redhat.com On 01/16/2015 11:28 AM, Max Reitz wrote: > 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 > Just because it's intentional doesn't make it a good idea. Fam is currently suggesting we might just scrap the "enable/disable" and "frozen/normal" modes altogether in favor of a single unified status, but the original way he proposed enabled/disabled is that it simply, and exclusively enabled/disabled new writes to the bitmap -- it never was intended to block clears/resets. So the bitmap is still able to be consumed while disabled, for instance. How useful is this? Perhaps not very useful. Probably more confusing than useful. I will *probably* try to unify these parameters as Fam suggested, but I am really eager to hear from Markus on the QMP interface side before I bother sending out another minor iteration. I'll skip the R-b for now, since Fam had some ideas on how to make it nicer. Thanks! --js