Re: [Qemu-devel] [PATCH v5 01/12] qcow2: Allow "full" discard

[Kevin Wolf]

Am 17.04.2014 um 23:59 hat Max Reitz geschrieben:
> Normally, discarded sectors should read back as zero. However, there are
> cases in which a sector (or rather cluster) should be discarded as if
> they were never written in the first place, that is, reading them should
> fall through to the backing file again.
> 
> Signed-off-by: Max Reitz <address@hidden>
> ---
>  block/qcow2-cluster.c  | 26 ++++++++++++++++----------
>  block/qcow2-snapshot.c |  2 +-
>  block/qcow2.c          |  2 +-
>  block/qcow2.h          |  2 +-
>  4 files changed, 19 insertions(+), 13 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 331ab08..9b73d97 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1343,7 +1343,7 @@ int qcow2_decompress_cluster(BlockDriverState *bs, 
> uint64_t cluster_offset)
>   * clusters.
>   */
>  static int discard_single_l2(BlockDriverState *bs, uint64_t offset,
> -    unsigned int nb_clusters, enum qcow2_discard_type type)
> +    unsigned int nb_clusters, enum qcow2_discard_type type, bool 
> full_discard)
>  {
>      BDRVQcowState *s = bs->opaque;
>      uint64_t *l2_table;
> @@ -1365,25 +1365,31 @@ static int discard_single_l2(BlockDriverState *bs, 
> uint64_t offset,
>          old_offset = be64_to_cpu(l2_table[l2_index + i]);
>  
>          /*
> -         * Make sure that a discarded area reads back as zeroes for v3 images
> -         * (we cannot do it for v2 without actually writing a zero-filled
> -         * buffer). We can skip the operation if the cluster is already 
> marked
> -         * as zero, or if it's unallocated and we don't have a backing file.
> +         * If full_discard is false, make sure that a discarded area reads 
> back
> +         * as zeroes for v3 images (we cannot do it for v2 without actually
> +         * writing a zero-filled buffer). We can skip the operation if the
> +         * cluster is already marked as zero, or if it's unallocated and we
> +         * don't have a backing file.
>           *
>           * TODO We might want to use bdrv_get_block_status(bs) here, but 
> we're
>           * holding s->lock, so that doesn't work today.
> +         *
> +         * In case of full_discard being true, the sector should not be read
> +         * back as zeroes, but rather fall through to the backing file.
>           */
> -        if (old_offset & QCOW_OFLAG_ZERO) {
> +        if (!full_discard && (old_offset & QCOW_OFLAG_ZERO)) {
>              continue;
>          }
>  
> -        if ((old_offset & L2E_OFFSET_MASK) == 0 && !bs->backing_hd) {
> +        if ((old_offset & L2E_OFFSET_MASK) == 0 &&
> +            (full_discard || !bs->backing_hd))
> +        {
>              continue;
>          }

I don't think that's right. You wouldn't discard (non-preallocated) zero
clusters with this code. You should probably check the cluster type.

Kevin