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

[Fam Zheng <famz@redhat.com>]

On Wed, 01/06 18:53, Max Reitz wrote:
> On 05.01.2016 09:46, 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 | 347 +++++++++++++++++++++++++++++++++++----------------------
> >  trace-events   |   1 -
> >  2 files changed, 216 insertions(+), 132 deletions(-)
> > 
> > diff --git a/block/mirror.c b/block/mirror.c
> > index f201f2b..e3e9fad 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,93 @@ 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;
> > +        if (tail_need_cow) {
> > +            /* In this case, tail must be aligned already, so we just make sure
> > +             * the shrink is also aligned. */
> > +            shrink -= shrink % s->target_cluster_sectors;
> > +        }
> > +        assert(shrink);
> > +        diff -= shrink;
> > +        *nb_sectors -= shrink;
> > +    }
> 
> Hm, looking at this closer... If we get here with tail_need_cow not
> being set, we may end up with an unaligned tail, which then may need COW
> (because it points to somewhere else than before).
> 
> On the other hand, if we get here with tail_need_cow being set, shrink
> will be decreased so that it will only remove an aligned number of
> sectors from *nb_sectors; however, because shrink is increased, that
> means that *nb_sectors may then still be too large. Also, because of the
> shrink, the tail may in fact not need COW any more.

You're right. I'll fix this again. But I don't think we care about the "not
need COW any more" case. Let's keep this function simple as it's not the
hottest path.

Fam

> 
> Should we do this check before we test whether we need COW and do the
> correction in a way that ensures that the cluster adjustment can never
> increase *nb_sectors beyond chunk_sectors * s->max_iov?