Re: [Qemu-devel] [RFC V6 27/33] qcow2: Adapt checking of QCOW_OFLAG_COPIED for dedup.

[Stefan Hajnoczi]

On Fri, Mar 01, 2013 at 09:59:33AM +0100, Kevin Wolf wrote:
> Am 28.02.2013 um 17:14 hat Benoît Canet geschrieben:
> > Le Thursday 28 Feb 2013 à 11:14:34 (+0100), Kevin Wolf a écrit :
> > > Am 28.02.2013 um 10:41 hat Stefan Hajnoczi geschrieben:
> > > > On Wed, Feb 27, 2013 at 04:00:28PM +0100, Benoît Canet wrote:
> > > > > > > -                if ((refcount == 1) != ((l2_entry & 
> > > > > > > QCOW_OFLAG_COPIED) != 0)) {
> > > > > > > +                if (!s->has_dedup &&
> > > > > > > +                    (refcount == 1) != ((l2_entry & 
> > > > > > > QCOW_OFLAG_COPIED) != 0)) {
> > > > > > > +                    fprintf(stderr, "ERROR OFLAG_COPIED: 
> > > > > > > offset=%"
> > > > > > > +                        PRIx64 " refcount=%d\n", l2_entry, 
> > > > > > > refcount);
> > > > > > > +                    res->corruptions++;
> > > > > > > +                }
> > > > > > 
> > > > > > Why is this warning suppressed when dedup is enabled?  The meaning 
> > > > > > of
> > > > > > QCOW_OFLAG_COPIED is that refcount == 1.  If this invariant is 
> > > > > > violated
> > > > > > then something is wrong.
> > > > > 
> > > > > When deduplication is done refcount will be bigger than one and
> > > > > QCOW_OFLAG_COPIED will be cleared.
> > > > > 
> > > > > Then if enough logical clustere pointing to the same physical cluster 
> > > > > are
> > > > > rewritten with something else the refcount will goes down back to one.
> > > > > 
> > > > > But this time QCOW_OFLAG_COPIED can be set back so this equality 
> > > > > won't be true.
> > > > 
> > > > When the refcount decreases to 1 again we need to set QCOW_OFLAG_COPIED
> > > > again.  qcow2-snapshot.c:qcow2_snapshot_delete() does this with:
> > > > 
> > > >     /* must update the copied flag on the current cluster offsets */
> > > >     ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, 
> > > > s->l1_size, 0);
> > > > 
> > > > Is dedup not restoring QCOW_OFLAG_COPIED?
> > > 
> > > This is a very expensive operation. I don't think that you can do it for
> > > each deduplicated cluster that is overwritten. Not doing it comes with
> > > the cost of doing more COW than is actually needed. And we need to
> > > mention in the spec that QCOW_OFLAG_COPIED can be missing on clusters
> > > with deduplication enabled.
> > 
> > Also when two logical clusters point to the same physical cluster and one 
> > of the
> > logical cluster get overwritten the deduplication code has no way to know 
> > the
> > index of the last logical cluster entry.
> 
> Well, strictly speaking you can. The qcow2_update_snapshot_refcount()
> call that Stefan mention does exactly that. It's just insanely expensive
> because it has to look at the refcounts for all clusters.

Okay, I agree that qcow2_update_snapshot_refcount() is too expensive.

Please add a comment explaining that QCOW_OFLAG_COPIED is not guaranteed
when dedup is enabled since it would be too expensive to do this
everything sharing breaks (refcount is decremented to 1).

Stefan