From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:49198) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UBMM8-0005jk-4e for qemu-devel@nongnu.org; Fri, 01 Mar 2013 04:34:22 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UBMM5-0003WK-G5 for qemu-devel@nongnu.org; Fri, 01 Mar 2013 04:34:20 -0500 Received: from mx1.redhat.com ([209.132.183.28]:58175) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UBMM5-0003WA-8y for qemu-devel@nongnu.org; Fri, 01 Mar 2013 04:34:17 -0500 Date: Fri, 1 Mar 2013 10:34:11 +0100 From: Stefan Hajnoczi Message-ID: <20130301093411.GA32262@stefanha-thinkpad.redhat.com> References: <1360153926-9492-1-git-send-email-benoit@irqsave.net> <1360153926-9492-28-git-send-email-benoit@irqsave.net> <20130208105746.GD7835@stefanha-thinkpad.redhat.com> <20130227150028.GC4010@irqsave.net> <20130228094139.GB12748@stefanha-thinkpad.redhat.com> <20130228101434.GD2429@dhcp-200-207.str.redhat.com> <20130228161418.GA4214@irqsave.net> <20130301085933.GA2461@dhcp-200-207.str.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20130301085933.GA2461@dhcp-200-207.str.redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RFC V6 27/33] qcow2: Adapt checking of QCOW_OFLAG_COPIED for dedup. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: =?iso-8859-1?Q?Beno=EEt?= Canet , qemu-devel@nongnu.org On Fri, Mar 01, 2013 at 09:59:33AM +0100, Kevin Wolf wrote: > Am 28.02.2013 um 17:14 hat Beno=EEt Canet geschrieben: > > Le Thursday 28 Feb 2013 =E0 11:14:34 (+0100), Kevin Wolf a =E9crit : > > > Am 28.02.2013 um 10:41 hat Stefan Hajnoczi geschrieben: > > > > On Wed, Feb 27, 2013 at 04:00:28PM +0100, Beno=EEt Canet wrote: > > > > > > > - if ((refcount =3D=3D 1) !=3D ((l2_entry & = QCOW_OFLAG_COPIED) !=3D 0)) { > > > > > > > + if (!s->has_dedup && > > > > > > > + (refcount =3D=3D 1) !=3D ((l2_entry & = QCOW_OFLAG_COPIED) !=3D 0)) { > > > > > > > + fprintf(stderr, "ERROR OFLAG_COPIED: o= ffset=3D%" > > > > > > > + PRIx64 " refcount=3D%d\n", l2_entr= y, refcount); > > > > > > > + res->corruptions++; > > > > > > > + } > > > > > >=20 > > > > > > Why is this warning suppressed when dedup is enabled? The me= aning of > > > > > > QCOW_OFLAG_COPIED is that refcount =3D=3D 1. If this invaria= nt is violated > > > > > > then something is wrong. > > > > >=20 > > > > > When deduplication is done refcount will be bigger than one and > > > > > QCOW_OFLAG_COPIED will be cleared. > > > > >=20 > > > > > Then if enough logical clustere pointing to the same physical c= luster are > > > > > rewritten with something else the refcount will goes down back = to one. > > > > >=20 > > > > > But this time QCOW_OFLAG_COPIED can be set back so this equalit= y won't be true. > > > >=20 > > > > When the refcount decreases to 1 again we need to set QCOW_OFLAG_= COPIED > > > > again. qcow2-snapshot.c:qcow2_snapshot_delete() does this with: > > > >=20 > > > > /* must update the copied flag on the current cluster offsets= */ > > > > ret =3D qcow2_update_snapshot_refcount(bs, s->l1_table_offset= , s->l1_size, 0); > > > >=20 > > > > Is dedup not restoring QCOW_OFLAG_COPIED? > > >=20 > > > This is a very expensive operation. I don't think that you can do i= t for > > > each deduplicated cluster that is overwritten. Not doing it comes w= ith > > > 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 cluste= rs > > > with deduplication enabled. > >=20 > > 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. >=20 > Well, strictly speaking you can. The qcow2_update_snapshot_refcount() > call that Stefan mention does exactly that. It's just insanely expensiv= e > 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