Re: [Qemu-block] [PATCH v5 09/21] block: Add bitmap successors

[John Snow]

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 <address@hidden>
> > Reviewed-by: Max Reitz <address@hidden>
> > Reviewed-by: Stefan Hajnoczi <address@hidden>
> > ---
> >   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 <address@hidden>