From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39119) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WleXm-0006XW-ST for qemu-devel@nongnu.org; Sat, 17 May 2014 09:21:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WleXd-00032E-Nz for qemu-devel@nongnu.org; Sat, 17 May 2014 09:20:54 -0400 Received: from mail-pb0-x229.google.com ([2607:f8b0:400e:c01::229]:42255) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WleXd-000329-C1 for qemu-devel@nongnu.org; Sat, 17 May 2014 09:20:45 -0400 Received: by mail-pb0-f41.google.com with SMTP id uo5so3783434pbc.14 for ; Sat, 17 May 2014 06:20:44 -0700 (PDT) Message-ID: <53776225.7000107@gmail.com> Date: Sat, 17 May 2014 21:20:37 +0800 From: Jun Li MIME-Version: 1.0 References: <1399648841-16640-1-git-send-email-junmuzi@gmail.com> <53740C06.9020100@redhat.com> In-Reply-To: <53740C06.9020100@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2] qcow2: Patch for shrinking qcow2 disk image List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-devel@nongnu.org Cc: kwolf@redhat.com, famz@redhat.com, juli@redhat.com, stefanha@redhat.com On 05/15/2014 08:36 AM, Max Reitz wrote: > On 09.05.2014 17:20, Jun Li wrote: >> As the realization of raw shrinking, so when do qcow2 shrinking, do not > > *when doing > >> check l1 entries. When resize to size1(size1 < "disk size"), the >> custemer > > *customer Sorry ~ :-) > >> knows this will destory the data. So no need to check the l1 entries >> which >> is used or not. > > I'd somehow like to disagree, but you're correct. raw-posix truncates > the file regardless of whether there is data or not as well. Maybe we > should later add support for reporting potential data loss and asking > the user what to do (I'm thinking of some "force" or "accept_loss" > boolean for bdrv_truncate()). ok, I will try to realize it in the update version of patch. > >> This is v2 of the original patch. thx. > > This should not be part of the commit message, but rather follow the > "---" under your Signed-off-by. sorry, thx, :-) > >> Signed-off-by: Jun Li >> --- >> block/qcow2-cluster.c | 17 ++++++++++++----- >> block/qcow2-snapshot.c | 2 +- >> block/qcow2.c | 10 ++-------- >> block/qcow2.h | 4 ++-- >> 4 files changed, 17 insertions(+), 16 deletions(-) >> >> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c >> index 76d2bcf..8fbbf7f 100644 >> --- a/block/qcow2-cluster.c >> +++ b/block/qcow2-cluster.c >> @@ -29,8 +29,8 @@ >> #include "block/qcow2.h" >> #include "trace.h" >> -int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, >> - bool exact_size) >> +int qcow2_truncate_l1_table(BlockDriverState *bs, uint64_t min_size, >> + bool exact_size) >> { >> BDRVQcowState *s = bs->opaque; >> int new_l1_size2, ret, i; >> @@ -39,8 +39,9 @@ int qcow2_grow_l1_table(BlockDriverState *bs, >> uint64_t min_size, >> int64_t new_l1_table_offset, new_l1_size; >> uint8_t data[12]; >> - if (min_size <= s->l1_size) >> + if (min_size == s->l1_size) { >> return 0; >> + } >> /* Do a sanity check on min_size before trying to calculate >> new_l1_size >> * (this prevents overflows during the while loop for the >> calculation of >> @@ -73,7 +74,13 @@ int qcow2_grow_l1_table(BlockDriverState *bs, >> uint64_t min_size, >> new_l1_size2 = sizeof(uint64_t) * new_l1_size; >> new_l1_table = g_malloc0(align_offset(new_l1_size2, 512)); >> - memcpy(new_l1_table, s->l1_table, s->l1_size * sizeof(uint64_t)); >> + >> + /* shrinking or growing l1 table */ >> + if (min_size < s->l1_size) { >> + memcpy(new_l1_table, s->l1_table, new_l1_size2); >> + } else { >> + memcpy(new_l1_table, s->l1_table, s->l1_size * >> sizeof(uint64_t)); >> + } >> /* write new table (align to cluster) */ >> BLKDBG_EVENT(bs->file, BLKDBG_L1_GROW_ALLOC_TABLE); > > So, as far as I understand the new logic of qcow2_truncate_l1_table(), > it will always grow the table unless exact_size == true, in which case > it may be shrunk as well. This should probably be reflected in the if > condition ("if (exact_size ? min_size == s->l1_size : min_size <= > s->l1_size)" or something like that). But on the other hand, I don't > like a function which suggests being usable for both shrinking and > growing, which then can be used for shrinking only in a special case > (exact_size == true). You should at least add a comment which states > that this function is generally intended for growing the L1 table with > min_size being the new minimal size, but may also be used for > shrinking if exact_size is true. ok, checking... > > Apart from that, if you're shrinking the L1 table, you should in my > opinion free all clusters referenced from the truncated area. It is > true that it's the user's responsibility to make sure no data is lost, > but if you just shrink the L1 table without doing anything about the > lost data references, clusters will be leaked. This can easily be > fixed by qemu-img check, but there are two problems with that: First, > data should be leaked only if it cannot be avoided. It can easily be > repaired, but unless there are errors during some operation, that > should not be necessary. Second, qemu-img check actually does not work > for all image sizes that qemu itself supports. This is even more > reason to avoid leaks: Normally, it can easily be repaired, but > sometimes, it cannot. ok, thx~ Agree to free all clusters referenced from the truncated area. Checking... Will realize this in v3 of patch. > >> @@ -558,7 +565,7 @@ static int get_cluster_table(BlockDriverState >> *bs, uint64_t offset, >> l1_index = offset >> (s->l2_bits + s->cluster_bits); >> if (l1_index >= s->l1_size) { >> - ret = qcow2_grow_l1_table(bs, l1_index + 1, false); >> + ret = qcow2_truncate_l1_table(bs, l1_index + 1, false); >> if (ret < 0) { >> return ret; >> } >> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c >> index 0aa9def..6ba460e 100644 >> --- a/block/qcow2-snapshot.c >> +++ b/block/qcow2-snapshot.c >> @@ -483,7 +483,7 @@ int qcow2_snapshot_goto(BlockDriverState *bs, >> const char *snapshot_id) >> * L1 table of the snapshot. If the snapshot L1 table is >> smaller, the >> * current one must be padded with zeros. >> */ >> - ret = qcow2_grow_l1_table(bs, sn->l1_size, true); >> + ret = qcow2_truncate_l1_table(bs, sn->l1_size, true); >> if (ret < 0) { >> goto fail; >> } >> diff --git a/block/qcow2.c b/block/qcow2.c >> index a4b97e8..70f951c 100644 >> --- a/block/qcow2.c >> +++ b/block/qcow2.c >> @@ -1877,7 +1877,7 @@ static int qcow2_truncate(BlockDriverState *bs, >> int64_t offset) >> int64_t new_l1_size; >> int ret; >> - if (offset & 511) { >> + if (offset <= 0 || offset & 511) { >> error_report("The new size must be a multiple of 512"); >> return -EINVAL; >> } >> @@ -1888,14 +1888,8 @@ static int qcow2_truncate(BlockDriverState >> *bs, int64_t offset) >> return -ENOTSUP; >> } >> - /* shrinking is currently not supported */ >> - if (offset < bs->total_sectors * 512) { >> - error_report("qcow2 doesn't support shrinking images yet"); >> - return -ENOTSUP; >> - } >> - >> new_l1_size = size_to_l1(s, offset); >> - ret = qcow2_grow_l1_table(bs, new_l1_size, true); >> + ret = qcow2_truncate_l1_table(bs, new_l1_size, true); >> if (ret < 0) { >> return ret; >> } >> diff --git a/block/qcow2.h b/block/qcow2.h >> index b49424b..fa36930 100644 >> --- a/block/qcow2.h >> +++ b/block/qcow2.h >> @@ -499,8 +499,8 @@ int >> qcow2_pre_write_overlap_check(BlockDriverState *bs, int ign, int64_t >> offset, >> int64_t size); >> /* qcow2-cluster.c functions */ >> -int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, >> - bool exact_size); >> +int qcow2_truncate_l1_table(BlockDriverState *bs, uint64_t min_size, >> + bool exact_size); >> int qcow2_write_l1_entry(BlockDriverState *bs, int l1_index); >> void qcow2_l2_cache_reset(BlockDriverState *bs); >> int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t >> cluster_offset); > > The rest of the code looks surprisingly simple, but I looks correct to > me. > > I'd really like a test case for qemu-iotests, though. :-) ok, I will try. Best Regards, Jun Li