Re: [PATCH] qcow2: Reduce write_zeroes size in handle_alloc_space()

[Eric Blake]

On 6/9/20 9:08 AM, Kevin Wolf wrote:
> Since commit c8bb23cbdbe, handle_alloc_space() is called for newly
> allocated clusters to efficiently initialise the COW areas with zeros if
> necessary. It skips the whole operation if both start_cow nor end_cow

s/nor/and/

> are empty. However, it requests zeroing the whole request size (possibly
> multiple megabytes) even if only one end of the request actually needs
> this.
>
> This patch reduces the write_zeroes request size in this case so that we
> don't unnecessarily zero-initialise a region that we're going to
> overwrite immediately.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/qcow2.c | 16 +++++++++++-----
>   1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 0cd2e6757e..77742877fb 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2403,6 +2403,8 @@ static int handle_alloc_space(BlockDriverState *bs, 
> QCowL2Meta *l2meta)
>       }
>   
>       for (m = l2meta; m != NULL; m = m->next) {
> +        uint64_t start = m->alloc_offset;
> +        uint64_t len = m->nb_clusters * s->cluster_size;
>           int ret;
>   
>           if (!m->cow_start.nb_bytes && !m->cow_end.nb_bytes) {
> @@ -2413,21 +2415,25 @@ static int handle_alloc_space(BlockDriverState *bs, 
> QCowL2Meta *l2meta)
>               continue;
>           }
>   
> +        if (!m->cow_start.nb_bytes) {
> +            start += m->cow_end.offset;
> +            len -= m->cow_end.offset;

So if the head was aligned, we reduce the request to only zero the tail;

> +        } else if (!m->cow_end.nb_bytes) {
> +            len = m->cow_start.nb_bytes;
> +        }

and if the tail was aligned, we reduce the request to only zero the head.

But if both ends are needed, the request still covers the entire 
request, including the clusters in the middle that will be overwritten.

> +
>           /*
>            * instead of writing zero COW buffers,
>            * efficiently zero out the whole clusters
>            */
>   
> -        ret = qcow2_pre_write_overlap_check(bs, 0, m->alloc_offset,
> -                                            m->nb_clusters * s->cluster_size,
> -                                            true);
> +        ret = qcow2_pre_write_overlap_check(bs, 0, start, len, true);
>           if (ret < 0) {
>               return ret;
>           }
>   
>           BLKDBG_EVENT(bs->file, BLKDBG_CLUSTER_ALLOC_SPACE);
> -        ret = bdrv_co_pwrite_zeroes(s->data_file, m->alloc_offset,
> -                                    m->nb_clusters * s->cluster_size,
> +        ret = bdrv_co_pwrite_zeroes(s->data_file, start, len,
>                                       BDRV_REQ_NO_FALLBACK);

If both head and tail are unaligned and need COW, but lie in different 
clusters, would we be better off using two separate 
bdrv_co_pwrite_zeroes() calls?

>           if (ret < 0) {
>               if (ret != -ENOTSUP && ret != -EAGAIN) {

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org