Re: [Qemu-devel] [PATCH v2 06/21] qcow2: Helper for refcount array reallocation

[Max Reitz <mreitz@redhat.com>]

On 2014-11-15 at 17:50, Eric Blake wrote:
> On 11/14/2014 06:05 AM, Max Reitz wrote:
>> Add a helper function for reallocating a refcount array, independently
> s/independently/independent/
>
>> of the refcount order. The newly allocated space is zeroed and the
>> function handles failed reallocations gracefully.
> This patch is doing two things: it is refactoring things into a nice
> helper function (mentioned), AND it is adding a guarantee that you now
> always allocate a table on cluster boundaries, even when you aren't
> using the full table (hinted at elsewhere in the series, but noticeably
> absent here).  I think you want to add more comments to the commit
> message making that more obvious, since it looks like you rely on that
> guarantee later.

Will do.

>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block/qcow2-refcount.c | 121 +++++++++++++++++++++++++++++--------------------
>>   1 file changed, 72 insertions(+), 49 deletions(-)
>>
>> +
>> +static int realloc_refcount_array(BDRVQcowState *s, uint16_t **array,
>> +                                  int64_t *size, int64_t new_size)
> I think this function deserves a comment stating that *array is actually
> allocated to full cluster size with a 0 tail, so that it can be written
> straight to disk.

OK, will add a comment.

>> +{
>> +    /* Round to clusters so the array can be directly written to disk */
>> +    size_t old_byte_size = ROUND_UP(refcount_array_byte_size(s, *size),
>> +                                    s->cluster_size);
>> +    size_t new_byte_size = ROUND_UP(refcount_array_byte_size(s, new_size),
>> +                                    s->cluster_size);
>> +    uint16_t *new_ptr;
> Can old_byte_size ever equal new_byte_size?  Or are we guaranteed that
> this will only be called when we really need to add another cluster to
> the reftable?
>
> [reading further]
>
> Yes, it looks like *size and new_size are not necessarily
> cluster-aligned, so as an example, it is very likely that we might call
> realloc_refcount_array with the existing size of 20 and a new size of
> 21, both of which fit within the same byte size when rounded up to
> cluster boundary.  But that means that the realloc is a no-op in that
> case; might it be worth special-casing rather than wasting time on the
> g_try_realloc and no-op memset?  [at least the code works correctly even
> without a special case shortcut]

Well, it's probably not necessary, but it will look most likely look 
better to catch that case.

>> +
>> +    new_ptr = g_try_realloc(*array, new_byte_size);
>> +    if (new_byte_size && !new_ptr) {
>> +        return -ENOMEM;
>> +    }
> Is it worth asserting that new_byte_size is non-zero?  Why would anyone
> ever call this to resize down to 0?  (But I can see where you DO call it
> with old_byte_size of zero, when initializing data structures and using
> this function for the first allocation.)

Hm, considering every image that can be opened using the qcow2 driver 
needs at least one cluster (the header), we can outrule that this is 
called with new_size == 0 (which would be the only way new_byte_size 
could ever be 0 either).

>> +
>> +    if (new_ptr) {
> If we assert that new_byte_size is non-zero, then at this point, new_ptr
> is non-NULL and this condition is pointless.
>
>> +        memset((void *)((uintptr_t)new_ptr + old_byte_size), 0,
>> +               new_byte_size - old_byte_size);
>> +    }
>> +
>> +    *array = new_ptr;
>> +    *size  = new_size;
>> +
>> +    return 0;
>> +}
>>   
> Code looks correct as written, whether or not you also add more
> comments, asserts, and/or shortcuts for no-op situations.  So:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Max