From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kevin Wolf Subject: Re: [RFC PATCH] kvm tools, qcow: Add support for growing refcount blocks Date: Tue, 13 Dec 2011 10:03:25 +0100 Message-ID: <4EE714DD.5090004@redhat.com> References: <1323655410-32000-1-git-send-email-tianyu.lan@intel.com> <4EE5CFA0.5000203@redhat.com> <1323745557.2585.62.camel@lantianyu-ws> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: "penberg@kernel.org" , "kvm@vger.kernel.org" , "asias.hejun@gmail.com" , "levinsasha928@gmail.com" , "prasadjoshi124@gmail.com" To: "lan,Tianyu" Return-path: Received: from mx1.redhat.com ([209.132.183.28]:63089 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751660Ab1LMJA2 (ORCPT ); Tue, 13 Dec 2011 04:00:28 -0500 In-Reply-To: <1323745557.2585.62.camel@lantianyu-ws> Sender: kvm-owner@vger.kernel.org List-ID: Am 13.12.2011 04:05, schrieb lan,Tianyu: > Thanks for your review. > On =E4=B8=80, 2011-12-12 at 17:55 +0800, Kevin Wolf wrote: >> Am 12.12.2011 03:03, schrieb Lan Tianyu: >>> This patch enables allocating new refcount blocks and so then kvm t= ools >>> could expand qcow2 image much larger. >>> >>> Signed-off-by: Lan Tianyu >>> --- >>> tools/kvm/disk/qcow.c | 105 +++++++++++++++++++++++++++++++++++++= ++++------- >>> 1 files changed, 89 insertions(+), 16 deletions(-) >>> >>> diff --git a/tools/kvm/disk/qcow.c b/tools/kvm/disk/qcow.c >>> index e139fa5..929ba69 100644 >>> --- a/tools/kvm/disk/qcow.c >>> +++ b/tools/kvm/disk/qcow.c >>> @@ -12,6 +12,7 @@ >>> #include >>> #include >>> #include >>> +#include >>> #ifdef CONFIG_HAS_ZLIB >>> #include >>> #endif >>> @@ -20,6 +21,10 @@ >>> #include >>> #include >>> =20 >>> +static int update_cluster_refcount(struct qcow *q, u64 clust_idx, = u16 append); >>> +static int qcow_write_refcount_table(struct qcow *q); >>> +static u64 qcow_alloc_clusters(struct qcow *q, u64 size, int updat= e_ref); >>> +static void qcow_free_clusters(struct qcow *q, u64 clust_start, u= 64 size); >>> =20 >>> static inline int qcow_pwrite_sync(int fd, >>> void *buf, size_t count, off_t offset) >>> @@ -657,6 +662,56 @@ static struct qcow_refcount_block *refcount_bl= ock_search(struct qcow *q, u64 off >>> return rfb; >>> } >>> =20 >>> +static struct qcow_refcount_block *qcow_grow_refcount_block(struct= qcow *q, >>> + u64 clust_idx) >>> +{ >>> + struct qcow_header *header =3D q->header; >>> + struct qcow_refcount_table *rft =3D &q->refcount_table; >>> + struct qcow_refcount_block *rfb; >>> + u64 new_block_offset; >>> + u64 rft_idx; >>> + >>> + rft_idx =3D clust_idx >> (header->cluster_bits - >>> + QCOW_REFCOUNT_BLOCK_SHIFT); >>> + >>> + if (rft_idx >=3D rft->rf_size) { >>> + pr_warning("Don't support grow refcount block table"); >>> + return NULL; >>> + } >>> + >>> + new_block_offset =3D qcow_alloc_clusters(q, q->cluster_size, 0); >>> + if (new_block_offset < 0) >>> + return NULL; >>> + >>> + rfb =3D new_refcount_block(q, new_block_offset); >>> + if (!rfb) >>> + return NULL; >>> + >>> + memset(rfb->entries, 0x00, q->cluster_size); >>> + rfb->dirty =3D 1; >>> + >>> + /* write refcount block */ >>> + if (write_refcount_block(q, rfb) < 0) >>> + goto free_rfb; >>> + >>> + if (cache_refcount_block(q, rfb) < 0) >>> + goto free_rfb; >>> + >>> + rft->rf_table[rft_idx] =3D cpu_to_be64(new_block_offset); >>> + if (qcow_write_refcount_table(q) < 0) >>> + goto free_rfb; >>> + >>> + if (update_cluster_refcount(q, new_block_offset >> >>> + header->cluster_bits, 1) < 0) >>> + goto free_rfb; >> >> This order is unsafe, you could end up with a refcount block that is >> already in use, but still has a refcount of 0. > How about following? > rft->rf_table[rft_idx] =3D cpu_to_be64(new_block_offset); >=20 > if (update_cluster_refcount(q, new_block_offset >> > header->cluster_bits, 1) < 0) { > rft->rf_table[rft_idx] =3D 0; > goto free_rfb; > } >=20 > if (qcow_write_refcount_table(q) < 0) { > rft->rf_table[rft_idx] =3D 0; > qcow_free_clusters(q, new_block_offset, q->cluster_size); > goto free_rfb; > } >=20 > update_cluster_refcount() will use the rft->rf_table. So updating the= rft->rf_table > must be before update_cluster_refcount(). Yes, something like this should work. Kevin