From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52849) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XoTfJ-0005Ag-Uy for qemu-devel@nongnu.org; Wed, 12 Nov 2014 03:52:44 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XoTfD-0001kW-QQ for qemu-devel@nongnu.org; Wed, 12 Nov 2014 03:52:37 -0500 Received: from mx1.redhat.com ([209.132.183.28]:35870) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XoTfD-0001kK-Iq for qemu-devel@nongnu.org; Wed, 12 Nov 2014 03:52:31 -0500 Message-ID: <54631FC6.40703@redhat.com> Date: Wed, 12 Nov 2014 09:52:22 +0100 From: Max Reitz MIME-Version: 1.0 References: <1415627159-15941-1-git-send-email-mreitz@redhat.com> <1415627159-15941-6-git-send-email-mreitz@redhat.com> <54612A27.7000801@redhat.com> <5461C751.3080607@redhat.com> <5462359D.4040503@redhat.com> <546236CD.30301@redhat.com> <5462683E.9000206@redhat.com> In-Reply-To: <5462683E.9000206@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 05/21] qcow2: Refcount overflow and qcow2_alloc_bytes() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: Kevin Wolf , Peter Lieven , Stefan Hajnoczi On 2014-11-11 at 20:49, Eric Blake wrote: > On 11/11/2014 09:18 AM, Max Reitz wrote: > >>> No, I was envisioning that we have a brand new image with one cluster >>> allocated (cluster 1 has refcount 1), then 5 times in a row we do >>> 'savevm' to take an internal snapshot. If I understand your code >>> correctly, the first two snapshots increase the refcount, so cluster 1 >>> has a refcount of 3. Then the next snapshot can't increase the refcount, >>> so it instead copies the contents to cluster 2. >> No, it just errors out. >> >> qcow2_alloc_bytes() is only used for allocating space for a compressed >> cluster. When taking a snapshot, update_refcount() will be called to >> increase the clusters' refcounts, and that function will simply throw an >> error. > That's okay for now (always better for an initial feature to be > conservative, then expand it later if there is demand). But I wonder if > we could be made smarter in the future and auto-COW any cluster that > would otherwise exceed max refcount. Thus, for a refcount_order=0 > (width=1) image, a snapshot now doubles the size of the image (as every > single cluster would COW into a new cluster) rather than erroring out. > Food for thought; maybe worth injecting comments into this series > (whether in code or in commit messages, as appropriate) pointing out > that we thought about the future possibility even though we chose not to > allow it for now. Ah, right, thank you. Yes, that sounds like a good idea, I'll see to it at some later point in time. I think adding comments will be hard because the snapshot functions aren't really modified. They just try to increase the refcount and that may now fail earlier than it did for a refcount width of 16 bits, so there's no real change in behavior there, it's just that it's now reasonably possible to hit that case. I will add appropriate comments to the test case (which tests this snapshotting issue), though. Max