On 07/07/2016 05:35 AM, Denis V. Lunev wrote: > We should not take into account zero blocks for delay calculations. > They are not read and thus IO throttling is not required. In the > other case VM migration with 16 Tb QCOW2 disk with 4 Gb of data takes > days.
Makes sense. > > Signed-off-by: Denis V. Lunev <[email protected]> > Reviewed-by: Vladimir Sementsov-Ogievskiy <[email protected]> > CC: Stefan Hajnoczi <[email protected]> > CC: Fam Zheng <[email protected]> > CC: Kevin Wolf <[email protected]> > CC: Max Reitz <[email protected]> > CC: Jeff Cody <[email protected]> > CC: Eric Blake <[email protected]> > --- > block/mirror.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/block/mirror.c b/block/mirror.c > index 4ecfbf1..3167de3 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -322,6 +322,7 @@ static uint64_t coroutine_fn > mirror_iteration(MirrorBlockJob *s) > int nb_chunks = 1; > int64_t end = s->bdev_length / BDRV_SECTOR_SIZE; > int sectors_per_chunk = s->granularity >> BDRV_SECTOR_BITS; > + bool write_zeroes_ok = > bdrv_can_write_zeroes_with_unmap(blk_bs(s->target)); > > sector_num = hbitmap_iter_next(&s->hbi); > if (sector_num < 0) { > @@ -372,7 +373,7 @@ static uint64_t coroutine_fn > mirror_iteration(MirrorBlockJob *s) > 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; > + int io_sectors, io_sectors_acct; > BlockDriverState *file; > enum MirrorMethod { > MIRROR_METHOD_COPY, > @@ -409,12 +410,17 @@ static uint64_t coroutine_fn > mirror_iteration(MirrorBlockJob *s) > switch (mirror_method) { > case MIRROR_METHOD_COPY: > io_sectors = mirror_do_read(s, sector_num, io_sectors); > + io_sectors_acct = 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); > + mirror_do_zero_or_discard(s, sector_num, io_sectors, > + mirror_method == > MIRROR_METHOD_DISCARD); > + if (write_zeroes_ok) { > + io_sectors_acct = 0; > + } else { > + io_sectors_acct = io_sectors; > + } I may have used a ternary, but that's just my preference for this pattern. > break; > default: > abort(); > @@ -422,7 +428,7 @@ static uint64_t coroutine_fn > mirror_iteration(MirrorBlockJob *s) > assert(io_sectors); > sector_num += io_sectors; > nb_chunks -= DIV_ROUND_UP(io_sectors, sectors_per_chunk); > - delay_ns += ratelimit_calculate_delay(&s->limit, io_sectors); > + delay_ns += ratelimit_calculate_delay(&s->limit, io_sectors_acct); > } > return delay_ns; > } > Seems OK in practice. What I wonder is if we shouldn't be deterministically reading how much data we are actually shuffling over the pipe and using that for ratelimiting instead of in a higher abstraction layer "guessing" how much data we're going to be sending. Jeff, do you have any input on this?
