Re: [Qemu-devel] [PATCH v2 5/6] qmp: Add blockdev-mirror command

[Markus Armbruster]

Fam Zheng <address@hidden> 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 <address@hidden>
> ---

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 <address@hidden>

Only Acked- because my review is partial.

Adressing my other nitpicks is desirable, but I'm not insisting on it.