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