Re: [Qemu-devel] [PATCH 03/12] block/backup: add 'never' policy to bitmap sync mode

From: John Snow <jsnow@redhat.com>
To: Max Reitz <mreitz@redhat.com>,
	qemu-devel@nongnu.org, qemu-block@nongnu.org
Cc: Kevin Wolf <kwolf@redhat.com>, Fam Zheng <fam@euphon.net>,
	vsementsov@virtuozzo.com, Wen Congyang <wencongyang2@huawei.com>,
	Xie Changlong <xiechanglong.d@gmail.com>,
	Markus Armbruster <armbru@redhat.com>
Subject: Re: [Qemu-devel] [PATCH 03/12] block/backup: add 'never' policy to bitmap sync mode
Date: Thu, 20 Jun 2019 12:11:50 -0400	[thread overview]
Message-ID: <997fce5c-4ffa-abe4-ea6a-3fec70c8533b@redhat.com> (raw)
In-Reply-To: <f3afc686-a37e-3c58-571c-96ae17c16414@redhat.com>

On 6/20/19 11:25 AM, Max Reitz wrote:
> On 20.06.19 03:03, John Snow wrote:
>> This adds a "never" policy for bitmap synchronization. Regardless of if
>> the job succeeds or fails, we never update the bitmap. This can be used
>> to perform differential backups, or simply to avoid the job modifying a
>> bitmap.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  qapi/block-core.json | 6 +++++-
>>  block/backup.c       | 5 +++--
>>  2 files changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 6d05ad8f47..0332dcaabc 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -1146,10 +1146,14 @@
>>  # @conditional: The bitmap is only synchronized when the operation is successul.
>>  #               This is useful for Incremental semantics.
>>  #
>> +# @never: The bitmap is never synchronized with the operation, and is
>> +#         treated solely as a manifest of blocks to copy.
>> +#         This is useful for Differential semantics.
>> +#
> 
> Again, this is too buzzword-y for my taste.  I don’t find it as bad
> because there is not much to explain about this mode, and you do explain
> it above, but still.
> 

Explained in my response to patch 2, I disagree.

> Like, I (me myself) read this and after the first sentence I think I’ve
> understood what this is.  Then I read “for Differential semantics” and
> I’m confused.  After a couple of seconds, I realize what you mean
> because I’ve described in my response to patch 1.
> 
> One reason it leaves the buzzword-y taste is because “differential” is
> never explained anywhere.  bitmaps.rst makes two mentions of it, but it
> too just assumes I know what you mean.  Also, incremental backups are
> just a certain kind of differential backups.
> 

This, however, is a real shortcoming of the doc. You'll notice I didn't
propose a doc update in this patchset, because secretly it's an RFC and
I did expect a v2+.

> So you need to explain “differential” somewhere and how it differs from
> “incremental” in this regard.  Why not here?
> 

Too broad of a concept to explain down in qapi comment strings, or I'd
have to explain it everywhere. bitmaps.rst is the correct place.

> “This is useful when you wish to repeatedly perform operations in
> reference to a constant synchronization point (when the bitmap was
> created).”
> 
> Or something.
> 
> Max
> 
>>  # Since: 4.1
>>  ##
>>  { 'enum': 'BitmapSyncMode',
>> -  'data': ['conditional'] }
>> +  'data': ['conditional', 'never'] }
>>  
>>  ##
>>  # @MirrorCopyMode:
> 