From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38351) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VF1vY-0000Qu-VI for qemu-devel@nongnu.org; Thu, 29 Aug 2013 09:06:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VF1vQ-0006dc-7N for qemu-devel@nongnu.org; Thu, 29 Aug 2013 09:06:20 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35687) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VF1vP-0006dO-V9 for qemu-devel@nongnu.org; Thu, 29 Aug 2013 09:06:12 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r7TD6AJE031603 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 29 Aug 2013 09:06:11 -0400 Message-ID: <521F473F.90609@redhat.com> Date: Thu, 29 Aug 2013 15:06:07 +0200 From: Max Reitz MIME-Version: 1.0 References: <1377771363-5198-1-git-send-email-mreitz@redhat.com> <20130829123343.GH2961@dhcp-200-207.str.redhat.com> <521F40D3.8000302@redhat.com> <20130829125301.GJ2961@dhcp-200-207.str.redhat.com> <521F44F2.5050700@redhat.com> <20130829130437.GL2961@dhcp-200-207.str.redhat.com> In-Reply-To: <20130829130437.GL2961@dhcp-200-207.str.redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] qcow2-refcount: Snapshot update for zero clusters List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, Stefan Hajnoczi Am 29.08.2013 15:04, schrieb Kevin Wolf: > Am 29.08.2013 um 14:56 hat Max Reitz geschrieben: >> Am 29.08.2013 14:53, schrieb Kevin Wolf: >>> Am 29.08.2013 um 14:38 hat Max Reitz geschrieben: >>>> Am 29.08.2013 14:33, schrieb Kevin Wolf: >>>>> Am 29.08.2013 um 12:16 hat Max Reitz geschrieben: >>>>>> Do not try to update the refcount for zero clusters in >>>>>> qcow2_update_snapshot_refcount. >>>>>> >>>>>> Signed-off-by: Max Reitz >>>>>> --- >>>>>> block/qcow2-refcount.c | 16 +++++++++++++--- >>>>>> 1 file changed, 13 insertions(+), 3 deletions(-) >>>>> Please don't forget to add a test case for v2. >>>> Okay. >>>> >>>>>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c >>>>>> index 1244693..7555242 100644 >>>>>> --- a/block/qcow2-refcount.c >>>>>> +++ b/block/qcow2-refcount.c >>>>>> @@ -863,9 +863,13 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, >>>>>> for(j = 0; j < s->l2_size; j++) { >>>>>> offset = be64_to_cpu(l2_table[j]); >>>>>> if (offset != 0) { >>>>>> + uint64_t cluster_index; >>>>>> + >>>>>> old_offset = offset; >>>>>> offset &= ~QCOW_OFLAG_COPIED; >>>>>> - if (offset & QCOW_OFLAG_COMPRESSED) { >>>>>> + >>>>>> + switch (qcow2_get_cluster_type(offset)) { >>>>>> + case QCOW2_CLUSTER_COMPRESSED: >>>>>> nb_csectors = ((offset >> s->csize_shift) & >>>>>> s->csize_mask) + 1; >>>>>> if (addend != 0) { >>>>>> @@ -880,8 +884,10 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, >>>>>> } >>>>>> /* compressed clusters are never modified */ >>>>>> refcount = 2; >>>>>> - } else { >>>>>> - uint64_t cluster_index = (offset & L2E_OFFSET_MASK) >> s->cluster_bits; >>>>>> + break; >>>>>> + >>>>>> + case QCOW2_CLUSTER_NORMAL: >>>>>> + cluster_index = (offset & L2E_OFFSET_MASK) >> s->cluster_bits; >>>>>> if (addend != 0) { >>>>>> refcount = update_cluster_refcount(bs, cluster_index, addend, >>>>>> QCOW2_DISCARD_SNAPSHOT); >>>>>> @@ -893,6 +899,10 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, >>>>>> ret = refcount; >>>>>> goto fail; >>>>>> } >>>>>> + break; >>>>> I think this part isn't quite right. Even zero clusters can have an >>>>> allocated offset, so I think what's really needed is that the same code >>>>> runs from QCOW2_CLUSTER_ZERO. However, add an if (cluster_index == 0) >>>>> case for the unallocated case. >>>> So then it would be enough to change: >>>> >>>> - if (offset != 0) { >>>> + if ((offset & L2E_OFFSET_MASK) != 0) { >>>> >>>> and just execute the same code for QCOW2_CLUSTER_NORMAL and >>>> QCOW2_CLUSTER_ZERO? >>> Doing only this change in the original code might happen to work, but >>> it's not really clean as the L2 entry format is different for compressed >>> clusters (L2E_OFFSET_MASK isn't valid for it). So having the switch for >>> the cluster type is a good thing, but the implementation for normal and >>> zero clusters should be shared indeed. >> Yes, of course I meant to include the switch as well. >> >>> The offset != 0 check ends up redundant when you introduce the switch, >>> so in theory it could as well be removed. >>> >> Well, I don't know; for zero clusters, the if statement is required >> anyway (since qcow2_get_cluster_type does not distinguish between >> allocated and non-allocated zero clusters), so I though we might as >> well just leave it where it is (just slightly adjusted in the way I >> proposed). > You do need a zero check, but it's only for normal/zero clusters, not > for compressed ones; and it must check offset & L2E_OFFSET_MASK (or > cluster_index) rather than offset itself. > > So I think you end up removing the outer if (offset != 0) and adding a > new inner check in case QCOW2_CLUSTER_NORMAL/ZERO. > > Kevin Ah, yes, you're right. Max