Re: [Qemu-block] [PATCH 02/12] block/backup: Add mirror sync mode 'bitmap'

[Max Reitz]

On 20.06.19 18:01, John Snow wrote:
> 
> 
> On 6/20/19 11:00 AM, Max Reitz wrote:
>> On 20.06.19 03:03, John Snow wrote:
>>> We don't need or want a new sync mode for simple differences in
>>> semantics.  Create a new mode simply named "BITMAP" that is designed to
>>> make use of the new Bitmap Sync Mode field.
>>>
>>> Because the only bitmap mode is 'conditional', this adds no new
>>> functionality to the backup job (yet). The old incremental backup mode
>>> is maintained as a syntactic sugar for sync=bitmap, mode=conditional.
>>>
>>> Add all of the plumbing necessary to support this new instruction.
>>>
>>> Signed-off-by: John Snow <address@hidden>
>>> ---
>>>  qapi/block-core.json      | 30 ++++++++++++++++++++++--------
>>>  include/block/block_int.h |  6 +++++-
>>>  block/backup.c            | 35 ++++++++++++++++++++++++++++-------
>>>  block/mirror.c            |  6 ++++--
>>>  block/replication.c       |  2 +-
>>>  blockdev.c                |  8 ++++++--
>>>  6 files changed, 66 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index caf28a71a0..6d05ad8f47 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -1127,12 +1127,15 @@
>>>  #
>>>  # @none: only copy data written from now on
>>>  #
>>> -# @incremental: only copy data described by the dirty bitmap. Since: 2.4
>>> +# @incremental: only copy data described by the dirty bitmap. (since: 2.4)
>>
>> Why not deprecate this in the process and note that this is equal to
>> sync=bitmap, bitmap-mode=conditional?
>>
>> (I don’t think there is a rule that forces us to actually remove
>> deprecated stuff after two releases if it doesn’t hurt to keep it.)
>>
> 
> Mostly I thought it would be fine to keep as sugar. In your replies so
> far I gather that "incremental" and "differential" don't mean specific
> backup paradigms to you, so maybe these seem like worthless words.
> 
> It was my general understanding that in terms of backup
> paradigms/methodologies that "incremental" and "differential" mean very
> specific things.
> 
> Incremental: Each backup contains only the delta from the last
> incremental backup.
> Differential: Each backup contains the delta from the last FULL backup.
> 
> You can search "incremental vs differential backup" on your search
> engine of choice and find many relevant results. I took a Networking/IT
> vocational degree in 2007 and these terms were taught in textbooks then.
> 
> So I will resist quite strongly changing them, and for this reason, felt
> that it was strictly a good thing to keep incremental as sugar, because
> I thought that people would know what it meant.

:C

OK.  I’m happy as long as it’s all explained somewhere (i.e.
bitmaps.rst).  Personally, I’d also like a pointer to that documentation
here.  (Sure, people should just look there if they don’t understand
something about bitmaps anyway, but I can’t see it hurting to just put a
pointer here anyway.)

> (More than "conditional", anyway, which is jargon I made up.)

But you make it up in this series, which is great for me, because that
means I get the definition (from the cover letter) without having to
look it up. O:-)

[...]

>>>  #
>>> +# @bitmap-mode: Specifies the type of data the bitmap should contain after
>>> +#               the operation concludes. Must be present if sync is 
>>> "bitmap".
>>> +#               Must NOT be present otherwise. (Since 4.1)
>>
>> Do we have any rule that qemu must enforce “must not”s? :-)
>>
>> (No, I don’t think so.  I think it’s very reasonable that you accept
>> bitmap-mode=conditional for sync=incremental.)
>>
> 
> Right, I left this a secret wiggle room. If you specify the correct
> bitmap sync mode for the incremental sugar, it will actually let it
> slide. If you specify the wrong one, it will error out.
> 
> However, I think this is perfectly correct advice from the API: Please
> use this mode with sync=bitmap and do not use it otherwise.
> 
> Would you like me to change it to be more technically correct and
> document the little affordance I made?

It’s probably better not to.  Better forbid as much as we can so that we
can break compatibility to users that happened to use it still “because
it works”.

Max

signature.asc
Description: OpenPGP digital signature