Re: [Qemu-devel] [PATCH v8 1/2] mirror: Rewrite mirror_iteration

[Fam Zheng <famz@redhat.com>]

On Mon, 01/04 20:27, Max Reitz wrote:
> On 24.12.2015 04:15, 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 <famz@redhat.com>
> > ---
> >  block/mirror.c | 344 +++++++++++++++++++++++++++++++++++----------------------
> >  trace-events   |   1 -
> >  2 files changed, 213 insertions(+), 132 deletions(-)
> > 
> > diff --git a/block/mirror.c b/block/mirror.c
> > index f201f2b..0081c2e 100644
> > --- a/block/mirror.c
> > +++ b/block/mirror.c
> > @@ -46,7 +46,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;
> > @@ -63,6 +62,8 @@ typedef struct MirrorBlockJob {
> >      int ret;
> >      bool unmap;
> >      bool waiting_for_io;
> > +    int target_cluster_sectors;
> > +    int max_iov;
> >  } MirrorBlockJob;
> >  
> >  typedef struct MirrorOp {
> > @@ -158,115 +159,90 @@ 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 tail sector against original. */
> > +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 chunk_sectors = s->granularity >> BDRV_SECTOR_BITS;
> > +
> > +    head_need_cow = !test_bit(*sector_num / chunk_sectors, s->cow_bitmap);
> > +    tail_need_cow = !test_bit((*sector_num + *nb_sectors - 1) / chunk_sectors,
> > +                              s->cow_bitmap);
> > +    if (head_need_cow || tail_need_cow) {
> > +        int64_t align_sector_num;
> > +        int align_nb_sectors;
> > +        bdrv_round_to_clusters(s->target, *sector_num, *nb_sectors,
> > +                               &align_sector_num, &align_nb_sectors);
> > +        if (tail_need_cow) {
> > +            diff = align_sector_num + align_nb_sectors
> > +                   - (*sector_num + *nb_sectors);
> > +            assert(diff >= 0);
> > +            *nb_sectors += diff;
> > +        }
> > +        if (head_need_cow) {
> > +            int d = *sector_num - align_sector_num;
> > +            assert(d >= 0);
> > +            *sector_num = align_sector_num;
> > +            *nb_sectors += d;
> > +        }
> > +    }
> > +
> > +    /* If the resulting chunks are more than max_iov, we have to shrink it
> > +     * under the alignment restriction. */
> > +    if (*nb_sectors / chunk_sectors > s->max_iov) {
> > +        int shrink = *nb_sectors / chunk_sectors - s->max_iov;
> 
> Isn't this missing a "shrink *= chunk_sectors"? Because after this line,
> shrink's unit seems to be chunks, but the following code seems to presume it

You are right, and the if condition above should be careful of partial chunks
too. Will fix.

Fam