Re: [PATCH v9 28/34] qcow2: Add subcluster support to qcow2_co_pwrite_zeroes()

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3679 bytes --]

On 28.06.20 13:02, Alberto Garcia wrote:
> This works now at the subcluster level and pwrite_zeroes_alignment is
> updated accordingly.
> 
> qcow2_cluster_zeroize() is turned into qcow2_subcluster_zeroize() with
> the following changes:
> 
>    - The request can now be subcluster-aligned.
> 
>    - The cluster-aligned body of the request is still zeroized using
>      zero_in_l2_slice() as before.
> 
>    - The subcluster-aligned head and tail of the request are zeroized
>      with the new zero_l2_subclusters() function.
> 
> There is just one thing to take into account for a possible future
> improvement: compressed clusters cannot be partially zeroized so
> zero_l2_subclusters() on the head or the tail can return -ENOTSUP.
> This makes the caller repeat the *complete* request and write actual
> zeroes to disk. This is sub-optimal because
> 
>    1) if the head area was compressed we would still be able to use
>       the fast path for the body and possibly the tail.
> 
>    2) if the tail area was compressed we are writing zeroes to the
>       head and the body areas, which are already zeroized.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block/qcow2.h         |  4 +--
>  block/qcow2-cluster.c | 80 +++++++++++++++++++++++++++++++++++++++----
>  block/qcow2.c         | 27 ++++++++-------
>  3 files changed, 90 insertions(+), 21 deletions(-)

[...]

> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index deff838fe8..1641976028 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -2015,12 +2015,58 @@ static int zero_in_l2_slice(BlockDriverState *bs, uint64_t offset,
>      return nb_clusters;
>  }
>  
> -int qcow2_cluster_zeroize(BlockDriverState *bs, uint64_t offset,
> -                          uint64_t bytes, int flags)
> +static int zero_l2_subclusters(BlockDriverState *bs, uint64_t offset,
> +                               unsigned nb_subclusters)
> +{
> +    BDRVQcow2State *s = bs->opaque;
> +    uint64_t *l2_slice;
> +    uint64_t old_l2_bitmap, l2_bitmap;
> +    int l2_index, ret, sc = offset_to_sc_index(s, offset);
> +
> +    /* For full clusters use zero_in_l2_slice() instead */
> +    assert(nb_subclusters > 0 && nb_subclusters < s->subclusters_per_cluster);
> +    assert(sc + nb_subclusters <= s->subclusters_per_cluster);

Maybe we should also assert that @offset is aligned to the subcluster size.

[...]

> diff --git a/block/qcow2.c b/block/qcow2.c
> index 86258fbc22..4edc3c72b9 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c

[...]

> @@ -4367,12 +4367,13 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>          uint64_t zero_start = QEMU_ALIGN_UP(old_length, s->cluster_size);

Can we instead align this to just subclusters?

>  
>          /*
> -         * Use zero clusters as much as we can. qcow2_cluster_zeroize()
> +         * Use zero clusters as much as we can. qcow2_subcluster_zeroize()
>           * requires a cluster-aligned start. The end may be unaligned if it is

s/cluster/subcluster/?

Max

>           * at the end of the image (which it is here).
>           */
>          if (offset > zero_start) {
> -            ret = qcow2_cluster_zeroize(bs, zero_start, offset - zero_start, 0);
> +            ret = qcow2_subcluster_zeroize(bs, zero_start, offset - zero_start,
> +                                           0);
>              if (ret < 0) {
>                  error_setg_errno(errp, -ret, "Failed to zero out new clusters");
>                  goto fail;
> 


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