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 <js...@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.)
> +#
> +# @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”.
> #
> +# @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.)
> # @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.
> +# @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.)
> + * @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
signature.asc
Description: OpenPGP digital signature
