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

[Max Reitz <mreitz@redhat.com>]

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

On 13.01.2016 03:50, 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             | 333 +++++++++++++++++++++++++++------------------
>  tests/qemu-iotests/109.out |  80 +++++------
>  trace-events               |   1 -
>  3 files changed, 242 insertions(+), 172 deletions(-)

Unfortunately, this patch now conflicts with your series that added the
@file parameter to bdrv_get_block_status():

> 
> diff --git a/block/mirror.c b/block/mirror.c
> index f201f2b..7606aea 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c


[...]

> @@ -158,115 +159,79 @@ static void mirror_read_complete(void *opaque, int ret)
>                      mirror_write_complete, op);

[...]

> +static int mirror_do_read(MirrorBlockJob *s, int64_t sector_num,
> +                          int nb_sectors)
>  {
>      BlockDriverState *source = s->common.bs;
> -    int nb_sectors, sectors_per_chunk, nb_chunks, max_iov;
> -    int64_t end, sector_num, next_chunk, next_sector, hbitmap_next_sector;
> -    uint64_t delay_ns = 0;
> +    int sectors_per_chunk, nb_chunks;
> +    int ret = nb_sectors;
>      MirrorOp *op;
> -    int pnum;
> -    int64_t ret;

I guess a

-    BlockDriverState *file;

is needed here.

>  
> -    max_iov = MIN(source->bl.max_iov, s->target->bl.max_iov);

[...]

>      /* Copy the dirty cluster.  */
>      s->in_flight++;
>      s->sectors_in_flight += nb_sectors;
>      trace_mirror_one_iteration(s, sector_num, nb_sectors);
>  
> -    ret = bdrv_get_block_status_above(source, NULL, sector_num,
> -                                      nb_sectors, &pnum);

This should now be:

-                                      nb_sectors, &pnum, &file);

> -    if (ret < 0 || pnum < nb_sectors ||
> -            (ret & BDRV_BLOCK_DATA && !(ret & BDRV_BLOCK_ZERO))) {
> -        bdrv_aio_readv(source, sector_num, &op->qiov, nb_sectors,
> -                       mirror_read_complete, op);
> -    } else if (ret & BDRV_BLOCK_ZERO) {
> +    bdrv_aio_readv(source, sector_num, &op->qiov, nb_sectors,
> +                   mirror_read_complete, op);
> +    return ret;
> +}

[...]

> +    /* Clear dirty bits before querying the block status, because
> +     * calling bdrv_get_block_status_above could yield - if some blocks are
> +     * marked dirty in this window, we need to know.
> +     */
> +    bdrv_reset_dirty_bitmap(s->dirty_bitmap, sector_num,
> +                            nb_chunks * sectors_per_chunk);
> +    bitmap_set(s->in_flight_bitmap, sector_num / sectors_per_chunk, nb_chunks);
> +    while (nb_chunks > 0 && sector_num < end) {
> +        int ret;
> +        int io_sectors;

+        BlockDriverState *file;

> +        enum MirrorMethod {
> +            MIRROR_METHOD_COPY,
> +            MIRROR_METHOD_ZERO,
> +            MIRROR_METHOD_DISCARD
> +        } mirror_method = MIRROR_METHOD_COPY;
> +
> +        assert(!(sector_num % sectors_per_chunk));
> +        ret = bdrv_get_block_status_above(source, NULL, sector_num,
> +                                          nb_chunks * sectors_per_chunk,
> +                                          &io_sectors);

+                                          &io_sectors, &file);

So it's not too difficult but it's not trivial either, so I was afraid
to do these changes myself while applying.

Max

> +        if (ret < 0) {
> +            io_sectors = nb_chunks * sectors_per_chunk;
> +        }

[...]


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