From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:32891) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XctuD-0006uj-KO for qemu-devel@nongnu.org; Sat, 11 Oct 2014 06:28:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xctu7-0007O0-GR for qemu-devel@nongnu.org; Sat, 11 Oct 2014 06:28:09 -0400 Received: from mx1.redhat.com ([209.132.183.28]:5872) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xctu7-0007Ib-9k for qemu-devel@nongnu.org; Sat, 11 Oct 2014 06:28:03 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s9BAS1Ze005260 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Sat, 11 Oct 2014 06:28:01 -0400 Message-ID: <54390624.2070905@redhat.com> Date: Sat, 11 Oct 2014 12:27:48 +0200 From: Max Reitz MIME-Version: 1.0 References: <1409088987-17207-1-git-send-email-mreitz@redhat.com> <1409088987-17207-4-git-send-email-mreitz@redhat.com> <5437D1E3.4070302@redhat.com> In-Reply-To: <5437D1E3.4070302@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v12 03/14] qcow2: Optimize bdrv_make_empty() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: Kevin Wolf , Stefan Hajnoczi Am 10.10.2014 um 14:32 schrieb Eric Blake: > On 08/26/2014 03:36 PM, 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) and creating a trivial >> refcount structure. >> >> If there are snapshots, fall back to the simple implementation (discard >> all clusters). >> >> Signed-off-by: Max Reitz >> --- >> block/blkdebug.c | 2 + >> block/qcow2.c | 137 ++++++++++++++++++++++++++++++++++++++++++++------ >> include/block/block.h | 2 + >> 3 files changed, 126 insertions(+), 15 deletions(-) >> >> + } else { >> + int l1_clusters; >> + int64_t offset; >> + uint64_t *new_reftable; >> + uint8_t l1_ofs_rt_ofs_cls[20]; /* L1 offset; RT offset and clusters */ >> + uint64_t rt_entry; >> + >> + ret = qcow2_cache_empty(bs, s->l2_table_cache); >> if (ret < 0) { >> - break; >> + return ret; >> + } >> + >> + ret = qcow2_cache_empty(bs, s->refcount_block_cache); >> + if (ret < 0) { >> + return ret; >> + } >> + >> + /* Refcounts will be broken utterly */ >> + ret = qcow2_mark_dirty(bs); > qcow2_mark_dirty does assert(s->qcow_version >= 3). But this code can > be reached when committing a qcow2 0.10 compat-level file, right? You're right, this code doesn't work for compat=0.10. As you proposed in your own answer to this, I'll just use the fallback code in that case (in v13) >> + /* "Create" an empty reftable (one cluster) directly after the image >> + * header and an empty L1 table three clusters after the image header; >> + * the cluster between those two will be used as the first refblock */ >> + cpu_to_be64w((uint64_t *)&l1_ofs_rt_ofs_cls[ 0], 3 * s->cluster_size); >> + cpu_to_be64w((uint64_t *)&l1_ofs_rt_ofs_cls[ 8], s->cluster_size); >> + cpu_to_be32w((uint32_t *)&l1_ofs_rt_ofs_cls[16], 1); > The offsets here feel a bit magic. Well, such byte offsets are everywhere in the qcow2 code which does such small writes to the image header. Making them non-magic would mean offsetof(...) - offsetof(...). Can and will do, but I probably won't find it much more readable. ;-) Max