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 <f...@redhat.com>
> >>> ---
> >>> 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 <mre...@redhat.com>
Thanks! Shall we merge this now?
Fam
