Re: [Qemu-devel] [PATCH 1/3] mirror: Don't extend the last sub-chunk

[Fam Zheng <famz@redhat.com>]

On Tue, 04/19 22:33, Max Reitz wrote:
> On 19.04.2016 12:09, Fam Zheng wrote:
> > The last sub-chunk is rounded up to the copy granularity in the target
> > image, resulting in a larger size than the source.
> > 
> > Add a function to clip the copied sectors to the end.
> > 
> > This undoes the "wrong" changes to tests/qemu-iotests/109.out in
> > e5b43573e28. The remaining two offset changes are okay.
> > 
> > Reported-by: Kevin Wolf <kwolf@redhat.com>
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block/mirror.c             | 10 ++++++++++
> >  tests/qemu-iotests/109.out | 44 ++++++++++++++++++++++----------------------
> >  2 files changed, 32 insertions(+), 22 deletions(-)
> > 
> > diff --git a/block/mirror.c b/block/mirror.c
> > index c2cfc1a..b6387f1 100644
> > --- a/block/mirror.c
> > +++ b/block/mirror.c
> > @@ -205,6 +205,14 @@ static inline void mirror_wait_for_io(MirrorBlockJob *s)
> >      s->waiting_for_io = false;
> >  }
> >  
> > +static inline void mirror_clip_sectors(MirrorBlockJob *s,
> > +                                       int64_t sector_num,
> > +                                       int *nb_sectors)
> > +{
> > +    *nb_sectors = MIN(*nb_sectors,
> > +                      s->bdev_length / BDRV_SECTOR_SIZE - sector_num);
> > +}
> > +
> >  /* 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
> > @@ -240,6 +248,7 @@ static int mirror_do_read(MirrorBlockJob *s, int64_t sector_num,
> >          mirror_wait_for_io(s);
> >      }
> >  
> > +    mirror_clip_sectors(s, sector_num, &nb_sectors);
> >      /* Allocate a MirrorOp that is used as an AIO callback.  */
> >      op = g_new(MirrorOp, 1);
> >      op->s = s;
> 
> I think you want to adjust the ret value, too. It doesn't really matter
> in practice (the caller just overshoots the end of the image instead of
> getting precisely to its end), but I wouldn't rely on this.
> 
> > @@ -276,6 +285,7 @@ static void mirror_do_zero_or_discard(MirrorBlockJob *s,
> >  {
> >      MirrorOp *op;
> >  
> > +    mirror_clip_sectors(s, sector_num, &nb_sectors);
> >      /* Allocate a MirrorOp that is used as an AIO callback. The qiov is zeroed
> >       * so the freeing in mirror_iteration_done is nop. */
> >      op = g_new0(MirrorOp, 1);
> 
> I think it would be best to just pull out the mirror_clip_sectors() from
> these functions and put it above the "switch (mirror_method)" in
> mirror_iteration().
> 
> We'd just have to make sure that mirror_cow_align() will not increase
> nb_sectors such that it points beyond the image end. It can do that,
> because and image's size does not need to be aligned to its cluster
> size. But just putting a mirror_clip_sectors() below the
> bdrv_round_to_clusters() call in mirror_cow_align() should fix that.
> 
> Then you wouldn't need to worry about fixing the ret value in
> mirror_do_read().

Sounds good, will do this.

Thanks,
Fam