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

[Jean-Louis Dupond]

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