Re: [Qemu-devel] [PATCH v14 19/19] docs: incremental backup documentation

[John Snow <jsnow@redhat.com>]

On 02/20/2015 06:55 PM, Eric Blake wrote:
> On 02/20/2015 04:07 PM, John Snow wrote:
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   docs/bitmaps.md | 253 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 253 insertions(+)
>>   create mode 100644 docs/bitmaps.md
>>
>> diff --git a/docs/bitmaps.md b/docs/bitmaps.md
>> new file mode 100644
>> index 0000000..7cda146
>> --- /dev/null
>> +++ b/docs/bitmaps.md
>> @@ -0,0 +1,253 @@
>> +# Dirty Bitmaps
>
> No copyright/license?  Might be good to add.
>

OK.

> Also, I'm a fan of reviewing docs before code (does the design make
> sense in isolation, and then did we implement it) rather than last in
> the series (we may have faithfully documented our code, but that
> includes baking in any design flaws that reviewers are now blind to
> because of reading the code)
>

Sorry, I definitely did write this *after* and I selfishly put it at the 
end of the series to avoid disrupting the patch numbers from the 
previous revision.

>> +* To create a new bitmap that tracks changes in 32KiB segments:
>> +
>> +```json
>> +{ "execute": "block-dirty-bitmap-add",
>> +  "arguments": {
>> +    "node": "drive0",
>> +    "name": "bitmap0",
>> +    "granularity": "32768"
>
> s/"32768"/32768/ (we are using a JSON integer, not string)
>
>> +  }
>> +}
>> +```
>> +
>> +### Deletion
>> +
>> +* Can be performed on a disabled bitmap, but not a frozen one.
>> +
>> +* Because bitmaps are only unique to the node to which they are attached,
>> +you must specify the node/drive name here, too.
>> +
>> +```json
>> +{ "execute": "block-dirty-bitmap-remove",
>> +  "arguments": {
>> +    "node": "drive0",
>> +    "name": "bitmap0",
>> +  }
>
> No trailing commas in JSON.
>
>> +}
>> +```
>> +
>> +### Enable/Disable:
>> +
>> +* Not very useful in current cases, but potentially useful for debugging in the
>> +future where we'd like to see what information changed only in a specific
>> +time period:
>> +
>> +* To enable (which is, again, the default state after add)
>> +
>> +```json
>> +{ "execute": "block-dirty-bitmap-enable",
>> +  "arguments": {
>> +    "node": "drive0",
>> +    "name": "bitmap0",
>> +  }
>
> and again.
>
>> +}
>> +```
>> +
>> +* To disable:
>> +
>> +```json
>> +{ "execute": "block-dirty-bitmap-disable",
>> +  "arguments": {
>> +    "node": "drive0",
>> +    "name": "bitmap0",
>> +  }
>> +}
>> +```
>
> and again. Also, maybe swap these two, since a bitmap defaults to
> enabled (that is, you would logically use disable first, then enable to
> re-enable, when testing these out, if there is no way to create an
> already-disabled bitmap).
>
>> +
>> +### Resetting
>> +
>> +* Resetting a bitmap will clear all information it holds.
>> +* An incremental backup created from an empty bitmap will copy no data,
>> +as if nothing has changed.
>> +
>> +```json
>> +{ "execute": "block-dirty-bitmap-clear",
>> +  "arguments": {
>> +    "node": "drive0",
>> +    "name": "bitmap0",
>> +  }
>
> I'll quit pointing out broken trailing JSON commas, on the assumption
> that you'll fix all of them rather than stopping here :)
>

Yeah, sorry! I have discovered today that I am *awful* at writing out 
json by hand. I promise the commands work otherwise!

I will fix up the documentation here; if this winds up being the *ONLY* 
thing wrong with V14, I would prefer this doc be dropped from this 
series and I will just send a v2 for the doc by itself afterwards.

>> +}
>> +```
>> +
>> +## Transactions
>> +
>> +### Justification
>> +Bitmaps can be safely modified when the VM is paused or halted by using
>> +the basic QMP commands. For instance, you might perform the following actions:
>> +
>> +1. Boot the VM in a paused state.
>> +2. Create a full drive backup of drive0
>> +3. Create a new bitmap attached to drive0
>> +4. resume execution of the VM
>> +5. Incremental backups are ready to be created.
>
> Consistency on using trailing '.'?
>
>> +
>> +At this point, the bitmap and drive backup would be correctly in sync,
>> +and incremental backups made from this point forward would be correctly aligned
>> +to the full drive backup.
>> +
>> +This is not particularly useful if we decide we want to start incremental
>> +backups after the VM has been running for a while, which will allow us to
>> +perform actions like the following:
>> +
>> +1. Boot the VM and begin execution
>> +2. Using transactions, perform the following operations:
>> +    * Create 'bitmap0'
>> +    * Create a full drive backup of drive0
>> +3. Incremental backups are now ready to be created.
>
> It's unclear whether point 2 is describing two separate transactions, or
> one transaction with two steps.  If it's just one, I'd word it:
>
> Using a single transaction, perform the following operations:
>
>

OK.

>> +
>> +The star of the show.
>> +
>> +**Nota Bene!** Only incremental backups of entire drives are supported for now.
>> +So despite the fact that you can attach a bitmap to any arbitrary node, they are
>> +only currently useful when attached to the root node. This is because
>> +drive-backup only supports drives/devices instead of arbitrary nodes.
>
> Maybe it's time to think about adding a node-backup :)  But doesn't have
> to be this series.
>
>

I will crawl before I walk. And certainly before I start running and 
flying :)

Thanks,
--JS