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

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 02/12] block/backup: Add mirror sync mode 'bitmap'
Date: Thu, 20 Jun 2019 12:01:11 -0400	[thread overview]
Message-ID: <5a873055-72c2-ac4c-074d-2c31c8b4c863@redhat.com> (raw)
In-Reply-To: <e7143bb8-afb6-8326-6e93-49a7470b9b98@redhat.com>

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 <jsnow@redhat.com>
>> ---
>>  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.

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

>> +#
>> +# @bitmap: only copy data described by the dirty bitmap. (since: 4.1)
>> +#          Behavior on completion is determined by the BitmapSyncMode.
>>  #
>>  # Since: 1.3
>>  ##
>>  { 'enum': 'MirrorSyncMode',
>> -  'data': ['top', 'full', 'none', 'incremental'] }
>> +  'data': ['top', 'full', 'none', 'incremental', 'bitmap'] }
>>  
>>  ##
>>  # @BitmapSyncMode:
>> @@ -1352,10 +1355,14 @@
>>  #
>>  # @speed: the maximum speed, in bytes per second
>>  #
>> -# @bitmap: the name of dirty bitmap if sync is "incremental".
>> -#          Must be present if sync is "incremental", must NOT be present
>> +# @bitmap: the name of dirty bitmap if sync is "bitmap".
>> +#          Must be present if sync is "bitmap", must NOT be present
>>  #          otherwise. (Since 2.4)
> 
> Er, well, now this is too fast of a deprecation. :-)  It must still also
> be present if sync is “incremental”.
> 

OK; I will try to phrase it better. This is reflecting too much the
implementation -- I think I was trying to communicate that incremental
was just sugar for "bitmap", so I was trusting that was understood here.

...But, depending on the order in which you read the docs, this could be
confusing, so I guess I will change that.

>>  #
>> +# @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?

>>  # @compress: true to compress data, if the target format supports it.
>>  #            (default: false) (since 2.8)
>>  #
>> @@ -1390,7 +1397,8 @@
>>    'data': { '*job-id': 'str', 'device': 'str', 'target': 'str',
>>              '*format': 'str', 'sync': 'MirrorSyncMode',
>>              '*mode': 'NewImageMode', '*speed': 'int',
>> -            '*bitmap': 'str', '*compress': 'bool',
>> +            '*bitmap': 'str', '*bitmap-mode': 'BitmapSyncMode',
>> +            '*compress': 'bool',
>>              '*on-source-error': 'BlockdevOnError',
>>              '*on-target-error': 'BlockdevOnError',
>>              '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
>> @@ -1412,10 +1420,14 @@
>>  # @speed: the maximum speed, in bytes per second. The default is 0,
>>  #         for unlimited.
>>  #
>> -# @bitmap: the name of dirty bitmap if sync is "incremental".
>> -#          Must be present if sync is "incremental", must NOT be present
>> +# @bitmap: the name of dirty bitmap if sync is "bitmap".
>> +#          Must be present if sync is "bitmap", must NOT be present
>>  #          otherwise. (Since 3.1)
> 
> Same as above.
> 

OK

>> +# @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)
>> +#
>>  # @compress: true to compress data, if the target format supports it.
>>  #            (default: false) (since 2.8)
>>  #
>> @@ -1449,7 +1461,9 @@
>>  { 'struct': 'BlockdevBackup',
>>    'data': { '*job-id': 'str', 'device': 'str', 'target': 'str',
>>              'sync': 'MirrorSyncMode', '*speed': 'int',
>> -            '*bitmap': 'str', '*compress': 'bool',
>> +            '*bitmap': 'str',
>> +            '*bitmap-mode': 'BitmapSyncMode',
>> +            '*compress': 'bool',
>>              '*on-source-error': 'BlockdevOnError',
>>              '*on-target-error': 'BlockdevOnError',
>>              '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index d6415b53c1..89370c1b9b 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -1132,7 +1132,9 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
>>   * @target: Block device to write to.
>>   * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
>>   * @sync_mode: What parts of the disk image should be copied to the destination.
>> - * @sync_bitmap: The dirty bitmap if sync_mode is MIRROR_SYNC_MODE_INCREMENTAL.
>> + * @sync_bitmap: The dirty bitmap if sync_mode is 'bitmap' or 'incremental'
>> + * @has_bitmap_mode: true if @bitmap_sync carries a meaningful value.
> 
> Hmm...  If you moved the conversion of incremental/- =>
> bitmap/conditional into blockdev.c, you could get rid of this parameter
> because it would be equal to (sync_bitmap != NULL).
> 
> (It itches me to get rid of this parameter because there is no other
> has* parameter for this function yet.)
> 

Yeah, it annoyed me too, and I believe later you do correctly guess why
I did it -- it's so that the sugar conversion occurs all in one place
where the logic was easiest to condense.

I ran into the issue that there's no way to define a QAPI enum that has
a "default"/"unset" state without also allowing that value to be entered
by the user explicitly; so there was no way to pass along an "unset
enum" down this far.

So... (thought continued below)

>> + * @bitmap_mode: The bitmap synchronization policy to use.
>>   * @on_source_error: The action to take upon error reading from the source.
>>   * @on_target_error: The action to take upon error writing to the target.
>>   * @creation_flags: Flags that control the behavior of the Job lifetime.
>> @@ -1148,6 +1150,8 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
>>                              BlockDriverState *target, int64_t speed,
>>                              MirrorSyncMode sync_mode,
>>                              BdrvDirtyBitmap *sync_bitmap,
>> +                            bool has_bitmap_mode,
>> +                            BitmapSyncMode bitmap_mode,
>>                              bool compress,
>>                              BlockdevOnError on_source_error,
>>                              BlockdevOnError on_target_error,
>> diff --git a/block/backup.c b/block/backup.c
>> index 715e1d3be8..c4f83d4ef7 100644
>> --- a/block/backup.c
>> +++ b/block/backup.c
> 
> [...]
> 
>> @@ -584,9 +586,28 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
>>      }
>>  
>>      if (sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) {
>> +        if (has_bitmap_mode &&
>> +            bitmap_mode != BITMAP_SYNC_MODE_CONDITIONAL) {
>> +            error_setg(errp, "Bitmap sync mode must be 'conditional' "
>> +                       "when using sync mode '%s'",
>> +                       MirrorSyncMode_str(sync_mode));
>> +            return NULL;
>> +        }
>> +        has_bitmap_mode = true;
>> +        bitmap_mode = BITMAP_SYNC_MODE_CONDITIONAL;
>> +        effective_mode = MIRROR_SYNC_MODE_BITMAP;
>> +    }
>> +
> 
> I also just don’t quite feel like this is the correct place to put this.
>  It’s a deprecated interface, so it should be translated in the
> interface code, i.e. in blockdev.c.
> 
> (Sure, this gives you a central place for the translation, but you can
> just as well add a function to the same effect to blockdev.c.)
> 
> Max
> 

... I can toy around with your idea of making a helper that can be
called in blockdev and see if I like it.

Thank you for taking a look!