From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55272) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJkoF-0006ft-Na for qemu-devel@nongnu.org; Fri, 06 Feb 2015 10:27:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YJkoB-0006PO-FI for qemu-devel@nongnu.org; Fri, 06 Feb 2015 10:27:07 -0500 Received: from mx1.redhat.com ([209.132.183.28]:60380) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJkoB-0006PJ-7p for qemu-devel@nongnu.org; Fri, 06 Feb 2015 10:27:03 -0500 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 t16FR2Y8008332 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Fri, 6 Feb 2015 10:27:02 -0500 Message-ID: <54D4DD45.5010809@redhat.com> Date: Fri, 06 Feb 2015 08:27:01 -0700 From: Eric Blake MIME-Version: 1.0 References: <1423233556-19394-1-git-send-email-mreitz@redhat.com> In-Reply-To: <1423233556-19394-1-git-send-email-mreitz@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="6L17WKX3mxKoOeLhfLtXLOQaTir4ulcc3" Subject: Re: [Qemu-devel] [PATCH v3] qcow2: Rewrite qcow2_alloc_bytes() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-devel@nongnu.org Cc: Kevin Wolf , Stefan Hajnoczi This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --6L17WKX3mxKoOeLhfLtXLOQaTir4ulcc3 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 02/06/2015 07:39 AM, Max Reitz wrote: > qcow2_alloc_bytes() is a function with insufficient error handling and > an unnecessary goto. This patch rewrites it. >=20 > Signed-off-by: Max Reitz > --- > v3: > - Use alloc_clusters_noref() and update_refcount() [Kevin] Ouch. Not done quite right. Kevin, you may want to remove this from your staging area, while Max works on a v4. > + > + free_in_cluster =3D s->cluster_size - offset_into_cluster(s, offse= t); > + if (!offset || free_in_cluster < size) { Let's consider all four possibilities: Case 1: !offset we won't be modifying any existing clusters. When we are done, the new cluster needs a refcount of 1 Case 2: free_in_cluster >=3D size we will only be modifying an existing cluster, assume it starts with refcount of 1. When we are done, it needs a refcount of 2 Case 3: free_in_cluster < size, allocation is not contiguous we won't be modifying any existing clusters. When we are done, the new cluster needs a refcount of 1 Case 4: free_in_cluster < size, allocation is contiguous we will be placing data into both an existing cluster and a new one; assume the existing cluster starts with a refcount of 1. When we are done, the old cluster needs a refcount of 2, and the new cluster needs a refcount of 1 > + int64_t new_cluster =3D alloc_clusters_noref(bs, s->cluster_si= ze); This says the new cluster has a refcount of 0. > + if (new_cluster < 0) { > + return new_cluster; > + } > + > + if (!offset || ROUND_UP(offset, s->cluster_size) !=3D new_clus= ter) { > + offset =3D new_cluster; > } > } > =20 > - /* The cluster refcount was incremented, either by qcow2_alloc_clu= sters() > - * or explicitly by qcow2_update_cluster_refcount(). Refcount blo= cks must > - * be flushed before the caller's L2 table updates. > - */ > + assert(offset); > + ret =3D update_refcount(bs, offset, size, 1, QCOW2_DISCARD_NEVER);= Case 1: This incremented the new cluster. Good Case 2: This incremented the old cluster. Good Case 3: This incremented the new cluster. Good Case 4: This incremented the old cluster. But the new cluster remains at refcount 0. BAD. v2 got it right, because it always put the NEW cluster at refcount 1 (if there was a new cluster), then incremented the old cluster when needed. > + if (ret < 0) { > + return ret; > + } > + > + /* The cluster refcount was incremented; refcount blocks must be f= lushed > + * before the caller's L2 table updates. */ > qcow2_cache_set_dependency(bs, s->l2_table_cache, s->refcount_bloc= k_cache); > + > + s->free_byte_offset =3D offset + size; > + if (!offset_into_cluster(s, s->free_byte_offset)) { > + s->free_byte_offset =3D 0; > + } > + > return offset; > } > =20 >=20 --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --6L17WKX3mxKoOeLhfLtXLOQaTir4ulcc3 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJU1N1FAAoJEKeha0olJ0NqQLEH/iiWIuR3vKMvcF1ixQ8WmHA+ tm4BTvlTO7fVcJAeR9C0hjUsp3EF1eLN4TiNPt4xV8y9Dg+SlvopKHd5IummHkHX K47b5DEVX+DK0bBl60g0TN1D0wMXpzDfdW/gGxNS41FrzzJhiIiwJjv6r1UiDVqu Y5/0Ice4LUz38JKtHCO0D6g8w08dKBuVcyrPDGkQL9ITmFrceyEzuA6njT82tFdg kYtSPTfWtEBl4f9KmOAfCgQYHJskxf3Q87YwxXYzfPtuphDIUHvwWcqYedMns+aJ bpBytBdgmydIh+d99QAIyApbNPaYiH5q2PtSB+O0iXowLIf/NSs5k0noP/B9Etg= =dm2A -----END PGP SIGNATURE----- --6L17WKX3mxKoOeLhfLtXLOQaTir4ulcc3--