| Message ID | 1432790990-25383-3-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 28/05/2015 07:29, Fam Zheng wrote: > If specified as "true", it allows discarding on target sectors where source is > not allocated. > > Signed-off-by: Fam Zheng <famz@redhat.com> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> > --- > block/mirror.c | 7 +++++-- > blockdev.c | 5 +++++ > hmp.c | 2 +- > include/block/block_int.h | 2 ++ > qapi/block-core.json | 8 +++++++- > qmp-commands.hx | 3 +++ > 6 files changed, 23 insertions(+), 4 deletions(-) > > diff --git a/block/mirror.c b/block/mirror.c > index 58f391a..85995b2 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -57,6 +57,7 @@ typedef struct MirrorBlockJob { > int in_flight; > int sectors_in_flight; > int ret; > + bool unmap; > } MirrorBlockJob; > > typedef struct MirrorOp { > @@ -651,6 +652,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target, > int64_t buf_size, > BlockdevOnError on_source_error, > BlockdevOnError on_target_error, > + bool unmap, > BlockCompletionFunc *cb, > void *opaque, Error **errp, > const BlockJobDriver *driver, > @@ -703,6 +705,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target, > int64_t speed, uint32_t granularity, int64_t buf_size, > MirrorSyncMode mode, BlockdevOnError on_source_error, > BlockdevOnError on_target_error, > + bool unmap, > BlockCompletionFunc *cb, > void *opaque, Error **errp) > { > @@ -717,7 +720,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target, > base = mode == MIRROR_SYNC_MODE_TOP ? bs->backing_hd : NULL; > mirror_start_job(bs, target, replaces, > speed, granularity, buf_size, > - on_source_error, on_target_error, cb, opaque, errp, > + on_source_error, on_target_error, unmap, cb, opaque, errp, > &mirror_job_driver, is_none_mode, base); > } > > @@ -765,7 +768,7 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base, > > bdrv_ref(base); > mirror_start_job(bs, base, NULL, speed, 0, 0, > - on_error, on_error, cb, opaque, &local_err, > + on_error, on_error, false, cb, opaque, &local_err, > &commit_active_job_driver, false, base); > if (local_err) { > error_propagate(errp, local_err); > diff --git a/blockdev.c b/blockdev.c > index 5eaf77e..4387e14 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -2632,6 +2632,7 @@ void qmp_drive_mirror(const char *device, const char *target, > 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, > + bool has_unmap, bool unmap, > Error **errp) > { > BlockBackend *blk; > @@ -2663,6 +2664,9 @@ void qmp_drive_mirror(const char *device, const char *target, > if (!has_buf_size) { > buf_size = DEFAULT_MIRROR_BUF_SIZE; > } > + if (!has_unmap) { > + unmap = true; > + } > > if (granularity != 0 && (granularity < 512 || granularity > 1048576 * 64)) { > error_set(errp, QERR_INVALID_PARAMETER_VALUE, "granularity", > @@ -2802,6 +2806,7 @@ void qmp_drive_mirror(const char *device, const char *target, > has_replaces ? replaces : NULL, > speed, granularity, buf_size, sync, > on_source_error, on_target_error, > + unmap, > block_job_cb, bs, &local_err); > if (local_err != NULL) { > bdrv_unref(target_bs); > diff --git a/hmp.c b/hmp.c > index e17852d..62c53e0 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -1056,7 +1056,7 @@ void hmp_drive_mirror(Monitor *mon, const QDict *qdict) > false, NULL, false, NULL, > full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP, > true, mode, false, 0, false, 0, false, 0, > - false, 0, false, 0, &err); > + false, 0, false, 0, false, true, &err); > hmp_handle_error(mon, &err); > } > > diff --git a/include/block/block_int.h b/include/block/block_int.h > index f004378..4e0d700 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -590,6 +590,7 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base, > * @mode: Whether to collapse all images in the chain to the target. > * @on_source_error: The action to take upon error reading from the source. > * @on_target_error: The action to take upon error writing to the target. > + * @unmap: Whether to unmap target where source sectors only contain zeroes. > * @cb: Completion function for the job. > * @opaque: Opaque pointer value passed to @cb. > * @errp: Error object. > @@ -604,6 +605,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target, > int64_t speed, uint32_t granularity, int64_t buf_size, > MirrorSyncMode mode, BlockdevOnError on_source_error, > BlockdevOnError on_target_error, > + bool unmap, > BlockCompletionFunc *cb, > void *opaque, Error **errp); > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 863ffea..a59063d 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -954,6 +954,11 @@ > # @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). > +# @unmap: #optional Whether to try to unmap target sectors where source has > +# only zero. If true, and target unallocated sectors will read as zero, > +# target image sectors will be unmapped; otherwise, zeroes will be > +# written. Both will result in identical contents. > +# Default is true. (Since 2.4) > # > # Returns: nothing on success > # If @device is not a valid block device, DeviceNotFound > @@ -966,7 +971,8 @@ > 'sync': 'MirrorSyncMode', '*mode': 'NewImageMode', > '*speed': 'int', '*granularity': 'uint32', > '*buf-size': 'int', '*on-source-error': 'BlockdevOnError', > - '*on-target-error': 'BlockdevOnError' } } > + '*on-target-error': 'BlockdevOnError', > + '*unmap': 'bool' } } > > ## > # @BlockDirtyBitmap > diff --git a/qmp-commands.hx b/qmp-commands.hx > index 14e109e..63c86fc 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -1503,6 +1503,7 @@ EQMP > .args_type = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?," > "node-name:s?,replaces:s?," > "on-source-error:s?,on-target-error:s?," > + "unmap:b?," > "granularity:i?,buf-size:i?", > .mhandler.cmd_new = qmp_marshal_input_drive_mirror, > }, > @@ -1542,6 +1543,8 @@ Arguments: > (BlockdevOnError, default 'report') > - "on-target-error": the action to take on an error on the target > (BlockdevOnError, default 'report') > +- "unmap": whether the target sectors should be discarded where source has only > + zeroes. (json-bool, optional, default true) > > 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 >
On 05/27/2015 11:29 PM, Fam Zheng wrote: > If specified as "true", it allows discarding on target sectors where source is > not allocated. > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > +++ b/qapi/block-core.json > @@ -954,6 +954,11 @@ > # @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). > +# @unmap: #optional Whether to try to unmap target sectors where source has > +# only zero. If true, and target unallocated sectors will read as zero, > +# target image sectors will be unmapped; otherwise, zeroes will be > +# written. Both will result in identical contents. > +# Default is true. (Since 2.4) Just making sure I understand: The guest sees identical contents, but with "unmap":true, the host file is potentially sparse, while with "unmap":false, the host file is fully-allocated. Also, while the default is now true, this doesn't tell me what the behavior was in 2.3. Is this a new default behavior (where in 2.3 you could not preserve sparseness), or a new knob (previously you always got a sparse copy, and could not request full allocation)? I'm okay either way, but I'm trying to understand whether libvirt should advertise this knob to higher-level apps, and if so, what libvirt should do when it detects qemu 2.3 (that is, should it tell upper-level apps that sparseness cannot be preserved, or that full allocation cannot be guaranteed, when the "unmap" parameter is not detected).
On Tue, 06/02 11:28, Eric Blake wrote: > On 05/27/2015 11:29 PM, Fam Zheng wrote: > > If specified as "true", it allows discarding on target sectors where source is > > not allocated. > > > > Signed-off-by: Fam Zheng <famz@redhat.com> > > --- > > > +++ b/qapi/block-core.json > > @@ -954,6 +954,11 @@ > > # @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). > > +# @unmap: #optional Whether to try to unmap target sectors where source has > > +# only zero. If true, and target unallocated sectors will read as zero, > > +# target image sectors will be unmapped; otherwise, zeroes will be > > +# written. Both will result in identical contents. > > +# Default is true. (Since 2.4) > > Just making sure I understand: > > The guest sees identical contents, but with "unmap":true, the host file > is potentially sparse, while with "unmap":false, the host file is > fully-allocated. > > Also, while the default is now true, this doesn't tell me what the > behavior was in 2.3. Is this a new default behavior (where in 2.3 you > could not preserve sparseness), or a new knob (previously you always got > a sparse copy, and could not request full allocation)? I'm okay either > way, but I'm trying to understand whether libvirt should advertise this > knob to higher-level apps, and if so, what libvirt should do when it > detects qemu 2.3 (that is, should it tell upper-level apps that > sparseness cannot be preserved, or that full allocation cannot be > guaranteed, when the "unmap" parameter is not detected). We always skip sectors which are initially unallocated (at the time of mirror job starting), actually this even stays true for both unmap=true and unmap=false now. The difference is how we handle sectors discarded *after* mirror job starts: Before, we ignore the *discard*, which means the target remains populated, with old data before discard. After, we honor the discard, depending on two factors: source read as zero unmap RESULT ========================================================================== Y true write zero with BDRV_REQ_MAY_UNMAP Y false write zero without BDRV_REQ_MAY_UNMAP N both discard (note that on some protocols this may be nop) In other words, the unmap option only affects sector X if: 1) in the beginning, sector X was allocated 2) drive-mirror starts 3) sector X got discarded at source side All in all, this is quite advisory. Fam
diff --git a/block/mirror.c b/block/mirror.c index 58f391a..85995b2 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -57,6 +57,7 @@ typedef struct MirrorBlockJob { int in_flight; int sectors_in_flight; int ret; + bool unmap; } MirrorBlockJob; typedef struct MirrorOp { @@ -651,6 +652,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target, int64_t buf_size, BlockdevOnError on_source_error, BlockdevOnError on_target_error, + bool unmap, BlockCompletionFunc *cb, void *opaque, Error **errp, const BlockJobDriver *driver, @@ -703,6 +705,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target, int64_t speed, uint32_t granularity, int64_t buf_size, MirrorSyncMode mode, BlockdevOnError on_source_error, BlockdevOnError on_target_error, + bool unmap, BlockCompletionFunc *cb, void *opaque, Error **errp) { @@ -717,7 +720,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target, base = mode == MIRROR_SYNC_MODE_TOP ? bs->backing_hd : NULL; mirror_start_job(bs, target, replaces, speed, granularity, buf_size, - on_source_error, on_target_error, cb, opaque, errp, + on_source_error, on_target_error, unmap, cb, opaque, errp, &mirror_job_driver, is_none_mode, base); } @@ -765,7 +768,7 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base, bdrv_ref(base); mirror_start_job(bs, base, NULL, speed, 0, 0, - on_error, on_error, cb, opaque, &local_err, + on_error, on_error, false, cb, opaque, &local_err, &commit_active_job_driver, false, base); if (local_err) { error_propagate(errp, local_err); diff --git a/blockdev.c b/blockdev.c index 5eaf77e..4387e14 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2632,6 +2632,7 @@ void qmp_drive_mirror(const char *device, const char *target, 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, + bool has_unmap, bool unmap, Error **errp) { BlockBackend *blk; @@ -2663,6 +2664,9 @@ void qmp_drive_mirror(const char *device, const char *target, if (!has_buf_size) { buf_size = DEFAULT_MIRROR_BUF_SIZE; } + if (!has_unmap) { + unmap = true; + } if (granularity != 0 && (granularity < 512 || granularity > 1048576 * 64)) { error_set(errp, QERR_INVALID_PARAMETER_VALUE, "granularity", @@ -2802,6 +2806,7 @@ void qmp_drive_mirror(const char *device, const char *target, has_replaces ? replaces : NULL, speed, granularity, buf_size, sync, on_source_error, on_target_error, + unmap, block_job_cb, bs, &local_err); if (local_err != NULL) { bdrv_unref(target_bs); diff --git a/hmp.c b/hmp.c index e17852d..62c53e0 100644 --- a/hmp.c +++ b/hmp.c @@ -1056,7 +1056,7 @@ void hmp_drive_mirror(Monitor *mon, const QDict *qdict) false, NULL, false, NULL, full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP, true, mode, false, 0, false, 0, false, 0, - false, 0, false, 0, &err); + false, 0, false, 0, false, true, &err); hmp_handle_error(mon, &err); } diff --git a/include/block/block_int.h b/include/block/block_int.h index f004378..4e0d700 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -590,6 +590,7 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base, * @mode: Whether to collapse all images in the chain to the target. * @on_source_error: The action to take upon error reading from the source. * @on_target_error: The action to take upon error writing to the target. + * @unmap: Whether to unmap target where source sectors only contain zeroes. * @cb: Completion function for the job. * @opaque: Opaque pointer value passed to @cb. * @errp: Error object. @@ -604,6 +605,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target, int64_t speed, uint32_t granularity, int64_t buf_size, MirrorSyncMode mode, BlockdevOnError on_source_error, BlockdevOnError on_target_error, + bool unmap, BlockCompletionFunc *cb, void *opaque, Error **errp); diff --git a/qapi/block-core.json b/qapi/block-core.json index 863ffea..a59063d 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -954,6 +954,11 @@ # @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). +# @unmap: #optional Whether to try to unmap target sectors where source has +# only zero. If true, and target unallocated sectors will read as zero, +# target image sectors will be unmapped; otherwise, zeroes will be +# written. Both will result in identical contents. +# Default is true. (Since 2.4) # # Returns: nothing on success # If @device is not a valid block device, DeviceNotFound @@ -966,7 +971,8 @@ 'sync': 'MirrorSyncMode', '*mode': 'NewImageMode', '*speed': 'int', '*granularity': 'uint32', '*buf-size': 'int', '*on-source-error': 'BlockdevOnError', - '*on-target-error': 'BlockdevOnError' } } + '*on-target-error': 'BlockdevOnError', + '*unmap': 'bool' } } ## # @BlockDirtyBitmap diff --git a/qmp-commands.hx b/qmp-commands.hx index 14e109e..63c86fc 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -1503,6 +1503,7 @@ EQMP .args_type = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?," "node-name:s?,replaces:s?," "on-source-error:s?,on-target-error:s?," + "unmap:b?," "granularity:i?,buf-size:i?", .mhandler.cmd_new = qmp_marshal_input_drive_mirror, }, @@ -1542,6 +1543,8 @@ Arguments: (BlockdevOnError, default 'report') - "on-target-error": the action to take on an error on the target (BlockdevOnError, default 'report') +- "unmap": whether the target sectors should be discarded where source has only + zeroes. (json-bool, optional, default true) 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
If specified as "true", it allows discarding on target sectors where source is not allocated. Signed-off-by: Fam Zheng <famz@redhat.com> --- block/mirror.c | 7 +++++-- blockdev.c | 5 +++++ hmp.c | 2 +- include/block/block_int.h | 2 ++ qapi/block-core.json | 8 +++++++- qmp-commands.hx | 3 +++ 6 files changed, 23 insertions(+), 4 deletions(-)