Re: [Qemu-block] [PATCH v7 3/3] qcow2: Discard unaligned tail when wiping image

[Max Reitz]

On 31.03.2017 00:36, Eric Blake wrote:
> The previous commit pointed out a subtle difference between the
> fast and slow path of qcow2_make_empty(), where we failed to discard
> the final (partial) cluster of an unaligned image.
> 
> The problem stems from the fact that qcow2_discard_clusters() was
> silently ignoring sub-cluster head and tail on unaligned requests.
> A quick audit of all callers shows that qcow2_snapshot_create() has
> always passed a cluster-aligned request since the call was added
> in commit 1ebf561; qcow2_co_pdiscard() has passed a cluster-aligned
> request since commit ecdbead taught the block layer about preferred
> discard alignment; and qcow2_make_empty() was fixed to pass an
> aligned start (but not necessarily end) in commit a3e1505.
> 
> Asserting that the start is always aligned also points out that we
> now have a dead check: rounding the end offset down can never result
> in a value less than the aligned start offset (the check was rendered
> dead with commit ecdbead).  Meanwhile, we do not want to round the
> end cluster down in the one case of the end offset matching the
> (unaligned) file size - that final partial cluster should still be
> discarded.
> 
> With those fixes in place, we can adjust the testsuite so that
> qemu-iotests 97 and 176 are once again identical for qcow2, showing
> that fast and slow paths are back in sync.
> 
> Signed-off-by: Eric Blake <address@hidden>
> 
> ---
> v7: new patch
> ---
>  block/qcow2-cluster.c      | 10 ++++------
>  tests/qemu-iotests/176.out |  2 +-
>  2 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 78c11d4..100398c 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1519,12 +1519,10 @@ int qcow2_discard_clusters(BlockDriverState *bs, 
> uint64_t offset,
> 
>      end_offset = offset + (nb_sectors << BDRV_SECTOR_BITS);
> 
> -    /* Round start up and end down */
> -    offset = align_offset(offset, s->cluster_size);
> -    end_offset = start_of_cluster(s, end_offset);
> -
> -    if (offset > end_offset) {
> -        return 0;
> +    /* The caller must cluster-align start; round end down except at EOF */
> +    assert(QEMU_IS_ALIGNED(offset, s->cluster_size));
> +    if (end_offset != bs->total_sectors * BDRV_SECTOR_SIZE) {
> +        end_offset = start_of_cluster(s, end_offset);
>      }

This change looks good and works for qcow2_make_empty(), but
qcow2_co_pdiscard() will still drop these requests:

$ ./qemu-img create -f qcow2 foo.qcow2 512
Formatting 'foo.qcow2', fmt=qcow2 size=512 encryption=off
cluster_size=65536 lazy_refcounts=off refcount_bits=16
$ ./qemu-io -c 'write 0 512' foo.qcow2
wrote 512/512 bytes at offset 0
512 bytes, 1 ops; 0.0062 sec (80.167 KiB/sec and 160.3335 ops/sec)
$ ./qemu-img map foo.qcow2
Offset          Length          Mapped to       File
0               0x200           0x50000         foo.qcow2
$ ./qemu-io -c 'discard 0 512' foo.qcow2
discard 512/512 bytes at offset 0
512 bytes, 1 ops; 0.0000 sec (34.877 MiB/sec and 71428.5714 ops/sec)
$ ./qemu-img map foo.qcow2
Offset          Length          Mapped to       File
0               0x200           0x50000         foo.qcow2

We don't necessarily have to fix it for 2.9, so:

Reviewed-by: Max Reitz <address@hidden>

> 
>      nb_clusters = size_to_clusters(s, end_offset - offset);
> diff --git a/tests/qemu-iotests/176.out b/tests/qemu-iotests/176.out
> index 990a41c..6271fa7 100644
> --- a/tests/qemu-iotests/176.out
> +++ b/tests/qemu-iotests/176.out
> @@ -35,7 +35,7 @@ Offset          Length          File
>  Offset          Length          File
>  0x7ffd0000      0x10000         TEST_DIR/t.IMGFMT.base
>  0x7ffe0000      0x20000         TEST_DIR/t.IMGFMT.itmd
> -0x83400000      0x200           TEST_DIR/t.IMGFMT
> +0x83400000      0x200           TEST_DIR/t.IMGFMT.itmd
> 
>  === Test pass 1 ===
>

signature.asc
Description: OpenPGP digital signature