From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41274) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xhfve-0003Oc-QH for qemu-devel@nongnu.org; Fri, 24 Oct 2014 10:33:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XhfvZ-000381-Qz for qemu-devel@nongnu.org; Fri, 24 Oct 2014 10:33:22 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51497) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XhfvZ-000372-JT for qemu-devel@nongnu.org; Fri, 24 Oct 2014 10:33:17 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s9OEXFQn025562 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Fri, 24 Oct 2014 10:33:16 -0400 Message-ID: <544A632A.30504@redhat.com> Date: Fri, 24 Oct 2014 08:33:14 -0600 From: Eric Blake MIME-Version: 1.0 References: <1414159063-25977-1-git-send-email-mreitz@redhat.com> <1414159063-25977-4-git-send-email-mreitz@redhat.com> In-Reply-To: <1414159063-25977-4-git-send-email-mreitz@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="UWgJP1CM5TGuAgTNTsbMc5JJJRb9bImvS" Subject: Re: [Qemu-devel] [PATCH v14 03/14] qcow2: Optimize bdrv_make_empty() 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) --UWgJP1CM5TGuAgTNTsbMc5JJJRb9bImvS Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 10/24/2014 07:57 AM, Max Reitz wrote: > bdrv_make_empty() is currently only called if the current image > represents an external snapshot that has been committed to its base > image; it is therefore unlikely to have internal snapshots. In this > case, bdrv_make_empty() can be greatly sped up by emptying the L1 and > refcount table (while having the dirty flag set, which only works for > compat=3D1.1) and creating a trivial refcount structure. >=20 > If there are snapshots or for compat=3D0.10, fall back to the simple > implementation (discard all clusters). >=20 > Signed-off-by: Max Reitz > --- > block/blkdebug.c | 2 + > block/qcow2.c | 165 ++++++++++++++++++++++++++++++++++++++++++= +++++++- > include/block/block.h | 2 + > 3 files changed, 168 insertions(+), 1 deletion(-) >=20 Looks like you resolved Kevin's review points correctly (I'm trusting his judgment here). > static int qcow2_make_empty(BlockDriverState *bs) > { > - int ret =3D 0; > + BDRVQcowState *s =3D bs->opaque; > uint64_t start_sector; > int sector_step =3D INT_MAX / BDRV_SECTOR_SIZE; > + int l1_clusters, ret =3D 0; > + > + l1_clusters =3D DIV_ROUND_UP(s->l1_size, s->cluster_size / sizeof(= uint64_t)); > + > + if (s->qcow_version >=3D 3 && !s->snapshots && > + 3 + l1_clusters <=3D s->refcount_block_size) { > + /* The following function only works for qcow2 v3 images (it r= equires > + * the dirty flag) and only as long as there are no snapshots = (because > + * it completely empties the image). Furthermore, the L1 table= and three > + * additional clusters (image header, refcount table, one refc= ount > + * block) have to fit inside one refcount block. */ > + return make_completely_empty(bs); > + } Is there ever a situation where if make_completely_empty() fails, you can still safely fall back to the slower loop code? For example, failure to do qcow2_cache_empty() at the very beginning might still be recoverable (basically, anywhere that did 'goto fail' instead of 'goto fail_broken_refcounts' might have a chance at the fallback working). But I'm okay with declaring all errors as fatal to the attempt, rather than trying to figure out which errors are worth still trying the fallback (especially since most errors are going to be caused by OOM or file I/O error, and those are likely to still be triggered again during fallback). > =20 > + /* This fallback code simply discards every active clusters; this = is slow, s/clusters/cluster/ > + * but works in all cases */ > for (start_sector =3D 0; start_sector < bs->total_sectors; > start_sector +=3D sector_step) > { With the comment typo fixed (the maintainer could do that without needing a v15): Reviewed-by: Eric Blake --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --UWgJP1CM5TGuAgTNTsbMc5JJJRb9bImvS 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 iQEcBAEBCAAGBQJUSmMqAAoJEKeha0olJ0Nqg8EH/1YCjRiTsi0QLAI0xUXdOtNZ 4WffD+jHIqaHNMnoavPM7Kciv7Z/PvTQMjnrhxJFMjab/gxL+VvsmUHPh6sBZ7am ZNQ8fIdD+TFhwRlN8dPfnZNm7zGB/HJNihLvGPevzUwsV4LZMI4f4i42m+LZu18/ yB36yXAVAekYdJ/g4LpY/RaSjZqcIb2dJAZVJBzBPE4ZinfJsj7saO1DdpoDaRR/ E5OHXnxnqMjFcC9yuE0NHDcNSN/Cgjq2OqLLtxSLDyXZL/G1N+pu/I2vXc7ns2cp OVMwCUp+qcHjD0VeUC8D29QxEobtjkneyavDJlkVpS0HN0xMT4os9P9p13QTXl8= =DK0x -----END PGP SIGNATURE----- --UWgJP1CM5TGuAgTNTsbMc5JJJRb9bImvS--