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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2214 bytes --]

On 03.02.2016 02:52, 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 <famz@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..8b06a31 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c

[...]

> @@ -449,16 +519,17 @@ 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);
> -        }
> +    ret = bdrv_get_info(s->target, &bdi);
> +    if (ret < 0) {
> +        goto immediate_exit;
>      }
> +    if (backing_filename[0] && !s->target->backing
> +        && s->granularity < bdi.cluster_size) {
> +        s->buf_size = MAX(s->buf_size, bdi.cluster_size);
> +        s->cow_bitmap = bitmap_new(length);
> +    }
> +    s->target_cluster_sectors = bdi.cluster_size >> BDRV_SECTOR_BITS;
> +    s->max_iov = MIN(s->common.bs->bl.max_iov, s->target->bl.max_iov);

Sorry for noticing this so late: bdi.cluster_size is optional. I think
we should default to BDRV_SECTOR_SIZE if bdi.has_cluster_size is false,
and maybe even if bdrv_get_info() failed.

(I noticed this because iotest 94 fails because nbd does not support
bdrv_get_info().)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]