Re: [PATCH v2] qcow2: keep reference on zeroize with discard-no-unref enabled

[Jean-Louis Dupond <jean-louis@dupond.be>]

On 15/09/2023 13:21, Hanna Czenczek wrote:
> On 05.09.23 15:08, Jean-Louis Dupond wrote:
>> When the discard-no-unref flag is enabled, we keep the reference for
>> normal discard requests.
>> But when a discard is executed on a snapshot/qcow2 image with backing,
>> the discards are saved as zero clusters in the snapshot image.
>>
>> When committing the snapshot to the backing file, not
>> discard_in_l2_slice is called but zero_in_l2_slice. Which did not had
>> any logic to keep the reference when discard-no-unref is enabled.
>>
>> Therefor we add logic in the zero_in_l2_slice call to keep the reference
>> on commit.
>>
>> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/1621
>> Signed-off-by: Jean-Louis Dupond <jean-louis@dupond.be>
>> ---
>>   block/qcow2-cluster.c | 22 ++++++++++++++++++----
>>   1 file changed, 18 insertions(+), 4 deletions(-)
>
> The code looks OK, but the obvious problem I find is that this is not 
> what the discard-no-unref option describes.  It talks about discards, 
> but this now changes the zero-write path.
But it's still touching the discard code in the zeroize code path.
Cause we modify the way zeroize does its discard (when BDRV_REQ_MAY_UNMAP)
>
> I’m fairly certain that you are the only one using this option for 
> now, so we might as well change its definition to include zero writes 
> for 8.2, but we should do that.
I agree. How would you name the option then? Cause it still involves 
discard-only code.
Next to that, the option was already added to libvirt also (so this 
needs to be fixed afterwards also).
>
> Hanna
>