Re: [Qemu-block] [PATCH v5 08/15] qcow2: skip writing zero buffers to empty COW areas

[Alberto Garcia]

On Wed 01 Nov 2017 04:44:01 PM CET, Anton Nefedov wrote:
> If COW areas of the newly allocated clusters are zeroes on the backing image,
> efficient bdrv_write_zeroes(flags=BDRV_REQ_ALLOCATE) can be used on the whole
> cluster instead of writing explicit zero buffers later in perform_cow().
>
> iotest 060:
> write to the discarded cluster does not trigger COW anymore.
> so, break on write_aio event instead, will work for the test
> (but write won't fail anymore, so update reference output)
>
> iotest 066:
> cluster-alignment areas that were not really COWed are now detected
> as zeroes, hence the initial write has to be exactly the same size for
> the maps to match
>
> Signed-off-by: Anton Nefedov <address@hidden>
> ---
>  block/qcow2.h              |  6 +++++
>  block/qcow2-cluster.c      |  3 ++-
>  block/qcow2.c              | 67 
> ++++++++++++++++++++++++++++++++++++++++++++--
>  block/trace-events         |  1 +
>  tests/qemu-iotests/060     |  2 +-
>  tests/qemu-iotests/060.out |  3 ++-
>  tests/qemu-iotests/066     |  2 +-
>  tests/qemu-iotests/066.out |  4 +--
>  8 files changed, 80 insertions(+), 8 deletions(-)
>
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 782a206..e312e48 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -377,6 +377,12 @@ typedef struct QCowL2Meta
>      Qcow2COWRegion cow_end;
>  
>      /**
> +     * Indicates that both COW areas are empty (nb_bytes == 0)
> +     * or filled with zeroes and do not require any more copying
> +     */
> +    bool zero_cow;

I think the terminology elsewhere is "COW regions".

Also, in this patch that field doesn't really seem to track the case
where both regions have nb_bytes == 0, or does it? It seems to be
checked separately in all cases.

Example:

> -    if (start->nb_bytes == 0 && end->nb_bytes == 0) {
> +    if ((start->nb_bytes == 0 && end->nb_bytes == 0) || m->zero_cow) {
>          return 0;
>      }

Here, 'if (m->zero_cow)' would suffice.

> +        /* If both COW regions are zeroes already, skip this too */
> +        if (m->zero_cow) {
> +            continue;
> +        }

Same as above

> +static bool is_zero_cow(BlockDriverState *bs, QCowL2Meta *m)
> +{
> +    if (bs->encrypted) {
> +        return false;
> +    }
> +
> +    if (m->cow_start.nb_bytes != 0 &&
> +        !is_zero(bs, m->offset + m->cow_start.offset, m->cow_start.nb_bytes))
> +    {
> +        return false;
> +    }
> +
> +    if (m->cow_end.nb_bytes != 0 &&
> +        !is_zero(bs, m->offset + m->cow_end.offset, m->cow_end.nb_bytes))
> +    {
> +        return false;
> +    }

Why do you need to check that nb_bytes != 0? Doesn't is_zero() do that
already?

Berto