From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50944) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YjFBt-0000Oj-0t for qemu-devel@nongnu.org; Fri, 17 Apr 2015 18:56:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YjFBp-00081S-Pk for qemu-devel@nongnu.org; Fri, 17 Apr 2015 18:56:52 -0400 Message-ID: <55318FAF.2070103@redhat.com> Date: Fri, 17 Apr 2015 18:56:47 -0400 From: John Snow MIME-Version: 1.0 References: <1428531604-9428-1-git-send-email-jsnow@redhat.com> <1428531604-9428-10-git-send-email-jsnow@redhat.com> <55318C85.7000909@redhat.com> In-Reply-To: <55318C85.7000909@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5 09/21] block: Add bitmap successors List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-block@nongnu.org Cc: kwolf@redhat.com, famz@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com, vsementsov@parallels.com, stefanha@redhat.com, mreitz@redhat.com On 04/17/2015 06:43 PM, Eric Blake wrote: > On 04/08/2015 04:19 PM, 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. > > Is this last paragraph stale now? > Yes, thank you. Those transactions no longer exist. The implementations, however, have *assertions* that protect frozen bitmaps and still exist. Bitmaps can still be enabled/disabled as an internal concept, and it is still invalid to try to enable/disable one that is frozen. >> >> Signed-off-by: John Snow >> Reviewed-by: Max Reitz >> Reviewed-by: Stefan Hajnoczi >> --- >> block.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++- >> blockdev.c | 7 ++++ >> include/block/block.h | 10 +++++ >> qapi/block-core.json | 1 + >> 4 files changed, 121 insertions(+), 1 deletion(-) >> > >> +/** >> + * 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; > > 'successor is false' reads awkwardly given that it is not a bool; maybe > 'successor is NULL' is better? > That is better. >> >> +bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap) >> +{ >> + return bitmap->successor; > > Good thing C99 requires conversion of pointer to bool to work :) > Is there a case to be made for explicit NULL checks? > >> + >> +/** >> + * For a bitmap with a successor, yield our name to the successor, >> + * Delete the old bitmap, and return a handle to the new bitmap. > > Sentences with capitals after comma, Even with a line break, Look weird. > (s/Delete/delete/) > You Are Right, Sorry About That. > >> + >> +/** >> + * In cases of failure where we can no longer safely delete the parent, >> + * We may wish to re-join the parent and child/successor. > > and again (s/We/we/) > It is an /extra/ royal 'we.' > Grammar fixes are minor, so: > Reviewed-by: Eric Blake >