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

[Vladimir Sementsov-Ogievskiy]

09.06.2020 17:08, 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
> 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;
> +        } else if (!m->cow_end.nb_bytes) {
> +            len = m->cow_start.nb_bytes;
> +        }
> +

Hmm, I'm afraid, that this may make things worse in some cases, as with one big 
write-zero request
we preallocate data-region in the protocol file, so we have better locality for 
the clusters we
are going to write. And, in the same time, with BDRV_REQ_NO_FALLBACK flag 
write-zero must be
fast anyway (especially in comparison with the following write request).

>           /*
>            * 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 (ret < 0) {
>               if (ret != -ENOTSUP && ret != -EAGAIN) {


--
Best regards,
Vladimir