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 <js...@redhat.com>
Reviewed-by: Max Reitz <mre...@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com>
---
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 <ebl...@redhat.com>
