Re: [PATCH 2/3] qcow2: fix snapshot data corruption with discard-no-unref=on

[Kevin Wolf <kwolf@redhat.com>]

Am 29.10.2026 um 15:58 hat Jean-Louis Dupond geschrieben:
> 
> On 27/10/2026 11:44, Andrey Drobyshev wrote:
> > Whenever option discard-no-unref=on is enabled, we bypass refcount
> > machinery and unconditionally punch hole in the data on 'discard' and
> > 'write -z -u' paths.  However, when this same cluster is also referenced
> > by an internal snapshot, such operations lead to problems.  The active L2
> > table is copied before it is changed, so the snapshot metadata stays
> > correct, but the host range under it is punched out and the snapshot reads
> > zeroes there afterwards. qemu-img check finds nothing wrong.
> > 
> > Fix this by guarding the 'keep_reference' flag by the presence of the
> > QCOW_OFLAG_COPIED bit.  I.e. only proceed with unconditional data
> > punching if we own this cluster exclusively.
> > 
> > Fixes: 42a2890a76f4 ("qcow2: add discard-no-unref option")
> > Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> > ---
> >   block/qcow2-cluster.c | 6 ++++--
> >   1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> > index e02fae6a0c6..cc473c7c796 100644
> > --- a/block/qcow2-cluster.c
> > +++ b/block/qcow2-cluster.c
> > @@ -1953,6 +1953,7 @@ discard_in_l2_slice(BlockDriverState *bs, uint64_t offset, uint64_t nb_clusters,
> >           QCow2ClusterType cluster_type =
> >               qcow2_get_cluster_type(bs, old_l2_entry);
> >           bool keep_reference = (cluster_type != QCOW2_CLUSTER_COMPRESSED) &&
> > +                              (old_l2_entry & QCOW_OFLAG_COPIED) &&
> >                                 !full_discard &&
> >                                 (s->discard_no_unref &&
> >                                  type == QCOW2_DISCARD_REQUEST);
> 
> Wouldn't it be better to gate only qcow2_discard_cluster when the cluster is
> QCOW_OFLAG_COPIED?
> Cause with the above change, we fully clear/remove the offset of the
> cluster, causing fragmentation again in the image afterwards.
> And that was just the goal for discard-no-unref :)

I thought of this, too, but merely because it seems to be a closer match
for the expectation the name "discard-no-unref" sets.

In practice, I don't expect that you'd see any advantage from it.
Writing again to this cluster will cause COW, so you'll get
fragmentation either way.

Kevin