Re: [PATCH v2 2/4] mirror: allow specifying working bitmap

[Markus Armbruster <armbru@redhat.com>]

Fiona Ebner <f.ebner@proxmox.com> writes:

> From: John Snow <jsnow@redhat.com>
>
> for the mirror job. The bitmap's granularity is used as the job's
> granularity.
>
> The new @bitmap parameter is marked unstable in the QAPI and can
> currently only be used for @sync=full mode.
>
> Clusters initially dirty in the bitmap as well as new writes are
> copied to the target.
>
> Using block-dirty-bitmap-clear and block-dirty-bitmap-merge API,
> callers can simulate the three kinds of @BitmapSyncMode (which is used
> by backup):
> 1. always: default, just pass bitmap as working bitmap.
> 2. never: copy bitmap and pass copy to the mirror job.
> 3. on-success: copy bitmap and pass copy to the mirror job and if
>    successful, merge bitmap into original afterwards.
>
> When the target image is a fresh "diff image", i.e. one that was not
> used as the target of a previous mirror and the target image's cluster
> size is larger than the bitmap's granularity, or when
> @copy-mode=write-blocking is used, there is a pitfall, because the
> cluster in the target image will be allocated, but not contain all the
> data corresponding to the same region in the source image.
>
> An idea to avoid the limitation would be to mark clusters which are
> affected by unaligned writes and are not allocated in the target image
> dirty, so they would be copied fully later. However, for migration,
> the invariant that an actively synced mirror stays actively synced
> (unless an error happens) is useful, because without that invariant,
> migration might inactivate block devices when mirror still got work
> to do and run into an assertion failure [0].
>
> Another approach would be to read the missing data from the source
> upon unaligned writes to be able to write the full target cluster
> instead.
>
> But certain targets like NBD do not allow querying the cluster size.
> To avoid limiting/breaking the use case of syncing to an existing
> target, which is arguably more common than the diff image use case,
> document the limiation in QAPI.
>
> This patch was originally based on one by Ma Haocong, but it has since
> been modified pretty heavily, first by John and then again by Fiona.
>
> [0]: https://lore.kernel.org/qemu-devel/1db7f571-cb7f-c293-04cc-cd856e060c3f@proxmox.com/
>
> Suggested-by: Ma Haocong <mahaocong@didichuxing.com>
> Signed-off-by: Ma Haocong <mahaocong@didichuxing.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> [FG: switch to bdrv_dirty_bitmap_merge_internal]
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
> [FE: rebase for 9.0
>      get rid of bitmap mode parameter
>      use caller-provided bitmap as working bitmap
>      turn bitmap parameter experimental]
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>

[...]

> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 59d75b0793..4859fffd48 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2191,6 +2191,18 @@
>  #     destination (all the disk, only the sectors allocated in the
>  #     topmost image, or only new I/O).
>  #
> +# @bitmap: The name of a bitmap to use as a working bitmap for
> +#     sync=full mode.  This argument must be not be present for other
> +#     sync modes and not at the same time as @granularity.  The
> +#     bitmap's granularity is used as the job's granularity.  When
> +#     the target is a diff image, i.e. one that should only contain
> +#     the delta and was not synced to previously, the target's
> +#     cluster size must not be larger than the bitmap's granularity.
> +#     For a diff image target, using copy-mode=write-blocking should
> +#     not be used, because unaligned writes will lead to allocated
> +#     clusters with partial data in the target image!  The bitmap
> +#     will be enabled after the job finishes.  (Since 9.0)

That's a lot of restrictions and caveats.  Okay as long as the thing
remains experimental, I guess.

> +#
>  # @granularity: granularity of the dirty bitmap, default is 64K if the
>  #     image format doesn't have clusters, 4K if the clusters are
>  #     smaller than that, else the cluster size.  Must be a power of 2
> @@ -2228,12 +2240,18 @@
>  #     disappear from the query list without user intervention.
>  #     Defaults to true.  (Since 3.1)
>  #
> +# Features:
> +#
> +# @unstable: Member @bitmap is experimental.
> +#
>  # Since: 1.3
>  ##
>  { 'struct': 'DriveMirror',
>    'data': { '*job-id': 'str', 'device': 'str', 'target': 'str',
>              '*format': 'str', '*node-name': 'str', '*replaces': 'str',
> -            'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
> +            'sync': 'MirrorSyncMode',
> +            '*bitmap': { 'type': 'str', 'features': [ 'unstable' ] },
> +            '*mode': 'NewImageMode',
>              '*speed': 'int', '*granularity': 'uint32',
>              '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
>              '*on-target-error': 'BlockdevOnError',
> @@ -2513,6 +2531,18 @@
>  #     destination (all the disk, only the sectors allocated in the
>  #     topmost image, or only new I/O).
>  #
> +# @bitmap: The name of a bitmap to use as a working bitmap for
> +#     sync=full mode.  This argument must be not be present for other
> +#     sync modes and not at the same time as @granularity.  The
> +#     bitmap's granularity is used as the job's granularity.  When
> +#     the target is a diff image, i.e. one that should only contain
> +#     the delta and was not synced to previously, the target's
> +#     cluster size must not be larger than the bitmap's granularity.
> +#     For a diff image target, using copy-mode=write-blocking should
> +#     not be used, because unaligned writes will lead to allocated
> +#     clusters with partial data in the target image!  The bitmap
> +#     will be enabled after the job finishes.  (Since 9.0)
> +#
>  # @granularity: granularity of the dirty bitmap, default is 64K if the
>  #     image format doesn't have clusters, 4K if the clusters are
>  #     smaller than that, else the cluster size.  Must be a power of 2
> @@ -2548,6 +2578,10 @@
>  #     disappear from the query list without user intervention.
>  #     Defaults to true.  (Since 3.1)
>  #
> +# Features:
> +#
> +# @unstable: Member @bitmap is experimental.
> +#
>  # Since: 2.6
>  #
>  # Example:
> @@ -2562,6 +2596,7 @@
>    'data': { '*job-id': 'str', 'device': 'str', 'target': 'str',
>              '*replaces': 'str',
>              'sync': 'MirrorSyncMode',
> +            '*bitmap': { 'type': 'str', 'features': [ 'unstable' ] },
>              '*speed': 'int', '*granularity': 'uint32',
>              '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
>              '*on-target-error': 'BlockdevOnError',

Acked-by: Markus Armbruster <armbru@redhat.com>

[...]