| Message ID | 1450850347-5291-5-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 23.12.2015 06:59, 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 > we don't have 'node-name' parameter. > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > blockdev.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > qapi/block-core.json | 47 +++++++++++++++++++++++++++++++++++++++ > qmp-commands.hx | 48 ++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 157 insertions(+) It appears you haven't addressed the comments for v2. I only had a single one (regarding documentation), but Markus had a couple ones, so those may be worth addressing. > > diff --git a/blockdev.c b/blockdev.c > index f42e171..2df0c6d 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -3345,6 +3345,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 && sync == MIRROR_SYNC_MODE_TOP) { > sync = MIRROR_SYNC_MODE_FULL; > @@ -3518,6 +3522,64 @@ 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, > + true, true, Shouldn't this be "false, false,", or, ideally, set by the user? > + &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + bdrv_unref(target_bs); > + } > + > + aio_context_release(aio_context); > +} > + Max
On Thu, 12/24 01:53, Max Reitz wrote: > On 23.12.2015 06:59, 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 > > we don't have 'node-name' parameter. > > > > Signed-off-by: Fam Zheng <famz@redhat.com> > > --- > > blockdev.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > > qapi/block-core.json | 47 +++++++++++++++++++++++++++++++++++++++ > > qmp-commands.hx | 48 ++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 157 insertions(+) > > It appears you haven't addressed the comments for v2. I only had a > single one (regarding documentation), but Markus had a couple ones, so > those may be worth addressing. Will look into that. > > > > > diff --git a/blockdev.c b/blockdev.c > > index f42e171..2df0c6d 100644 > > --- a/blockdev.c > > +++ b/blockdev.c > > @@ -3345,6 +3345,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 && sync == MIRROR_SYNC_MODE_TOP) { > > sync = MIRROR_SYNC_MODE_FULL; > > @@ -3518,6 +3522,64 @@ 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, > > + true, true, > > Shouldn't this be "false, false,", or, ideally, set by the user? I think true is correct here because then it will be effectively controlled by open flags of target. I.e. mirror.c always sets BDRV_REQ_MAY_UNMAP, and bdrv_co_write_zeroes has: if (!(bs->open_flags & BDRV_O_UNMAP)) { flags &= ~BDRV_REQ_MAY_UNMAP; } Fam > > > + &local_err); > > + if (local_err) { > > + error_propagate(errp, local_err); > > + bdrv_unref(target_bs); > > + } > > + > > + aio_context_release(aio_context); > > +} > > + > > Max >
On 24.12.2015 04:25, Fam Zheng wrote: > On Thu, 12/24 01:53, Max Reitz wrote: >> On 23.12.2015 06:59, 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 >>> we don't have 'node-name' parameter. >>> >>> Signed-off-by: Fam Zheng <famz@redhat.com> >>> --- >>> blockdev.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> qapi/block-core.json | 47 +++++++++++++++++++++++++++++++++++++++ >>> qmp-commands.hx | 48 ++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 157 insertions(+) >> >> It appears you haven't addressed the comments for v2. I only had a >> single one (regarding documentation), but Markus had a couple ones, so >> those may be worth addressing. > > Will look into that. > >> >>> >>> diff --git a/blockdev.c b/blockdev.c >>> index f42e171..2df0c6d 100644 >>> --- a/blockdev.c >>> +++ b/blockdev.c >>> @@ -3345,6 +3345,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 && sync == MIRROR_SYNC_MODE_TOP) { >>> sync = MIRROR_SYNC_MODE_FULL; >>> @@ -3518,6 +3522,64 @@ 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, >>> + true, true, >> >> Shouldn't this be "false, false,", or, ideally, set by the user? > > I think true is correct here because then it will be effectively controlled by > open flags of target. I.e. mirror.c always sets BDRV_REQ_MAY_UNMAP, and > bdrv_co_write_zeroes has: > > if (!(bs->open_flags & BDRV_O_UNMAP)) { > flags &= ~BDRV_REQ_MAY_UNMAP; > } I was asking because it differs from what drive-mirror does - but that is probably a good thing (drive-mirror takes this flag from the user (defaulting to false, which is why I was asking), but it takes the open_flags for the new image from the mirror source, which is... Interesting. So it's probably better this way, right. Max
diff --git a/blockdev.c b/blockdev.c index f42e171..2df0c6d 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3345,6 +3345,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 && sync == MIRROR_SYNC_MODE_TOP) { sync = MIRROR_SYNC_MODE_FULL; @@ -3518,6 +3522,64 @@ 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, + true, true, + &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 1a5d9ce..1712f6d 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1184,6 +1184,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.6 +## +{ '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 7b235ee..d357218 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -1665,6 +1665,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_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_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 we don't have 'node-name' parameter. Signed-off-by: Fam Zheng <famz@redhat.com> --- blockdev.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++ qapi/block-core.json | 47 +++++++++++++++++++++++++++++++++++++++ qmp-commands.hx | 48 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 157 insertions(+)