Re: [Qemu-block] [PATCH v2 7/7] mirror: improve performance of mirroring of empty disk

[Fam Zheng]

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 <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;
> > +            }

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