From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48190) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XqotH-0005zi-MG for qemu-devel@nongnu.org; Tue, 18 Nov 2014 14:56:48 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XqotC-00022f-MT for qemu-devel@nongnu.org; Tue, 18 Nov 2014 14:56:43 -0500 Received: from mx1.redhat.com ([209.132.183.28]:35059) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XqotC-00022X-Ex for qemu-devel@nongnu.org; Tue, 18 Nov 2014 14:56:38 -0500 Message-ID: <546BA46F.4070007@redhat.com> Date: Tue, 18 Nov 2014 12:56:31 -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> <546B87F8.5090600@redhat.com> <546B96DC.1090007@redhat.com> In-Reply-To: <546B96DC.1090007@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="OD1w8IFCEOfNvURd3XBwgJPGNixTfcsbs" 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) --OD1w8IFCEOfNvURd3XBwgJPGNixTfcsbs Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 11/18/2014 11:58 AM, Max Reitz wrote: > On 18.11.2014 18:55, Eric Blake wrote: >> On 11/14/2014 06:06 AM, Max Reitz wrote: >>> Add a function qcow2_change_refcount_order() which allows changing th= e >>> refcount order of a qcow2 image. >>> >>> Signed-off-by: Max Reitz >>> --- >>> + if (new_allocation) { >>> + if (new_reftable_offset) { >>> + qcow2_free_clusters(bs, new_reftable_offset, >>> + allocated_reftable_size * >>> sizeof(uint64_t), >>> + QCOW2_DISCARD_NEVER); >> Any reason you picked QCOW2_DISCARD_NEVER instead of some other policy= ? >=20 > Ah, discarding is always interesting... Last year I used > QCOW2_DISCARD_ALWAYS, then asked Kevin and he basically said never to > use ALWAYS unless one is really sure about it. I could have used > QCOW2_DISCARD_OTHER... But the idea behind using NEVER in cases like > this is that the clusters may get picked up by the following allocation= , > in which case having discarded them is not a good idea (there are some > other places in the qcow2 code which use NEVER for the same reason). >=20 > So, in this case, I think NEVER is good. Makes sense. Yes, for THIS case, we are probably going to reuse the just-discarded cluster on the very next walk, so it's not worth punching a hole just to reinstate it. >>> +done: >>> + if (new_reftable) { >>> + /* On success, new_reftable actually points to the old >>> reftable (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? >=20 > Here, I have nothing to justify it. I'll use QCOW2_DISCARD_OTHER in v3.= Thanks, and now I know a bit more about discard policy. >=20 >> Fix the MIN vs. MAX bug, and the two dead assignment statements, and y= ou >> can have: >> >> Reviewed-by: Eric Blake >=20 > I'll also use QCOW2_DISCARD_OTHER for freeing the refblocks and the > reftable after the "done" label, if you're fine with that. Yes, works for me. >=20 > Once again, thanks a lot! And thank you for a mentally engaging review :) I'm still in the middle of an email on a possible test you can write to provoke a different 3-pass scenario thanks to all-zero refblocks, so you may want to wait for that before posting v3... --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --OD1w8IFCEOfNvURd3XBwgJPGNixTfcsbs 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 iQEcBAEBCAAGBQJUa6RvAAoJEKeha0olJ0NqWrkH/0XqUL0cfaGr9r+/q3hYkaRe HmJ5GVpmL8G42unbWPo83V1buUMK9xPy3RY+X+BhZto4aczaEJl3JLjDGPTw4gxp WxdEErACymBC7LEqx7kmKhY3MRwDME+svFXknPfjqIUTtpehWfyXd9GHAGKiykC+ gQQ0bG1c2QuL+fz8wK1pTIlUaIJT0gGg8BBWKUHzfbIxFyW4Dbb0tIHrXM3w1F3D qgg/N7JndW6IIwakeNEn9xPZV/djW8MZJSvC/vu9kFtAYqT2w51+YR1Q3dFZ5wML eCy6KZ2E8496kfbHULaskk2/9G26KVn4S+vudikS+zX1fMJWaCsvjnzKEarB0m4= =8eEB -----END PGP SIGNATURE----- --OD1w8IFCEOfNvURd3XBwgJPGNixTfcsbs--