Re: [Qemu-devel] [PATCH v3 06/22] qcow2: Helper for refcount array reallocation
On 2014-11-28 at 11:46, Stefan Hajnoczi wrote:
On Thu, Nov 27, 2014 at 04:11:12PM +0100, Max Reitz wrote:
On 2014-11-27 at 16:09, Stefan Hajnoczi wrote:
On Thu, Nov 20, 2014 at 06:06:22PM +0100, Max Reitz wrote:
+/**
+ * Reallocates *array so that it can hold new_size entries. *size must contain
+ * the current number of entries in *array. If the reallocation fails, *array
+ * and *size will not be modified and -errno will be returned. If the
+ * reallocation is successful, *array will be set to the new buffer and *size
+ * will be set to new_size. The size of the reallocated refcount array buffer
+ * will be aligned to a cluster boundary, and the newly allocated area will be
+ * zeroed.
+ */
+static int realloc_refcount_array(BDRVQcowState *s, uint16_t **array,
+ int64_t *size, int64_t new_size)
+{
+/* 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;
+
+if (new_byte_size <= old_byte_size) {
+*size = new_size;
+return 0;
+}
Why not realloc the array to the new smaller size? ...
Because such a call will actually never happen. I could replace this if ()
by assert(new_byte_size >= old_byte_size); if (new_byte_size ==
old_byte_size), but as I said before, I'm not a friend of assertions when
the code can deal perfectly well with the "unsupported" case.
It is simpler to put an if statement around the memset.
Well, I personally find an assertion simpler; and I will not drop the if
(new_byte_size == old_byte_size) because this is a very common case so I
don't want to rely on g_realloc() to detect it. Also, Eric proposed it
and I'd like to avoid a ping-pong discussion.
Then the function actually frees unused memory
Which will never happen, though, because new_byte_size will never be
less than old_byte_size.
and readers don't wonder why you decided not to shrink the array.
An assertion will clear up things as well.
Less code and slightly better behavior.
Well, an assert() is one line, while an if () takes two (if () { and });
the behavior will (hopefully) not change either.
But anyway, I'll go with your proposition because, as I said, I don't
like assertions if the code can deal perfectly fine with the bad cases.
Therefore, if at some point in time we decide to use
realloc_refcount_array() to shrink a refcount array, it's better to have
planned for that case.
Max
Re: [Qemu-devel] [PATCH v3 06/22] qcow2: Helper for refcount array reallocation
On Thu, Nov 27, 2014 at 04:11:12PM +0100, Max Reitz wrote:
> On 2014-11-27 at 16:09, Stefan Hajnoczi wrote:
> >On Thu, Nov 20, 2014 at 06:06:22PM +0100, Max Reitz wrote:
> >>+/**
> >>+ * Reallocates *array so that it can hold new_size entries. *size must
> >>contain
> >>+ * the current number of entries in *array. If the reallocation fails,
> >>*array
> >>+ * and *size will not be modified and -errno will be returned. If the
> >>+ * reallocation is successful, *array will be set to the new buffer and
> >>*size
> >>+ * will be set to new_size. The size of the reallocated refcount array
> >>buffer
> >>+ * will be aligned to a cluster boundary, and the newly allocated area
> >>will be
> >>+ * zeroed.
> >>+ */
> >>+static int realloc_refcount_array(BDRVQcowState *s, uint16_t **array,
> >>+ int64_t *size, int64_t new_size)
> >>+{
> >>+/* 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;
> >>+
> >>+if (new_byte_size <= old_byte_size) {
> >>+*size = new_size;
> >>+return 0;
> >>+}
> >Why not realloc the array to the new smaller size? ...
>
> Because such a call will actually never happen. I could replace this if ()
> by assert(new_byte_size >= old_byte_size); if (new_byte_size ==
> old_byte_size), but as I said before, I'm not a friend of assertions when
> the code can deal perfectly well with the "unsupported" case.
It is simpler to put an if statement around the memset. Then the
function actually frees unused memory and readers don't wonder why you
decided not to shrink the array.
Less code and slightly better behavior.
Stefan
pgpEfp6wutS8o.pgp
Description: PGP signature
Re: [Qemu-devel] [PATCH v3 06/22] qcow2: Helper for refcount array reallocation
On 2014-11-27 at 16:09, Stefan Hajnoczi wrote:
On Thu, Nov 20, 2014 at 06:06:22PM +0100, Max Reitz wrote:
+/**
+ * Reallocates *array so that it can hold new_size entries. *size must contain
+ * the current number of entries in *array. If the reallocation fails, *array
+ * and *size will not be modified and -errno will be returned. If the
+ * reallocation is successful, *array will be set to the new buffer and *size
+ * will be set to new_size. The size of the reallocated refcount array buffer
+ * will be aligned to a cluster boundary, and the newly allocated area will be
+ * zeroed.
+ */
+static int realloc_refcount_array(BDRVQcowState *s, uint16_t **array,
+ int64_t *size, int64_t new_size)
+{
+/* 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;
+
+if (new_byte_size <= old_byte_size) {
+*size = new_size;
+return 0;
+}
Why not realloc the array to the new smaller size? ...
Because such a call will actually never happen. I could replace this if
() by assert(new_byte_size >= old_byte_size); if (new_byte_size ==
old_byte_size), but as I said before, I'm not a friend of assertions
when the code can deal perfectly well with the "unsupported" case.
Max
+
+assert(new_byte_size > 0);
+
+new_ptr = g_try_realloc(*array, new_byte_size);
+if (!new_ptr) {
+return -ENOMEM;
+}
+
+memset((void *)((uintptr_t)new_ptr + old_byte_size), 0,
+ new_byte_size - old_byte_size);
...we just need to skip the memset in when new_byte_size is smaller
than old_byte_size.
Re: [Qemu-devel] [PATCH v3 06/22] qcow2: Helper for refcount array reallocation
On Thu, Nov 20, 2014 at 06:06:22PM +0100, Max Reitz wrote:
> +/**
> + * Reallocates *array so that it can hold new_size entries. *size must
> contain
> + * the current number of entries in *array. If the reallocation fails, *array
> + * and *size will not be modified and -errno will be returned. If the
> + * reallocation is successful, *array will be set to the new buffer and *size
> + * will be set to new_size. The size of the reallocated refcount array buffer
> + * will be aligned to a cluster boundary, and the newly allocated area will
> be
> + * zeroed.
> + */
> +static int realloc_refcount_array(BDRVQcowState *s, uint16_t **array,
> + int64_t *size, int64_t new_size)
> +{
> +/* 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;
> +
> +if (new_byte_size <= old_byte_size) {
> +*size = new_size;
> +return 0;
> +}
Why not realloc the array to the new smaller size? ...
> +
> +assert(new_byte_size > 0);
> +
> +new_ptr = g_try_realloc(*array, new_byte_size);
> +if (!new_ptr) {
> +return -ENOMEM;
> +}
> +
> +memset((void *)((uintptr_t)new_ptr + old_byte_size), 0,
> + new_byte_size - old_byte_size);
...we just need to skip the memset in when new_byte_size is smaller
than old_byte_size.
pgpGB_wFduFDj.pgp
Description: PGP signature
Re: [Qemu-devel] [PATCH v3 06/22] qcow2: Helper for refcount array reallocation
On 2014-11-20 at 22:43, Eric Blake wrote:
On 11/20/2014 10:06 AM, Max Reitz wrote:
Add a helper function for reallocating a refcount array, independent of
the refcount order. The newly allocated space is zeroed and the function
handles failed reallocations gracefully.
The helper function will always align the buffer size to a cluster
boundary; if storing the refcounts in such an array in big endian byte
order, this makes it possible to write parts of the array directly as
refcount blocks into the image file.
Signed-off-by: Max Reitz
Reviewed-by: Eric Blake
Perhaps the changes since v2 warranted removing my earlier R-b to make
sure I review closely?
Well, normally I would not have taken the R-b. But you explicitly wrote:
Code looks correct as written, whether or not you also add more
comments, asserts, and/or shortcuts for no-op situations.
So I took that to mean "You may change the commit message as proposed
(s/independently/independent/ and add a note about the cluster
alignment), add a comment to realloc_refcount_array() about its calling
contract, an assert(new_byte_size > 0) and/or an early-out for when the
byte size of the refcount array does not increase", which is pretty
broad, but that's what you wrote. That's why I kept the R-b in.
---
block/qcow2-refcount.c | 135 +++--
1 file changed, 86 insertions(+), 49 deletions(-)
+static int realloc_refcount_array(BDRVQcowState *s, uint16_t **array,
+ int64_t *size, int64_t new_size)
+{
+/* 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;
+
+if (new_byte_size <= old_byte_size) {
+*size = new_size;
+return 0;
+}
+
+assert(new_byte_size > 0);
Should this assert be moved before the early exit?
Could do, but it should not matter. It's important that new_byte_size >
0 after the early-out; the caller should not use new_size == 0 at all,
but I'd rather not add an assertion against a contract breach somewhere
where the code can handle that breach actually just fine.
Hmm, that's my only finding, and you did incorporate improvements mostly
to comments or asserts as compared to v2. Moving the assert is small
enough, and not a show-stopper if you leave it in place, so:
Reviewed-by: Eric Blake
Phew :-)
Thanks!
Max
Re: [Qemu-devel] [PATCH v3 06/22] qcow2: Helper for refcount array reallocation
On 11/20/2014 10:06 AM, Max Reitz wrote:
> Add a helper function for reallocating a refcount array, independent of
> the refcount order. The newly allocated space is zeroed and the function
> handles failed reallocations gracefully.
>
> The helper function will always align the buffer size to a cluster
> boundary; if storing the refcounts in such an array in big endian byte
> order, this makes it possible to write parts of the array directly as
> refcount blocks into the image file.
>
> Signed-off-by: Max Reitz
> Reviewed-by: Eric Blake
Perhaps the changes since v2 warranted removing my earlier R-b to make
sure I review closely?
> ---
> block/qcow2-refcount.c | 135
> +++--
> 1 file changed, 86 insertions(+), 49 deletions(-)
>
> +static int realloc_refcount_array(BDRVQcowState *s, uint16_t **array,
> + int64_t *size, int64_t new_size)
> +{
> +/* 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;
> +
> +if (new_byte_size <= old_byte_size) {
> +*size = new_size;
> +return 0;
> +}
> +
> +assert(new_byte_size > 0);
Should this assert be moved before the early exit?
Hmm, that's my only finding, and you did incorporate improvements mostly
to comments or asserts as compared to v2. Moving the assert is small
enough, and not a show-stopper if you leave it in place, so:
Reviewed-by: Eric Blake
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
[Qemu-devel] [PATCH v3 06/22] qcow2: Helper for refcount array reallocation
Add a helper function for reallocating a refcount array, independent of
the refcount order. The newly allocated space is zeroed and the function
handles failed reallocations gracefully.
The helper function will always align the buffer size to a cluster
boundary; if storing the refcounts in such an array in big endian byte
order, this makes it possible to write parts of the array directly as
refcount blocks into the image file.
Signed-off-by: Max Reitz
Reviewed-by: Eric Blake
---
block/qcow2-refcount.c | 135 +++--
1 file changed, 86 insertions(+), 49 deletions(-)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 66c78c0..3cd540c 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1108,6 +1108,68 @@ fail:
/* refcount checking functions */
+static size_t refcount_array_byte_size(BDRVQcowState *s, uint64_t entries)
+{
+if (s->refcount_order < 3) {
+/* sub-byte width */
+int shift = 3 - s->refcount_order;
+return (entries + (1 << shift) - 1) >> shift;
+} else if (s->refcount_order == 3) {
+/* byte width */
+return entries;
+} else {
+/* multiple bytes wide */
+
+/* This assertion holds because there is no way we can address more
than
+ * 2^(64 - 9) clusters at once (with cluster size 512 = 2^9, and
because
+ * offsets have to be representable in bytes); due to every cluster
+ * corresponding to one refcount entry and because refcount_order has
to
+ * be below 7, we are far below that limit */
+assert(!(entries >> (64 - (s->refcount_order - 3;
+
+return entries << (s->refcount_order - 3);
+}
+}
+
+/**
+ * Reallocates *array so that it can hold new_size entries. *size must contain
+ * the current number of entries in *array. If the reallocation fails, *array
+ * and *size will not be modified and -errno will be returned. If the
+ * reallocation is successful, *array will be set to the new buffer and *size
+ * will be set to new_size. The size of the reallocated refcount array buffer
+ * will be aligned to a cluster boundary, and the newly allocated area will be
+ * zeroed.
+ */
+static int realloc_refcount_array(BDRVQcowState *s, uint16_t **array,
+ int64_t *size, int64_t new_size)
+{
+/* 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;
+
+if (new_byte_size <= old_byte_size) {
+*size = new_size;
+return 0;
+}
+
+assert(new_byte_size > 0);
+
+new_ptr = g_try_realloc(*array, new_byte_size);
+if (!new_ptr) {
+return -ENOMEM;
+}
+
+memset((void *)((uintptr_t)new_ptr + old_byte_size), 0,
+ new_byte_size - old_byte_size);
+
+*array = new_ptr;
+*size = new_size;
+
+return 0;
+}
/*
* Increases the refcount for a range of clusters in a given refcount table.
@@ -1124,6 +1186,7 @@ static int inc_refcounts(BlockDriverState *bs,
{
BDRVQcowState *s = bs->opaque;
uint64_t start, last, cluster_offset, k;
+int ret;
if (size <= 0) {
return 0;
@@ -1135,23 +1198,12 @@ static int inc_refcounts(BlockDriverState *bs,
cluster_offset += s->cluster_size) {
k = cluster_offset >> s->cluster_bits;
if (k >= *refcount_table_size) {
-int64_t old_refcount_table_size = *refcount_table_size;
-uint16_t *new_refcount_table;
-
-*refcount_table_size = k + 1;
-new_refcount_table = g_try_realloc(*refcount_table,
- *refcount_table_size *
- sizeof(**refcount_table));
-if (!new_refcount_table) {
-*refcount_table_size = old_refcount_table_size;
+ret = realloc_refcount_array(s, refcount_table,
+ refcount_table_size, k + 1);
+if (ret < 0) {
res->check_errors++;
-return -ENOMEM;
+return ret;
}
-*refcount_table = new_refcount_table;
-
-memset(*refcount_table + old_refcount_table_size, 0,
- (*refcount_table_size - old_refcount_table_size) *
- sizeof(**refcount_table));
}
if (++(*refcount_table)[k] == 0) {
@@ -1518,8 +1570,7 @@ static int check_refblocks(BlockDriverState *bs,
BdrvCheckResult *res,
fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
if (fix & BDRV_FIX_ERRORS) {
-int64_t old_nb_clusters = *nb_clusters;
-uint16_t *new_refcount_table;
+
