* [Qemu-devel] [PATCH v6 1/8] block: Add bdrv_get_block_status_above
2015-05-28 5:29 [Qemu-devel] [PATCH v6 0/8] block: Mirror discarded sectors Fam Zheng
@ 2015-05-28 5:29 ` Fam Zheng
2015-05-28 8:27 ` Paolo Bonzini
2015-05-28 5:29 ` [Qemu-devel] [PATCH v6 2/8] qmp: Add optional bool "unmap" to drive-mirror Fam Zheng
` (7 subsequent siblings)
8 siblings, 1 reply; 17+ messages in thread
From: Fam Zheng @ 2015-05-28 5:29 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, qemu-block, Jeff Cody, qemu-stable, Stefan Hajnoczi,
pbonzini, jsnow, wangxiaolong
Like bdrv_is_allocated_above, this function follows the backing chain until seeing
BDRV_BLOCK_ALLOCATED. Base is not included.
Reimplement bdrv_is_allocated on top.
Signed-off-by: Fam Zheng <famz@redhat.com>
---
block/io.c | 56 +++++++++++++++++++++++++++++++++++++++++----------
include/block/block.h | 4 ++++
2 files changed, 49 insertions(+), 11 deletions(-)
diff --git a/block/io.c b/block/io.c
index e394d92..41463a6 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1560,28 +1560,54 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
return ret;
}
-/* Coroutine wrapper for bdrv_get_block_status() */
-static void coroutine_fn bdrv_get_block_status_co_entry(void *opaque)
+static int64_t coroutine_fn bdrv_co_get_block_status_above(BlockDriverState *bs,
+ BlockDriverState *base,
+ int64_t sector_num,
+ int nb_sectors,
+ int *pnum)
+{
+ BlockDriverState *p;
+ int64_t ret;
+
+ assert(bs != base);
+ for (p = bs; p != base; p = p->backing_hd) {
+ ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, pnum);
+ if (ret < 0 || ret & BDRV_BLOCK_ALLOCATED) {
+ break;
+ }
+ /* [sector_num, pnum] unallocated on this layer, which could be only
+ * the first part of [sector_num, nb_sectors]. */
+ nb_sectors = MIN(nb_sectors, *pnum);
+ }
+ return ret;
+}
+
+/* Coroutine wrapper for bdrv_get_block_status_above() */
+static void coroutine_fn bdrv_get_block_status_above_co_entry(void *opaque)
{
BdrvCoGetBlockStatusData *data = opaque;
- BlockDriverState *bs = data->bs;
- data->ret = bdrv_co_get_block_status(bs, data->sector_num, data->nb_sectors,
- data->pnum);
+ data->ret = bdrv_co_get_block_status_above(data->bs, data->base,
+ data->sector_num,
+ data->nb_sectors,
+ data->pnum);
data->done = true;
}
/*
- * Synchronous wrapper around bdrv_co_get_block_status().
+ * Synchronous wrapper around bdrv_co_get_block_status_above().
*
- * See bdrv_co_get_block_status() for details.
+ * See bdrv_co_get_block_status_above() for details.
*/
-int64_t bdrv_get_block_status(BlockDriverState *bs, int64_t sector_num,
- int nb_sectors, int *pnum)
+int64_t bdrv_get_block_status_above(BlockDriverState *bs,
+ BlockDriverState *base,
+ int64_t sector_num,
+ int nb_sectors, int *pnum)
{
Coroutine *co;
BdrvCoGetBlockStatusData data = {
.bs = bs,
+ .base = base,
.sector_num = sector_num,
.nb_sectors = nb_sectors,
.pnum = pnum,
@@ -1590,11 +1616,11 @@ int64_t bdrv_get_block_status(BlockDriverState *bs, int64_t sector_num,
if (qemu_in_coroutine()) {
/* Fast-path if already in coroutine context */
- bdrv_get_block_status_co_entry(&data);
+ bdrv_get_block_status_above_co_entry(&data);
} else {
AioContext *aio_context = bdrv_get_aio_context(bs);
- co = qemu_coroutine_create(bdrv_get_block_status_co_entry);
+ co = qemu_coroutine_create(bdrv_get_block_status_above_co_entry);
qemu_coroutine_enter(co, &data);
while (!data.done) {
aio_poll(aio_context, true);
@@ -1603,6 +1629,14 @@ int64_t bdrv_get_block_status(BlockDriverState *bs, int64_t sector_num,
return data.ret;
}
+int64_t bdrv_get_block_status(BlockDriverState *bs,
+ int64_t sector_num,
+ int nb_sectors, int *pnum)
+{
+ return bdrv_get_block_status_above(bs, bs->backing_hd,
+ sector_num, nb_sectors, pnum);
+}
+
int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num,
int nb_sectors, int *pnum)
{
diff --git a/include/block/block.h b/include/block/block.h
index c1c963e..8a13bed 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -361,6 +361,10 @@ bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs);
bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs);
int64_t bdrv_get_block_status(BlockDriverState *bs, int64_t sector_num,
int nb_sectors, int *pnum);
+int64_t bdrv_get_block_status_above(BlockDriverState *bs,
+ BlockDriverState *base,
+ int64_t sector_num,
+ int nb_sectors, int *pnum);
int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
int *pnum);
int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
--
2.4.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH v6 1/8] block: Add bdrv_get_block_status_above
2015-05-28 5:29 ` [Qemu-devel] [PATCH v6 1/8] block: Add bdrv_get_block_status_above Fam Zheng
@ 2015-05-28 8:27 ` Paolo Bonzini
0 siblings, 0 replies; 17+ messages in thread
From: Paolo Bonzini @ 2015-05-28 8:27 UTC (permalink / raw)
To: Fam Zheng, qemu-devel
Cc: Kevin Wolf, qemu-block, Jeff Cody, qemu-stable, Stefan Hajnoczi,
jsnow, wangxiaolong
On 28/05/2015 07:29, Fam Zheng wrote:
> Like bdrv_is_allocated_above, this function follows the backing chain until seeing
> BDRV_BLOCK_ALLOCATED. Base is not included.
>
> Reimplement bdrv_is_allocated on top.
"Reimplement bdrv_get_block_status on top".
Can be fixed by whoever commits this, so
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH v6 2/8] qmp: Add optional bool "unmap" to drive-mirror
2015-05-28 5:29 [Qemu-devel] [PATCH v6 0/8] block: Mirror discarded sectors Fam Zheng
2015-05-28 5:29 ` [Qemu-devel] [PATCH v6 1/8] block: Add bdrv_get_block_status_above Fam Zheng
@ 2015-05-28 5:29 ` Fam Zheng
2015-05-28 8:28 ` Paolo Bonzini
2015-06-02 17:28 ` Eric Blake
2015-05-28 5:29 ` [Qemu-devel] [PATCH v6 3/8] mirror: Do zero write on target if sectors not allocated Fam Zheng
` (6 subsequent siblings)
8 siblings, 2 replies; 17+ messages in thread
From: Fam Zheng @ 2015-05-28 5:29 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, qemu-block, Jeff Cody, qemu-stable, Stefan Hajnoczi,
pbonzini, jsnow, wangxiaolong
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(-)
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
--
2.4.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH v6 2/8] qmp: Add optional bool "unmap" to drive-mirror
2015-05-28 5:29 ` [Qemu-devel] [PATCH v6 2/8] qmp: Add optional bool "unmap" to drive-mirror Fam Zheng
@ 2015-05-28 8:28 ` Paolo Bonzini
2015-06-02 17:28 ` Eric Blake
1 sibling, 0 replies; 17+ messages in thread
From: Paolo Bonzini @ 2015-05-28 8:28 UTC (permalink / raw)
To: Fam Zheng, qemu-devel
Cc: Kevin Wolf, qemu-block, Jeff Cody, qemu-stable, Stefan Hajnoczi,
jsnow, wangxiaolong
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
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH v6 2/8] qmp: Add optional bool "unmap" to drive-mirror
2015-05-28 5:29 ` [Qemu-devel] [PATCH v6 2/8] qmp: Add optional bool "unmap" to drive-mirror Fam Zheng
2015-05-28 8:28 ` Paolo Bonzini
@ 2015-06-02 17:28 ` Eric Blake
2015-06-03 7:13 ` Fam Zheng
1 sibling, 1 reply; 17+ messages in thread
From: Eric Blake @ 2015-06-02 17:28 UTC (permalink / raw)
To: Fam Zheng, qemu-devel
Cc: Kevin Wolf, qemu-block, Jeff Cody, qemu-stable, Stefan Hajnoczi,
pbonzini, jsnow, wangxiaolong
[-- Attachment #1: Type: text/plain, Size: 1755 bytes --]
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).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH v6 2/8] qmp: Add optional bool "unmap" to drive-mirror
2015-06-02 17:28 ` Eric Blake
@ 2015-06-03 7:13 ` Fam Zheng
0 siblings, 0 replies; 17+ messages in thread
From: Fam Zheng @ 2015-06-03 7:13 UTC (permalink / raw)
To: Eric Blake
Cc: Kevin Wolf, qemu-block, Jeff Cody, qemu-devel, qemu-stable,
Stefan Hajnoczi, pbonzini, jsnow, wangxiaolong
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH v6 3/8] mirror: Do zero write on target if sectors not allocated
2015-05-28 5:29 [Qemu-devel] [PATCH v6 0/8] block: Mirror discarded sectors Fam Zheng
2015-05-28 5:29 ` [Qemu-devel] [PATCH v6 1/8] block: Add bdrv_get_block_status_above Fam Zheng
2015-05-28 5:29 ` [Qemu-devel] [PATCH v6 2/8] qmp: Add optional bool "unmap" to drive-mirror Fam Zheng
@ 2015-05-28 5:29 ` Fam Zheng
2015-05-28 8:28 ` Paolo Bonzini
2015-05-28 5:29 ` [Qemu-devel] [PATCH v6 4/8] block: Fix dirty bitmap in bdrv_co_discard Fam Zheng
` (5 subsequent siblings)
8 siblings, 1 reply; 17+ messages in thread
From: Fam Zheng @ 2015-05-28 5:29 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, qemu-block, Jeff Cody, qemu-stable, Stefan Hajnoczi,
pbonzini, jsnow, wangxiaolong
If guest discards a source cluster, mirroring with bdrv_aio_readv is overkill.
Some protocols do zero upon discard, where it's best to use
bdrv_aio_write_zeroes, otherwise, bdrv_aio_discard will be enough.
Signed-off-by: Fam Zheng <famz@redhat.com>
---
block/mirror.c | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index 85995b2..ba33254 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -164,6 +164,8 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
int64_t end, sector_num, next_chunk, next_sector, hbitmap_next_sector;
uint64_t delay_ns = 0;
MirrorOp *op;
+ int pnum;
+ int64_t ret;
s->sector_num = hbitmap_iter_next(&s->hbi);
if (s->sector_num < 0) {
@@ -290,8 +292,22 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
s->in_flight++;
s->sectors_in_flight += nb_sectors;
trace_mirror_one_iteration(s, sector_num, nb_sectors);
- bdrv_aio_readv(source, sector_num, &op->qiov, nb_sectors,
- mirror_read_complete, op);
+
+ ret = bdrv_get_block_status_above(source, NULL, sector_num,
+ nb_sectors, &pnum);
+ if (ret < 0 || pnum < nb_sectors ||
+ (ret & BDRV_BLOCK_DATA && !(ret & BDRV_BLOCK_ZERO))) {
+ bdrv_aio_readv(source, sector_num, &op->qiov, nb_sectors,
+ mirror_read_complete, op);
+ } else if (ret & BDRV_BLOCK_ZERO) {
+ bdrv_aio_write_zeroes(s->target, sector_num, op->nb_sectors,
+ s->unmap ? BDRV_REQ_MAY_UNMAP : 0,
+ mirror_write_complete, op);
+ } else {
+ assert(!(ret & BDRV_BLOCK_DATA));
+ bdrv_aio_discard(s->target, sector_num, op->nb_sectors,
+ mirror_write_complete, op);
+ }
return delay_ns;
}
--
2.4.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH v6 3/8] mirror: Do zero write on target if sectors not allocated
2015-05-28 5:29 ` [Qemu-devel] [PATCH v6 3/8] mirror: Do zero write on target if sectors not allocated Fam Zheng
@ 2015-05-28 8:28 ` Paolo Bonzini
0 siblings, 0 replies; 17+ messages in thread
From: Paolo Bonzini @ 2015-05-28 8:28 UTC (permalink / raw)
To: Fam Zheng, qemu-devel
Cc: Kevin Wolf, qemu-block, Jeff Cody, qemu-stable, Stefan Hajnoczi,
jsnow, wangxiaolong
On 28/05/2015 07:29, Fam Zheng wrote:
> If guest discards a source cluster, mirroring with bdrv_aio_readv is overkill.
> Some protocols do zero upon discard, where it's best to use
> bdrv_aio_write_zeroes, otherwise, bdrv_aio_discard will be enough.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> block/mirror.c | 20 ++++++++++++++++++--
> 1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index 85995b2..ba33254 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -164,6 +164,8 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
> int64_t end, sector_num, next_chunk, next_sector, hbitmap_next_sector;
> uint64_t delay_ns = 0;
> MirrorOp *op;
> + int pnum;
> + int64_t ret;
>
> s->sector_num = hbitmap_iter_next(&s->hbi);
> if (s->sector_num < 0) {
> @@ -290,8 +292,22 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
> s->in_flight++;
> s->sectors_in_flight += nb_sectors;
> trace_mirror_one_iteration(s, sector_num, nb_sectors);
> - bdrv_aio_readv(source, sector_num, &op->qiov, nb_sectors,
> - mirror_read_complete, op);
> +
> + ret = bdrv_get_block_status_above(source, NULL, sector_num,
> + nb_sectors, &pnum);
> + if (ret < 0 || pnum < nb_sectors ||
> + (ret & BDRV_BLOCK_DATA && !(ret & BDRV_BLOCK_ZERO))) {
> + bdrv_aio_readv(source, sector_num, &op->qiov, nb_sectors,
> + mirror_read_complete, op);
> + } else if (ret & BDRV_BLOCK_ZERO) {
> + bdrv_aio_write_zeroes(s->target, sector_num, op->nb_sectors,
> + s->unmap ? BDRV_REQ_MAY_UNMAP : 0,
> + mirror_write_complete, op);
> + } else {
> + assert(!(ret & BDRV_BLOCK_DATA));
> + bdrv_aio_discard(s->target, sector_num, op->nb_sectors,
> + mirror_write_complete, op);
> + }
> return delay_ns;
> }
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH v6 4/8] block: Fix dirty bitmap in bdrv_co_discard
2015-05-28 5:29 [Qemu-devel] [PATCH v6 0/8] block: Mirror discarded sectors Fam Zheng
` (2 preceding siblings ...)
2015-05-28 5:29 ` [Qemu-devel] [PATCH v6 3/8] mirror: Do zero write on target if sectors not allocated Fam Zheng
@ 2015-05-28 5:29 ` Fam Zheng
2015-05-28 5:29 ` [Qemu-devel] [PATCH v6 5/8] block: Remove bdrv_reset_dirty Fam Zheng
` (4 subsequent siblings)
8 siblings, 0 replies; 17+ messages in thread
From: Fam Zheng @ 2015-05-28 5:29 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, qemu-block, Jeff Cody, qemu-stable, Stefan Hajnoczi,
pbonzini, jsnow, wangxiaolong
Unsetting dirty globally with discard is not very correct. The discard may zero
out sectors (depending on can_write_zeroes_with_unmap), we should replicate
this change to destination side to make sure that the guest sees the same data.
Calling bdrv_reset_dirty also troubles mirror job because the hbitmap iterator
doesn't expect unsetting of bits after current position.
So let's do it the opposite way which fixes both problems: set the dirty bits
if we are to discard it.
Reported-by: wangxiaolong@ucloud.cn
Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block/io.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/io.c b/block/io.c
index 41463a6..a71346c 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2440,8 +2440,6 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
return -EPERM;
}
- bdrv_reset_dirty(bs, sector_num, nb_sectors);
-
/* Do nothing if disabled. */
if (!(bs->open_flags & BDRV_O_UNMAP)) {
return 0;
@@ -2451,6 +2449,8 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
return 0;
}
+ bdrv_set_dirty(bs, sector_num, nb_sectors);
+
max_discard = MIN_NON_ZERO(bs->bl.max_discard, BDRV_REQUEST_MAX_SECTORS);
while (nb_sectors > 0) {
int ret;
--
2.4.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [Qemu-devel] [PATCH v6 5/8] block: Remove bdrv_reset_dirty
2015-05-28 5:29 [Qemu-devel] [PATCH v6 0/8] block: Mirror discarded sectors Fam Zheng
` (3 preceding siblings ...)
2015-05-28 5:29 ` [Qemu-devel] [PATCH v6 4/8] block: Fix dirty bitmap in bdrv_co_discard Fam Zheng
@ 2015-05-28 5:29 ` Fam Zheng
2015-05-28 5:29 ` [Qemu-devel] [PATCH v6 6/8] qemu-iotests: Make block job methods common Fam Zheng
` (3 subsequent siblings)
8 siblings, 0 replies; 17+ messages in thread
From: Fam Zheng @ 2015-05-28 5:29 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, qemu-block, Jeff Cody, qemu-stable, Stefan Hajnoczi,
pbonzini, jsnow, wangxiaolong
Using this function would always be wrong because a dirty bitmap must
have a specific owner that consumes the dirty bits and calls
bdrv_reset_dirty_bitmap().
Remove the unused function to avoid future misuse.
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
block.c | 12 ------------
include/block/block_int.h | 2 --
2 files changed, 14 deletions(-)
diff --git a/block.c b/block.c
index f42d70e..f6ee9db 100644
--- a/block.c
+++ b/block.c
@@ -3336,18 +3336,6 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
}
}
-void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector,
- int nr_sectors)
-{
- BdrvDirtyBitmap *bitmap;
- QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
- if (!bdrv_dirty_bitmap_enabled(bitmap)) {
- continue;
- }
- hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
- }
-}
-
/**
* Advance an HBitmapIter to an arbitrary offset.
*/
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 4e0d700..459fe1c 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -640,7 +640,5 @@ bool blk_dev_is_medium_locked(BlockBackend *blk);
void blk_dev_resize_cb(BlockBackend *blk);
void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
-void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector,
- int nr_sectors);
#endif /* BLOCK_INT_H */
--
2.4.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [Qemu-devel] [PATCH v6 6/8] qemu-iotests: Make block job methods common
2015-05-28 5:29 [Qemu-devel] [PATCH v6 0/8] block: Mirror discarded sectors Fam Zheng
` (4 preceding siblings ...)
2015-05-28 5:29 ` [Qemu-devel] [PATCH v6 5/8] block: Remove bdrv_reset_dirty Fam Zheng
@ 2015-05-28 5:29 ` Fam Zheng
2015-05-28 5:29 ` [Qemu-devel] [PATCH v6 7/8] qemu-iotests: Add test case for mirror with unmap Fam Zheng
` (2 subsequent siblings)
8 siblings, 0 replies; 17+ messages in thread
From: Fam Zheng @ 2015-05-28 5:29 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, qemu-block, Jeff Cody, qemu-stable, Stefan Hajnoczi,
pbonzini, jsnow, wangxiaolong
Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
---
tests/qemu-iotests/041 | 66 ++++++++++---------------------------------
tests/qemu-iotests/iotests.py | 28 ++++++++++++++++++
2 files changed, 43 insertions(+), 51 deletions(-)
diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index 59a8f73..3d46ed7 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -34,38 +34,8 @@ quorum_img3 = os.path.join(iotests.test_dir, 'quorum3.img')
quorum_repair_img = os.path.join(iotests.test_dir, 'quorum_repair.img')
quorum_snapshot_file = os.path.join(iotests.test_dir, 'quorum_snapshot.img')
-class ImageMirroringTestCase(iotests.QMPTestCase):
- '''Abstract base class for image mirroring test cases'''
- def wait_ready(self, drive='drive0'):
- '''Wait until a block job BLOCK_JOB_READY event'''
- ready = False
- while not ready:
- for event in self.vm.get_qmp_events(wait=True):
- if event['event'] == 'BLOCK_JOB_READY':
- self.assert_qmp(event, 'data/type', 'mirror')
- self.assert_qmp(event, 'data/device', drive)
- ready = True
-
- def wait_ready_and_cancel(self, drive='drive0'):
- self.wait_ready(drive=drive)
- event = self.cancel_and_wait(drive=drive)
- self.assertEquals(event['event'], 'BLOCK_JOB_COMPLETED')
- self.assert_qmp(event, 'data/type', 'mirror')
- self.assert_qmp(event, 'data/offset', event['data']['len'])
-
- def complete_and_wait(self, drive='drive0', wait_ready=True):
- '''Complete a block job and wait for it to finish'''
- if wait_ready:
- self.wait_ready(drive=drive)
-
- result = self.vm.qmp('block-job-complete', device=drive)
- self.assert_qmp(result, 'return', {})
-
- event = self.wait_until_completed(drive=drive)
- self.assert_qmp(event, 'data/type', 'mirror')
-
-class TestSingleDrive(ImageMirroringTestCase):
+class TestSingleDrive(iotests.QMPTestCase):
image_len = 1 * 1024 * 1024 # MB
def setUp(self):
@@ -221,17 +191,9 @@ class TestSingleDriveUnalignedLength(TestSingleDrive):
test_small_buffer2 = None
test_large_cluster = None
-class TestMirrorNoBacking(ImageMirroringTestCase):
+class TestMirrorNoBacking(iotests.QMPTestCase):
image_len = 2 * 1024 * 1024 # MB
- def complete_and_wait(self, drive='drive0', wait_ready=True):
- iotests.create_image(target_backing_img, TestMirrorNoBacking.image_len)
- return ImageMirroringTestCase.complete_and_wait(self, drive, wait_ready)
-
- def compare_images(self, img1, img2):
- iotests.create_image(target_backing_img, TestMirrorNoBacking.image_len)
- return iotests.compare_images(img1, img2)
-
def setUp(self):
iotests.create_image(backing_img, TestMirrorNoBacking.image_len)
qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, test_img)
@@ -242,7 +204,10 @@ class TestMirrorNoBacking(ImageMirroringTestCase):
self.vm.shutdown()
os.remove(test_img)
os.remove(backing_img)
- os.remove(target_backing_img)
+ try:
+ os.remove(target_backing_img)
+ except:
+ pass
os.remove(target_img)
def test_complete(self):
@@ -257,7 +222,7 @@ class TestMirrorNoBacking(ImageMirroringTestCase):
result = self.vm.qmp('query-block')
self.assert_qmp(result, 'return[0]/inserted/file', target_img)
self.vm.shutdown()
- self.assertTrue(self.compare_images(test_img, target_img),
+ self.assertTrue(iotests.compare_images(test_img, target_img),
'target image does not match source after mirroring')
def test_cancel(self):
@@ -272,7 +237,7 @@ class TestMirrorNoBacking(ImageMirroringTestCase):
result = self.vm.qmp('query-block')
self.assert_qmp(result, 'return[0]/inserted/file', test_img)
self.vm.shutdown()
- self.assertTrue(self.compare_images(test_img, target_img),
+ self.assertTrue(iotests.compare_images(test_img, target_img),
'target image does not match source after mirroring')
def test_large_cluster(self):
@@ -283,7 +248,6 @@ class TestMirrorNoBacking(ImageMirroringTestCase):
%(TestMirrorNoBacking.image_len), target_backing_img)
qemu_img('create', '-f', iotests.imgfmt, '-o', 'cluster_size=%d,backing_file=%s'
% (TestMirrorNoBacking.image_len, target_backing_img), target_img)
- os.remove(target_backing_img)
result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
mode='existing', target=target_img)
@@ -293,10 +257,10 @@ class TestMirrorNoBacking(ImageMirroringTestCase):
result = self.vm.qmp('query-block')
self.assert_qmp(result, 'return[0]/inserted/file', target_img)
self.vm.shutdown()
- self.assertTrue(self.compare_images(test_img, target_img),
+ self.assertTrue(iotests.compare_images(test_img, target_img),
'target image does not match source after mirroring')
-class TestMirrorResized(ImageMirroringTestCase):
+class TestMirrorResized(iotests.QMPTestCase):
backing_len = 1 * 1024 * 1024 # MB
image_len = 2 * 1024 * 1024 # MB
@@ -344,7 +308,7 @@ class TestMirrorResized(ImageMirroringTestCase):
self.assertTrue(iotests.compare_images(test_img, target_img),
'target image does not match source after mirroring')
-class TestReadErrors(ImageMirroringTestCase):
+class TestReadErrors(iotests.QMPTestCase):
image_len = 2 * 1024 * 1024 # MB
# this should be a multiple of twice the default granularity
@@ -498,7 +462,7 @@ new_state = "1"
self.assert_no_active_block_jobs()
self.vm.shutdown()
-class TestWriteErrors(ImageMirroringTestCase):
+class TestWriteErrors(iotests.QMPTestCase):
image_len = 2 * 1024 * 1024 # MB
# this should be a multiple of twice the default granularity
@@ -624,7 +588,7 @@ new_state = "1"
self.assert_no_active_block_jobs()
self.vm.shutdown()
-class TestSetSpeed(ImageMirroringTestCase):
+class TestSetSpeed(iotests.QMPTestCase):
image_len = 80 * 1024 * 1024 # MB
def setUp(self):
@@ -690,7 +654,7 @@ class TestSetSpeed(ImageMirroringTestCase):
self.wait_ready_and_cancel()
-class TestUnbackedSource(ImageMirroringTestCase):
+class TestUnbackedSource(iotests.QMPTestCase):
image_len = 2 * 1024 * 1024 # MB
def setUp(self):
@@ -731,7 +695,7 @@ class TestUnbackedSource(ImageMirroringTestCase):
self.complete_and_wait()
self.assert_no_active_block_jobs()
-class TestRepairQuorum(ImageMirroringTestCase):
+class TestRepairQuorum(iotests.QMPTestCase):
""" This class test quorum file repair using drive-mirror.
It's mostly a fork of TestSingleDrive """
image_len = 1 * 1024 * 1024 # MB
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 04a294d..63de726 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -326,6 +326,34 @@ class QMPTestCase(unittest.TestCase):
self.assert_no_active_block_jobs()
return event
+ def wait_ready(self, drive='drive0'):
+ '''Wait until a block job BLOCK_JOB_READY event'''
+ ready = False
+ while not ready:
+ for event in self.vm.get_qmp_events(wait=True):
+ if event['event'] == 'BLOCK_JOB_READY':
+ self.assert_qmp(event, 'data/type', 'mirror')
+ self.assert_qmp(event, 'data/device', drive)
+ ready = True
+
+ def wait_ready_and_cancel(self, drive='drive0'):
+ self.wait_ready(drive=drive)
+ event = self.cancel_and_wait(drive=drive)
+ self.assertEquals(event['event'], 'BLOCK_JOB_COMPLETED')
+ self.assert_qmp(event, 'data/type', 'mirror')
+ self.assert_qmp(event, 'data/offset', event['data']['len'])
+
+ def complete_and_wait(self, drive='drive0', wait_ready=True):
+ '''Complete a block job and wait for it to finish'''
+ if wait_ready:
+ self.wait_ready(drive=drive)
+
+ result = self.vm.qmp('block-job-complete', device=drive)
+ self.assert_qmp(result, 'return', {})
+
+ event = self.wait_until_completed(drive=drive)
+ self.assert_qmp(event, 'data/type', 'mirror')
+
def notrun(reason):
'''Skip this test suite'''
# Each test in qemu-iotests has a number ("seq")
--
2.4.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [Qemu-devel] [PATCH v6 7/8] qemu-iotests: Add test case for mirror with unmap
2015-05-28 5:29 [Qemu-devel] [PATCH v6 0/8] block: Mirror discarded sectors Fam Zheng
` (5 preceding siblings ...)
2015-05-28 5:29 ` [Qemu-devel] [PATCH v6 6/8] qemu-iotests: Make block job methods common Fam Zheng
@ 2015-05-28 5:29 ` Fam Zheng
2015-05-28 8:29 ` Paolo Bonzini
2015-05-28 5:29 ` [Qemu-devel] [PATCH v6 8/8] iotests: Use event_wait in wait_ready Fam Zheng
2015-06-02 16:02 ` [Qemu-devel] [Qemu-block] [PATCH v6 0/8] block: Mirror discarded sectors Stefan Hajnoczi
8 siblings, 1 reply; 17+ messages in thread
From: Fam Zheng @ 2015-05-28 5:29 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, qemu-block, Jeff Cody, qemu-stable, Stefan Hajnoczi,
pbonzini, jsnow, wangxiaolong
This checks that the discard on mirror source that effectively zeroes
data is also reflected by the data of target.
Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
tests/qemu-iotests/132 | 59 ++++++++++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/132.out | 5 ++++
tests/qemu-iotests/group | 1 +
3 files changed, 65 insertions(+)
create mode 100644 tests/qemu-iotests/132
create mode 100644 tests/qemu-iotests/132.out
diff --git a/tests/qemu-iotests/132 b/tests/qemu-iotests/132
new file mode 100644
index 0000000..f53ef6e
--- /dev/null
+++ b/tests/qemu-iotests/132
@@ -0,0 +1,59 @@
+#!/usr/bin/env python
+#
+# Test mirror with unmap
+#
+# Copyright (C) 2015 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+import time
+import os
+import iotests
+from iotests import qemu_img, qemu_io
+
+test_img = os.path.join(iotests.test_dir, 'test.img')
+target_img = os.path.join(iotests.test_dir, 'target.img')
+
+class TestSingleDrive(iotests.QMPTestCase):
+ image_len = 2 * 1024 * 1024 # MB
+
+ def setUp(self):
+ # Write data to the image so we can compare later
+ qemu_img('create', '-f', iotests.imgfmt, test_img, str(TestSingleDrive.image_len))
+ qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 0 2M', test_img)
+
+ self.vm = iotests.VM().add_drive(test_img, 'discard=unmap')
+ self.vm.launch()
+
+ def tearDown(self):
+ self.vm.shutdown()
+ os.remove(test_img)
+ try:
+ os.remove(target_img)
+ except OSError:
+ pass
+
+ def test_mirror_discard(self):
+ result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
+ target=target_img)
+ self.assert_qmp(result, 'return', {})
+ self.vm.hmp_qemu_io('drive0', 'discard 0 64k')
+ self.complete_and_wait('drive0')
+ self.vm.shutdown()
+ self.assertTrue(iotests.compare_images(test_img, target_img),
+ 'target image does not match source after mirroring')
+
+if __name__ == '__main__':
+ iotests.main(supported_fmts=['raw', 'qcow2'])
diff --git a/tests/qemu-iotests/132.out b/tests/qemu-iotests/132.out
new file mode 100644
index 0000000..ae1213e
--- /dev/null
+++ b/tests/qemu-iotests/132.out
@@ -0,0 +1,5 @@
+.
+----------------------------------------------------------------------
+Ran 1 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 0b817ca..757ac1b 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -129,4 +129,5 @@
129 rw auto quick
130 rw auto quick
131 rw auto quick
+132 rw auto quick
134 rw auto quick
--
2.4.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH v6 7/8] qemu-iotests: Add test case for mirror with unmap
2015-05-28 5:29 ` [Qemu-devel] [PATCH v6 7/8] qemu-iotests: Add test case for mirror with unmap Fam Zheng
@ 2015-05-28 8:29 ` Paolo Bonzini
0 siblings, 0 replies; 17+ messages in thread
From: Paolo Bonzini @ 2015-05-28 8:29 UTC (permalink / raw)
To: Fam Zheng, qemu-devel
Cc: Kevin Wolf, qemu-block, Jeff Cody, qemu-stable, Stefan Hajnoczi,
jsnow, wangxiaolong
On 28/05/2015 07:29, Fam Zheng wrote:
> This checks that the discard on mirror source that effectively zeroes
> data is also reflected by the data of target.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: John Snow <jsnow@redhat.com>
> ---
> tests/qemu-iotests/132 | 59 ++++++++++++++++++++++++++++++++++++++++++++++
> tests/qemu-iotests/132.out | 5 ++++
> tests/qemu-iotests/group | 1 +
> 3 files changed, 65 insertions(+)
> create mode 100644 tests/qemu-iotests/132
> create mode 100644 tests/qemu-iotests/132.out
>
> diff --git a/tests/qemu-iotests/132 b/tests/qemu-iotests/132
> new file mode 100644
> index 0000000..f53ef6e
> --- /dev/null
> +++ b/tests/qemu-iotests/132
> @@ -0,0 +1,59 @@
> +#!/usr/bin/env python
> +#
> +# Test mirror with unmap
> +#
> +# Copyright (C) 2015 Red Hat, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +import time
> +import os
> +import iotests
> +from iotests import qemu_img, qemu_io
> +
> +test_img = os.path.join(iotests.test_dir, 'test.img')
> +target_img = os.path.join(iotests.test_dir, 'target.img')
> +
> +class TestSingleDrive(iotests.QMPTestCase):
> + image_len = 2 * 1024 * 1024 # MB
> +
> + def setUp(self):
> + # Write data to the image so we can compare later
> + qemu_img('create', '-f', iotests.imgfmt, test_img, str(TestSingleDrive.image_len))
> + qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 0 2M', test_img)
> +
> + self.vm = iotests.VM().add_drive(test_img, 'discard=unmap')
> + self.vm.launch()
> +
> + def tearDown(self):
> + self.vm.shutdown()
> + os.remove(test_img)
> + try:
> + os.remove(target_img)
> + except OSError:
> + pass
> +
> + def test_mirror_discard(self):
> + result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
> + target=target_img)
> + self.assert_qmp(result, 'return', {})
> + self.vm.hmp_qemu_io('drive0', 'discard 0 64k')
> + self.complete_and_wait('drive0')
> + self.vm.shutdown()
> + self.assertTrue(iotests.compare_images(test_img, target_img),
> + 'target image does not match source after mirroring')
> +
> +if __name__ == '__main__':
> + iotests.main(supported_fmts=['raw', 'qcow2'])
> diff --git a/tests/qemu-iotests/132.out b/tests/qemu-iotests/132.out
> new file mode 100644
> index 0000000..ae1213e
> --- /dev/null
> +++ b/tests/qemu-iotests/132.out
> @@ -0,0 +1,5 @@
> +.
> +----------------------------------------------------------------------
> +Ran 1 tests
> +
> +OK
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 0b817ca..757ac1b 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -129,4 +129,5 @@
> 129 rw auto quick
> 130 rw auto quick
> 131 rw auto quick
> +132 rw auto quick
> 134 rw auto quick
>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH v6 8/8] iotests: Use event_wait in wait_ready
2015-05-28 5:29 [Qemu-devel] [PATCH v6 0/8] block: Mirror discarded sectors Fam Zheng
` (6 preceding siblings ...)
2015-05-28 5:29 ` [Qemu-devel] [PATCH v6 7/8] qemu-iotests: Add test case for mirror with unmap Fam Zheng
@ 2015-05-28 5:29 ` Fam Zheng
2015-05-28 8:30 ` Paolo Bonzini
2015-06-02 16:02 ` [Qemu-devel] [Qemu-block] [PATCH v6 0/8] block: Mirror discarded sectors Stefan Hajnoczi
8 siblings, 1 reply; 17+ messages in thread
From: Fam Zheng @ 2015-05-28 5:29 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, qemu-block, Jeff Cody, qemu-stable, Stefan Hajnoczi,
pbonzini, jsnow, wangxiaolong
Only poll the specific type of event we are interested in, to avoid
stealing events that should be consumed by someone else.
Suggested-by: John Snow <jsnow@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
tests/qemu-iotests/iotests.py | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 63de726..8615b10 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -328,13 +328,8 @@ class QMPTestCase(unittest.TestCase):
def wait_ready(self, drive='drive0'):
'''Wait until a block job BLOCK_JOB_READY event'''
- ready = False
- while not ready:
- for event in self.vm.get_qmp_events(wait=True):
- if event['event'] == 'BLOCK_JOB_READY':
- self.assert_qmp(event, 'data/type', 'mirror')
- self.assert_qmp(event, 'data/device', drive)
- ready = True
+ f = {'data': {'type': 'mirror', 'device': drive } }
+ event = self.vm.event_wait(name='BLOCK_JOB_READY', match=f)
def wait_ready_and_cancel(self, drive='drive0'):
self.wait_ready(drive=drive)
--
2.4.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH v6 8/8] iotests: Use event_wait in wait_ready
2015-05-28 5:29 ` [Qemu-devel] [PATCH v6 8/8] iotests: Use event_wait in wait_ready Fam Zheng
@ 2015-05-28 8:30 ` Paolo Bonzini
0 siblings, 0 replies; 17+ messages in thread
From: Paolo Bonzini @ 2015-05-28 8:30 UTC (permalink / raw)
To: Fam Zheng, qemu-devel
Cc: Kevin Wolf, qemu-block, Jeff Cody, qemu-stable, Stefan Hajnoczi,
jsnow, wangxiaolong
On 28/05/2015 07:29, Fam Zheng wrote:
> Only poll the specific type of event we are interested in, to avoid
> stealing events that should be consumed by someone else.
>
> Suggested-by: John Snow <jsnow@redhat.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: John Snow <jsnow@redhat.com>
> ---
> tests/qemu-iotests/iotests.py | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 63de726..8615b10 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -328,13 +328,8 @@ class QMPTestCase(unittest.TestCase):
>
> def wait_ready(self, drive='drive0'):
> '''Wait until a block job BLOCK_JOB_READY event'''
> - ready = False
> - while not ready:
> - for event in self.vm.get_qmp_events(wait=True):
> - if event['event'] == 'BLOCK_JOB_READY':
> - self.assert_qmp(event, 'data/type', 'mirror')
> - self.assert_qmp(event, 'data/device', drive)
> - ready = True
> + f = {'data': {'type': 'mirror', 'device': drive } }
> + event = self.vm.event_wait(name='BLOCK_JOB_READY', match=f)
>
> def wait_ready_and_cancel(self, drive='drive0'):
> self.wait_ready(drive=drive)
>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v6 0/8] block: Mirror discarded sectors
2015-05-28 5:29 [Qemu-devel] [PATCH v6 0/8] block: Mirror discarded sectors Fam Zheng
` (7 preceding siblings ...)
2015-05-28 5:29 ` [Qemu-devel] [PATCH v6 8/8] iotests: Use event_wait in wait_ready Fam Zheng
@ 2015-06-02 16:02 ` Stefan Hajnoczi
8 siblings, 0 replies; 17+ messages in thread
From: Stefan Hajnoczi @ 2015-06-02 16:02 UTC (permalink / raw)
To: Fam Zheng
Cc: qemu-block, qemu-devel, qemu-stable, Stefan Hajnoczi, pbonzini,
wangxiaolong
[-- Attachment #1: Type: text/plain, Size: 2590 bytes --]
On Thu, May 28, 2015 at 01:29:42PM +0800, Fam Zheng wrote:
> v6: Fix pnum in bdrv_get_block_status_above. [Paolo]
>
> v5: Rewrite patch 1.
> Address Eric's comments on patch 3.
> Add Eric's rev-by to patches 2 & 4.
> Check BDRV_BLOCK_DATA in patch 3. [Paolo]
>
> This fixes the mirror assert failure reported by wangxiaolong:
>
> https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg04458.html
>
> The direct cause is that hbitmap code couldn't handle unset of bits *after*
> iterator's current position. We could fix that, but the bdrv_reset_dirty() call
> is more questionable:
>
> Before, if guest discarded some sectors during migration, it could see
> different data after moving to dest side, depending on block backends of the
> src and the dest. This is IMO worse than mirroring the actual reading as done
> in this series, because we don't know what the guest is doing.
>
> For example if a guest first issues WRITE SAME to wipe out the area then issues
> UNMAP to discard it, just to get rid of some sensitive data completely, we may
> miss both operations and leave stale data on dest image.
>
>
> Fam Zheng (8):
> block: Add bdrv_get_block_status_above
> qmp: Add optional bool "unmap" to drive-mirror
> mirror: Do zero write on target if sectors not allocated
> block: Fix dirty bitmap in bdrv_co_discard
> block: Remove bdrv_reset_dirty
> qemu-iotests: Make block job methods common
> qemu-iotests: Add test case for mirror with unmap
> iotests: Use event_wait in wait_ready
>
> block.c | 12 --------
> block/io.c | 60 ++++++++++++++++++++++++++++++---------
> block/mirror.c | 27 +++++++++++++++---
> blockdev.c | 5 ++++
> hmp.c | 2 +-
> include/block/block.h | 4 +++
> include/block/block_int.h | 4 +--
> qapi/block-core.json | 8 +++++-
> qmp-commands.hx | 3 ++
> tests/qemu-iotests/041 | 66 ++++++++++---------------------------------
> tests/qemu-iotests/132 | 59 ++++++++++++++++++++++++++++++++++++++
> tests/qemu-iotests/132.out | 5 ++++
> tests/qemu-iotests/group | 1 +
> tests/qemu-iotests/iotests.py | 23 +++++++++++++++
> 14 files changed, 195 insertions(+), 84 deletions(-)
> create mode 100644 tests/qemu-iotests/132
> create mode 100644 tests/qemu-iotests/132.out
>
> --
> 2.4.1
>
>
Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block
Stefan
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread