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

[John Snow <jsnow@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 <jsnow@redhat.com>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>> Reviewed-by: Stefan Hajnoczi <stefanha@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 <eblake@redhat.com>
>