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

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

On 25/09/2023 16:17, Hanna Czenczek wrote:
> On 25.09.23 13:40, Jean-Louis Dupond wrote:
>> 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 find there’s a difference between discard code handling discards 
> from the guest, and code handling zero-writes from the guest that 
> internally issues discards.  I see your POV, but the documentation 
> isn’t clear that not unref'ing on discards not only affects discards 
> issued by the guest, but also internal discards that have been 
> generated upon write-zero from the guest.
>
>>>
>>> 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.
>
> I wouldn’t change the name, just the definition (description).
Posted a new version with fixed description.
>
> Hanna
>
>> Next to that, the option was already added to libvirt also (so this 
>> needs to be fixed afterwards also).
>>>
>>> Hanna
>>>
>>
>