Fam Zheng <f...@redhat.com> writes:
> This will start a mirror job from a named device to another named
> device, its relation with drive-mirror is similar with blockdev-backup
> to drive-backup.
>
> In blockdev-mirror, the target node should be prepared by blockdev-add,
> which will be responsible for assigning a name to the new node, so
> 'node-name' in drive-mirror is dropped.
>
> Signed-off-by: Fam Zheng <f...@redhat.com>
> ---
Reviewing only the QAPI/QMP interface.
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index b5c4559..fe440e1 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1059,6 +1059,53 @@
> 'data': 'BlockDirtyBitmap' }
>
> ##
> +# @blockdev-mirror
> +#
> +# Start mirroring a block device's writes to a new destination.
> +#
> +# @device: the name of the device whose writes should be mirrored.
> +#
> +# @target: the name of the device to mirror to.
> +#
> +# @replaces: #optional with sync=full graph node name to be replaced by the
> new
> +# image when a whole image copy is done. This can be used to
> repair
> +# broken Quorum files.
> +#
> +# @speed: #optional the maximum speed, in bytes per second
> +#
> +# @sync: what parts of the disk image should be copied to the destination
> +# (all the disk, only the sectors allocated in the topmost image, or
> +# only new I/O).
> +#
> +# @granularity: #optional 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 between 512 and 64M
> +#
> +# @buf-size: #optional maximum amount of data in flight from source to
> +# target
If you abbreviate, go all the way and call it bufsize. But we tend to
avoid abbreviations in QAPI/QMP, so make it buffer-size, please.
> +#
> +# @on-source-error: #optional the action to take on an error on the source,
> +# default 'report'. 'stop' and 'enospc' can only be used
> +# if the block device supports io-status (see BlockInfo).
> +#
> +# @on-target-error: #optional the action to take on an error on the target,
> +# default 'report' (no limitations, since this applies to
> +# a different block device than @device).
> +#
> +# Returns: nothing on success; error message on failure.
All commands return an error on failure, that's the nature of the
protocol. It's an error *reply*, though, not just a *message*.
I'd drop this sentence.
> +#
> +# Since 2.4
> +##
> +{ 'command': 'blockdev-mirror',
> + 'data': { 'device': 'str', 'target': 'str',
> + '*replaces': 'str',
> + 'sync': 'MirrorSyncMode',
> + '*speed': 'int', '*granularity': 'uint32',
> + '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
> + '*on-target-error': 'BlockdevOnError' } }
> +
> +##
> # @block_set_io_throttle:
> #
> # Change I/O throttle limits for a block drive.
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 90e0135..646db78 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1560,6 +1560,54 @@ Example:
> EQMP
>
> {
> + .name = "blockdev-mirror",
> + .args_type = "sync:s,device:B,target:B,replaces:s?,speed:i?,"
> + "on-source-error:s?,on-target-error:s?,"
> + "granularity:i?,buf-size:i?",
> + .mhandler.cmd_new = qmp_marshal_input_blockdev_mirror,
> + },
> +
> +SQMP
> +blockdev-mirror
> +------------
> +
> +Start mirroring a block device's writes to another block device. target
> +specifies the target of mirror operation.
> +
> +Arguments:
> +
> +- "device": device name to operate on (json-string)
> +- "target": device name to mirror to (json-string)
> +- "replaces": the block driver node name to replace when finished
> + (json-string, optional)
> +- "speed": maximum speed of the streaming job, in bytes per second
> + (json-int)
> +- "granularity": granularity of the dirty bitmap, in bytes (json-int,
> optional)
> +- "buf_size": maximum amount of data in flight from source to target, in
> bytes
> + (json-int, default 10M)
buf_size doesn't match qapi-schema.json's buf-size.
> +- "sync": what parts of the disk image should be copied to the destination;
> + possibilities include "full" for all the disk, "top" for only the sectors
> + allocated in the topmost image, or "none" to only replicate new I/O
> + (MirrorSyncMode).
> +- "on-source-error": the action to take on an error on the source
> + (BlockdevOnError, default 'report')
> +- "on-target-error": the action to take on an error on the target
> + (BlockdevOnError, default 'report')
> +
> +The default value of the granularity is the image cluster size clamped
> +between 4096 and 65536, if the image format defines one. If the format
> +does not define a cluster size, the default value of the granularity
> +is 65536.
> +
> +Example:
> +
> +-> { "execute": "blockdev-mirror", "arguments": { "device": "ide-hd0",
> + "target": "target0",
> + "sync": "full" } }
> +<- { "return": {} }
> +
> +EQMP
> + {
> .name = "change-backing-file",
> .args_type = "device:s,image-node-name:s,backing-file:s",
> .mhandler.cmd_new = qmp_marshal_input_change_backing_file,
Fix at least the name mismatch between qmp-commands.hx and
block-core.json, and you can add
Acked-by: Markus Armbruster <arm...@redhat.com>
Only Acked- because my review is partial.
Adressing my other nitpicks is desirable, but I'm not insisting on it.
