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

[Fam Zheng]

On Sun, 02/14 22:49, Jeff Cody wrote:
> On Feb 14, 2016 21:19, "Fam Zheng" <address@hidden> wrote:
> >
> > On Mon, 02/08 13:54, Max Reitz wrote:
> > > On 07.02.2016 13:46, Fam Zheng wrote:
> > > > On Sat, 02/06 14:24, Max Reitz wrote:
> > > >> On 05.02.2016 03:00, 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.
> > > >>>
> > > >>> The output of iotests 109 is updated because we now report the
> offset
> > > >>> and len slightly differently in mirroring progress.
> > > >>>
> > > >>> Signed-off-by: Fam Zheng <address@hidden>
> > > >>> ---
> > > >>>  block/mirror.c             | 335
> +++++++++++++++++++++++++++------------------
> > > >>>  tests/qemu-iotests/109.out |  80 +++++------
> > > >>>  trace-events               |   1 -
> > > >>>  3 files changed, 243 insertions(+), 173 deletions(-)
> > > >>>
> > > >>> diff --git a/block/mirror.c b/block/mirror.c
> > > >>> index 2c0edfa..48cd0b3 100644
> > > >>> --- a/block/mirror.c
> > > >>> +++ b/block/mirror.c
> > > >>
> > > >> [...]
> > > >>
> > > >>> @@ -449,16 +520,16 @@ static void coroutine_fn mirror_run(void
> *opaque)
> > > >>>       */
> > > >>>      bdrv_get_backing_filename(s->target, backing_filename,
> > > >>>                                sizeof(backing_filename));
> > > >>> -    if (backing_filename[0] && !s->target->backing) {
> > > >>> -        ret = bdrv_get_info(s->target, &bdi);
> > > >>> -        if (ret < 0) {
> > > >>> -            goto immediate_exit;
> > > >>> -        }
> > > >>> -        if (s->granularity < bdi.cluster_size) {
> > > >>> -            s->buf_size = MAX(s->buf_size, bdi.cluster_size);
> > > >>> -            s->cow_bitmap = bitmap_new(length);
> > > >>> -        }
> > > >>> +    if (!bdrv_get_info(s->target, &bdi) && bdi.cluster_size) {
> > > >>
> > > >> This should be bdi.has_cluster_size...
> > > >
> > > > has_cluster_size is a member of ImageInfo not BlockDriverInfo, and is
> derived
> > > > from (bdi.cluster_size != 0).
> > >
> > > You're right, my bad.
> > >
> > > >>> +        target_cluster_size = bdi.cluster_size;
> > > >>
> > > >> ...and maybe we want an explicit minimum of BDRV_SECTOR_SIZE here;
> but I
> > > >> guess this is already assumed all over the block layer, so it may be
> > > >> fine without.
> > > >
> > > > Okay, it doesn't hurt to add an assert here.
> > >
> > > I'd be happy to take the patch without, too (although I wouldn't decline
> > > a follow-up adding an assertion).
> > >
> > > Reviewed-by: Max Reitz <address@hidden>
> >
> > Thanks! Shall we merge this now?
> >
> > Fam
> >
> 
> I think so - I'll go ahead and apply it to my block branch, unless there
> are any objections.

Great! Thanks Jeff.

Fam