From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43815) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xqmzm-0000Rh-Fw for qemu-devel@nongnu.org; Tue, 18 Nov 2014 12:55:23 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xqmzh-0000vk-GB for qemu-devel@nongnu.org; Tue, 18 Nov 2014 12:55:18 -0500 Received: from mx1.redhat.com ([209.132.183.28]:55374) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xqmzh-0000vZ-8k for qemu-devel@nongnu.org; Tue, 18 Nov 2014 12:55:13 -0500 Message-ID: <546B87F8.5090600@redhat.com> Date: Tue, 18 Nov 2014 10:55:04 -0700 From: Eric Blake MIME-Version: 1.0 References: <1415970374-16811-1-git-send-email-mreitz@redhat.com> <1415970374-16811-19-git-send-email-mreitz@redhat.com> In-Reply-To: <1415970374-16811-19-git-send-email-mreitz@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Vq9Df4X9K5QFD3eagDSpCfG1uFLEwsbpr" Subject: Re: [Qemu-devel] [PATCH v2 18/21] qcow2: Add function for refcount order amendment List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-devel@nongnu.org Cc: Kevin Wolf , Peter Lieven , Stefan Hajnoczi This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --Vq9Df4X9K5QFD3eagDSpCfG1uFLEwsbpr Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 11/14/2014 06:06 AM, Max Reitz wrote: > Add a function qcow2_change_refcount_order() which allows changing the > refcount order of a qcow2 image. >=20 > Signed-off-by: Max Reitz > --- > block/qcow2-refcount.c | 457 +++++++++++++++++++++++++++++++++++++++++= ++++++++ > block/qcow2.h | 4 + > 2 files changed, 461 insertions(+) >=20 > +static int walk_over_reftable(BlockDriverState *bs, uint64_t **new_ref= table, > + > + status_cb(bs, (uint64_t)index * s->refcount_table_size + refta= ble_index, > + (uint64_t)total * s->refcount_table_size, cb_opaque)= ; Not sure if the casts are needed (isn't s->refcount_table_size already uint64_t, and 'int * uint64_t' does the right thing); but I guess it doesn't hurt to leave them. > +int qcow2_change_refcount_order(BlockDriverState *bs, int refcount_ord= er, > + BlockDriverAmendStatusCB *status_cb, > + void *cb_opaque, Error **errp) > +{ > + do { > + int total_walks; > + > + new_allocation =3D false; > + > + /* At least we have to do this walk and the one which writes t= he > + * refblocks; also, at least we have to do this loop here at l= east > + * twice (normally), first to do the allocations, and second t= o > + * determine that everything is correctly allocated, this then= makes > + * three walks in total */ > + total_walks =3D MIN(walk_index + 2, 3); This feels wrong... > + > + /* First, allocate the structures so they are present in the r= efcount > + * structures */ > + ret =3D walk_over_reftable(bs, &new_reftable, &new_reftable_in= dex, > + &new_reftable_size, NULL, new_refbloc= k_size, > + new_refcount_bits, &alloc_refblock, > + &new_allocation, NULL, status_cb, cb_= opaque, > + walk_index++, total_walks, errp); =2E..In the common case of just two iterations of the do loop (second iteration confirms no allocations needed), you call with index 0/2, 1/3, and then the later non-allocation walk is index 2/3. In the rare case of three iterations of the do loop, you call with index 0/2, 1/3, 2/3, and then the later non-allocation walk is 3/4. I highly doubt that it is possible to trigger four iterations of the do loop, but if it were, you would call with 0/2, 1/3, 2/3, 3/3, and then 4/= 5. I think you instead want to have: total_walks =3D MAX(walk_index + 2, 3) then the common case will call with 0/3, 1/3, and the later walk as 2/3 the three-iteration loop will call with 0/3, 1/3, 2/4, and the later walk as 3/4 the unlikely four-iteration loop will call with 0/3, 1/3, 2/4, 3/5, and the later walk as 4/5. > + > + new_reftable_index =3D 0; > + > + if (new_allocation) { > + if (new_reftable_offset) { > + qcow2_free_clusters(bs, new_reftable_offset, > + allocated_reftable_size * sizeof(u= int64_t), > + QCOW2_DISCARD_NEVER); Any reason you picked QCOW2_DISCARD_NEVER instead of some other policy? Why not punch holes in the file when throwing out a failed too-small new table, or when cleaning up the old table once the new table is good? > + } > + > + new_reftable_offset =3D qcow2_alloc_clusters(bs, new_refta= ble_size * > + sizeof(uint= 64_t)); > + if (new_reftable_offset < 0) { > + error_setg_errno(errp, -new_reftable_offset, > + "Failed to allocate the new reftable"= ); > + ret =3D new_reftable_offset; > + goto done; > + } > + allocated_reftable_size =3D new_reftable_size; > + > + new_allocation =3D true; This assignment is dead code (it already occurs inside an 'if (new_allocation)' condition). > + } > + } while (new_allocation); > + > + /* Second, write the new refblocks */ > + new_allocation =3D false; This assignment is dead code (it can only be reached if the earlier do loop ended, which is only possible when no allocations are recorded). > + ret =3D walk_over_reftable(bs, &new_reftable, &new_reftable_index,= > + &new_reftable_size, new_refblock, > + new_refblock_size, new_refcount_bits, > + &flush_refblock, &new_allocation, new_set= _refcount, > + status_cb, cb_opaque, walk_index, walk_in= dex + 1, > + errp); > + if (ret < 0) { > + goto done; > + } > + assert(!new_allocation); > + Correct. > +done: > + if (new_reftable) { > + /* On success, new_reftable actually points to the old reftabl= e (and > + * new_reftable_size is the old reftable's size); but that is = just > + * fine */ > + for (i =3D 0; i < new_reftable_size; i++) { > + uint64_t offset =3D new_reftable[i] & REFT_OFFSET_MASK; > + if (offset) { > + qcow2_free_clusters(bs, offset, s->cluster_size, > + QCOW2_DISCARD_NEVER); Again, why the QCOW2_DISCARD_NEVER policy? Fix the MIN vs. MAX bug, and the two dead assignment statements, and you can have: Reviewed-by: Eric Blake --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --Vq9Df4X9K5QFD3eagDSpCfG1uFLEwsbpr 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 iQEcBAEBCAAGBQJUa4f4AAoJEKeha0olJ0NqIQQH/jZaAuZCieKph+CXxUvrWrsX yxrTw84xs9Po36WMmNMUrlRKAcilF1UYhUbb84vUTEr3dPxklNfQeRUce1wJmhzj aOCOBtfgG6lO58ipvWH6QSbOaSLa0bDrI+OiVXwMeq2XswFgz3+/qHhuRF1rGYRC 4pBjOx+QOBlG2GAYN8pA7GOVb/nYKzuBcsqo2CwfQOqGDpvyXUyxLpHNS8mB2rcv Bw3ekAMoH3XIBiPHriHkHkFB9hXcSQsirxHsCyapc+kRwizl5fBQZWr8LjED8nFd 28MQg/IISjpQVo5TckCDzGtKkU4rAiddjgVD1F0K9BlsLkXWmjJASmiIGzcsAGg= =7rvN -----END PGP SIGNATURE----- --Vq9Df4X9K5QFD3eagDSpCfG1uFLEwsbpr--