Re: [Qemu-block] [PATCH v6 1/2] mirror: Rewrite mirror_iteration

[Fam Zheng]

On Fri, 12/18 17:41, Max Reitz wrote:
> On 20.11.2015 11:12, Fam Zheng wrote:
> > The "pnum < nb_sectors" condition in deciding whether to actually copy
> > data is unnecessarily strict, and the qiov initialization is
> > unnecessarily for bdrv_aio_write_zeroes and bdrv_aio_discard.
> > 
> > Rewrite mirror_iteration to fix both flaws.
> > 
> > Signed-off-by: Fam Zheng <address@hidden>
> > 
> > ---
> > v5: Address Max's review comments:
> >     - Fix parameter name of mirror_do_read().
> >     - Simplify the buffer waiting loop in mirror_do_read.
> >     - Don't skip next dirty chunk when collecting consective dirty
> >       chunks.
> >     - Check sector range when collecting consective dirty chunks.
> >     - Don't misuse a negative return value of
> >       bdrv_get_block_status_above.
> > ---
> >  block/mirror.c | 307 
> > +++++++++++++++++++++++++++++++++++----------------------
> >  1 file changed, 187 insertions(+), 120 deletions(-)
> > 
> > diff --git a/block/mirror.c b/block/mirror.c
> > index 52c9abf..ff8149d 100644
> > --- a/block/mirror.c
> > +++ b/block/mirror.c
> > @@ -45,7 +45,6 @@ typedef struct MirrorBlockJob {
> >      BlockdevOnError on_source_error, on_target_error;
> >      bool synced;
> >      bool should_complete;
> > -    int64_t sector_num;
> >      int64_t granularity;
> >      size_t buf_size;
> >      int64_t bdev_length;
> > @@ -157,113 +156,76 @@ static void mirror_read_complete(void *opaque, int 
> > ret)
> >                      mirror_write_complete, op);
> >  }
> >  
> > -static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
> > +/* Round sector_num and/or nb_sectors to target cluster if COW is needed, 
> > and
> > + * return the offset of the adjusted ending sector against
> > + * sector_num + nb_sectors. */
> > +static int mirror_cow_align(MirrorBlockJob *s,
> > +                            int64_t *sector_num,
> > +                            int *nb_sectors)
> > +{
> > +    bool head_need_cow, tail_need_cow;
> > +    int diff = 0;
> > +    int sectors_per_chunk = s->granularity >> BDRV_SECTOR_BITS;
> > +
> > +    head_need_cow = !test_bit(*sector_num / sectors_per_chunk, 
> > s->cow_bitmap);
> > +    tail_need_cow = !test_bit((*sector_num + *nb_sectors) / 
> > sectors_per_chunk,
> 
> Should this be (*sector_num + *nb_sectors - 1) so that we actually check
> the last chunk of the sector range (instead of the next chunk if
> *sector_num + *nb_sectors is divisible by sectors_per_chunk).

Yes, you're right.

> 
> > +                             s->cow_bitmap);
> > +    if (head_need_cow || tail_need_cow) {
> > +        int64_t rounded_sector_num;
> > +        int rounded_nb_sectors;
> > +        bdrv_round_to_clusters(s->target, *sector_num, *nb_sectors,
> > +                               &rounded_sector_num, &rounded_nb_sectors);
> > +        assert(*sector_num >= rounded_sector_num);
> > +        assert(rounded_nb_sectors >= *nb_sectors);
> 
> You could move these assertions into the following conditional blocks,
> that would make more sense to me:
> 
> > +        if (tail_need_cow) {
> > +            int diff = rounded_sector_num + rounded_nb_sectors
> > +                        - (*sector_num + *nb_sectors);
> 
> assert(diff >= 0);
> 
> Also, I don't like shadowing of variables very much.
> 
> > +            *nb_sectors += diff;
> > +        }
> > +        if (head_need_cow) {
> > +            int diff = *sector_num - rounded_sector_num;
> 
> assert(diff >= 0);

Okay.

> 
> > +            *sector_num = rounded_sector_num;
> > +            *nb_sectors += diff;
> > +        }
> > +    }
> > +    return diff;
> 
> diff is always 0.
> 
> (This is why I don't like shadowing of variables very much.)

The fist shadowing is a mistake, will fix, and I'll also rename the second.

> 
> > +}
> > +
> > +/* Submit async read while handling COW.
> > + * Returns: nb_sectors if no alignment is necessary, or
> > + *          (new_end - sector_num) if tail is rounded up or down due to
> > + *          alignment or buffer limit.
> > + */
> > +static int mirror_do_read(MirrorBlockJob *s, int64_t sector_num,
> > +                          int nb_sectors)
> >  {
> >      BlockDriverState *source = s->common.bs;
> > -    int nb_sectors, sectors_per_chunk, nb_chunks;
> > -    int64_t end, sector_num, next_chunk, next_sector, hbitmap_next_sector;
> > -    uint64_t delay_ns = 0;
> > +    int sectors_per_chunk, nb_chunks;
> > +    int ret = nb_sectors;
> >      MirrorOp *op;
> > -    int pnum;
> > -    int64_t ret;
> >  
> > -    s->sector_num = hbitmap_iter_next(&s->hbi);
> > -    if (s->sector_num < 0) {
> > -        bdrv_dirty_iter_init(s->dirty_bitmap, &s->hbi);
> > -        s->sector_num = hbitmap_iter_next(&s->hbi);
> > -        trace_mirror_restart_iter(s, 
> > bdrv_get_dirty_count(s->dirty_bitmap));
> > -        assert(s->sector_num >= 0);
> > -    }
> > -
> > -    hbitmap_next_sector = s->sector_num;
> > -    sector_num = s->sector_num;
> >      sectors_per_chunk = s->granularity >> BDRV_SECTOR_BITS;
> > -    end = s->bdev_length / BDRV_SECTOR_SIZE;
> >  
> > -    /* Extend the QEMUIOVector to include all adjacent blocks that will
> > -     * be copied in this operation.
> > -     *
> > -     * We have to do this if we have no backing file yet in the 
> > destination,
> > -     * and the cluster size is very large.  Then we need to do COW 
> > ourselves.
> > -     * The first time a cluster is copied, copy it entirely.  Note that,
> > -     * because both the granularity and the cluster size are powers of two,
> > -     * the number of sectors to copy cannot exceed one cluster.
> > -     *
> > -     * We also want to extend the QEMUIOVector to include more adjacent
> > -     * dirty blocks if possible, to limit the number of I/O operations and
> > -     * run efficiently even with a small granularity.
> > -     */
> > -    nb_chunks = 0;
> > -    nb_sectors = 0;
> > -    next_sector = sector_num;
> > -    next_chunk = sector_num / sectors_per_chunk;
> > +    if (s->cow_bitmap) {
> > +        ret += mirror_cow_align(s, &sector_num, &nb_sectors);
> 
> mirror_cow_align() always returns 0, but I assume it is supposed to
> return the difference of nb_sectors before and after the call (in which
> case this line is correct).

Yes, except it is supposed to return the "progress" (in case of rounding to
target, count the last sector instead of nb_sectors).

> 
> > +    }
> > +    /* We can only handle as much as buf_size at a time. */
> > +    nb_sectors = MIN(s->buf_size >> BDRV_SECTOR_BITS, nb_sectors);
> > +    assert(nb_sectors);
> 
> Maybe we should move these three lines before the mirror_cow_align()
> call. I don't think it will make a difference in practice, but it seems
> cleaner to me that way.
> 
> > +    /* The sector range must meet granularity because:
> > +     * 1) Caller passes in aligned values;
> > +     * 2) mirror_cow_align is used only when target cluster is larger. */
> > +    assert(!(nb_sectors % sectors_per_chunk));
> > +    assert(!(sector_num % sectors_per_chunk));
> > +    nb_chunks = nb_sectors / sectors_per_chunk;
> 
> [...]
> 
> > +static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
> > +{
> 
> [...]
> 
> > +    while (nb_chunks > 0 && sector_num < end) {
> > +        int ret;
> > +        int io_sectors;
> > +        enum MirrorMethod {
> > +            MIRROR_METHOD_COPY,
> > +            MIRROR_METHOD_ZERO,
> > +            MIRROR_METHOD_DISCARD
> > +        } mirror_method = MIRROR_METHOD_COPY;
> > +
> > +        assert(!(sector_num % sectors_per_chunk));
> > +        ret = bdrv_get_block_status_above(source, NULL, sector_num,
> > +                                          nb_chunks * sectors_per_chunk,
> > +                                          &io_sectors);
> > +        if (ret < 0) {
> > +            io_sectors = nb_chunks * sectors_per_chunk;
> > +        }
> > +
> > +        io_sectors -= io_sectors % sectors_per_chunk;
> > +        if (io_sectors < sectors_per_chunk) {
> > +            io_sectors = sectors_per_chunk;
> > +        } else if (ret > 0 && !(ret & BDRV_BLOCK_DATA)) {
> 
> Why > 0 and not >= 0?

Good idea, >= 0 looks better (0 means unknown or unallocated, and discard is
appropriate).

Fam

> 
> Max
> 
> > +            int64_t target_sector_num;
> > +            int target_nb_sectors;
> > +            bdrv_round_to_clusters(s->target, sector_num, io_sectors,
> > +                                   &target_sector_num, &target_nb_sectors);
> > +            if (target_sector_num == sector_num &&
> > +                target_nb_sectors == io_sectors) {
> > +                mirror_method = ret & BDRV_BLOCK_ZERO ?
> > +                                    MIRROR_METHOD_ZERO :
> > +                                    MIRROR_METHOD_DISCARD;
> > +            }
> > +        }
> > +
> > +        switch (mirror_method) {
> > +        case MIRROR_METHOD_COPY:
> > +            io_sectors = mirror_do_read(s, sector_num, 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);
> > +            break;
> > +        default:
> > +            abort();
> > +        }
> > +        assert(io_sectors);
> > +        sector_num += io_sectors;
> > +        nb_chunks -= io_sectors / sectors_per_chunk;
> > +        delay_ns += ratelimit_calculate_delay(&s->limit, io_sectors);
> >      }
> >      return delay_ns;
> >  }
> > 
> 
>