Re: [Qemu-devel] [PATCH v4 2/7] qcow2: Discard/zero clusters by byte count

[Max Reitz]

On 20.12.2016 20:15, 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 both offset and count as bytes, while
> still keeping the assertion added previously that the caller
> must align the values to a cluster.  Then rename things to
> make sure backports don't get confused by changed units:
> instead of qcow2_discard_clusters() and qcow2_zero_clusters(),
> we now have qcow2_cluster_discard and qcow2_cluster_zeroize().
> 
> Signed-off-by: Eric Blake <address@hidden>
> 
> ---
> v4: improve function names, split assertion additions into earlier patch
> [no v3 or v2]
> v1: https://lists.gnu.org/archive/html/qemu-devel/2016-12/msg00339.html
> ---
>  block/qcow2.h          |  9 +++++----
>  block/qcow2-cluster.c  | 17 ++++++++---------
>  block/qcow2-snapshot.c |  7 +++----
>  block/qcow2.c          | 21 +++++++++------------
>  4 files changed, 25 insertions(+), 29 deletions(-)

Nothing functionally bad, so:

Reviewed-by: Max Reitz <address@hidden>


Some notes, though:

> diff --git a/block/qcow2.h b/block/qcow2.h
> index 1823414..a131b72 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -543,10 +543,11 @@ 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_cluster_discard(BlockDriverState *bs, uint64_t offset,
> +                          uint64_t count, enum qcow2_discard_type type,
> +                          bool full_discard);
> +int qcow2_cluster_zeroize(BlockDriverState *bs, uint64_t offset,
> +                          uint64_t count, int flags);

I know byte count parameters are often called "count", but I find it a
bit problematic if the function name contains a word that can be a unit.
In these cases, it's not entirely clear whether "count" refers to bytes
or clusters. Maybe "length" or "size" would be better?

Purely subjective and thus optional, of course.

Another thing: I'm not quite positive whether qcow2_cluster_zeroize()
can actually handle a byte count greater than INT_MAX. If you could pass
such a number to it, the multiplication "ret * s->cluster_size" might
overflow. It's only caller qcow2_co_pwrite_zeroes() will never exceed
that limit, but maybe this should be made clear somewhere -- either by
making the parameter an int instead of a uint64_t or by asserting it in
the function.

> 
>  int qcow2_expand_zero_clusters(BlockDriverState *bs,
>                                 BlockDriverAmendStatusCB *status_cb,
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 3304a15..aad5183 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1511,16 +1511,15 @@ 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_cluster_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 end_offset = offset + count;
>      uint64_t nb_clusters;
>      int ret;
> 
> -    end_offset = offset + (nb_sectors << BDRV_SECTOR_BITS);
> -
>      /* Caller must pass aligned values */
>      assert(QEMU_IS_ALIGNED(offset, s->cluster_size));
>      assert(QEMU_IS_ALIGNED(end_offset, s->cluster_size));

Directly below this we have "offset - end_offset" which could be
shortened to "count" (or "length" or "size" or...).

Max

signature.asc
Description: OpenPGP digital signature