From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53213) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YCBXC-0002go-I5 for qemu-devel@nongnu.org; Fri, 16 Jan 2015 13:22:16 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YCBX7-0006V3-Ga for qemu-devel@nongnu.org; Fri, 16 Jan 2015 13:22:14 -0500 Received: from mx1.redhat.com ([209.132.183.28]:58278) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YCBX7-0006Un-6c for qemu-devel@nongnu.org; Fri, 16 Jan 2015 13:22:09 -0500 Message-ID: <54B956CC.40900@redhat.com> Date: Fri, 16 Jan 2015 13:22:04 -0500 From: John Snow MIME-Version: 1.0 References: <1421080265-2228-1-git-send-email-jsnow@redhat.com> <1421080265-2228-9-git-send-email-jsnow@redhat.com> <20150113092409.GC31703@fam-t430.nay.redhat.com> In-Reply-To: <20150113092409.GC31703@fam-t430.nay.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v11 08/13] block: Add bitmap successors List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: kwolf@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com, mreitz@redhat.com, vsementsov@parallels.com, stefanha@redhat.com, pbonzini@redhat.com On 01/13/2015 04:24 AM, Fam Zheng wrote: > On Mon, 01/12 11:31, John Snow wrote: >> A bitmap successor is an anonymous BdrvDirtyBitmap that is intended to >> be created just prior to a sensitive operation (e.g. Incremental Backup) >> that can either succeed or fail, but during the course of which we still >> want a bitmap tracking writes. >> >> On creating a successor, we "freeze" the parent bitmap which prevents >> its deletion, enabling, anonymization, or creating a bitmap with the >> same name. >> >> On success, the parent bitmap can "abdicate" responsibility to the >> successor, which will inherit its name. The successor will have been >> tracking writes during the course of the backup operation. The parent >> will be safely deleted. >> >> On failure, we can "reclaim" the successor from the parent, unifying >> them such that the resulting bitmap describes all writes occurring since >> the last successful backup, for instance. Reclamation will thaw the >> parent, but not explicitly re-enable it. > > If we compare this with image snapshot and overlay, it fits the current backing > chain model very well. Given that we're on the way to persistent dirty bitmap, > which will probably be read/written with general block.c code, I'm wondering if > it will be any better to reuse the block layer backing file code to handle > "successor" logic? > > Also with the frozen feature built in dirty bitmaps, is it okay to drop > "enabled" state? > > I think there are three states for a bitmap: > > 1) Active, no successor (similar to an read-write top image) > 2) Frozen, no successor (similar to an read-only top image) > 3) Frozen, with successor (similar to an read-only backing file, with an > overlap) > > Admittedly, more code is needed than this patch, in order to glue hbitmap and > block layer together, but it would probably make live migration of dirty bitmap > very easy, but I'm not sure without a closer look. > >> >> Signed-off-by: John Snow >> --- >> block.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++-- >> blockdev.c | 14 ++++++ >> include/block/block.h | 10 ++++ >> 3 files changed, 144 insertions(+), 3 deletions(-) >> >> diff --git a/block.c b/block.c >> index bd4b449..3f33b9d 100644 >> --- a/block.c >> +++ b/block.c >> @@ -53,10 +53,12 @@ >> >> struct BdrvDirtyBitmap { >> HBitmap *bitmap; >> + BdrvDirtyBitmap *successor; >> int64_t size; >> int64_t granularity; >> char *name; >> bool enabled; >> + bool frozen; >> QLIST_ENTRY(BdrvDirtyBitmap) list; >> }; >> >> @@ -5342,6 +5344,7 @@ BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, const char *name) >> >> void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap) >> { >> + assert(!bitmap->frozen); >> g_free(bitmap->name); >> bitmap->name = NULL; >> } >> @@ -5379,9 +5382,114 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, >> return bitmap; >> } >> >> +/** >> + * A frozen bitmap cannot be renamed, deleted, cleared, >> + * or set. A frozen bitmap can only abdicate, reclaim, >> + * or thaw. >> + */ >> +static void bdrv_freeze_dirty_bitmap(BdrvDirtyBitmap *bitmap) >> +{ >> + bitmap->frozen = true; >> +} >> + >> +static void bdrv_thaw_dirty_bitmap(BdrvDirtyBitmap *bitmap) >> +{ >> + bitmap->frozen = false; >> +} >> + >> +bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap) >> +{ >> + return bitmap->frozen; >> +} >> + >> bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap) >> { >> - return bitmap->enabled; >> + return bitmap->enabled && !bitmap->frozen; > > An indication that "enabled" could be replaced by "frozen". Otherwise it looks > confusing to me. > >> +} >> + >> +/** >> + * Create a successor bitmap destined to replace this bitmap after an operation. >> + * Requires that the bitmap is not frozen and has no successor. >> + */ >> +int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, >> + BdrvDirtyBitmap *bitmap, Error **errp) >> +{ >> + uint64_t granularity; >> + >> + if (bdrv_dirty_bitmap_frozen(bitmap)) { >> + error_setg(errp, >> + "Cannot create a successor for a bitmap currently in-use."); >> + return -1; >> + } else if (bitmap->successor) { >> + error_setg(errp, "Cannot create a successor for a bitmap that " >> + "already has one."); >> + return -1; >> + } >> + >> + /* Create an anonymous successor */ >> + granularity = bdrv_dirty_bitmap_granularity(bs, bitmap); >> + bitmap->successor = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp); >> + if (!bitmap->successor) { >> + return -1; >> + } >> + >> + /* Successor will be on or off based on our current state. >> + * Parent will be disabled and frozen. */ >> + bitmap->successor->enabled = bitmap->enabled; >> + bdrv_disable_dirty_bitmap(bitmap); >> + bdrv_freeze_dirty_bitmap(bitmap); >> + return 0; >> +} >> + >> +/** >> + * For a bitmap with a successor, yield our name to the successor, >> + * Delete the old bitmap, and return a handle to the new bitmap. >> + */ >> +BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, >> + BdrvDirtyBitmap *bitmap, >> + Error **errp) >> +{ >> + char *name; >> + BdrvDirtyBitmap *successor = bitmap->successor; >> + >> + if (successor == NULL) { >> + error_setg(errp, "Cannot relinquish control if " >> + "there's no successor present.\n"); >> + return NULL; >> + } >> + >> + name = bitmap->name; >> + bitmap->name = NULL; >> + successor->name = name; >> + >> + bdrv_thaw_dirty_bitmap(bitmap); >> + bdrv_release_dirty_bitmap(bs, bitmap); >> + >> + return successor; >> +} >> + >> +/** >> + * In cases of failure where we can no longer safely delete the parent, >> + * We may wish to re-join the parent and child/successor. >> + * The merged parent will be un-frozen, but not explicitly re-enabled. >> + */ >> +BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, >> + BdrvDirtyBitmap *parent, >> + Error **errp) >> +{ >> + BdrvDirtyBitmap *successor = parent->successor; >> + if (!successor) { >> + error_setg(errp, "Cannot reclaim a successor when none is present.\n"); >> + return NULL; >> + } >> + >> + hbitmap_merge(parent->bitmap, successor->bitmap); >> + bdrv_release_dirty_bitmap(bs, successor); >> + >> + bdrv_thaw_dirty_bitmap(parent); >> + parent->successor = NULL; >> + >> + return parent; >> } >> >> void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap) >> @@ -5389,6 +5497,7 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap) >> BdrvDirtyBitmap *bm, *next; >> QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) { >> if (bm == bitmap) { >> + assert(!bm->frozen); >> QLIST_REMOVE(bitmap, list); >> hbitmap_free(bitmap->bitmap); >> g_free(bitmap->name); >> @@ -5405,6 +5514,7 @@ void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap) >> >> void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap) >> { >> + assert(!bitmap->frozen); >> bitmap->enabled = true; >> } >> >> @@ -5483,7 +5593,9 @@ void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, >> 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->frozen) { > > Probably just assert it's not frozen? > >> + hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors); >> + } >> } >> >> /** >> @@ -5492,7 +5604,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->frozen) { >> + hbitmap_reset(bitmap->bitmap, 0, bitmap->size); > > The same: probably just assert it's not frozen? > Looking at this again, I see what I was trying to accomplish. Similar to how (!enabled) will silently ignore writes, I just wanted frozen bitmaps to silently ignore attempts to clear or reset bits, not assert or error out. Just in case, in some future state, there is a more generic "set/clear bits" mechanism that may blindly try to clear bits to all attached bitmaps of a particular BDS, I didn't want to throw an error in that case -- just ignore it. It would be a logical error to try to clear a bitmap we KNOW is frozen, but there may be cases where clears and resets may be done with less discrimination. I think I will leave this as-is, but I can write some extra commentary for it. Is that OK? --JS >> + } >> } >> >> static void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, >> @@ -5512,6 +5626,9 @@ static void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, >> { >> BdrvDirtyBitmap *bitmap; >> QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) { >> + if (bitmap->frozen) { >> + continue; >> + } >> hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors); >> } >> } >> diff --git a/blockdev.c b/blockdev.c >> index 118cb6c..8dde72a 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -1988,9 +1988,16 @@ void qmp_block_dirty_bitmap_remove(const char *node_ref, const char *name, >> aio_context = bdrv_get_aio_context(bs); >> aio_context_acquire(aio_context); >> >> + if (bdrv_dirty_bitmap_frozen(bitmap)) { >> + error_setg(errp, >> + "Bitmap %s is currently frozen and cannot be removed yet.\n", >> + name); >> + goto out; >> + } >> bdrv_dirty_bitmap_make_anon(bs, bitmap); >> bdrv_release_dirty_bitmap(bs, bitmap); >> >> + out: >> aio_context_release(aio_context); >> } >> >> @@ -2009,8 +2016,15 @@ void qmp_block_dirty_bitmap_enable(const char *node_ref, const char *name, >> aio_context = bdrv_get_aio_context(bs); >> aio_context_acquire(aio_context); >> >> + if (bdrv_dirty_bitmap_frozen(bitmap)) { >> + error_setg(errp, "Bitmap %s is frozen and cannot be enabled yet.\n", >> + name); >> + goto out; >> + } >> + >> bdrv_enable_dirty_bitmap(bitmap); >> >> + out: >> aio_context_release(aio_context); >> } >> >> diff --git a/include/block/block.h b/include/block/block.h >> index a5bc249..6138544 100644 >> --- a/include/block/block.h >> +++ b/include/block/block.h >> @@ -433,6 +433,15 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, >> int granularity, >> const char *name, >> Error **errp); >> +int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, >> + BdrvDirtyBitmap *bitmap, >> + Error **errp); >> +BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, >> + BdrvDirtyBitmap *bitmap, >> + Error **errp); >> +BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, >> + BdrvDirtyBitmap *bitmap, >> + Error **errp); >> BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, >> const char *name); >> void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap); >> @@ -444,6 +453,7 @@ uint64_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs); >> uint64_t bdrv_dirty_bitmap_granularity(BlockDriverState *bs, >> BdrvDirtyBitmap *bitmap); >> bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap); >> +bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap); >> int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector); >> void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, >> int64_t cur_sector, int nr_sectors); >> -- >> 1.9.3 >> >