Re: [Qemu-block] [Qemu-devel] [PATCH 1/1] qcow2: handle cluster leak happening with a guest TRIM command

[Denis V. Lunev]

On 05/22/2017 02:37 PM, Eric Blake wrote:
> On 05/21/2017 09:21 AM, Denis V. Lunev wrote:
>>   qemu-img create -f qcow2 1.img 64G
>>   qemu-io -c "write -P 0x32 0 64k" 1.img
>> results in
>>   324 -rw-r--r-- 1 den den 393216 May 21 16:48 1.img
>> Subsequent
>>   qemu-io -c "write -z 0 64k" 1.img
>>   qemu-io -c "write -P 0x32 0 64k" 1.img
>> results in
>>   388 -rw-r--r-- 1 den den 458752 May 21 16:50 1.img
>> which looks like we have 1 cluster leaked.
>>
>> Indeed, qcow2_co_pwrite_zeroes calls qcow2_zero_clusters/zero_single_l2,
>> which does not update refcount for the host cluster and keep the offset
>> as used. Later on handle_copied() does not take into account
>> QCOW2_CLUSTER_ZERO type of the cluster.
> NACK.  We've already fixed this upstream, at commit 564a6b693.
My bad. I am updating sources too infrequently :(

> $ ./qemu-img create -f qcow2 1.img 64g
> Formatting '1.img', fmt=qcow2 size=68719476736 encryption=off
> cluster_size=65536 lazy_refcounts=off refcount_bits=16
> $ ./qemu-io -c 'w -P 0x32 0 64k' 1.img
> wrote 65536/65536 bytes at offset 0
> 64 KiB, 1 ops; 0.0410 sec (1.524 MiB/sec and 24.3902 ops/sec)
> $ ls -l 1.img
> -rw-r--r--. 1 eblake eblake 393216 May 22 06:35 1.img
> $ ./qemu-io -c 'w -z 0 64k' 1.img
> wrote 65536/65536 bytes at offset 0
> 64 KiB, 1 ops; 0.0055 sec (11.345 MiB/sec and 181.5211 ops/sec)
> $ ./qemu-io -c 'w -P 0x32 0 64k' 1.img
> wrote 65536/65536 bytes at offset 0
> 64 KiB, 1 ops; 0.0160 sec (3.903 MiB/sec and 62.4415 ops/sec)
> $ ls -l 1.img
> -rw-r--r--. 1 eblake eblake 393216 May 22 06:35 1.img
>
>
> As it is, your patch doesn't apply to master.  And even if it did, your
> patch breaks semantics - we CANNOT discard clusters that must read back
> as zeroes.
>
Can you pls give some details why the cluster can not be
discarded? This is something that I can not understand.

At my opinion the cluster is clearly marked with QCOW_OFLAG_ZERO
and thus will be read with zeroes regardless of the content of the
cluster which is pointed by the offset. This is actually what
happens on the FS without PUNCH_HOLE support.

That is why this offset can be actually reused for any other
block, what is I am trying to propose.

Den