[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 06/21] qcow2: Helper for refcount array reall
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v2 06/21] qcow2: Helper for refcount array reallocation |
Date: |
Sat, 15 Nov 2014 09:50:12 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 |
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.
>
> Signed-off-by: Max Reitz <address@hidden>
> ---
> 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.
> +{
> + /* 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]
> +
> + 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.)
> +
> + 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 <address@hidden>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH v2 00/21] qcow2: Support refcount orders != 4, Max Reitz, 2014/11/14
- [Qemu-devel] [PATCH v2 01/21] qcow2: Add two new fields to BDRVQcowState, Max Reitz, 2014/11/14
- [Qemu-devel] [PATCH v2 02/21] qcow2: Add refcount_width to format-specific info, Max Reitz, 2014/11/14
- [Qemu-devel] [PATCH v2 03/21] qcow2: Use 64 bits for refcount values, Max Reitz, 2014/11/14
- [Qemu-devel] [PATCH v2 04/21] qcow2: Respect error in qcow2_alloc_bytes(), Max Reitz, 2014/11/14
- [Qemu-devel] [PATCH v2 05/21] qcow2: Refcount overflow and qcow2_alloc_bytes(), Max Reitz, 2014/11/14
- [Qemu-devel] [PATCH v2 06/21] qcow2: Helper for refcount array reallocation, Max Reitz, 2014/11/14
- Re: [Qemu-devel] [PATCH v2 06/21] qcow2: Helper for refcount array reallocation,
Eric Blake <=
- [Qemu-devel] [PATCH v2 07/21] qcow2: Helper function for refcount modification, Max Reitz, 2014/11/14
- [Qemu-devel] [PATCH v2 08/21] qcow2: More helpers for refcount modification, Max Reitz, 2014/11/14
- [Qemu-devel] [PATCH v2 09/21] qcow2: Open images with refcount order != 4, Max Reitz, 2014/11/14
- [Qemu-devel] [PATCH v2 11/21] iotests: Prepare for refcount_width option, Max Reitz, 2014/11/14