| Message ID | 1435202570-12360-6-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
Fam Zheng <famz@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 <famz@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 <armbru@redhat.com> Only Acked- because my review is partial. Adressing my other nitpicks is desirable, but I'm not insisting on it.
On 25.06.2015 05:22, Fam Zheng wrote: > 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 <famz@redhat.com> > --- > blockdev.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > qapi/block-core.json | 47 ++++++++++++++++++++++++++++++++++++++++ > qmp-commands.hx | 48 +++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 156 insertions(+) > > diff --git a/blockdev.c b/blockdev.c > index de6383f..e0dba07 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -2932,6 +2932,10 @@ static void blockdev_mirror_common(BlockDriverState *bs, > if (bdrv_op_is_blocked(target, BLOCK_OP_TYPE_MIRROR_TARGET, errp)) { > return; > } > + if (target->blk) { > + error_setg(errp, "Cannot mirror to an attached block device"); > + return; > + } 1) Why? 2) I think this should be noted in the QMP interface documentation. "the name of the device to mirror to" sounds like it is actually meant to be an attached block device. Max
On Mon, 07/20 17:33, Max Reitz wrote: > On 25.06.2015 05:22, Fam Zheng wrote: > >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 <famz@redhat.com> > >--- > > blockdev.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > > qapi/block-core.json | 47 ++++++++++++++++++++++++++++++++++++++++ > > qmp-commands.hx | 48 +++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 156 insertions(+) > > > >diff --git a/blockdev.c b/blockdev.c > >index de6383f..e0dba07 100644 > >--- a/blockdev.c > >+++ b/blockdev.c > >@@ -2932,6 +2932,10 @@ static void blockdev_mirror_common(BlockDriverState *bs, > > if (bdrv_op_is_blocked(target, BLOCK_OP_TYPE_MIRROR_TARGET, errp)) { > > return; > > } > >+ if (target->blk) { > >+ error_setg(errp, "Cannot mirror to an attached block device"); > >+ return; > >+ } > > 1) Why? To match the limitation of drive-mirror. We don't yet have a block job that writes to attached device yet (except for stream, but that's only copy on read). I've no idea what that implies, and I don't know if there is even a use case. > > 2) I think this should be noted in the QMP interface documentation. "the > name of the device to mirror to" sounds like it is actually meant to be an > attached block device. I'll update the documentation. Fam
diff --git a/blockdev.c b/blockdev.c index de6383f..e0dba07 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2932,6 +2932,10 @@ static void blockdev_mirror_common(BlockDriverState *bs, if (bdrv_op_is_blocked(target, BLOCK_OP_TYPE_MIRROR_TARGET, errp)) { return; } + if (target->blk) { + error_setg(errp, "Cannot mirror to an attached block device"); + return; + } if (!bs->backing_hd && sync == MIRROR_SYNC_MODE_TOP) { sync = MIRROR_SYNC_MODE_FULL; @@ -3107,6 +3111,63 @@ out: aio_context_release(aio_context); } +void qmp_blockdev_mirror(const char *device, const char *target, + bool has_replaces, const char *replaces, + MirrorSyncMode sync, + bool has_speed, int64_t speed, + bool has_granularity, uint32_t granularity, + bool has_buf_size, int64_t buf_size, + bool has_on_source_error, + BlockdevOnError on_source_error, + bool has_on_target_error, + BlockdevOnError on_target_error, + Error **errp) +{ + BlockDriverState *bs; + BlockBackend *blk; + BlockDriverState *target_bs; + AioContext *aio_context; + Error *local_err = NULL; + + blk = blk_by_name(device); + if (!blk) { + error_setg(errp, "Device '%s' not found", device); + return; + } + bs = blk_bs(blk); + + if (!bs) { + error_setg(errp, "Device '%s' has no media", device); + return; + } + + target_bs = bdrv_lookup_bs(target, target, errp); + if (!target_bs) { + return; + } + + aio_context = bdrv_get_aio_context(bs); + aio_context_acquire(aio_context); + + bdrv_ref(target_bs); + bdrv_set_aio_context(target_bs, aio_context); + + blockdev_mirror_common(bs, target_bs, + has_replaces, replaces, sync, + has_speed, speed, + has_granularity, granularity, + has_buf_size, buf_size, + has_on_source_error, on_source_error, + has_on_target_error, on_target_error, + &local_err); + if (local_err) { + error_propagate(errp, local_err); + bdrv_unref(target_bs); + } + + aio_context_release(aio_context); +} + /* Get the block job for a given device name and acquire its AioContext */ static BlockJob *find_block_job(const char *device, AioContext **aio_context, Error **errp) 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 +# +# @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. +# +# 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) +- "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,
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 <famz@redhat.com> --- blockdev.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++ qapi/block-core.json | 47 ++++++++++++++++++++++++++++++++++++++++ qmp-commands.hx | 48 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 156 insertions(+)