Re: [Qemu-devel] [PATCH v2 7/7] qcow2: Merge the writing of the COW regions with the guest data

[Kevin Wolf <kwolf@redhat.com>]

Am 07.06.2017 um 16:08 hat Alberto Garcia geschrieben:
> If the guest tries to write data that results on the allocation of a
> new cluster, instead of writing the guest data first and then the data
> from the COW regions, write everything together using one single I/O
> operation.
> 
> This can improve the write performance by 25% or more, depending on
> several factors such as the media type, the cluster size and the I/O
> request size.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>

> diff --git a/block/qcow2.c b/block/qcow2.c
> index b3ba5daa93..89be2083d4 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1575,6 +1575,44 @@ fail:
>      return ret;
>  }
>  
> +/* Check if it's possible to merge a write request with the writing of
> + * the data from the COW regions */
> +static bool can_merge_cow(uint64_t offset, unsigned bytes,
> +                          QEMUIOVector *hd_qiov, QCowL2Meta *l2meta)
> +{
> +    QCowL2Meta *m;
> +
> +    for (m = l2meta; m != NULL; m = m->next) {
> +        /* If both COW regions are empty then there's nothing to merge */
> +        if (m->cow_start.nb_bytes == 0 && m->cow_end.nb_bytes == 0) {
> +            continue;
> +        }
> +
> +        /* The data (middle) region must be immediately after the
> +         * start region */
> +        if (l2meta_cow_start(m) + m->cow_start.nb_bytes != offset) {
> +            continue;
> +        }
> +
> +        /* The end region must be immediately after the data (middle)
> +         * region */
> +        if (m->offset + m->cow_end.offset != offset + bytes) {
> +            continue;
> +        }
> +
> +        /* Make sure that adding both COW regions to the QEMUIOVector
> +         * does not exceed IOV_MAX */
> +        if (hd_qiov->niov > IOV_MAX - 2) {
> +            continue;
> +        }
> +
> +        m->data_qiov = hd_qiov;

I don't think having this side effect is good for a function that is
called can_xyz(). I'd either call it merge_cow() or move the assignment
to the caller.

If we consider allowing to merge multiple L2Metas in the future, the
actual merging could become more complicated, so maybe merge_cow() is
the better option.

Kevin