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

[Max Reitz]

On 05.01.2016 09:46, 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.
> 
> Signed-off-by: Fam Zheng <address@hidden>
> ---
>  block/mirror.c | 347 
> +++++++++++++++++++++++++++++++++++----------------------
>  trace-events   |   1 -
>  2 files changed, 216 insertions(+), 132 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index f201f2b..e3e9fad 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -46,7 +46,6 @@ typedef struct MirrorBlockJob {
>      BlockdevOnError on_source_error, on_target_error;
>      bool synced;
>      bool should_complete;
> -    int64_t sector_num;
>      int64_t granularity;
>      size_t buf_size;
>      int64_t bdev_length;
> @@ -63,6 +62,8 @@ typedef struct MirrorBlockJob {
>      int ret;
>      bool unmap;
>      bool waiting_for_io;
> +    int target_cluster_sectors;
> +    int max_iov;
>  } MirrorBlockJob;
>  
>  typedef struct MirrorOp {
> @@ -158,115 +159,93 @@ static void mirror_read_complete(void *opaque, int ret)
>                      mirror_write_complete, op);
>  }
>  
> -static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
> +/* Round sector_num and/or nb_sectors to target cluster if COW is needed, and
> + * return the offset of the adjusted tail sector against original. */
> +static int mirror_cow_align(MirrorBlockJob *s,
> +                            int64_t *sector_num,
> +                            int *nb_sectors)
> +{
> +    bool head_need_cow, tail_need_cow;
> +    int diff = 0;
> +    int chunk_sectors = s->granularity >> BDRV_SECTOR_BITS;
> +
> +    head_need_cow = !test_bit(*sector_num / chunk_sectors, s->cow_bitmap);
> +    tail_need_cow = !test_bit((*sector_num + *nb_sectors - 1) / 
> chunk_sectors,
> +                              s->cow_bitmap);
> +    if (head_need_cow || tail_need_cow) {
> +        int64_t align_sector_num;
> +        int align_nb_sectors;
> +        bdrv_round_to_clusters(s->target, *sector_num, *nb_sectors,
> +                               &align_sector_num, &align_nb_sectors);
> +        if (tail_need_cow) {
> +            diff = align_sector_num + align_nb_sectors
> +                   - (*sector_num + *nb_sectors);
> +            assert(diff >= 0);
> +            *nb_sectors += diff;
> +        }
> +        if (head_need_cow) {
> +            int d = *sector_num - align_sector_num;
> +            assert(d >= 0);
> +            *sector_num = align_sector_num;
> +            *nb_sectors += d;
> +        }
> +    }
> +
> +    /* If the resulting chunks are more than max_iov, we have to shrink it
> +     * under the alignment restriction. */
> +    if (*nb_sectors > chunk_sectors * s->max_iov) {
> +        int shrink = *nb_sectors - chunk_sectors * s->max_iov;
> +        if (tail_need_cow) {
> +            /* In this case, tail must be aligned already, so we just make 
> sure
> +             * the shrink is also aligned. */
> +            shrink -= shrink % s->target_cluster_sectors;
> +        }
> +        assert(shrink);
> +        diff -= shrink;
> +        *nb_sectors -= shrink;
> +    }

Hm, looking at this closer... If we get here with tail_need_cow not
being set, we may end up with an unaligned tail, which then may need COW
(because it points to somewhere else than before).

On the other hand, if we get here with tail_need_cow being set, shrink
will be decreased so that it will only remove an aligned number of
sectors from *nb_sectors; however, because shrink is increased, that
means that *nb_sectors may then still be too large. Also, because of the
shrink, the tail may in fact not need COW any more.

Should we do this check before we test whether we need COW and do the
correction in a way that ensures that the cluster adjustment can never
increase *nb_sectors beyond chunk_sectors * s->max_iov?

Max

signature.asc
Description: OpenPGP digital signature