Re: [Qemu-devel] [PATCH v7 8/9] qcow2: skip writing zero buffers to empty COW areas

[Max Reitz <mreitz@redhat.com>]

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

On 2018-01-18 18:49, 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.
> Use a backing image instead.
> 
> 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 <anton.nefedov@virtuozzo.com>
> ---
>  qapi/block-core.json       |  4 ++-
>  block/qcow2.h              |  6 +++++
>  block/qcow2-cluster.c      |  2 +-
>  block/qcow2.c              | 66 ++++++++++++++++++++++++++++++++++++++++++++--
>  block/trace-events         |  1 +
>  tests/qemu-iotests/060     | 26 +++++++++++-------
>  tests/qemu-iotests/060.out |  5 +++-
>  tests/qemu-iotests/066     |  2 +-
>  tests/qemu-iotests/066.out |  4 +--
>  9 files changed, 98 insertions(+), 18 deletions(-)

[...]

> @@ -1875,6 +1880,52 @@ static bool is_zero(BlockDriverState *bs, int64_t offset, int64_t bytes)
>      return res >= 0 && (res & BDRV_BLOCK_ZERO) && nr == bytes;
>  }
>  
> +static bool is_zero_cow(BlockDriverState *bs, QCowL2Meta *m)
> +{
> +    return is_zero(bs, m->offset + m->cow_start.offset,
> +                   m->cow_start.nb_bytes) &&
> +           is_zero(bs, m->offset + m->cow_end.offset, m->cow_end.nb_bytes);
> +}
> +
> +static int handle_alloc_space(BlockDriverState *bs, QCowL2Meta *l2meta)
> +{
> +    BDRVQcow2State *s = bs->opaque;
> +    QCowL2Meta *m;
> +
> +    for (m = l2meta; m != NULL; m = m->next) {
> +        int ret;
> +
> +        if (!m->cow_start.nb_bytes && !m->cow_end.nb_bytes) {
> +            continue;
> +        }
> +
> +        if (bs->encrypted) {
> +            continue;
> +        }

Not sure if the compiler optimizes this anyway, but I'd pull this out of
the loop.

Maybe you could put all of the conditions under which this function can
actually do something at its beginning: That is, we can't do anything if
the BDS is encrypted or if bs->file does not support BDRV_REQ_ALLOCATE
(and then you just call this function unconditionally in
qcow2_co_pwritev()).

> +        if (!is_zero_cow(bs, m)) {
> +            continue;
> +        }

Is this really efficient?  I remember someone complaining about
bdrv_co_block_status() being kind of slow on some filesystems, so
there'd be a tradeoff depending on how it compares to just reading up to
two clusters from the backing file -- especially considering that the OS
can query the same information just as quickly, and thus the only
overhead the read should have is a memset() (assuming the OS is clever).

So basically my question is whether it would be better to just skip this
if we have any backing file at all and only do this optimization if
there is none.

> +
> +        BLKDBG_EVENT(bs->file, BLKDBG_CLUSTER_ALLOC_SPACE);
> +        /* instead of writing zero COW buffers,
> +           efficiently zero out the whole clusters */
> +        ret = bdrv_co_pwrite_zeroes(bs->file, m->alloc_offset,
> +                                    m->nb_clusters * s->cluster_size,
> +                                    BDRV_REQ_ALLOCATE);
> +        if (ret < 0) {
> +            if (ret != -ENOTSUP && ret != -EAGAIN) {
> +                return ret;
> +            }
> +            continue;
> +        }
> +
> +        trace_qcow2_skip_cow(qemu_coroutine_self(), m->offset, m->nb_clusters);
> +        m->skip_cow = true;
> +    }
> +    return 0;
> +}
> +
>  static coroutine_fn int qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset,
>                                           uint64_t bytes, QEMUIOVector *qiov,
>                                           int flags)

[...]

> diff --git a/block/trace-events b/block/trace-events
> index 11c8d5f..c9fa596 100644
> --- a/block/trace-events
> +++ b/block/trace-events
> @@ -61,6 +61,7 @@ qcow2_writev_done_part(void *co, int cur_bytes) "co %p cur_bytes %d"
>  qcow2_writev_data(void *co, uint64_t offset) "co %p offset 0x%" PRIx64
>  qcow2_pwrite_zeroes_start_req(void *co, int64_t offset, int count) "co %p offset 0x%" PRIx64 " count %d"
>  qcow2_pwrite_zeroes(void *co, int64_t offset, int count) "co %p offset 0x%" PRIx64 " count %d"
> +qcow2_skip_cow(void* co, uint64_t offset, int nb_clusters) "co %p offset 0x%" PRIx64 " nb_clusters %d"

Nit: Should be "void *co" to match the rest.

>  
>  # block/qcow2-cluster.c
>  qcow2_alloc_clusters_offset(void *co, uint64_t offset, int bytes) "co %p offset 0x%" PRIx64 " bytes %d"

[...]

> diff --git a/tests/qemu-iotests/066 b/tests/qemu-iotests/066
> index 8638217..3c216a1 100755
> --- a/tests/qemu-iotests/066
> +++ b/tests/qemu-iotests/066
> @@ -71,7 +71,7 @@ echo
>  _make_test_img $IMG_SIZE
>  
>  # Create data clusters (not aligned to an L2 table)
> -$QEMU_IO -c 'write -P 42 1M 256k' "$TEST_IMG" | _filter_qemu_io
> +$QEMU_IO -c "write -P 42 $(((1024 + 32) * 1024)) 192k" "$TEST_IMG" | _filter_qemu_io

The comment should probably be updated to note that we don't write head
and tail because we want them to stay zero, so the mapping information
matches.

Max

>  orig_map=$($QEMU_IMG map --output=json "$TEST_IMG")
>  
>  # Convert the data clusters to preallocated zero clusters


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