On Fri, 01/16 13:22, John Snow wrote:
>
>
> 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 <js...@redhat.com>
> >>---
> >> 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
OK.
I thought "better be safe than be sorry" when suggesting the stricter and more
explicit calling contract, but since this is not wrong, it's your decision.
Fam
