* [Qemu-devel] [PATCH v2] qcow2: Patch for shrinking qcow2 disk image @ 2014-05-09 15:20 Jun Li 2014-05-15 0:36 ` Max Reitz 0 siblings, 1 reply; 3+ messages in thread From: Jun Li @ 2014-05-09 15:20 UTC (permalink / raw) To: qemu-devel; +Cc: kwolf, juli, famz, Jun Li, stefanha As the realization of raw shrinking, so when do qcow2 shrinking, do not check l1 entries. When resize to size1(size1 < "disk size"), the custemer knows this will destory the data. So no need to check the l1 entries which is used or not. This is v2 of the original patch. thx. Signed-off-by: Jun Li <junmuzi@gmail.com> --- 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); @@ -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); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qcow2: Patch for shrinking qcow2 disk image 2014-05-09 15:20 [Qemu-devel] [PATCH v2] qcow2: Patch for shrinking qcow2 disk image Jun Li @ 2014-05-15 0:36 ` Max Reitz 2014-05-17 13:20 ` Jun Li 0 siblings, 1 reply; 3+ messages in thread From: Max Reitz @ 2014-05-15 0:36 UTC (permalink / raw) To: Jun Li, qemu-devel; +Cc: kwolf, famz, juli, stefanha 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 > 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()). > 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. > Signed-off-by: Jun Li <junmuzi@gmail.com> > --- > 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. 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. > @@ -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. :-) Max ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH v2] qcow2: Patch for shrinking qcow2 disk image 2014-05-15 0:36 ` Max Reitz @ 2014-05-17 13:20 ` Jun Li 0 siblings, 0 replies; 3+ messages in thread From: Jun Li @ 2014-05-17 13:20 UTC (permalink / raw) To: Max Reitz, qemu-devel; +Cc: kwolf, famz, juli, stefanha 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 <junmuzi@gmail.com> >> --- >> 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-05-17 13:21 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-05-09 15:20 [Qemu-devel] [PATCH v2] qcow2: Patch for shrinking qcow2 disk image Jun Li 2014-05-15 0:36 ` Max Reitz 2014-05-17 13:20 ` Jun Li
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).