Re: [Qemu-block] [PATCH] qcow2: Discard/zero clusters by byte count

[Max Reitz]

On 03.12.2016 19:19, Eric Blake wrote:
> Passing a byte offset, but sector count, when we ultimately
> want to operate on cluster granularity, is madness.  Clean up
> the interfaces to take byte offset and count.  Rename
> qcow2_discard_clusters() and qcow2_zero_clusters() to the
> shorter qcow2_discard() and qcow2_zero() to make sure backports
> don't get confused by changed units.
> 
> Signed-off-by: Eric Blake <address@hidden>
> ---
> 
> 2.9 material, depends on 'Don't strand clusters near 2G intervals
> during commit'
> 
>  block/qcow2.h          |  8 ++++----
>  block/qcow2-cluster.c  | 20 +++++++++++---------
>  block/qcow2-snapshot.c |  7 +++----
>  block/qcow2.c          | 22 ++++++++++------------
>  4 files changed, 28 insertions(+), 29 deletions(-)
> 
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 1823414..a0d169b 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -543,10 +543,10 @@ uint64_t 
> qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
>                                           int compressed_size);
> 
>  int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m);
> -int qcow2_discard_clusters(BlockDriverState *bs, uint64_t offset,
> -    int nb_sectors, enum qcow2_discard_type type, bool full_discard);
> -int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int 
> nb_sectors,
> -                        int flags);
> +int qcow2_discard(BlockDriverState *bs, uint64_t offset, uint64_t count,
> +                  enum qcow2_discard_type type, bool full_discard);
> +int qcow2_zero(BlockDriverState *bs, uint64_t offset, uint64_t count,
> +               int flags);
> 
>  int qcow2_expand_zero_clusters(BlockDriverState *bs,
>                                 BlockDriverAmendStatusCB *status_cb,
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 928c1e2..3ee0815 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1511,19 +1511,17 @@ static int discard_single_l2(BlockDriverState *bs, 
> uint64_t offset,
>      return nb_clusters;
>  }
> 
> -int qcow2_discard_clusters(BlockDriverState *bs, uint64_t offset,
> -    int nb_sectors, enum qcow2_discard_type type, bool full_discard)
> +int qcow2_discard(BlockDriverState *bs, uint64_t offset, uint64_t count,
> +                  enum qcow2_discard_type type, bool full_discard)
>  {
>      BDRVQcow2State *s = bs->opaque;
>      uint64_t end_offset;
>      uint64_t nb_clusters;
>      int ret;
> 
> -    end_offset = offset + (nb_sectors << BDRV_SECTOR_BITS);
> -
> -    /* Round start up and end down */
> +    /* Round start up and end down to cluster boundary */
> +    end_offset = start_of_cluster(s, offset + count);
>      offset = align_offset(offset, s->cluster_size);
> -    end_offset = start_of_cluster(s, end_offset);
> 
>      if (offset > end_offset) {
>          return 0;
> @@ -1595,20 +1593,24 @@ static int zero_single_l2(BlockDriverState *bs, 
> uint64_t offset,
>      return nb_clusters;
>  }
> 
> -int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int 
> nb_sectors,
> -                        int flags)
> +int qcow2_zero(BlockDriverState *bs, uint64_t offset, uint64_t count,
> +               int flags)

Hmm, I personally liked qcow2_zero_clusters() better. qcow2_zero()
doesn't really express that it means the verb "to zero".

Also, while you are making a good point why the function should be
renamed, qcow2_zero_clusters() was much more accurate because offset and
count are expected to be cluster-aligned.

The only alternative I can come up with would be "qcow2_write_zeroes";
that at least solves the first issue I have with this, but not the
second one...

>  {
>      BDRVQcow2State *s = bs->opaque;
>      uint64_t nb_clusters;
>      int ret;
> 
> +    /* Block layer guarantees cluster alignment */

Hm, it's rather qcow2_co_pwrite_zeroes(), isn't it? The block layer will
split unaligned requests into head, body and tail and it will still
submit head and tail (though separately).

As far as I can see, it's qcow2_co_pwrite_zeroes() which then guarantees
that only the aligned part gets through to qcow2_zero().


The patch looks good apart from these nit picks, though.

Max

> +    assert(QEMU_IS_ALIGNED(offset, s->cluster_size));
> +    assert(QEMU_IS_ALIGNED(count, s->cluster_size));
> +
>      /* The zero flag is only supported by version 3 and newer */
>      if (s->qcow_version < 3) {
>          return -ENOTSUP;
>      }
> 
>      /* Each L2 table is handled by its own loop iteration */
> -    nb_clusters = size_to_clusters(s, nb_sectors << BDRV_SECTOR_BITS);
> +    nb_clusters = size_to_clusters(s, count);
> 
>      s->cache_discards = true;

signature.asc
Description: OpenPGP digital signature