[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v2 7/7] mirror: improve performance of mirroring
From: |
John Snow |
Subject: |
Re: [Qemu-block] [PATCH v2 7/7] mirror: improve performance of mirroring of empty disk |
Date: |
Mon, 11 Jul 2016 15:53:08 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 |
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 <address@hidden>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> CC: Stefan Hajnoczi <address@hidden>
> CC: Fam Zheng <address@hidden>
> CC: Kevin Wolf <address@hidden>
> CC: Max Reitz <address@hidden>
> CC: Jeff Cody <address@hidden>
> CC: Eric Blake <address@hidden>
> ---
> 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?