From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36807) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YLaUK-0001xZ-P1 for qemu-devel@nongnu.org; Wed, 11 Feb 2015 11:50:14 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YLaUH-0003GZ-H0 for qemu-devel@nongnu.org; Wed, 11 Feb 2015 11:50:08 -0500 Received: from mx1.redhat.com ([209.132.183.28]:59898) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YLaUH-0003FX-AF for qemu-devel@nongnu.org; Wed, 11 Feb 2015 11:50:05 -0500 Message-ID: <54DB8839.3060901@redhat.com> Date: Wed, 11 Feb 2015 11:50:01 -0500 From: Max Reitz MIME-Version: 1.0 References: <1423532117-14490-1-git-send-email-jsnow@redhat.com> <1423532117-14490-7-git-send-email-jsnow@redhat.com> In-Reply-To: <1423532117-14490-7-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 v12 06/17] block: Add bitmap successors 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-02-09 at 20:35, 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. > > BdrvDirtyBitmap operations that target a single bitmap are protected > by assertions that the bitmap is not frozen and/or disabled. > > BdrvDirtyBitmap operations that target a group of bitmaps, such as > bdrv_{set,reset}_dirty will ignore frozen/disabled drives with a > conditional instead. > > QMP transactions that enable/disable bitmaps have extra error checking > surrounding them that prevent modifying bitmaps that are frozen. > > Signed-off-by: John Snow > --- > block.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++- > blockdev.c | 22 +++++++++++ > include/block/block.h | 10 +++++ > qapi/block-core.json | 3 ++ > 4 files changed, 138 insertions(+), 1 deletion(-) > > diff --git a/block.c b/block.c > index 8d63375..8d84ace 100644 > --- a/block.c > +++ b/block.c > @@ -51,8 +51,17 @@ > #include > #endif > > +/** > + * A BdrvDirtyBitmap can be in three possible states: > + * (1) successor is false and disabled is false: full r/w mode > + * (2) successor is false and disabled is true: read only mode ("disabled") > + * (3) successor is set: frozen mode. > + * A frozen bitmap cannot be renamed, deleted, anonymized, cleared, set, > + * or enabled. A frozen bitmap can only abdicate() or reclaim(). > + */ > struct BdrvDirtyBitmap { > HBitmap *bitmap; > + BdrvDirtyBitmap *successor; > char *name; > bool disabled; > QLIST_ENTRY(BdrvDirtyBitmap) list; > @@ -5383,6 +5392,7 @@ BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, const char *name) > > void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap) > { > + assert(!bdrv_dirty_bitmap_frozen(bitmap)); > g_free(bitmap->name); > bitmap->name = NULL; > } > @@ -5418,9 +5428,98 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, > return bitmap; > } > > +bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap) > +{ > + return bitmap->successor; > +} > + > bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap) > { > - return !bitmap->disabled; > + return !(bitmap->disabled || bitmap->successor); > +} > + > +/** > + * 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; > + BdrvDirtyBitmap *child; > + > + if (bdrv_dirty_bitmap_frozen(bitmap)) { > + error_setg(errp, "Cannot create a successor for a bitmap that is " > + "currently frozen."); Hm, I seem to remember Markus saying something about full stops at the end of error messages... > + return -1; > + } > + assert(!bitmap->successor); > + > + /* Create an anonymous successor */ > + granularity = bdrv_dirty_bitmap_granularity(bitmap); > + child = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp); > + if (!child) { > + return -1; > + } > + > + /* Successor will be on or off based on our current state. */ > + child->disabled = bitmap->disabled; > + > + /* Install the successor and freeze the parent */ > + bitmap->successor = child; > + 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"); So while I'm unsure about full stops, I am pretty sure that we don't really want newlines. With the full stops and newlines removed (in all error_setg() calls in this patch): Reviewed-by: Max Reitz