From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38563) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XOU5i-0004hz-VI for qemu-devel@nongnu.org; Mon, 01 Sep 2014 12:04:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XOU5Z-0002mv-UK for qemu-devel@nongnu.org; Mon, 01 Sep 2014 12:04:26 -0400 Received: from mail-pa0-x232.google.com ([2607:f8b0:400e:c03::232]:61932) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XOU5Z-0002mS-MG for qemu-devel@nongnu.org; Mon, 01 Sep 2014 12:04:17 -0400 Received: by mail-pa0-f50.google.com with SMTP id kq14so12654925pab.9 for ; Mon, 01 Sep 2014 09:04:13 -0700 (PDT) Date: Tue, 2 Sep 2014 00:04:08 +0800 From: Jun Li Message-ID: <20140901160408.GA4193@localhost.localdomain> References: <1409568798-2292-1-git-send-email-junmuzi@gmail.com> <20140901111119.GO15537@irqsave.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20140901111119.GO15537@irqsave.net> Subject: Re: [Qemu-devel] [PATCH v2] qcow2: add update refcount table realization for update_refcount List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?iso-8859-1?Q?Beno=EEt?= Canet Cc: kwolf@redhat.com, juli@redhat.com, famz@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com On Mon, 09/01 13:11, Benoît Canet wrote: > The Monday 01 Sep 2014 à 18:52:48 (+0800), Jun Li wrote : > > When every item of refcount block is NULL, free refcount block and reset the > > corresponding item of refcount table with NULL. > > > > Signed-off-by: Jun Li > > --- > > > > The v2 do following change to modify some potential issue. > > > > +------- Here should start from "0". > > | > > for (k = 0; k < refcount_block_entries; k++) { > > if (refcount_block[k] != cpu_to_be16(0)) { > > ... | | > > } | | > > } | +---- Using "0" is more safe. > > | > > +-------- This should be "k" not "++k". > > --- > > block/qcow2-refcount.c | 31 +++++++++++++++++++++++++++++++ > > 1 file changed, 31 insertions(+) > > > > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c > > index 43665b8..63f36e6 100644 > > --- a/block/qcow2-refcount.c > > +++ b/block/qcow2-refcount.c > > @@ -586,6 +586,37 @@ static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs, > > if (refcount == 0 && s->discard_passthrough[type]) { > > update_refcount_discard(bs, cluster_offset, s->cluster_size); > > } > > + > > + /* When refcount block is NULL, update refcount table */ > > + if (block_index == 0) { > > + int k = block_index; > > + int refcount_block_entries = s->cluster_size / sizeof(uint16_t); > > + for (k = 0; k < refcount_block_entries; k++) { > > + if (refcount_block[k] != cpu_to_be16(0)) { > > + break; > > + } > > + } > > + > > + if (k == refcount_block_entries) { > > + qemu_vfree(refcount_block); > > + /* update refcount table */ > > + unsigned int refcount_table_index; > > + uint64_t data64 = cpu_to_be64(0); > > + refcount_table_index = cluster_index >> (s->cluster_bits - > > + REFCOUNT_SHIFT); > > + ret = bdrv_pwrite_sync(bs->file, > > + s->refcount_table_offset + > > + refcount_table_index * > > + sizeof(uint64_t), > > + &data64, sizeof(data64)); > > + if (ret < 0) { > > + goto fail; > > + } > > + > > > + s->refcount_table[refcount_table_index] = data64; > > Shouldn't the in memory version be be in cpu order ? like > s->refcount_table[refcount_table_index] = 0; I don't think so. See following: (gdb) p sizeof(s->refcount_table[0]) $5 = 8 (gdb) p sizeof(s->refcount_table[1]) $6 = 8 (gdb) p sizeof(0) $7 = 4 So I think here is right. Thank you for sharing Max's patch(qcow2: Drop REFCOUNT_SHIFT) with me. I find this patch has been reviewed, but it has not been merged. Maybe I will modify my realization after this patch merged. Thanks again. Jun Li > > Best regards > > Benoît > > + > > + } > > + } > > } > > > > ret = 0; > > -- > > 1.9.3 > > > >