qemu-block
[Top][All Lists]
Advanced

[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?



reply via email to

[Prev in Thread] Current Thread [Next in Thread]