Re: [Qemu-devel] [PATCH 2/3] blockdev: n-ary bitmap merge

[John Snow]

On 12/7/18 11:25 AM, Eric Blake wrote:
> On 12/6/18 1:25 PM, John Snow wrote:
>> Especially outside of transactions, it is helpful to provide
>> all-or-nothing semantics for bitmap merges. This facilitates
>> the coalescing of multiple bitmaps into a single target for
>> the "checkpoint" interpretation when assembling bitmaps that
>> represent arbitrary points in time from component bitmaps.
>>
>> Signed-off-by: John Snow <address@hidden>
>> ---
>>   blockdev.c           | 64 +++++++++++++++++++++++++++++---------------
>>   qapi/block-core.json | 22 +++++++--------
>>   2 files changed, 53 insertions(+), 33 deletions(-)
>>
> 
>> +++ b/qapi/block-core.json
>> @@ -1818,14 +1818,14 @@
>>   #
>>   # @node: name of device/node which the bitmap is tracking
>>   #
>> -# @dst_name: name of the destination dirty bitmap
>> +# @target: name of the destination dirty bitmap
>>   #
>> -# @src_name: name of the source dirty bitmap
>> +# @bitmaps: name(s) of the source dirty bitmap(s)
>>   #
>>   # Since: 3.0
>>   ##
>>   { 'struct': 'BlockDirtyBitmapMerge',
>> -  'data': { 'node': 'str', 'dst_name': 'str', 'src_name': 'str' } }
>> +  'data': { 'node': 'str', 'target': 'str', 'bitmaps': ['str'] } }
> 
> Definitely worthwhile!
> 
> I'll update my pending libvirt patches to use this.
> 
>>     ##
>>   # @block-dirty-bitmap-add:
>> @@ -1940,23 +1940,23 @@
>>   ##
>>   # @x-block-dirty-bitmap-merge:
>>   #
>> -# FIXME: Rename @src_name and @dst_name to src-name and dst-name.
>> -#
>> -# Merge @src_name dirty bitmap to @dst_name dirty bitmap. @src_name
>> dirty
>> -# bitmap is unchanged. On error, @dst_name is unchanged.
>> +# Merge dirty bitmaps listed in @bitmaps to the @target dirty bitmap.
>> +# The @bitmaps dirty bitmaps are unchanged.
> 
> Well, except in the corner case of when @bitmaps also lists the
> destination (I presume that merging a bitmap into itself silently
> succeeds with no further changes, but therefore the inclusion of the
> destination in the list of sources means that that particular source is
> changing due to merging in the other sources).  Not worth rewording this
>  sentence, but does make you want to consider ensuring that the
> testsuite covers merging a bitmap into itself.
> 

OK, noted.

> Reviewed-by: Eric Blake <address@hidden>
> 
Thanks!