Re: [Qemu-devel] [PATCH for-2.9?] qcow2: Allow discard of final unaligned cluster

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2781 bytes --]

On 07.04.2017 03:37, Eric Blake wrote:
> As mentioned in commit 0c1bd46, we ignored requests to
> discard the trailing cluster of an unaligned image.  While
> discard is an advisory operation from the guest standpoint,
> (and we are therefore free to ignore any request), our
> qcow2 implementation exploits the fact that a discarded
> cluster reads back as 0.  As long as we discard on cluster
> boundaries, we are fine; but that means we could observe
> non-zero data leaked at the tail of an unaligned image.
> 
> Enhance iotest 66 to cover this case, and fix the implementation
> to honor a discard request on the final partial cluster.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---

Thanks!

> I can't convince myself whether we strongly rely on aligned discards
> to guarantee that we read back zeroes on qcow2

No, we don't.

>                                                (it would be a
> stronger contract than what the block layer requires of pdiscard,
> since the guest cannot guarantee that a discard does anything). If
> we do, then this is a bug fix worthy of 2.9.

I'm not sure it would be, even if we "relied" on it. For instance,
intra-cluster discarding will be silently ignored (in contrast to
intra-cluster zero writes).

(Obviously one might argue that the desired behavior is to read back
zeroes because for compat=1.1 images we actually write zero clusters
instead of just completely discarding clusters, but...)

>                                              If we don't, then the
> changes to test 66 rely on internal implementation (but the test is
> already specific to qcow2), and the patch can wait for 2.10.

If you want to write zeroes, use zero writing.

Note that discarding on compat=0.10 images will actually really discard
the clusters instead of creating zero clusters (because those don't
exist in compat=0.10). So if you have a backing file, afterwards you'll
see its contents in the discarded areas.

(I personally actually really like that discard behavior. If I had to
decide, I would like that behavior for compat=1.1 images, too, but I
don't, so it reads back as zero there.)

> At any rate, I do know that we don't want to make discard work on
> sub-cluster boundaries anywhere except at the tail of the image
> (that's what write-zeroes is for, and it may be slower when it
> has to do COW or read-modify-write).  Any reliance that we (might)
> have on whole-cluster discards reading back as 0 is also relying
> on using aligned operations.

No, we don't, because discard doesn't give you guarantees about what
kind of data you'll read back.

Therefore: Applied to my block-next branch for 2.10:

https://github.com/XanClic/qemu/commits/block-next

Max


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