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

[Max Reitz]

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.)

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