On Mon, 07/11 15:53, John Snow wrote: > > > 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; > > + }
Why the MIRROR_METHOD_ZERO has to consult write_zeroes_ok, if unmap is not used at all? > > 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. It's a good question, I wonder if it is possible that block layer can even use a fallback of write write_zeroes even write_zeroes_ok is true? Fam
