* [Qemu-devel] [PATCH v7 1/8] block: Add bdrv_get_block_status_above
2015-06-08 5:56 [Qemu-devel] [PATCH v7 0/8] block: Mirror discarded sectors Fam Zheng
@ 2015-06-08 5:56 ` Fam Zheng
2015-06-08 5:56 ` [Qemu-devel] [PATCH v7 2/8] qmp: Add optional bool "unmap" to drive-mirror Fam Zheng
` (8 subsequent siblings)
9 siblings, 0 replies; 24+ messages in thread
From: Fam Zheng @ 2015-06-08 5:56 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 f7680b6..4d9b555 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.2
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [Qemu-devel] [PATCH v7 2/8] qmp: Add optional bool "unmap" to drive-mirror
2015-06-08 5:56 [Qemu-devel] [PATCH v7 0/8] block: Mirror discarded sectors Fam Zheng
2015-06-08 5:56 ` [Qemu-devel] [PATCH v7 1/8] block: Add bdrv_get_block_status_above Fam Zheng
@ 2015-06-08 5:56 ` Fam Zheng
2015-06-08 14:51 ` Eric Blake
2015-06-08 5:56 ` [Qemu-devel] [PATCH v7 3/8] mirror: Do zero write on target if sectors not allocated Fam Zheng
` (7 subsequent siblings)
9 siblings, 1 reply; 24+ messages in thread
From: Fam Zheng @ 2015-06-08 5:56 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 | 8 ++++++--
blockdev.c | 5 +++++
hmp.c | 2 +-
include/block/block_int.h | 2 ++
qapi/block-core.json | 8 +++++++-
qmp-commands.hx | 3 +++
6 files changed, 24 insertions(+), 4 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index 58f391a..d2515c7 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,
@@ -685,6 +687,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
s->base = base;
s->granularity = granularity;
s->buf_size = MAX(buf_size, granularity);
+ s->unmap = unmap;
s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
if (!s->dirty_bitmap) {
@@ -703,6 +706,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 +721,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 +769,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 de94a8b..6a45555 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2631,6 +2631,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;
@@ -2662,6 +2663,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",
@@ -2801,6 +2805,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 514f22f..d5b9ebb 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1057,7 +1057,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 8411d4f..71ed838 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -973,6 +973,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
@@ -985,7 +990,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 867a21f..3b33496 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.2
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH v7 2/8] qmp: Add optional bool "unmap" to drive-mirror
2015-06-08 5:56 ` [Qemu-devel] [PATCH v7 2/8] qmp: Add optional bool "unmap" to drive-mirror Fam Zheng
@ 2015-06-08 14:51 ` Eric Blake
2015-06-08 14:54 ` Paolo Bonzini
0 siblings, 1 reply; 24+ messages in thread
From: Eric Blake @ 2015-06-08 14:51 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: 1416 bytes --]
On 06/07/2015 11:56 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>
> ---
> block/mirror.c | 8 ++++++--
> blockdev.c | 5 +++++
> hmp.c | 2 +-
> include/block/block_int.h | 2 ++
> qapi/block-core.json | 8 +++++++-
> qmp-commands.hx | 3 +++
> 6 files changed, 24 insertions(+), 4 deletions(-)
>
> +++ b/qapi/block-core.json
> @@ -973,6 +973,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)
> #
Per the other thread on adding 'detect-zeroes', should we instead be
using an enum here that describes one of several behaviors without an
explosion into multiple knobs?
--
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] 24+ messages in thread
* Re: [Qemu-devel] [PATCH v7 2/8] qmp: Add optional bool "unmap" to drive-mirror
2015-06-08 14:51 ` Eric Blake
@ 2015-06-08 14:54 ` Paolo Bonzini
0 siblings, 0 replies; 24+ messages in thread
From: Paolo Bonzini @ 2015-06-08 14:54 UTC (permalink / raw)
To: Eric Blake, Fam Zheng, qemu-devel
Cc: Kevin Wolf, qemu-block, Jeff Cody, qemu-stable, Stefan Hajnoczi,
jsnow, wangxiaolong
On 08/06/2015 16:51, Eric Blake wrote:
>> > +# @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)
>> > #
> Per the other thread on adding 'detect-zeroes', should we instead be
> using an enum here that describes one of several behaviors without an
> explosion into multiple knobs?
See my answer there. :)
Paolo
^ permalink raw reply [flat|nested] 24+ messages in thread
* [Qemu-devel] [PATCH v7 3/8] mirror: Do zero write on target if sectors not allocated
2015-06-08 5:56 [Qemu-devel] [PATCH v7 0/8] block: Mirror discarded sectors Fam Zheng
2015-06-08 5:56 ` [Qemu-devel] [PATCH v7 1/8] block: Add bdrv_get_block_status_above Fam Zheng
2015-06-08 5:56 ` [Qemu-devel] [PATCH v7 2/8] qmp: Add optional bool "unmap" to drive-mirror Fam Zheng
@ 2015-06-08 5:56 ` Fam Zheng
2015-11-04 18:35 ` Kevin Wolf
2015-06-08 5:56 ` [Qemu-devel] [PATCH v7 4/8] block: Fix dirty bitmap in bdrv_co_discard Fam Zheng
` (6 subsequent siblings)
9 siblings, 1 reply; 24+ messages in thread
From: Fam Zheng @ 2015-06-08 5:56 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 d2515c7..3c38695 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.2
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH v7 3/8] mirror: Do zero write on target if sectors not allocated
2015-06-08 5:56 ` [Qemu-devel] [PATCH v7 3/8] mirror: Do zero write on target if sectors not allocated Fam Zheng
@ 2015-11-04 18:35 ` Kevin Wolf
2015-11-05 5:42 ` Fam Zheng
0 siblings, 1 reply; 24+ messages in thread
From: Kevin Wolf @ 2015-11-04 18:35 UTC (permalink / raw)
To: Fam Zheng
Cc: qemu-block, rjones, Jeff Cody, qemu-devel, qemu-stable,
Stefan Hajnoczi, pbonzini, jsnow, wangxiaolong
Am 08.06.2015 um 07:56 hat Fam Zheng geschrieben:
> 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 d2515c7..3c38695 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 ||
Earlier today I told Richard Jones that qemu-img commit should really
be using zero cluster support in the backing file since 2.4 because I
remembered this commit. Turns out it doesn't actually use it but writes
explicit zeros instead.
The reason is the condition 'pnum < nb_sectors' here, which makes mirror
fall back to explicit writes if bdrv_get_block_status_above() doesn't
return enough sectors (enough being relatively large here, I think in
qemu-img commit it's always the full 10 MB buffer).
In other words, we are ignoring any zero areas smaller than 10 MB!
(What made this worse is that qcow2 had a bug that reports only a single
zero cluster at a time, so it would never report more than 10 MB, even
if the image was completely zeroed. I've sent a fix for that one.)
In order to fix this, we'll probably need to move the call to
bdrv_get_block_status_above() before actually allocating memory and
all that for the full nb_chunks. We should detect zeros on the usual
block job granularity (64k by default, I think).
> + (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;
> }
Paolo also noticed that there's no reason at all to allocate buffers
and a qiov for the write_zeroes and discard cases.
Kevin
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH v7 3/8] mirror: Do zero write on target if sectors not allocated
2015-11-04 18:35 ` Kevin Wolf
@ 2015-11-05 5:42 ` Fam Zheng
2015-11-05 9:55 ` Kevin Wolf
0 siblings, 1 reply; 24+ messages in thread
From: Fam Zheng @ 2015-11-05 5:42 UTC (permalink / raw)
To: Kevin Wolf
Cc: qemu-block, rjones, Jeff Cody, qemu-devel, qemu-stable,
Stefan Hajnoczi, pbonzini, jsnow, wangxiaolong
On Wed, 11/04 19:35, Kevin Wolf wrote:
> Am 08.06.2015 um 07:56 hat Fam Zheng geschrieben:
> > 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 d2515c7..3c38695 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 ||
>
> Earlier today I told Richard Jones that qemu-img commit should really
> be using zero cluster support in the backing file since 2.4 because I
> remembered this commit. Turns out it doesn't actually use it but writes
> explicit zeros instead.
>
> The reason is the condition 'pnum < nb_sectors' here, which makes mirror
> fall back to explicit writes if bdrv_get_block_status_above() doesn't
> return enough sectors (enough being relatively large here, I think in
> qemu-img commit it's always the full 10 MB buffer).
>
> In other words, we are ignoring any zero areas smaller than 10 MB!
>
> (What made this worse is that qcow2 had a bug that reports only a single
> zero cluster at a time, so it would never report more than 10 MB, even
> if the image was completely zeroed. I've sent a fix for that one.)
>
> In order to fix this, we'll probably need to move the call to
> bdrv_get_block_status_above() before actually allocating memory and
> all that for the full nb_chunks. We should detect zeros on the usual
> block job granularity (64k by default, I think).
>
> > + (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;
> > }
>
> Paolo also noticed that there's no reason at all to allocate buffers
> and a qiov for the write_zeroes and discard cases.
I'll write a patch to address these. Thanks!
Fam
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH v7 3/8] mirror: Do zero write on target if sectors not allocated
2015-11-05 5:42 ` Fam Zheng
@ 2015-11-05 9:55 ` Kevin Wolf
0 siblings, 0 replies; 24+ messages in thread
From: Kevin Wolf @ 2015-11-05 9:55 UTC (permalink / raw)
To: Fam Zheng
Cc: qemu-block, rjones, Jeff Cody, qemu-devel, qemu-stable,
Stefan Hajnoczi, pbonzini, jsnow, wangxiaolong
Am 05.11.2015 um 06:42 hat Fam Zheng geschrieben:
> On Wed, 11/04 19:35, Kevin Wolf wrote:
> > Am 08.06.2015 um 07:56 hat Fam Zheng geschrieben:
> > > 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 d2515c7..3c38695 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 ||
> >
> > Earlier today I told Richard Jones that qemu-img commit should really
> > be using zero cluster support in the backing file since 2.4 because I
> > remembered this commit. Turns out it doesn't actually use it but writes
> > explicit zeros instead.
> >
> > The reason is the condition 'pnum < nb_sectors' here, which makes mirror
> > fall back to explicit writes if bdrv_get_block_status_above() doesn't
> > return enough sectors (enough being relatively large here, I think in
> > qemu-img commit it's always the full 10 MB buffer).
> >
> > In other words, we are ignoring any zero areas smaller than 10 MB!
> >
> > (What made this worse is that qcow2 had a bug that reports only a single
> > zero cluster at a time, so it would never report more than 10 MB, even
> > if the image was completely zeroed. I've sent a fix for that one.)
> >
> > In order to fix this, we'll probably need to move the call to
> > bdrv_get_block_status_above() before actually allocating memory and
> > all that for the full nb_chunks. We should detect zeros on the usual
> > block job granularity (64k by default, I think).
> >
> > > + (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;
> > > }
> >
> > Paolo also noticed that there's no reason at all to allocate buffers
> > and a qiov for the write_zeroes and discard cases.
>
> I'll write a patch to address these. Thanks!
Thanks, Fam!
Kevin
^ permalink raw reply [flat|nested] 24+ messages in thread
* [Qemu-devel] [PATCH v7 4/8] block: Fix dirty bitmap in bdrv_co_discard
2015-06-08 5:56 [Qemu-devel] [PATCH v7 0/8] block: Mirror discarded sectors Fam Zheng
` (2 preceding siblings ...)
2015-06-08 5:56 ` [Qemu-devel] [PATCH v7 3/8] mirror: Do zero write on target if sectors not allocated Fam Zheng
@ 2015-06-08 5:56 ` Fam Zheng
2015-06-08 5:56 ` [Qemu-devel] [PATCH v7 5/8] block: Remove bdrv_reset_dirty Fam Zheng
` (5 subsequent siblings)
9 siblings, 0 replies; 24+ messages in thread
From: Fam Zheng @ 2015-06-08 5:56 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.2
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [Qemu-devel] [PATCH v7 5/8] block: Remove bdrv_reset_dirty
2015-06-08 5:56 [Qemu-devel] [PATCH v7 0/8] block: Mirror discarded sectors Fam Zheng
` (3 preceding siblings ...)
2015-06-08 5:56 ` [Qemu-devel] [PATCH v7 4/8] block: Fix dirty bitmap in bdrv_co_discard Fam Zheng
@ 2015-06-08 5:56 ` Fam Zheng
2015-06-08 5:56 ` [Qemu-devel] [PATCH v7 6/8] qemu-iotests: Make block job methods common Fam Zheng
` (4 subsequent siblings)
9 siblings, 0 replies; 24+ messages in thread
From: Fam Zheng @ 2015-06-08 5:56 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 2b9ceae..9890707 100644
--- a/block.c
+++ b/block.c
@@ -3347,18 +3347,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.2
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [Qemu-devel] [PATCH v7 6/8] qemu-iotests: Make block job methods common
2015-06-08 5:56 [Qemu-devel] [PATCH v7 0/8] block: Mirror discarded sectors Fam Zheng
` (4 preceding siblings ...)
2015-06-08 5:56 ` [Qemu-devel] [PATCH v7 5/8] block: Remove bdrv_reset_dirty Fam Zheng
@ 2015-06-08 5:56 ` Fam Zheng
2015-06-08 5:56 ` [Qemu-devel] [PATCH v7 7/8] qemu-iotests: Add test case for mirror with unmap Fam Zheng
` (3 subsequent siblings)
9 siblings, 0 replies; 24+ messages in thread
From: Fam Zheng @ 2015-06-08 5:56 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.2
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [Qemu-devel] [PATCH v7 7/8] qemu-iotests: Add test case for mirror with unmap
2015-06-08 5:56 [Qemu-devel] [PATCH v7 0/8] block: Mirror discarded sectors Fam Zheng
` (5 preceding siblings ...)
2015-06-08 5:56 ` [Qemu-devel] [PATCH v7 6/8] qemu-iotests: Make block job methods common Fam Zheng
@ 2015-06-08 5:56 ` Fam Zheng
2015-06-08 5:56 ` [Qemu-devel] [PATCH v7 8/8] iotests: Use event_wait in wait_ready Fam Zheng
` (2 subsequent siblings)
9 siblings, 0 replies; 24+ messages in thread
From: Fam Zheng @ 2015-06-08 5:56 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.2
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [Qemu-devel] [PATCH v7 8/8] iotests: Use event_wait in wait_ready
2015-06-08 5:56 [Qemu-devel] [PATCH v7 0/8] block: Mirror discarded sectors Fam Zheng
` (6 preceding siblings ...)
2015-06-08 5:56 ` [Qemu-devel] [PATCH v7 7/8] qemu-iotests: Add test case for mirror with unmap Fam Zheng
@ 2015-06-08 5:56 ` Fam Zheng
2015-06-08 13:02 ` [Qemu-devel] [PATCH v7 0/8] block: Mirror discarded sectors Stefan Hajnoczi
2015-06-26 13:19 ` [Qemu-devel] " Stefan Hajnoczi
9 siblings, 0 replies; 24+ messages in thread
From: Fam Zheng @ 2015-06-08 5:56 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.2
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH v7 0/8] block: Mirror discarded sectors
2015-06-08 5:56 [Qemu-devel] [PATCH v7 0/8] block: Mirror discarded sectors Fam Zheng
` (7 preceding siblings ...)
2015-06-08 5:56 ` [Qemu-devel] [PATCH v7 8/8] iotests: Use event_wait in wait_ready Fam Zheng
@ 2015-06-08 13:02 ` Stefan Hajnoczi
2015-06-11 8:29 ` Fam Zheng
2015-06-26 13:19 ` [Qemu-devel] " Stefan Hajnoczi
9 siblings, 1 reply; 24+ messages in thread
From: Stefan Hajnoczi @ 2015-06-08 13:02 UTC (permalink / raw)
To: Fam Zheng
Cc: Kevin Wolf, qemu-block, Jeff Cody, qemu-devel, qemu-stable,
pbonzini, jsnow, wangxiaolong
[-- Attachment #1: Type: text/plain, Size: 2634 bytes --]
On Mon, Jun 08, 2015 at 01:56:06PM +0800, Fam Zheng wrote:
> v7: Fix the lost assignment of s->unmap.
>
> 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 | 28 +++++++++++++++---
> 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, 196 insertions(+), 84 deletions(-)
> create mode 100644 tests/qemu-iotests/132
> create mode 100644 tests/qemu-iotests/132.out
>
> --
> 2.4.2
>
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] 24+ messages in thread
* Re: [Qemu-devel] [PATCH v7 0/8] block: Mirror discarded sectors
2015-06-08 13:02 ` [Qemu-devel] [PATCH v7 0/8] block: Mirror discarded sectors Stefan Hajnoczi
@ 2015-06-11 8:29 ` Fam Zheng
2015-06-24 9:08 ` [Qemu-devel] [Qemu-stable] " Fam Zheng
0 siblings, 1 reply; 24+ messages in thread
From: Fam Zheng @ 2015-06-11 8:29 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Kevin Wolf, qemu-block, Jeff Cody, qemu-devel, qemu-stable,
pbonzini, jsnow, wangxiaolong
On Mon, 06/08 14:02, Stefan Hajnoczi wrote:
> On Mon, Jun 08, 2015 at 01:56:06PM +0800, Fam Zheng wrote:
> > v7: Fix the lost assignment of s->unmap.
> >
> > 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 | 28 +++++++++++++++---
> > 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, 196 insertions(+), 84 deletions(-)
> > create mode 100644 tests/qemu-iotests/132
> > create mode 100644 tests/qemu-iotests/132.out
> >
> > --
> > 2.4.2
> >
>
> Thanks, applied to my block tree:
> https://github.com/stefanha/qemu/commits/block
Stefan,
The only controversial patches are the qmp/drive-mirror ones (1-3), while
patches 4-8 are still useful on their own: they fix the mentioned crash and
improve iotests.
Shall we merge the second half (of course none of them depend on 1-3) now that
softfreeze is approaching?
Fam
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [Qemu-stable] [PATCH v7 0/8] block: Mirror discarded sectors
2015-06-11 8:29 ` Fam Zheng
@ 2015-06-24 9:08 ` Fam Zheng
2015-06-24 17:01 ` Paolo Bonzini
0 siblings, 1 reply; 24+ messages in thread
From: Fam Zheng @ 2015-06-24 9:08 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Kevin Wolf, qemu-block, jsnow, Jeff Cody, qemu-devel,
qemu-stable, pbonzini, wangxiaolong
On Thu, 06/11 16:29, Fam Zheng wrote:
> On Mon, 06/08 14:02, Stefan Hajnoczi wrote:
> > On Mon, Jun 08, 2015 at 01:56:06PM +0800, Fam Zheng wrote:
> > > v7: Fix the lost assignment of s->unmap.
> > >
> > > 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 | 28 +++++++++++++++---
> > > 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, 196 insertions(+), 84 deletions(-)
> > > create mode 100644 tests/qemu-iotests/132
> > > create mode 100644 tests/qemu-iotests/132.out
> > >
> > > --
> > > 2.4.2
> > >
> >
> > Thanks, applied to my block tree:
> > https://github.com/stefanha/qemu/commits/block
>
> Stefan,
>
> The only controversial patches are the qmp/drive-mirror ones (1-3), while
> patches 4-8 are still useful on their own: they fix the mentioned crash and
> improve iotests.
>
> Shall we merge the second half (of course none of them depend on 1-3) now that
> softfreeze is approaching?
Stefan, would you consider applying patches 4-8?
Fam
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [Qemu-stable] [PATCH v7 0/8] block: Mirror discarded sectors
2015-06-24 9:08 ` [Qemu-devel] [Qemu-stable] " Fam Zheng
@ 2015-06-24 17:01 ` Paolo Bonzini
2015-06-25 1:02 ` Fam Zheng
0 siblings, 1 reply; 24+ messages in thread
From: Paolo Bonzini @ 2015-06-24 17:01 UTC (permalink / raw)
To: Fam Zheng, Stefan Hajnoczi
Cc: Kevin Wolf, qemu-block, jsnow, Jeff Cody, qemu-devel,
qemu-stable, wangxiaolong
On 24/06/2015 11:08, Fam Zheng wrote:
>> Stefan,
>>
>> The only controversial patches are the qmp/drive-mirror ones (1-3), while
>> patches 4-8 are still useful on their own: they fix the mentioned crash and
>> improve iotests.
>>
>> Shall we merge the second half (of course none of them depend on 1-3) now that
>> softfreeze is approaching?
>
> Stefan, would you consider applying patches 4-8?
Actually why not apply all of them? Even if blockdev-mirror is a
superior interface in the long run, the current behavior of drive-mirror
can cause images to balloon up to the full size, which is bad.
Extending drive-mirror is okay IMHO for 2.4.
Paolo
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [Qemu-stable] [PATCH v7 0/8] block: Mirror discarded sectors
2015-06-24 17:01 ` Paolo Bonzini
@ 2015-06-25 1:02 ` Fam Zheng
2015-06-25 10:45 ` Fam Zheng
0 siblings, 1 reply; 24+ messages in thread
From: Fam Zheng @ 2015-06-25 1:02 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Kevin Wolf, qemu-block, jsnow, Jeff Cody, qemu-devel,
qemu-stable, Stefan Hajnoczi, wangxiaolong
On Wed, 06/24 19:01, Paolo Bonzini wrote:
>
>
> On 24/06/2015 11:08, Fam Zheng wrote:
> >> Stefan,
> >>
> >> The only controversial patches are the qmp/drive-mirror ones (1-3), while
> >> patches 4-8 are still useful on their own: they fix the mentioned crash and
> >> improve iotests.
> >>
> >> Shall we merge the second half (of course none of them depend on 1-3) now that
> >> softfreeze is approaching?
> >
> > Stefan, would you consider applying patches 4-8?
>
> Actually why not apply all of them? Even if blockdev-mirror is a
> superior interface in the long run, the current behavior of drive-mirror
> can cause images to balloon up to the full size, which is bad.
> Extending drive-mirror is okay IMHO for 2.4.
>
Before we do that, Andrey Korolyov has reported a hang issue with unmap=true,
I'll take a look at it today.
Fam
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [Qemu-stable] [PATCH v7 0/8] block: Mirror discarded sectors
2015-06-25 1:02 ` Fam Zheng
@ 2015-06-25 10:45 ` Fam Zheng
2015-06-26 13:36 ` Alexandre DERUMIER
0 siblings, 1 reply; 24+ messages in thread
From: Fam Zheng @ 2015-06-25 10:45 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Kevin Wolf, qemu-block, Jeff Cody, qemu-devel, qemu-stable,
Stefan Hajnoczi, jsnow, wangxiaolong
On Thu, 06/25 09:02, Fam Zheng wrote:
> On Wed, 06/24 19:01, Paolo Bonzini wrote:
> >
> >
> > On 24/06/2015 11:08, Fam Zheng wrote:
> > >> Stefan,
> > >>
> > >> The only controversial patches are the qmp/drive-mirror ones (1-3), while
> > >> patches 4-8 are still useful on their own: they fix the mentioned crash and
> > >> improve iotests.
> > >>
> > >> Shall we merge the second half (of course none of them depend on 1-3) now that
> > >> softfreeze is approaching?
> > >
> > > Stefan, would you consider applying patches 4-8?
> >
> > Actually why not apply all of them? Even if blockdev-mirror is a
> > superior interface in the long run, the current behavior of drive-mirror
> > can cause images to balloon up to the full size, which is bad.
> > Extending drive-mirror is okay IMHO for 2.4.
> >
>
> Before we do that, Andrey Korolyov has reported a hang issue with unmap=true,
> I'll take a look at it today.
There is no problem, the observasion by Andrey was just that qmp command takes
a few minutes before returning, because he didn't apply
https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg02511.html
Fam
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [Qemu-stable] [PATCH v7 0/8] block: Mirror discarded sectors
2015-06-25 10:45 ` Fam Zheng
@ 2015-06-26 13:36 ` Alexandre DERUMIER
2015-06-26 13:58 ` Alexandre DERUMIER
2015-06-29 1:03 ` Fam Zheng
0 siblings, 2 replies; 24+ messages in thread
From: Alexandre DERUMIER @ 2015-06-26 13:36 UTC (permalink / raw)
To: Fam Zheng
Cc: Kevin Wolf, qemu-block, Jeff Cody, qemu-devel, qemu-stable,
stefanha, pbonzini, jsnow, wangxiaolong
Hi,
>>There is no problem, the observasion by Andrey was just that qmp command takes
>>a few minutes before returning, because he didn't apply
>>
>>https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg02511.html
Is this patch already apply on the block tree ?
With nfs as source storage, it's really slow currently (lseek slow + a lot of nfs ops).
----- Mail original -----
De: "Fam Zheng" <famz@redhat.com>
À: "pbonzini" <pbonzini@redhat.com>
Cc: "Kevin Wolf" <kwolf@redhat.com>, qemu-block@nongnu.org, "Jeff Cody" <jcody@redhat.com>, "qemu-devel" <qemu-devel@nongnu.org>, "qemu-stable" <qemu-stable@nongnu.org>, "stefanha" <stefanha@redhat.com>, jsnow@redhat.com, wangxiaolong@ucloud.cn
Envoyé: Jeudi 25 Juin 2015 12:45:38
Objet: Re: [Qemu-devel] [Qemu-stable] [PATCH v7 0/8] block: Mirror discarded sectors
On Thu, 06/25 09:02, Fam Zheng wrote:
> On Wed, 06/24 19:01, Paolo Bonzini wrote:
> >
> >
> > On 24/06/2015 11:08, Fam Zheng wrote:
> > >> Stefan,
> > >>
> > >> The only controversial patches are the qmp/drive-mirror ones (1-3), while
> > >> patches 4-8 are still useful on their own: they fix the mentioned crash and
> > >> improve iotests.
> > >>
> > >> Shall we merge the second half (of course none of them depend on 1-3) now that
> > >> softfreeze is approaching?
> > >
> > > Stefan, would you consider applying patches 4-8?
> >
> > Actually why not apply all of them? Even if blockdev-mirror is a
> > superior interface in the long run, the current behavior of drive-mirror
> > can cause images to balloon up to the full size, which is bad.
> > Extending drive-mirror is okay IMHO for 2.4.
> >
>
> Before we do that, Andrey Korolyov has reported a hang issue with unmap=true,
> I'll take a look at it today.
There is no problem, the observasion by Andrey was just that qmp command takes
a few minutes before returning, because he didn't apply
https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg02511.html
Fam
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [Qemu-stable] [PATCH v7 0/8] block: Mirror discarded sectors
2015-06-26 13:36 ` Alexandre DERUMIER
@ 2015-06-26 13:58 ` Alexandre DERUMIER
2015-06-29 1:03 ` Fam Zheng
1 sibling, 0 replies; 24+ messages in thread
From: Alexandre DERUMIER @ 2015-06-26 13:58 UTC (permalink / raw)
To: Fam Zheng
Cc: Kevin Wolf, qemu-block, Jeff Cody, qemu-devel, qemu-stable,
stefanha, pbonzini, jsnow, wangxiaolong
>>With nfs as source storage, it's really slow currently (lseek slow + a lot of nfs ops).
it's blocked around 30min for 300GB, with a raw file on a netapp san array through nfs.
----- Mail original -----
De: "aderumier" <aderumier@odiso.com>
À: "Fam Zheng" <famz@redhat.com>
Cc: "Kevin Wolf" <kwolf@redhat.com>, qemu-block@nongnu.org, "Jeff Cody" <jcody@redhat.com>, "qemu-devel" <qemu-devel@nongnu.org>, "qemu-stable" <qemu-stable@nongnu.org>, "stefanha" <stefanha@redhat.com>, "pbonzini" <pbonzini@redhat.com>, jsnow@redhat.com, wangxiaolong@ucloud.cn
Envoyé: Vendredi 26 Juin 2015 15:36:10
Objet: Re: [Qemu-devel] [Qemu-stable] [PATCH v7 0/8] block: Mirror discarded sectors
Hi,
>>There is no problem, the observasion by Andrey was just that qmp command takes
>>a few minutes before returning, because he didn't apply
>>
>>https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg02511.html
Is this patch already apply on the block tree ?
With nfs as source storage, it's really slow currently (lseek slow + a lot of nfs ops).
----- Mail original -----
De: "Fam Zheng" <famz@redhat.com>
À: "pbonzini" <pbonzini@redhat.com>
Cc: "Kevin Wolf" <kwolf@redhat.com>, qemu-block@nongnu.org, "Jeff Cody" <jcody@redhat.com>, "qemu-devel" <qemu-devel@nongnu.org>, "qemu-stable" <qemu-stable@nongnu.org>, "stefanha" <stefanha@redhat.com>, jsnow@redhat.com, wangxiaolong@ucloud.cn
Envoyé: Jeudi 25 Juin 2015 12:45:38
Objet: Re: [Qemu-devel] [Qemu-stable] [PATCH v7 0/8] block: Mirror discarded sectors
On Thu, 06/25 09:02, Fam Zheng wrote:
> On Wed, 06/24 19:01, Paolo Bonzini wrote:
> >
> >
> > On 24/06/2015 11:08, Fam Zheng wrote:
> > >> Stefan,
> > >>
> > >> The only controversial patches are the qmp/drive-mirror ones (1-3), while
> > >> patches 4-8 are still useful on their own: they fix the mentioned crash and
> > >> improve iotests.
> > >>
> > >> Shall we merge the second half (of course none of them depend on 1-3) now that
> > >> softfreeze is approaching?
> > >
> > > Stefan, would you consider applying patches 4-8?
> >
> > Actually why not apply all of them? Even if blockdev-mirror is a
> > superior interface in the long run, the current behavior of drive-mirror
> > can cause images to balloon up to the full size, which is bad.
> > Extending drive-mirror is okay IMHO for 2.4.
> >
>
> Before we do that, Andrey Korolyov has reported a hang issue with unmap=true,
> I'll take a look at it today.
There is no problem, the observasion by Andrey was just that qmp command takes
a few minutes before returning, because he didn't apply
https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg02511.html
Fam
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [Qemu-stable] [PATCH v7 0/8] block: Mirror discarded sectors
2015-06-26 13:36 ` Alexandre DERUMIER
2015-06-26 13:58 ` Alexandre DERUMIER
@ 2015-06-29 1:03 ` Fam Zheng
1 sibling, 0 replies; 24+ messages in thread
From: Fam Zheng @ 2015-06-29 1:03 UTC (permalink / raw)
To: Alexandre DERUMIER
Cc: Kevin Wolf, qemu-block, Jeff Cody, qemu-devel, qemu-stable,
stefanha, pbonzini, jsnow, wangxiaolong
On Fri, 06/26 15:36, Alexandre DERUMIER wrote:
> Hi,
>
> >>There is no problem, the observasion by Andrey was just that qmp command takes
> >>a few minutes before returning, because he didn't apply
> >>
> >>https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg02511.html
>
> Is this patch already apply on the block tree ?
>
> With nfs as source storage, it's really slow currently (lseek slow + a lot of nfs ops).
The patch is waiting for the next PULL in Jeff Cody's tree I think.
Fam
>
>
>
> ----- Mail original -----
> De: "Fam Zheng" <famz@redhat.com>
> À: "pbonzini" <pbonzini@redhat.com>
> Cc: "Kevin Wolf" <kwolf@redhat.com>, qemu-block@nongnu.org, "Jeff Cody" <jcody@redhat.com>, "qemu-devel" <qemu-devel@nongnu.org>, "qemu-stable" <qemu-stable@nongnu.org>, "stefanha" <stefanha@redhat.com>, jsnow@redhat.com, wangxiaolong@ucloud.cn
> Envoyé: Jeudi 25 Juin 2015 12:45:38
> Objet: Re: [Qemu-devel] [Qemu-stable] [PATCH v7 0/8] block: Mirror discarded sectors
>
> On Thu, 06/25 09:02, Fam Zheng wrote:
> > On Wed, 06/24 19:01, Paolo Bonzini wrote:
> > >
> > >
> > > On 24/06/2015 11:08, Fam Zheng wrote:
> > > >> Stefan,
> > > >>
> > > >> The only controversial patches are the qmp/drive-mirror ones (1-3), while
> > > >> patches 4-8 are still useful on their own: they fix the mentioned crash and
> > > >> improve iotests.
> > > >>
> > > >> Shall we merge the second half (of course none of them depend on 1-3) now that
> > > >> softfreeze is approaching?
> > > >
> > > > Stefan, would you consider applying patches 4-8?
> > >
> > > Actually why not apply all of them? Even if blockdev-mirror is a
> > > superior interface in the long run, the current behavior of drive-mirror
> > > can cause images to balloon up to the full size, which is bad.
> > > Extending drive-mirror is okay IMHO for 2.4.
> > >
> >
> > Before we do that, Andrey Korolyov has reported a hang issue with unmap=true,
> > I'll take a look at it today.
>
> There is no problem, the observasion by Andrey was just that qmp command takes
> a few minutes before returning, because he didn't apply
>
> https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg02511.html
>
> Fam
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH v7 0/8] block: Mirror discarded sectors
2015-06-08 5:56 [Qemu-devel] [PATCH v7 0/8] block: Mirror discarded sectors Fam Zheng
` (8 preceding siblings ...)
2015-06-08 13:02 ` [Qemu-devel] [PATCH v7 0/8] block: Mirror discarded sectors Stefan Hajnoczi
@ 2015-06-26 13:19 ` Stefan Hajnoczi
9 siblings, 0 replies; 24+ messages in thread
From: Stefan Hajnoczi @ 2015-06-26 13:19 UTC (permalink / raw)
To: Fam Zheng
Cc: Kevin Wolf, qemu-block, Jeff Cody, qemu-devel, qemu-stable,
pbonzini, jsnow, wangxiaolong
[-- Attachment #1: Type: text/plain, Size: 2687 bytes --]
On Mon, Jun 08, 2015 at 01:56:06PM +0800, Fam Zheng wrote:
> v7: Fix the lost assignment of s->unmap.
>
> 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 | 28 +++++++++++++++---
> 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, 196 insertions(+), 84 deletions(-)
> create mode 100644 tests/qemu-iotests/132
> create mode 100644 tests/qemu-iotests/132.out
>
> --
> 2.4.2
Thanks, applied to my block tree again now that the bool/enum question
has been resolved:
https://github.com/stefanha/qemu/commits/block
Stefan
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread