| Message ID | 20170313214117.27350-4-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Mar 13, 2017 at 10:41:14PM +0100, Max Reitz wrote: > This patch extends qcow2_calc_size_usage() so it can calculate the > additional space needed for preallocating image growth. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block/qcow2.c | 137 +++++++++++++++++++++++++++++++++++++++++----------------- > 1 file changed, 98 insertions(+), 39 deletions(-) > > diff --git a/block/qcow2.c b/block/qcow2.c > index 21b2b3cd53..80fb815b15 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -2101,7 +2101,15 @@ done: > return ret; > } > > -static uint64_t qcow2_calc_size_usage(uint64_t new_size, > +/** > + * Returns the number of bytes that must be allocated in the underlying file > + * to accomodate an image growth from @current_size to @new_size. > + * > + * @current_size must be 0 when creating a new image. In that case, @bs is > + * ignored; otherwise it must be valid. > + */ > +static uint64_t qcow2_calc_size_usage(BlockDriverState *bs, > + uint64_t current_size, uint64_t new_size, > int cluster_bits, int refcount_order) > { > size_t cluster_size = 1u << cluster_bits; > @@ -2122,47 +2130,97 @@ static uint64_t qcow2_calc_size_usage(uint64_t new_size, > refblock_bits = cluster_bits - (refcount_order - 3); > refblock_size = 1 << refblock_bits; > > - /* header: 1 cluster */ > - meta_size += cluster_size; > - > - /* total size of L2 tables */ > - nl2e = aligned_total_size / cluster_size; > - nl2e = align_offset(nl2e, cluster_size / sizeof(uint64_t)); > - meta_size += nl2e * sizeof(uint64_t); > + if (!current_size) { This massive if statement effectively makes two functions: the old qcow2_calc_size_usage() and the new qcow2_calc_size_change(bs) function. It might be nicer to split the two functions.
On 20.03.2017 12:26, Stefan Hajnoczi wrote: > On Mon, Mar 13, 2017 at 10:41:14PM +0100, Max Reitz wrote: >> This patch extends qcow2_calc_size_usage() so it can calculate the >> additional space needed for preallocating image growth. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> block/qcow2.c | 137 +++++++++++++++++++++++++++++++++++++++++----------------- >> 1 file changed, 98 insertions(+), 39 deletions(-) >> >> diff --git a/block/qcow2.c b/block/qcow2.c >> index 21b2b3cd53..80fb815b15 100644 >> --- a/block/qcow2.c >> +++ b/block/qcow2.c >> @@ -2101,7 +2101,15 @@ done: >> return ret; >> } >> >> -static uint64_t qcow2_calc_size_usage(uint64_t new_size, >> +/** >> + * Returns the number of bytes that must be allocated in the underlying file >> + * to accomodate an image growth from @current_size to @new_size. >> + * >> + * @current_size must be 0 when creating a new image. In that case, @bs is >> + * ignored; otherwise it must be valid. >> + */ >> +static uint64_t qcow2_calc_size_usage(BlockDriverState *bs, >> + uint64_t current_size, uint64_t new_size, >> int cluster_bits, int refcount_order) >> { >> size_t cluster_size = 1u << cluster_bits; >> @@ -2122,47 +2130,97 @@ static uint64_t qcow2_calc_size_usage(uint64_t new_size, >> refblock_bits = cluster_bits - (refcount_order - 3); >> refblock_size = 1 << refblock_bits; >> >> - /* header: 1 cluster */ >> - meta_size += cluster_size; >> - >> - /* total size of L2 tables */ >> - nl2e = aligned_total_size / cluster_size; >> - nl2e = align_offset(nl2e, cluster_size / sizeof(uint64_t)); >> - meta_size += nl2e * sizeof(uint64_t); >> + if (!current_size) { > > This massive if statement effectively makes two functions: the old > qcow2_calc_size_usage() and the new qcow2_calc_size_change(bs) function. > > It might be nicer to split the two functions. Hm, yes, but at that point I might as well just write two completely separate functions. I'll see what I can do, maybe I'll just put the new logic that's needed into a completely new function after all. Max
On 20.03.2017 16:14, Max Reitz wrote: > On 20.03.2017 12:26, Stefan Hajnoczi wrote: >> On Mon, Mar 13, 2017 at 10:41:14PM +0100, Max Reitz wrote: >>> This patch extends qcow2_calc_size_usage() so it can calculate the >>> additional space needed for preallocating image growth. >>> >>> Signed-off-by: Max Reitz <mreitz@redhat.com> >>> --- >>> block/qcow2.c | 137 +++++++++++++++++++++++++++++++++++++++++----------------- >>> 1 file changed, 98 insertions(+), 39 deletions(-) >>> >>> diff --git a/block/qcow2.c b/block/qcow2.c >>> index 21b2b3cd53..80fb815b15 100644 >>> --- a/block/qcow2.c >>> +++ b/block/qcow2.c >>> @@ -2101,7 +2101,15 @@ done: >>> return ret; >>> } >>> >>> -static uint64_t qcow2_calc_size_usage(uint64_t new_size, >>> +/** >>> + * Returns the number of bytes that must be allocated in the underlying file >>> + * to accomodate an image growth from @current_size to @new_size. >>> + * >>> + * @current_size must be 0 when creating a new image. In that case, @bs is >>> + * ignored; otherwise it must be valid. >>> + */ >>> +static uint64_t qcow2_calc_size_usage(BlockDriverState *bs, >>> + uint64_t current_size, uint64_t new_size, >>> int cluster_bits, int refcount_order) >>> { >>> size_t cluster_size = 1u << cluster_bits; >>> @@ -2122,47 +2130,97 @@ static uint64_t qcow2_calc_size_usage(uint64_t new_size, >>> refblock_bits = cluster_bits - (refcount_order - 3); >>> refblock_size = 1 << refblock_bits; >>> >>> - /* header: 1 cluster */ >>> - meta_size += cluster_size; >>> - >>> - /* total size of L2 tables */ >>> - nl2e = aligned_total_size / cluster_size; >>> - nl2e = align_offset(nl2e, cluster_size / sizeof(uint64_t)); >>> - meta_size += nl2e * sizeof(uint64_t); >>> + if (!current_size) { >> >> This massive if statement effectively makes two functions: the old >> qcow2_calc_size_usage() and the new qcow2_calc_size_change(bs) function. >> >> It might be nicer to split the two functions. > > Hm, yes, but at that point I might as well just write two completely > separate functions. I'll see what I can do, maybe I'll just put the new > logic that's needed into a completely new function after all. I'm having a bit of trouble with the split, due to a couple of reasons: First, I can't think of a good name for the new function. qcow2_calc_growth_size_usage() is a bit long and still not really meaningful... Second, having all the functionality in a single function means we can "share" the explanation of how nrefblocke is calculated. I don't really want to just put a reference comment there ("look it up in qcow2_create2()"), but I definitely don't want to duplicate it either. Finally, having it in a single function may not make much sense from the inside but it does very much so from the outside. Even though we have to follow much different code paths, from the outside it's pretty much the same thing. Therefore, I'm probably going to keep this patch as-is in v2 (for now), expecting more criticism and stronger wording than "might be nicer", forcing me to finally do the split in an eventual v3. O:-) Max
diff --git a/block/qcow2.c b/block/qcow2.c index 21b2b3cd53..80fb815b15 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2101,7 +2101,15 @@ done: return ret; } -static uint64_t qcow2_calc_size_usage(uint64_t new_size, +/** + * Returns the number of bytes that must be allocated in the underlying file + * to accomodate an image growth from @current_size to @new_size. + * + * @current_size must be 0 when creating a new image. In that case, @bs is + * ignored; otherwise it must be valid. + */ +static uint64_t qcow2_calc_size_usage(BlockDriverState *bs, + uint64_t current_size, uint64_t new_size, int cluster_bits, int refcount_order) { size_t cluster_size = 1u << cluster_bits; @@ -2122,47 +2130,97 @@ static uint64_t qcow2_calc_size_usage(uint64_t new_size, refblock_bits = cluster_bits - (refcount_order - 3); refblock_size = 1 << refblock_bits; - /* header: 1 cluster */ - meta_size += cluster_size; - - /* total size of L2 tables */ - nl2e = aligned_total_size / cluster_size; - nl2e = align_offset(nl2e, cluster_size / sizeof(uint64_t)); - meta_size += nl2e * sizeof(uint64_t); + if (!current_size) { + /* header: 1 cluster */ + meta_size += cluster_size; + + /* total size of L2 tables */ + nl2e = aligned_total_size / cluster_size; + nl2e = align_offset(nl2e, cluster_size / sizeof(uint64_t)); + meta_size += nl2e * sizeof(uint64_t); + + /* total size of L1 tables */ + nl1e = nl2e * sizeof(uint64_t) / cluster_size; + nl1e = align_offset(nl1e, cluster_size / sizeof(uint64_t)); + meta_size += nl1e * sizeof(uint64_t); + + /* total size of refcount blocks + * + * note: every host cluster is reference-counted, including metadata + * (even refcount blocks are recursively included). + * Let: + * a = total_size (this is the guest disk size) + * m = meta size not including refcount blocks and refcount tables + * c = cluster size + * y1 = number of refcount blocks entries + * y2 = meta size including everything + * rces = refcount entry size in bytes + * then, + * y1 = (y2 + a)/c + * y2 = y1 * rces + y1 * rces * sizeof(u64) / c + m + * we can get y1: + * y1 = (a + m) / (c - rces - rces * sizeof(u64) / c) + */ + nrefblocke = (aligned_total_size + meta_size + cluster_size) + / (cluster_size - rces - rces * sizeof(uint64_t) + / cluster_size); + meta_size += DIV_ROUND_UP(nrefblocke, refblock_size) * cluster_size; - /* total size of L1 tables */ - nl1e = nl2e * sizeof(uint64_t) / cluster_size; - nl1e = align_offset(nl1e, cluster_size / sizeof(uint64_t)); - meta_size += nl1e * sizeof(uint64_t); + /* total size of refcount tables */ + nreftablee = nrefblocke / refblock_size; + nreftablee = align_offset(nreftablee, cluster_size / sizeof(uint64_t)); + meta_size += nreftablee * sizeof(uint64_t); - /* total size of refcount blocks - * - * note: every host cluster is reference-counted, including metadata - * (even refcount blocks are recursively included). - * Let: - * a = total_size (this is the guest disk size) - * m = meta size not including refcount blocks and refcount tables - * c = cluster size - * y1 = number of refcount blocks entries - * y2 = meta size including everything - * rces = refcount entry size in bytes - * then, - * y1 = (y2 + a)/c - * y2 = y1 * rces + y1 * rces * sizeof(u64) / c + m - * we can get y1: - * y1 = (a + m) / (c - rces - rces * sizeof(u64) / c) - */ - nrefblocke = (aligned_total_size + meta_size + cluster_size) - / (cluster_size - rces - rces * sizeof(uint64_t) - / cluster_size); - meta_size += DIV_ROUND_UP(nrefblocke, refblock_size) * cluster_size; + return aligned_total_size + meta_size; + } else { + BDRVQcow2State *s = bs->opaque; + uint64_t aligned_cur_size = align_offset(current_size, cluster_size); + uint64_t creftable_length; + uint64_t i; + + /* new total size of L2 tables */ + nl2e = aligned_total_size / cluster_size; + nl2e = align_offset(nl2e, cluster_size / sizeof(uint64_t)); + meta_size += nl2e * sizeof(uint64_t); + + /* Subtract L2 tables which are already present */ + for (i = 0; i < s->l1_size; i++) { + if (s->l1_table[i] & L1E_OFFSET_MASK) { + meta_size -= cluster_size; + } + } - /* total size of refcount tables */ - nreftablee = nrefblocke / refblock_size; - nreftablee = align_offset(nreftablee, cluster_size / sizeof(uint64_t)); - meta_size += nreftablee * sizeof(uint64_t); + /* Do not add L1 table size because the only caller of this path + * (qcow2_truncate) has increased its size already. */ - return aligned_total_size + meta_size; + /* Calculate size of the additional refblocks (this assumes that all of + * the existing image is covered by refblocks, which is extremely + * likely); this may result in overallocation because parts of the newly + * added space may be covered by existing refblocks, but that is fine. + * + * This only considers the newly added space. Since we cannot update the + * reftable in-place, we will have to able to store both the old and the + * new one at the same time, though. Therefore, we need to add the size + * of the old reftable here. + */ + creftable_length = ROUND_UP(s->refcount_table_size * sizeof(uint64_t), + cluster_size); + nrefblocke = ((aligned_total_size - aligned_cur_size) + meta_size + + creftable_length + cluster_size) + / (cluster_size - rces - + rces * sizeof(uint64_t) / cluster_size); + meta_size += DIV_ROUND_UP(nrefblocke, refblock_size) * cluster_size; + + /* total size of the new refcount table (again, may be too much because + * it assumes that the new area is not covered by any refcount blocks + * yet) */ + nreftablee = s->max_refcount_table_index + 1 + + nrefblocke / refblock_size; + nreftablee = align_offset(nreftablee, cluster_size / sizeof(uint64_t)); + meta_size += nreftablee * sizeof(uint64_t); + + return (aligned_total_size - aligned_cur_size) + meta_size; + } } static int qcow2_create2(const char *filename, int64_t total_size, @@ -2203,7 +2261,8 @@ static int qcow2_create2(const char *filename, int64_t total_size, int ret; if (prealloc == PREALLOC_MODE_FULL || prealloc == PREALLOC_MODE_FALLOC) { - uint64_t file_size = qcow2_calc_size_usage(total_size, cluster_bits, + uint64_t file_size = qcow2_calc_size_usage(NULL, 0, total_size, + cluster_bits, refcount_order); qemu_opt_set_number(opts, BLOCK_OPT_SIZE, file_size, &error_abort);
This patch extends qcow2_calc_size_usage() so it can calculate the additional space needed for preallocating image growth. Signed-off-by: Max Reitz <mreitz@redhat.com> --- block/qcow2.c | 137 +++++++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 98 insertions(+), 39 deletions(-)