|
| From: | John Snow |
| Subject: | Re: [Qemu-devel] [PATCH v11 08/13] block: Add bitmap successors |
| Date: | Tue, 13 Jan 2015 12:26:26 -0500 |
| User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0 |
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.
Hmm -- I suppose so. I was just treating "frozen" as a stricter "disabled."Disabled just stops tracking writes, but frozen was intended to stop deletion, renaming, etc. If there is no use case for the softer "disabled," then it can really be just a single parameter.
Signed-off-by: John Snow <address@hidden> --- 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.
Perhaps. Here I mean to say that "Frozen implies disabled." The stronger condition implies the weaker.
+} + +/** + * 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?
If the only callers are other BdrvDirtyBitmap related functions, then OK. I want to avoid assertions by functions invoked by other layers, because a user could freeze/disable a bitmap at any time.
+ 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?
As above, OK.
+ } } 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
Thanks! --John
| [Prev in Thread] | Current Thread | [Next in Thread] |