From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37275) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XPvWP-00030w-JM for qemu-devel@nongnu.org; Fri, 05 Sep 2014 11:34:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XPvWG-0000cP-HR for qemu-devel@nongnu.org; Fri, 05 Sep 2014 11:33:57 -0400 Received: from mail-wi0-x232.google.com ([2a00:1450:400c:c05::232]:44701) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XPvWG-0000cH-B2 for qemu-devel@nongnu.org; Fri, 05 Sep 2014 11:33:48 -0400 Received: by mail-wi0-f178.google.com with SMTP id r20so3198968wiv.17 for ; Fri, 05 Sep 2014 08:33:47 -0700 (PDT) Date: Fri, 5 Sep 2014 16:33:43 +0100 From: Stefan Hajnoczi Message-ID: <20140905153343.GH27649@stefanha-thinkpad.redhat.com> References: <1409568798-2292-1-git-send-email-junmuzi@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="vtJ+CqYNzKB4ukR4" Content-Disposition: inline In-Reply-To: <1409568798-2292-1-git-send-email-junmuzi@gmail.com> 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: Jun Li Cc: kwolf@redhat.com, juli@redhat.com, famz@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com --vtJ+CqYNzKB4ukR4 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Mon, Sep 01, 2014 at 06:52:48PM +0800, Jun Li wrote: How does this patch handle self-describing refcount blocks? I think they will keep the refcount block alive forever because your code will not decide to free them. This patch should also discard the refcount block if we decide to free it (in the same way that we discard at cluster_offset). > 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) { What is the purpose of 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); You can't do this, the buffer belongs to the refcount block cache. Please look at the cache get/put as well as qcow2_cache_create/destroy. > + /* 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; > + } Plase use write_reftable_entry(). --vtJ+CqYNzKB4ukR4 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJUCdfXAAoJEJykq7OBq3PIiQ4H/i2QcprzlZmwpoAx6HuzSUSK CT+vk/ANC4V6Gb2nB7uc8YXHN6Y9KM0+5AVOmXF1Ftds43kXqZTE70NLwHWYDmY5 ASx6xdeCs9AeFU2W4JWtpZTR6KqtL/lyEo9CB7U+ZVufSChmCuuMn0VUyzF62i3S 89EQ6vU8dWVJuTnF7MJatqdEXMwx/WZ5h6RVnZHV0PLy4DPWRcXNYyqgpDq9ajwz oQxF+rZYjxY0Ka9zZW0jGWLRZVO7Up2DI8xnWXgib5zELZIcC1WaZhi8xzch3hh2 76ZtF2JN4spPqbvHjX/qInr5zrs3yrYNQlpXWewGtU0+FR8z7agCxygkL5AU/lU= =5oAt -----END PGP SIGNATURE----- --vtJ+CqYNzKB4ukR4--