* [Qemu-devel] [PATCH v12 0/2] mirror: Improve zero write and discard
@ 2016-02-05 2:00 Fam Zheng
2016-02-05 2:00 ` [Qemu-devel] [PATCH v12 1/2] mirror: Rewrite mirror_iteration Fam Zheng
2016-02-05 2:00 ` [Qemu-devel] [PATCH v12 2/2] mirror: Add mirror_wait_for_io Fam Zheng
0 siblings, 2 replies; 9+ messages in thread
From: Fam Zheng @ 2016-02-05 2:00 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, pbonzini, Jeff Cody, qemu-block, mreitz
v12: Treat target cluster size as BDRV_SECTOR_SIZE if bdrv_get_info failed or
doesn't set bdi.cluster_size;
v11: Rebase. The only difference is the "file" parameter of
bdrv_get_block_status_above.
Patch 1 rewrites mirror_iteration. Patch 2 is a small DRY cleaning up.
The main benefit is copying unallocated sectors (both zeroed and discarded)
doesn't go through the iov setup loop, as they don't need it.
Fam Zheng (2):
mirror: Rewrite mirror_iteration
mirror: Add mirror_wait_for_io
block/mirror.c | 353 +++++++++++++++++++++++++++------------------
tests/qemu-iotests/109.out | 80 +++++-----
trace-events | 1 -
3 files changed, 252 insertions(+), 182 deletions(-)
--
2.4.3
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH v12 1/2] mirror: Rewrite mirror_iteration
2016-02-05 2:00 [Qemu-devel] [PATCH v12 0/2] mirror: Improve zero write and discard Fam Zheng
@ 2016-02-05 2:00 ` Fam Zheng
2016-02-06 13:24 ` Max Reitz
2016-02-05 2:00 ` [Qemu-devel] [PATCH v12 2/2] mirror: Add mirror_wait_for_io Fam Zheng
1 sibling, 1 reply; 9+ messages in thread
From: Fam Zheng @ 2016-02-05 2:00 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, pbonzini, Jeff Cody, qemu-block, mreitz
The "pnum < nb_sectors" condition in deciding whether to actually copy
data is unnecessarily strict, and the qiov initialization is
unnecessarily for bdrv_aio_write_zeroes and bdrv_aio_discard.
Rewrite mirror_iteration to fix both flaws.
The output of iotests 109 is updated because we now report the offset
and len slightly differently in mirroring progress.
Signed-off-by: Fam Zheng <famz@redhat.com>
---
block/mirror.c | 335 +++++++++++++++++++++++++++------------------
tests/qemu-iotests/109.out | 80 +++++------
trace-events | 1 -
3 files changed, 243 insertions(+), 173 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index 2c0edfa..48cd0b3 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -47,7 +47,6 @@ typedef struct MirrorBlockJob {
BlockdevOnError on_source_error, on_target_error;
bool synced;
bool should_complete;
- int64_t sector_num;
int64_t granularity;
size_t buf_size;
int64_t bdev_length;
@@ -64,6 +63,8 @@ typedef struct MirrorBlockJob {
int ret;
bool unmap;
bool waiting_for_io;
+ int target_cluster_sectors;
+ int max_iov;
} MirrorBlockJob;
typedef struct MirrorOp {
@@ -159,116 +160,79 @@ static void mirror_read_complete(void *opaque, int ret)
mirror_write_complete, op);
}
-static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
+/* Round sector_num and/or nb_sectors to target cluster if COW is needed, and
+ * return the offset of the adjusted tail sector against original. */
+static int mirror_cow_align(MirrorBlockJob *s,
+ int64_t *sector_num,
+ int *nb_sectors)
+{
+ bool need_cow;
+ int ret = 0;
+ int chunk_sectors = s->granularity >> BDRV_SECTOR_BITS;
+ int64_t align_sector_num = *sector_num;
+ int align_nb_sectors = *nb_sectors;
+ int max_sectors = chunk_sectors * s->max_iov;
+
+ need_cow = !test_bit(*sector_num / chunk_sectors, s->cow_bitmap);
+ need_cow |= !test_bit((*sector_num + *nb_sectors - 1) / chunk_sectors,
+ s->cow_bitmap);
+ if (need_cow) {
+ bdrv_round_to_clusters(s->target, *sector_num, *nb_sectors,
+ &align_sector_num, &align_nb_sectors);
+ }
+
+ if (align_nb_sectors > max_sectors) {
+ align_nb_sectors = max_sectors;
+ if (need_cow) {
+ align_nb_sectors = QEMU_ALIGN_DOWN(align_nb_sectors,
+ s->target_cluster_sectors);
+ }
+ }
+
+ ret = align_sector_num + align_nb_sectors - (*sector_num + *nb_sectors);
+ *sector_num = align_sector_num;
+ *nb_sectors = align_nb_sectors;
+ assert(ret >= 0);
+ return ret;
+}
+
+/* Submit async read while handling COW.
+ * Returns: nb_sectors if no alignment is necessary, or
+ * (new_end - sector_num) if tail is rounded up or down due to
+ * alignment or buffer limit.
+ */
+static int mirror_do_read(MirrorBlockJob *s, int64_t sector_num,
+ int nb_sectors)
{
BlockDriverState *source = s->common.bs;
- int nb_sectors, sectors_per_chunk, nb_chunks, max_iov;
- int64_t end, sector_num, next_chunk, next_sector, hbitmap_next_sector;
- uint64_t delay_ns = 0;
+ int sectors_per_chunk, nb_chunks;
+ int ret = nb_sectors;
MirrorOp *op;
- int pnum;
- int64_t ret;
- BlockDriverState *file;
- max_iov = MIN(source->bl.max_iov, s->target->bl.max_iov);
-
- s->sector_num = hbitmap_iter_next(&s->hbi);
- if (s->sector_num < 0) {
- bdrv_dirty_iter_init(s->dirty_bitmap, &s->hbi);
- s->sector_num = hbitmap_iter_next(&s->hbi);
- trace_mirror_restart_iter(s, bdrv_get_dirty_count(s->dirty_bitmap));
- assert(s->sector_num >= 0);
- }
-
- hbitmap_next_sector = s->sector_num;
- sector_num = s->sector_num;
sectors_per_chunk = s->granularity >> BDRV_SECTOR_BITS;
- end = s->bdev_length / BDRV_SECTOR_SIZE;
- /* Extend the QEMUIOVector to include all adjacent blocks that will
- * be copied in this operation.
- *
- * We have to do this if we have no backing file yet in the destination,
- * and the cluster size is very large. Then we need to do COW ourselves.
- * The first time a cluster is copied, copy it entirely. Note that,
- * because both the granularity and the cluster size are powers of two,
- * the number of sectors to copy cannot exceed one cluster.
- *
- * We also want to extend the QEMUIOVector to include more adjacent
- * dirty blocks if possible, to limit the number of I/O operations and
- * run efficiently even with a small granularity.
- */
- nb_chunks = 0;
- nb_sectors = 0;
- next_sector = sector_num;
- next_chunk = sector_num / sectors_per_chunk;
+ /* We can only handle as much as buf_size at a time. */
+ nb_sectors = MIN(s->buf_size >> BDRV_SECTOR_BITS, nb_sectors);
+ assert(nb_sectors);
- /* Wait for I/O to this cluster (from a previous iteration) to be done. */
- while (test_bit(next_chunk, s->in_flight_bitmap)) {
+ if (s->cow_bitmap) {
+ ret += mirror_cow_align(s, §or_num, &nb_sectors);
+ }
+ assert(nb_sectors << BDRV_SECTOR_BITS <= s->buf_size);
+ /* The sector range must meet granularity because:
+ * 1) Caller passes in aligned values;
+ * 2) mirror_cow_align is used only when target cluster is larger. */
+ assert(!(nb_sectors % sectors_per_chunk));
+ assert(!(sector_num % sectors_per_chunk));
+ nb_chunks = nb_sectors / sectors_per_chunk;
+
+ while (s->buf_free_count < nb_chunks) {
trace_mirror_yield_in_flight(s, sector_num, s->in_flight);
s->waiting_for_io = true;
qemu_coroutine_yield();
s->waiting_for_io = false;
}
- do {
- int added_sectors, added_chunks;
-
- if (!bdrv_get_dirty(source, s->dirty_bitmap, next_sector) ||
- test_bit(next_chunk, s->in_flight_bitmap)) {
- assert(nb_sectors > 0);
- break;
- }
-
- added_sectors = sectors_per_chunk;
- if (s->cow_bitmap && !test_bit(next_chunk, s->cow_bitmap)) {
- bdrv_round_to_clusters(s->target,
- next_sector, added_sectors,
- &next_sector, &added_sectors);
-
- /* On the first iteration, the rounding may make us copy
- * sectors before the first dirty one.
- */
- if (next_sector < sector_num) {
- assert(nb_sectors == 0);
- sector_num = next_sector;
- next_chunk = next_sector / sectors_per_chunk;
- }
- }
-
- added_sectors = MIN(added_sectors, end - (sector_num + nb_sectors));
- added_chunks = (added_sectors + sectors_per_chunk - 1) / sectors_per_chunk;
-
- /* When doing COW, it may happen that there is not enough space for
- * a full cluster. Wait if that is the case.
- */
- while (nb_chunks == 0 && s->buf_free_count < added_chunks) {
- trace_mirror_yield_buf_busy(s, nb_chunks, s->in_flight);
- s->waiting_for_io = true;
- qemu_coroutine_yield();
- s->waiting_for_io = false;
- }
- if (s->buf_free_count < nb_chunks + added_chunks) {
- trace_mirror_break_buf_busy(s, nb_chunks, s->in_flight);
- break;
- }
- if (max_iov < nb_chunks + added_chunks) {
- trace_mirror_break_iov_max(s, nb_chunks, added_chunks);
- break;
- }
-
- /* We have enough free space to copy these sectors. */
- bitmap_set(s->in_flight_bitmap, next_chunk, added_chunks);
-
- nb_sectors += added_sectors;
- nb_chunks += added_chunks;
- next_sector += added_sectors;
- next_chunk += added_chunks;
- if (!s->synced && s->common.speed) {
- delay_ns = ratelimit_calculate_delay(&s->limit, added_sectors);
- }
- } while (delay_ns == 0 && next_sector < end);
-
/* Allocate a MirrorOp that is used as an AIO callback. */
op = g_new(MirrorOp, 1);
op->s = s;
@@ -279,47 +243,153 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
* from s->buf_free.
*/
qemu_iovec_init(&op->qiov, nb_chunks);
- next_sector = sector_num;
while (nb_chunks-- > 0) {
MirrorBuffer *buf = QSIMPLEQ_FIRST(&s->buf_free);
- size_t remaining = (nb_sectors * BDRV_SECTOR_SIZE) - op->qiov.size;
+ size_t remaining = nb_sectors * BDRV_SECTOR_SIZE - op->qiov.size;
QSIMPLEQ_REMOVE_HEAD(&s->buf_free, next);
s->buf_free_count--;
qemu_iovec_add(&op->qiov, buf, MIN(s->granularity, remaining));
-
- /* Advance the HBitmapIter in parallel, so that we do not examine
- * the same sector twice.
- */
- if (next_sector > hbitmap_next_sector
- && bdrv_get_dirty(source, s->dirty_bitmap, next_sector)) {
- hbitmap_next_sector = hbitmap_iter_next(&s->hbi);
- }
-
- next_sector += sectors_per_chunk;
}
- bdrv_reset_dirty_bitmap(s->dirty_bitmap, sector_num, nb_sectors);
-
/* Copy the dirty cluster. */
s->in_flight++;
s->sectors_in_flight += nb_sectors;
trace_mirror_one_iteration(s, sector_num, nb_sectors);
- ret = bdrv_get_block_status_above(source, NULL, sector_num,
- nb_sectors, &pnum, &file);
- 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_readv(source, sector_num, &op->qiov, nb_sectors,
+ mirror_read_complete, op);
+ return ret;
+}
+
+static void mirror_do_zero_or_discard(MirrorBlockJob *s,
+ int64_t sector_num,
+ int nb_sectors,
+ bool is_discard)
+{
+ MirrorOp *op;
+
+ /* Allocate a MirrorOp that is used as an AIO callback. The qiov is zeroed
+ * so the freeing in mirror_iteration_done is nop. */
+ op = g_new0(MirrorOp, 1);
+ op->s = s;
+ op->sector_num = sector_num;
+ op->nb_sectors = nb_sectors;
+
+ s->in_flight++;
+ s->sectors_in_flight += nb_sectors;
+ if (is_discard) {
+ bdrv_aio_discard(s->target, sector_num, op->nb_sectors,
+ mirror_write_complete, op);
+ } else {
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);
+ }
+}
+
+static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
+{
+ BlockDriverState *source = s->common.bs;
+ int64_t sector_num;
+ uint64_t delay_ns = 0;
+ /* At least the first dirty chunk is mirrored in one iteration. */
+ int nb_chunks = 1;
+ int64_t end = s->bdev_length / BDRV_SECTOR_SIZE;
+ int sectors_per_chunk = s->granularity >> BDRV_SECTOR_BITS;
+
+ sector_num = hbitmap_iter_next(&s->hbi);
+ if (sector_num < 0) {
+ bdrv_dirty_iter_init(s->dirty_bitmap, &s->hbi);
+ sector_num = hbitmap_iter_next(&s->hbi);
+ trace_mirror_restart_iter(s, bdrv_get_dirty_count(s->dirty_bitmap));
+ assert(sector_num >= 0);
+ }
+
+ /* Find the number of consective dirty chunks following the first dirty
+ * one, and wait for in flight requests in them. */
+ while (nb_chunks * sectors_per_chunk < (s->buf_size >> BDRV_SECTOR_BITS)) {
+ int64_t hbitmap_next;
+ int64_t next_sector = sector_num + nb_chunks * sectors_per_chunk;
+ int64_t next_chunk = next_sector / sectors_per_chunk;
+ if (next_sector >= end ||
+ !bdrv_get_dirty(source, s->dirty_bitmap, next_sector)) {
+ break;
+ }
+ if (test_bit(next_chunk, s->in_flight_bitmap)) {
+ if (nb_chunks > 0) {
+ break;
+ }
+ trace_mirror_yield_in_flight(s, next_sector, s->in_flight);
+ s->waiting_for_io = true;
+ qemu_coroutine_yield();
+ s->waiting_for_io = false;
+ /* Now retry. */
+ } else {
+ hbitmap_next = hbitmap_iter_next(&s->hbi);
+ assert(hbitmap_next == next_sector);
+ nb_chunks++;
+ }
+ }
+
+ /* Clear dirty bits before querying the block status, because
+ * calling bdrv_get_block_status_above could yield - if some blocks are
+ * marked dirty in this window, we need to know.
+ */
+ bdrv_reset_dirty_bitmap(s->dirty_bitmap, sector_num,
+ nb_chunks * sectors_per_chunk);
+ bitmap_set(s->in_flight_bitmap, sector_num / sectors_per_chunk, nb_chunks);
+ while (nb_chunks > 0 && sector_num < end) {
+ int ret;
+ int io_sectors;
+ BlockDriverState *file;
+ enum MirrorMethod {
+ MIRROR_METHOD_COPY,
+ MIRROR_METHOD_ZERO,
+ MIRROR_METHOD_DISCARD
+ } mirror_method = MIRROR_METHOD_COPY;
+
+ assert(!(sector_num % sectors_per_chunk));
+ ret = bdrv_get_block_status_above(source, NULL, sector_num,
+ nb_chunks * sectors_per_chunk,
+ &io_sectors, &file);
+ if (ret < 0) {
+ io_sectors = nb_chunks * sectors_per_chunk;
+ }
+
+ io_sectors -= io_sectors % sectors_per_chunk;
+ if (io_sectors < sectors_per_chunk) {
+ io_sectors = sectors_per_chunk;
+ } else if (ret >= 0 && !(ret & BDRV_BLOCK_DATA)) {
+ int64_t target_sector_num;
+ int target_nb_sectors;
+ bdrv_round_to_clusters(s->target, sector_num, io_sectors,
+ &target_sector_num, &target_nb_sectors);
+ if (target_sector_num == sector_num &&
+ target_nb_sectors == io_sectors) {
+ mirror_method = ret & BDRV_BLOCK_ZERO ?
+ MIRROR_METHOD_ZERO :
+ MIRROR_METHOD_DISCARD;
+ }
+ }
+
+ switch (mirror_method) {
+ case MIRROR_METHOD_COPY:
+ io_sectors = mirror_do_read(s, sector_num, io_sectors);
+ break;
+ case MIRROR_METHOD_ZERO:
+ mirror_do_zero_or_discard(s, sector_num, io_sectors, false);
+ break;
+ case MIRROR_METHOD_DISCARD:
+ mirror_do_zero_or_discard(s, sector_num, io_sectors, true);
+ break;
+ default:
+ abort();
+ }
+ assert(io_sectors);
+ sector_num += io_sectors;
+ nb_chunks -= io_sectors / sectors_per_chunk;
+ delay_ns += ratelimit_calculate_delay(&s->limit, io_sectors);
}
return delay_ns;
}
@@ -420,6 +490,7 @@ static void coroutine_fn mirror_run(void *opaque)
checking for a NULL string */
int ret = 0;
int n;
+ int target_cluster_size = BDRV_SECTOR_SIZE;
if (block_job_is_cancelled(&s->common)) {
goto immediate_exit;
@@ -449,16 +520,16 @@ static void coroutine_fn mirror_run(void *opaque)
*/
bdrv_get_backing_filename(s->target, backing_filename,
sizeof(backing_filename));
- if (backing_filename[0] && !s->target->backing) {
- ret = bdrv_get_info(s->target, &bdi);
- if (ret < 0) {
- goto immediate_exit;
- }
- if (s->granularity < bdi.cluster_size) {
- s->buf_size = MAX(s->buf_size, bdi.cluster_size);
- s->cow_bitmap = bitmap_new(length);
- }
+ if (!bdrv_get_info(s->target, &bdi) && bdi.cluster_size) {
+ target_cluster_size = bdi.cluster_size;
}
+ if (backing_filename[0] && !s->target->backing
+ && s->granularity < target_cluster_size) {
+ s->buf_size = MAX(s->buf_size, target_cluster_size);
+ s->cow_bitmap = bitmap_new(length);
+ }
+ s->target_cluster_sectors = target_cluster_size >> BDRV_SECTOR_BITS;
+ s->max_iov = MIN(s->common.bs->bl.max_iov, s->target->bl.max_iov);
end = s->bdev_length / BDRV_SECTOR_SIZE;
s->buf = qemu_try_blockalign(bs, s->buf_size);
diff --git a/tests/qemu-iotests/109.out b/tests/qemu-iotests/109.out
index 7db92c9..b3358de 100644
--- a/tests/qemu-iotests/109.out
+++ b/tests/qemu-iotests/109.out
@@ -2,57 +2,57 @@ QA output created by 109
=== Writing a qcow header into raw ===
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-Formatting 'TEST_DIR/t.raw.src', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.raw.src', fmt=IMGFMT size=67108864
{"return": {}}
WARNING: Image format was not specified for 'TEST_DIR/t.raw' and probing guessed raw.
Automatically detecting the format is dangerous for raw images, write operations on block 0 will be restricted.
Specify the 'raw' format explicitly to remove the restrictions.
{"return": {}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_ERROR", "data": {"device": "src", "operation": "write", "action": "report"}}
-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 1024, "offset": 0, "speed": 0, "type": "mirror", "error": "Operation not permitted"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 65536, "offset": 0, "speed": 0, "type": "mirror", "error": "Operation not permitted"}}
{"return": []}
read 65536/65536 bytes at offset 0
64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
{"return": {}}
{"return": {}}
-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 1024, "offset": 1024, "speed": 0, "type": "mirror"}}
-{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 1024, "offset": 1024, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 65536, "offset": 65536, "speed": 0, "type": "mirror"}}
+{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 65536, "offset": 65536, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]}
Warning: Image size mismatch!
Images are identical.
=== Writing a qcow2 header into raw ===
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-Formatting 'TEST_DIR/t.raw.src', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.raw.src', fmt=IMGFMT size=67108864
{"return": {}}
WARNING: Image format was not specified for 'TEST_DIR/t.raw' and probing guessed raw.
Automatically detecting the format is dangerous for raw images, write operations on block 0 will be restricted.
Specify the 'raw' format explicitly to remove the restrictions.
{"return": {}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_ERROR", "data": {"device": "src", "operation": "write", "action": "report"}}
-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 197120, "offset": 0, "speed": 0, "type": "mirror", "error": "Operation not permitted"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 262144, "offset": 65536, "speed": 0, "type": "mirror", "error": "Operation not permitted"}}
{"return": []}
read 65536/65536 bytes at offset 0
64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
{"return": {}}
{"return": {}}
-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 197120, "offset": 197120, "speed": 0, "type": "mirror"}}
-{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 197120, "offset": 197120, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 262144, "offset": 262144, "speed": 0, "type": "mirror"}}
+{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 262144, "offset": 262144, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]}
Warning: Image size mismatch!
Images are identical.
=== Writing a qed header into raw ===
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-Formatting 'TEST_DIR/t.raw.src', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.raw.src', fmt=IMGFMT size=67108864
{"return": {}}
WARNING: Image format was not specified for 'TEST_DIR/t.raw' and probing guessed raw.
Automatically detecting the format is dangerous for raw images, write operations on block 0 will be restricted.
Specify the 'raw' format explicitly to remove the restrictions.
{"return": {}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_ERROR", "data": {"device": "src", "operation": "write", "action": "report"}}
-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 327680, "offset": 0, "speed": 0, "type": "mirror", "error": "Operation not permitted"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 327680, "offset": 262144, "speed": 0, "type": "mirror", "error": "Operation not permitted"}}
{"return": []}
read 65536/65536 bytes at offset 0
64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
@@ -65,29 +65,29 @@ Images are identical.
=== Writing a vdi header into raw ===
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-Formatting 'TEST_DIR/t.raw.src', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.raw.src', fmt=IMGFMT size=67108864
{"return": {}}
WARNING: Image format was not specified for 'TEST_DIR/t.raw' and probing guessed raw.
Automatically detecting the format is dangerous for raw images, write operations on block 0 will be restricted.
Specify the 'raw' format explicitly to remove the restrictions.
{"return": {}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_ERROR", "data": {"device": "src", "operation": "write", "action": "report"}}
-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 1024, "offset": 0, "speed": 0, "type": "mirror", "error": "Operation not permitted"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 65536, "offset": 0, "speed": 0, "type": "mirror", "error": "Operation not permitted"}}
{"return": []}
read 65536/65536 bytes at offset 0
64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
{"return": {}}
{"return": {}}
-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 1024, "offset": 1024, "speed": 0, "type": "mirror"}}
-{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 1024, "offset": 1024, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 65536, "offset": 65536, "speed": 0, "type": "mirror"}}
+{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 65536, "offset": 65536, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]}
Warning: Image size mismatch!
Images are identical.
=== Writing a vmdk header into raw ===
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-Formatting 'TEST_DIR/t.raw.src', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.raw.src', fmt=IMGFMT size=67108864
{"return": {}}
WARNING: Image format was not specified for 'TEST_DIR/t.raw' and probing guessed raw.
Automatically detecting the format is dangerous for raw images, write operations on block 0 will be restricted.
@@ -107,49 +107,49 @@ Images are identical.
=== Writing a vpc header into raw ===
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-Formatting 'TEST_DIR/t.raw.src', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.raw.src', fmt=IMGFMT size=67108864
{"return": {}}
WARNING: Image format was not specified for 'TEST_DIR/t.raw' and probing guessed raw.
Automatically detecting the format is dangerous for raw images, write operations on block 0 will be restricted.
Specify the 'raw' format explicitly to remove the restrictions.
{"return": {}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_ERROR", "data": {"device": "src", "operation": "write", "action": "report"}}
-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 2560, "offset": 0, "speed": 0, "type": "mirror", "error": "Operation not permitted"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 65536, "offset": 0, "speed": 0, "type": "mirror", "error": "Operation not permitted"}}
{"return": []}
read 65536/65536 bytes at offset 0
64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
{"return": {}}
{"return": {}}
-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 2560, "offset": 2560, "speed": 0, "type": "mirror"}}
-{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 2560, "offset": 2560, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 65536, "offset": 65536, "speed": 0, "type": "mirror"}}
+{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 65536, "offset": 65536, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]}
Warning: Image size mismatch!
Images are identical.
=== Copying sample image empty.bochs into raw ===
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
{"return": {}}
WARNING: Image format was not specified for 'TEST_DIR/t.raw' and probing guessed raw.
Automatically detecting the format is dangerous for raw images, write operations on block 0 will be restricted.
Specify the 'raw' format explicitly to remove the restrictions.
{"return": {}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_ERROR", "data": {"device": "src", "operation": "write", "action": "report"}}
-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 2560, "offset": 0, "speed": 0, "type": "mirror", "error": "Operation not permitted"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 65536, "offset": 0, "speed": 0, "type": "mirror", "error": "Operation not permitted"}}
{"return": []}
read 65536/65536 bytes at offset 0
64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
{"return": {}}
{"return": {}}
-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 2560, "offset": 2560, "speed": 0, "type": "mirror"}}
-{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 2560, "offset": 2560, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 65536, "offset": 65536, "speed": 0, "type": "mirror"}}
+{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 65536, "offset": 65536, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]}
Image resized.
Warning: Image size mismatch!
Images are identical.
=== Copying sample image iotest-dirtylog-10G-4M.vhdx into raw ===
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
{"return": {}}
WARNING: Image format was not specified for 'TEST_DIR/t.raw' and probing guessed raw.
Automatically detecting the format is dangerous for raw images, write operations on block 0 will be restricted.
@@ -170,7 +170,7 @@ Images are identical.
=== Copying sample image parallels-v1 into raw ===
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
{"return": {}}
WARNING: Image format was not specified for 'TEST_DIR/t.raw' and probing guessed raw.
Automatically detecting the format is dangerous for raw images, write operations on block 0 will be restricted.
@@ -191,41 +191,41 @@ Images are identical.
=== Copying sample image simple-pattern.cloop into raw ===
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
{"return": {}}
WARNING: Image format was not specified for 'TEST_DIR/t.raw' and probing guessed raw.
Automatically detecting the format is dangerous for raw images, write operations on block 0 will be restricted.
Specify the 'raw' format explicitly to remove the restrictions.
{"return": {}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_ERROR", "data": {"device": "src", "operation": "write", "action": "report"}}
-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 2048, "offset": 0, "speed": 0, "type": "mirror", "error": "Operation not permitted"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 65536, "offset": 0, "speed": 0, "type": "mirror", "error": "Operation not permitted"}}
{"return": []}
read 65536/65536 bytes at offset 0
64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
{"return": {}}
{"return": {}}
-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 2048, "offset": 2048, "speed": 0, "type": "mirror"}}
-{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 2048, "offset": 2048, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 65536, "offset": 65536, "speed": 0, "type": "mirror"}}
+{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 65536, "offset": 65536, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]}
Image resized.
Warning: Image size mismatch!
Images are identical.
=== Write legitimate MBR into raw ===
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
{"return": {}}
WARNING: Image format was not specified for 'TEST_DIR/t.raw' and probing guessed raw.
Automatically detecting the format is dangerous for raw images, write operations on block 0 will be restricted.
Specify the 'raw' format explicitly to remove the restrictions.
{"return": {}}
-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 512, "offset": 512, "speed": 0, "type": "mirror"}}
-{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 512, "offset": 512, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 65536, "offset": 65536, "speed": 0, "type": "mirror"}}
+{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 65536, "offset": 65536, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]}
Warning: Image size mismatch!
Images are identical.
{"return": {}}
{"return": {}}
-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 512, "offset": 512, "speed": 0, "type": "mirror"}}
-{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 512, "offset": 512, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 65536, "offset": 65536, "speed": 0, "type": "mirror"}}
+{"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 65536, "offset": 65536, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]}
Warning: Image size mismatch!
Images are identical.
*** done
diff --git a/trace-events b/trace-events
index c9ac144..fffcfb7 100644
--- a/trace-events
+++ b/trace-events
@@ -97,7 +97,6 @@ mirror_yield(void *s, int64_t cnt, int buf_free_count, int in_flight) "s %p dirt
mirror_yield_in_flight(void *s, int64_t sector_num, int in_flight) "s %p sector_num %"PRId64" in_flight %d"
mirror_yield_buf_busy(void *s, int nb_chunks, int in_flight) "s %p requested chunks %d in_flight %d"
mirror_break_buf_busy(void *s, int nb_chunks, int in_flight) "s %p requested chunks %d in_flight %d"
-mirror_break_iov_max(void *s, int nb_chunks, int added_chunks) "s %p requested chunks %d added_chunks %d"
# block/backup.c
backup_do_cow_enter(void *job, int64_t start, int64_t sector_num, int nb_sectors) "job %p start %"PRId64" sector_num %"PRId64" nb_sectors %d"
--
2.4.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH v12 2/2] mirror: Add mirror_wait_for_io
2016-02-05 2:00 [Qemu-devel] [PATCH v12 0/2] mirror: Improve zero write and discard Fam Zheng
2016-02-05 2:00 ` [Qemu-devel] [PATCH v12 1/2] mirror: Rewrite mirror_iteration Fam Zheng
@ 2016-02-05 2:00 ` Fam Zheng
1 sibling, 0 replies; 9+ messages in thread
From: Fam Zheng @ 2016-02-05 2:00 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, pbonzini, Jeff Cody, qemu-block, mreitz
The three lines are duplicated a number of times now, refactor a
function.
Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block/mirror.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index 48cd0b3..9635fa8 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -196,6 +196,14 @@ static int mirror_cow_align(MirrorBlockJob *s,
return ret;
}
+static inline void mirror_wait_for_io(MirrorBlockJob *s)
+{
+ assert(!s->waiting_for_io);
+ s->waiting_for_io = true;
+ qemu_coroutine_yield();
+ s->waiting_for_io = false;
+}
+
/* Submit async read while handling COW.
* Returns: nb_sectors if no alignment is necessary, or
* (new_end - sector_num) if tail is rounded up or down due to
@@ -228,9 +236,7 @@ static int mirror_do_read(MirrorBlockJob *s, int64_t sector_num,
while (s->buf_free_count < nb_chunks) {
trace_mirror_yield_in_flight(s, sector_num, s->in_flight);
- s->waiting_for_io = true;
- qemu_coroutine_yield();
- s->waiting_for_io = false;
+ mirror_wait_for_io(s);
}
/* Allocate a MirrorOp that is used as an AIO callback. */
@@ -321,9 +327,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
break;
}
trace_mirror_yield_in_flight(s, next_sector, s->in_flight);
- s->waiting_for_io = true;
- qemu_coroutine_yield();
- s->waiting_for_io = false;
+ mirror_wait_for_io(s);
/* Now retry. */
} else {
hbitmap_next = hbitmap_iter_next(&s->hbi);
@@ -414,9 +418,7 @@ static void mirror_free_init(MirrorBlockJob *s)
static void mirror_drain(MirrorBlockJob *s)
{
while (s->in_flight > 0) {
- s->waiting_for_io = true;
- qemu_coroutine_yield();
- s->waiting_for_io = false;
+ mirror_wait_for_io(s);
}
}
@@ -604,9 +606,7 @@ static void coroutine_fn mirror_run(void *opaque)
if (s->in_flight == MAX_IN_FLIGHT || s->buf_free_count == 0 ||
(cnt == 0 && s->in_flight > 0)) {
trace_mirror_yield(s, s->in_flight, s->buf_free_count, cnt);
- s->waiting_for_io = true;
- qemu_coroutine_yield();
- s->waiting_for_io = false;
+ mirror_wait_for_io(s);
continue;
} else if (cnt != 0) {
delay_ns = mirror_iteration(s);
--
2.4.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v12 1/2] mirror: Rewrite mirror_iteration
2016-02-05 2:00 ` [Qemu-devel] [PATCH v12 1/2] mirror: Rewrite mirror_iteration Fam Zheng
@ 2016-02-06 13:24 ` Max Reitz
2016-02-07 12:46 ` Fam Zheng
0 siblings, 1 reply; 9+ messages in thread
From: Max Reitz @ 2016-02-06 13:24 UTC (permalink / raw)
To: Fam Zheng, qemu-devel; +Cc: Kevin Wolf, pbonzini, Jeff Cody, qemu-block
[-- Attachment #1: Type: text/plain, Size: 2179 bytes --]
On 05.02.2016 03:00, Fam Zheng wrote:
> The "pnum < nb_sectors" condition in deciding whether to actually copy
> data is unnecessarily strict, and the qiov initialization is
> unnecessarily for bdrv_aio_write_zeroes and bdrv_aio_discard.
>
> Rewrite mirror_iteration to fix both flaws.
>
> The output of iotests 109 is updated because we now report the offset
> and len slightly differently in mirroring progress.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> block/mirror.c | 335 +++++++++++++++++++++++++++------------------
> tests/qemu-iotests/109.out | 80 +++++------
> trace-events | 1 -
> 3 files changed, 243 insertions(+), 173 deletions(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index 2c0edfa..48cd0b3 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
[...]
> @@ -449,16 +520,16 @@ static void coroutine_fn mirror_run(void *opaque)
> */
> bdrv_get_backing_filename(s->target, backing_filename,
> sizeof(backing_filename));
> - if (backing_filename[0] && !s->target->backing) {
> - ret = bdrv_get_info(s->target, &bdi);
> - if (ret < 0) {
> - goto immediate_exit;
> - }
> - if (s->granularity < bdi.cluster_size) {
> - s->buf_size = MAX(s->buf_size, bdi.cluster_size);
> - s->cow_bitmap = bitmap_new(length);
> - }
> + if (!bdrv_get_info(s->target, &bdi) && bdi.cluster_size) {
This should be bdi.has_cluster_size...
> + target_cluster_size = bdi.cluster_size;
...and maybe we want an explicit minimum of BDRV_SECTOR_SIZE here; but I
guess this is already assumed all over the block layer, so it may be
fine without.
> }
> + if (backing_filename[0] && !s->target->backing
> + && s->granularity < target_cluster_size) {
> + s->buf_size = MAX(s->buf_size, target_cluster_size);
> + s->cow_bitmap = bitmap_new(length);
> + }
> + s->target_cluster_sectors = target_cluster_size >> BDRV_SECTOR_BITS;
(this shouldn't be 0, so target_cluster_size needs to be at least
BDRV_SECTOR_SIZE)
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v12 1/2] mirror: Rewrite mirror_iteration
2016-02-06 13:24 ` Max Reitz
@ 2016-02-07 12:46 ` Fam Zheng
2016-02-08 12:54 ` Max Reitz
0 siblings, 1 reply; 9+ messages in thread
From: Fam Zheng @ 2016-02-07 12:46 UTC (permalink / raw)
To: Max Reitz; +Cc: Kevin Wolf, pbonzini, Jeff Cody, qemu-devel, qemu-block
On Sat, 02/06 14:24, Max Reitz wrote:
> On 05.02.2016 03:00, Fam Zheng wrote:
> > The "pnum < nb_sectors" condition in deciding whether to actually copy
> > data is unnecessarily strict, and the qiov initialization is
> > unnecessarily for bdrv_aio_write_zeroes and bdrv_aio_discard.
> >
> > Rewrite mirror_iteration to fix both flaws.
> >
> > The output of iotests 109 is updated because we now report the offset
> > and len slightly differently in mirroring progress.
> >
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> > block/mirror.c | 335 +++++++++++++++++++++++++++------------------
> > tests/qemu-iotests/109.out | 80 +++++------
> > trace-events | 1 -
> > 3 files changed, 243 insertions(+), 173 deletions(-)
> >
> > diff --git a/block/mirror.c b/block/mirror.c
> > index 2c0edfa..48cd0b3 100644
> > --- a/block/mirror.c
> > +++ b/block/mirror.c
>
> [...]
>
> > @@ -449,16 +520,16 @@ static void coroutine_fn mirror_run(void *opaque)
> > */
> > bdrv_get_backing_filename(s->target, backing_filename,
> > sizeof(backing_filename));
> > - if (backing_filename[0] && !s->target->backing) {
> > - ret = bdrv_get_info(s->target, &bdi);
> > - if (ret < 0) {
> > - goto immediate_exit;
> > - }
> > - if (s->granularity < bdi.cluster_size) {
> > - s->buf_size = MAX(s->buf_size, bdi.cluster_size);
> > - s->cow_bitmap = bitmap_new(length);
> > - }
> > + if (!bdrv_get_info(s->target, &bdi) && bdi.cluster_size) {
>
> This should be bdi.has_cluster_size...
has_cluster_size is a member of ImageInfo not BlockDriverInfo, and is derived
from (bdi.cluster_size != 0).
>
> > + target_cluster_size = bdi.cluster_size;
>
> ...and maybe we want an explicit minimum of BDRV_SECTOR_SIZE here; but I
> guess this is already assumed all over the block layer, so it may be
> fine without.
Okay, it doesn't hurt to add an assert here.
Fam
>
> > }
> > + if (backing_filename[0] && !s->target->backing
> > + && s->granularity < target_cluster_size) {
> > + s->buf_size = MAX(s->buf_size, target_cluster_size);
> > + s->cow_bitmap = bitmap_new(length);
> > + }
> > + s->target_cluster_sectors = target_cluster_size >> BDRV_SECTOR_BITS;
>
> (this shouldn't be 0, so target_cluster_size needs to be at least
> BDRV_SECTOR_SIZE)
>
> Max
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v12 1/2] mirror: Rewrite mirror_iteration
2016-02-07 12:46 ` Fam Zheng
@ 2016-02-08 12:54 ` Max Reitz
2016-02-15 2:19 ` Fam Zheng
0 siblings, 1 reply; 9+ messages in thread
From: Max Reitz @ 2016-02-08 12:54 UTC (permalink / raw)
To: Fam Zheng; +Cc: Kevin Wolf, pbonzini, Jeff Cody, qemu-devel, qemu-block
[-- Attachment #1: Type: text/plain, Size: 2744 bytes --]
On 07.02.2016 13:46, Fam Zheng wrote:
> On Sat, 02/06 14:24, Max Reitz wrote:
>> On 05.02.2016 03:00, Fam Zheng wrote:
>>> The "pnum < nb_sectors" condition in deciding whether to actually copy
>>> data is unnecessarily strict, and the qiov initialization is
>>> unnecessarily for bdrv_aio_write_zeroes and bdrv_aio_discard.
>>>
>>> Rewrite mirror_iteration to fix both flaws.
>>>
>>> The output of iotests 109 is updated because we now report the offset
>>> and len slightly differently in mirroring progress.
>>>
>>> Signed-off-by: Fam Zheng <famz@redhat.com>
>>> ---
>>> block/mirror.c | 335 +++++++++++++++++++++++++++------------------
>>> tests/qemu-iotests/109.out | 80 +++++------
>>> trace-events | 1 -
>>> 3 files changed, 243 insertions(+), 173 deletions(-)
>>>
>>> diff --git a/block/mirror.c b/block/mirror.c
>>> index 2c0edfa..48cd0b3 100644
>>> --- a/block/mirror.c
>>> +++ b/block/mirror.c
>>
>> [...]
>>
>>> @@ -449,16 +520,16 @@ static void coroutine_fn mirror_run(void *opaque)
>>> */
>>> bdrv_get_backing_filename(s->target, backing_filename,
>>> sizeof(backing_filename));
>>> - if (backing_filename[0] && !s->target->backing) {
>>> - ret = bdrv_get_info(s->target, &bdi);
>>> - if (ret < 0) {
>>> - goto immediate_exit;
>>> - }
>>> - if (s->granularity < bdi.cluster_size) {
>>> - s->buf_size = MAX(s->buf_size, bdi.cluster_size);
>>> - s->cow_bitmap = bitmap_new(length);
>>> - }
>>> + if (!bdrv_get_info(s->target, &bdi) && bdi.cluster_size) {
>>
>> This should be bdi.has_cluster_size...
>
> has_cluster_size is a member of ImageInfo not BlockDriverInfo, and is derived
> from (bdi.cluster_size != 0).
You're right, my bad.
>>> + target_cluster_size = bdi.cluster_size;
>>
>> ...and maybe we want an explicit minimum of BDRV_SECTOR_SIZE here; but I
>> guess this is already assumed all over the block layer, so it may be
>> fine without.
>
> Okay, it doesn't hurt to add an assert here.
I'd be happy to take the patch without, too (although I wouldn't decline
a follow-up adding an assertion).
Reviewed-by: Max Reitz <mreitz@redhat.com>
>>> }
>>> + if (backing_filename[0] && !s->target->backing
>>> + && s->granularity < target_cluster_size) {
>>> + s->buf_size = MAX(s->buf_size, target_cluster_size);
>>> + s->cow_bitmap = bitmap_new(length);
>>> + }
>>> + s->target_cluster_sectors = target_cluster_size >> BDRV_SECTOR_BITS;
>>
>> (this shouldn't be 0, so target_cluster_size needs to be at least
>> BDRV_SECTOR_SIZE)
>>
>> Max
>>
>
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v12 1/2] mirror: Rewrite mirror_iteration
2016-02-08 12:54 ` Max Reitz
@ 2016-02-15 2:19 ` Fam Zheng
2016-02-15 3:49 ` Jeff Cody
0 siblings, 1 reply; 9+ messages in thread
From: Fam Zheng @ 2016-02-15 2:19 UTC (permalink / raw)
To: Max Reitz, kwolf; +Cc: pbonzini, Jeff Cody, qemu-devel, qemu-block
On Mon, 02/08 13:54, Max Reitz wrote:
> On 07.02.2016 13:46, Fam Zheng wrote:
> > On Sat, 02/06 14:24, Max Reitz wrote:
> >> On 05.02.2016 03:00, Fam Zheng wrote:
> >>> The "pnum < nb_sectors" condition in deciding whether to actually copy
> >>> data is unnecessarily strict, and the qiov initialization is
> >>> unnecessarily for bdrv_aio_write_zeroes and bdrv_aio_discard.
> >>>
> >>> Rewrite mirror_iteration to fix both flaws.
> >>>
> >>> The output of iotests 109 is updated because we now report the offset
> >>> and len slightly differently in mirroring progress.
> >>>
> >>> Signed-off-by: Fam Zheng <famz@redhat.com>
> >>> ---
> >>> block/mirror.c | 335 +++++++++++++++++++++++++++------------------
> >>> tests/qemu-iotests/109.out | 80 +++++------
> >>> trace-events | 1 -
> >>> 3 files changed, 243 insertions(+), 173 deletions(-)
> >>>
> >>> diff --git a/block/mirror.c b/block/mirror.c
> >>> index 2c0edfa..48cd0b3 100644
> >>> --- a/block/mirror.c
> >>> +++ b/block/mirror.c
> >>
> >> [...]
> >>
> >>> @@ -449,16 +520,16 @@ static void coroutine_fn mirror_run(void *opaque)
> >>> */
> >>> bdrv_get_backing_filename(s->target, backing_filename,
> >>> sizeof(backing_filename));
> >>> - if (backing_filename[0] && !s->target->backing) {
> >>> - ret = bdrv_get_info(s->target, &bdi);
> >>> - if (ret < 0) {
> >>> - goto immediate_exit;
> >>> - }
> >>> - if (s->granularity < bdi.cluster_size) {
> >>> - s->buf_size = MAX(s->buf_size, bdi.cluster_size);
> >>> - s->cow_bitmap = bitmap_new(length);
> >>> - }
> >>> + if (!bdrv_get_info(s->target, &bdi) && bdi.cluster_size) {
> >>
> >> This should be bdi.has_cluster_size...
> >
> > has_cluster_size is a member of ImageInfo not BlockDriverInfo, and is derived
> > from (bdi.cluster_size != 0).
>
> You're right, my bad.
>
> >>> + target_cluster_size = bdi.cluster_size;
> >>
> >> ...and maybe we want an explicit minimum of BDRV_SECTOR_SIZE here; but I
> >> guess this is already assumed all over the block layer, so it may be
> >> fine without.
> >
> > Okay, it doesn't hurt to add an assert here.
>
> I'd be happy to take the patch without, too (although I wouldn't decline
> a follow-up adding an assertion).
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
Thanks! Shall we merge this now?
Fam
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v12 1/2] mirror: Rewrite mirror_iteration
2016-02-15 2:19 ` Fam Zheng
@ 2016-02-15 3:49 ` Jeff Cody
2016-02-15 4:31 ` Fam Zheng
0 siblings, 1 reply; 9+ messages in thread
From: Jeff Cody @ 2016-02-15 3:49 UTC (permalink / raw)
To: Fam Zheng; +Cc: kwolf, qemu-block, jcody, qemu-devel, Max Reitz, pbonzini
[-- Attachment #1: Type: text/plain, Size: 2707 bytes --]
On Feb 14, 2016 21:19, "Fam Zheng" <famz@redhat.com> wrote:
>
> On Mon, 02/08 13:54, Max Reitz wrote:
> > On 07.02.2016 13:46, Fam Zheng wrote:
> > > On Sat, 02/06 14:24, Max Reitz wrote:
> > >> On 05.02.2016 03:00, Fam Zheng wrote:
> > >>> The "pnum < nb_sectors" condition in deciding whether to actually
copy
> > >>> data is unnecessarily strict, and the qiov initialization is
> > >>> unnecessarily for bdrv_aio_write_zeroes and bdrv_aio_discard.
> > >>>
> > >>> Rewrite mirror_iteration to fix both flaws.
> > >>>
> > >>> The output of iotests 109 is updated because we now report the
offset
> > >>> and len slightly differently in mirroring progress.
> > >>>
> > >>> Signed-off-by: Fam Zheng <famz@redhat.com>
> > >>> ---
> > >>> block/mirror.c | 335
+++++++++++++++++++++++++++------------------
> > >>> tests/qemu-iotests/109.out | 80 +++++------
> > >>> trace-events | 1 -
> > >>> 3 files changed, 243 insertions(+), 173 deletions(-)
> > >>>
> > >>> diff --git a/block/mirror.c b/block/mirror.c
> > >>> index 2c0edfa..48cd0b3 100644
> > >>> --- a/block/mirror.c
> > >>> +++ b/block/mirror.c
> > >>
> > >> [...]
> > >>
> > >>> @@ -449,16 +520,16 @@ static void coroutine_fn mirror_run(void
*opaque)
> > >>> */
> > >>> bdrv_get_backing_filename(s->target, backing_filename,
> > >>> sizeof(backing_filename));
> > >>> - if (backing_filename[0] && !s->target->backing) {
> > >>> - ret = bdrv_get_info(s->target, &bdi);
> > >>> - if (ret < 0) {
> > >>> - goto immediate_exit;
> > >>> - }
> > >>> - if (s->granularity < bdi.cluster_size) {
> > >>> - s->buf_size = MAX(s->buf_size, bdi.cluster_size);
> > >>> - s->cow_bitmap = bitmap_new(length);
> > >>> - }
> > >>> + if (!bdrv_get_info(s->target, &bdi) && bdi.cluster_size) {
> > >>
> > >> This should be bdi.has_cluster_size...
> > >
> > > has_cluster_size is a member of ImageInfo not BlockDriverInfo, and is
derived
> > > from (bdi.cluster_size != 0).
> >
> > You're right, my bad.
> >
> > >>> + target_cluster_size = bdi.cluster_size;
> > >>
> > >> ...and maybe we want an explicit minimum of BDRV_SECTOR_SIZE here;
but I
> > >> guess this is already assumed all over the block layer, so it may be
> > >> fine without.
> > >
> > > Okay, it doesn't hurt to add an assert here.
> >
> > I'd be happy to take the patch without, too (although I wouldn't decline
> > a follow-up adding an assertion).
> >
> > Reviewed-by: Max Reitz <mreitz@redhat.com>
>
> Thanks! Shall we merge this now?
>
> Fam
>
I think so - I'll go ahead and apply it to my block branch, unless there
are any objections.
Jeff
[-- Attachment #2: Type: text/html, Size: 4202 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v12 1/2] mirror: Rewrite mirror_iteration
2016-02-15 3:49 ` Jeff Cody
@ 2016-02-15 4:31 ` Fam Zheng
0 siblings, 0 replies; 9+ messages in thread
From: Fam Zheng @ 2016-02-15 4:31 UTC (permalink / raw)
To: Jeff Cody; +Cc: kwolf, qemu-block, jcody, qemu-devel, Max Reitz, pbonzini
On Sun, 02/14 22:49, Jeff Cody wrote:
> On Feb 14, 2016 21:19, "Fam Zheng" <famz@redhat.com> wrote:
> >
> > On Mon, 02/08 13:54, Max Reitz wrote:
> > > On 07.02.2016 13:46, Fam Zheng wrote:
> > > > On Sat, 02/06 14:24, Max Reitz wrote:
> > > >> On 05.02.2016 03:00, Fam Zheng wrote:
> > > >>> The "pnum < nb_sectors" condition in deciding whether to actually
> copy
> > > >>> data is unnecessarily strict, and the qiov initialization is
> > > >>> unnecessarily for bdrv_aio_write_zeroes and bdrv_aio_discard.
> > > >>>
> > > >>> Rewrite mirror_iteration to fix both flaws.
> > > >>>
> > > >>> The output of iotests 109 is updated because we now report the
> offset
> > > >>> and len slightly differently in mirroring progress.
> > > >>>
> > > >>> Signed-off-by: Fam Zheng <famz@redhat.com>
> > > >>> ---
> > > >>> block/mirror.c | 335
> +++++++++++++++++++++++++++------------------
> > > >>> tests/qemu-iotests/109.out | 80 +++++------
> > > >>> trace-events | 1 -
> > > >>> 3 files changed, 243 insertions(+), 173 deletions(-)
> > > >>>
> > > >>> diff --git a/block/mirror.c b/block/mirror.c
> > > >>> index 2c0edfa..48cd0b3 100644
> > > >>> --- a/block/mirror.c
> > > >>> +++ b/block/mirror.c
> > > >>
> > > >> [...]
> > > >>
> > > >>> @@ -449,16 +520,16 @@ static void coroutine_fn mirror_run(void
> *opaque)
> > > >>> */
> > > >>> bdrv_get_backing_filename(s->target, backing_filename,
> > > >>> sizeof(backing_filename));
> > > >>> - if (backing_filename[0] && !s->target->backing) {
> > > >>> - ret = bdrv_get_info(s->target, &bdi);
> > > >>> - if (ret < 0) {
> > > >>> - goto immediate_exit;
> > > >>> - }
> > > >>> - if (s->granularity < bdi.cluster_size) {
> > > >>> - s->buf_size = MAX(s->buf_size, bdi.cluster_size);
> > > >>> - s->cow_bitmap = bitmap_new(length);
> > > >>> - }
> > > >>> + if (!bdrv_get_info(s->target, &bdi) && bdi.cluster_size) {
> > > >>
> > > >> This should be bdi.has_cluster_size...
> > > >
> > > > has_cluster_size is a member of ImageInfo not BlockDriverInfo, and is
> derived
> > > > from (bdi.cluster_size != 0).
> > >
> > > You're right, my bad.
> > >
> > > >>> + target_cluster_size = bdi.cluster_size;
> > > >>
> > > >> ...and maybe we want an explicit minimum of BDRV_SECTOR_SIZE here;
> but I
> > > >> guess this is already assumed all over the block layer, so it may be
> > > >> fine without.
> > > >
> > > > Okay, it doesn't hurt to add an assert here.
> > >
> > > I'd be happy to take the patch without, too (although I wouldn't decline
> > > a follow-up adding an assertion).
> > >
> > > Reviewed-by: Max Reitz <mreitz@redhat.com>
> >
> > Thanks! Shall we merge this now?
> >
> > Fam
> >
>
> I think so - I'll go ahead and apply it to my block branch, unless there
> are any objections.
Great! Thanks Jeff.
Fam
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-02-15 4:31 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-05 2:00 [Qemu-devel] [PATCH v12 0/2] mirror: Improve zero write and discard Fam Zheng
2016-02-05 2:00 ` [Qemu-devel] [PATCH v12 1/2] mirror: Rewrite mirror_iteration Fam Zheng
2016-02-06 13:24 ` Max Reitz
2016-02-07 12:46 ` Fam Zheng
2016-02-08 12:54 ` Max Reitz
2016-02-15 2:19 ` Fam Zheng
2016-02-15 3:49 ` Jeff Cody
2016-02-15 4:31 ` Fam Zheng
2016-02-05 2:00 ` [Qemu-devel] [PATCH v12 2/2] mirror: Add mirror_wait_for_io Fam Zheng
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).