Re: [Qemu-devel] [PATCH v4 06/26] qcow2: Use 64 bits for refcount values
On Wed, Dec 03, 2014 at 02:37:26PM +0100, Max Reitz wrote: > Refcounts may have a width of up to 64 bits, so qemu should use the same > width to represent refcount values internally. > > Signed-off-by: Max Reitz > --- > block/qcow2-cluster.c | 2 +- > block/qcow2-refcount.c | 42 -- > block/qcow2.h | 4 ++-- > 3 files changed, 23 insertions(+), 25 deletions(-) Reviewed-by: Stefan Hajnoczi pgpMnBTs1zeWE.pgp Description: PGP signature
Re: [Qemu-devel] [PATCH v4 06/26] qcow2: Use 64 bits for refcount values
On 2014-12-03 at 17:11, Eric Blake wrote:
On 12/03/2014 06:37 AM, Max Reitz wrote:
Refcounts may have a width of up to 64 bits, so qemu should use the same
width to represent refcount values internally.
Signed-off-by: Max Reitz
---
block/qcow2-cluster.c | 2 +-
block/qcow2-refcount.c | 42 --
block/qcow2.h | 4 ++--
3 files changed, 23 insertions(+), 25 deletions(-)
@@ -1698,7 +1696,7 @@ static void compare_refcounts(BlockDriverState *bs,
BdrvCheckResult *res,
refcount2 - refcount1,
refcount1 > refcount2,
QCOW2_DISCARD_ALWAYS);
-if (ret >= 0) {
+if (ret == 0) {
(*num_fixed)++;
continue;
}
This hunk looks a bit misplaced (it is unrelated to a sizing change).
Ah, right, I think it should have been in patch 3. I even looked through
the patches before sending them, but somehow I missed this...
Patch 5 altered update_refcount, but does not document its return value.
But a careful step through update_refcount() shows that all error paths
return negative, and the only places where we do things like 'ret =
alloc_refcount_block(...)" are later followed by an unconditional 'ret =
0' on success paths (so I didn't check whether alloc_refcount_block
returns positive values, but didn't need to). If you have to respin, it
might be better to add documentation of the return value and update
callers to assume a non-positive return as a separate patch, rather than
sliding it in with this one. But as I don't see any code faults, and am
not sure it is worth a respin just for this issue,
Hm, I don't think this is something that should be fixed up by the
applying maintainer. I will respin, even if it's just for this, but I'll
wait until this series has been sufficiently looked at.
Reviewed-by: Eric Blake
Once again, thank you!
Max
Re: [Qemu-devel] [PATCH v4 06/26] qcow2: Use 64 bits for refcount values
On 12/03/2014 06:37 AM, Max Reitz wrote:
> Refcounts may have a width of up to 64 bits, so qemu should use the same
> width to represent refcount values internally.
>
> Signed-off-by: Max Reitz
> ---
> block/qcow2-cluster.c | 2 +-
> block/qcow2-refcount.c | 42 --
> block/qcow2.h | 4 ++--
> 3 files changed, 23 insertions(+), 25 deletions(-)
>
> @@ -1698,7 +1696,7 @@ static void compare_refcounts(BlockDriverState *bs,
> BdrvCheckResult *res,
>refcount2 - refcount1,
>refcount1 > refcount2,
>QCOW2_DISCARD_ALWAYS);
> -if (ret >= 0) {
> +if (ret == 0) {
> (*num_fixed)++;
> continue;
> }
This hunk looks a bit misplaced (it is unrelated to a sizing change).
Patch 5 altered update_refcount, but does not document its return value.
But a careful step through update_refcount() shows that all error paths
return negative, and the only places where we do things like 'ret =
alloc_refcount_block(...)" are later followed by an unconditional 'ret =
0' on success paths (so I didn't check whether alloc_refcount_block
returns positive values, but didn't need to). If you have to respin, it
might be better to add documentation of the return value and update
callers to assume a non-positive return as a separate patch, rather than
sliding it in with this one. But as I don't see any code faults, and am
not sure it is worth a respin just for this issue,
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 v4 06/26] qcow2: Use 64 bits for refcount values
Refcounts may have a width of up to 64 bits, so qemu should use the same
width to represent refcount values internally.
Signed-off-by: Max Reitz
---
block/qcow2-cluster.c | 2 +-
block/qcow2-refcount.c | 42 --
block/qcow2.h | 4 ++--
3 files changed, 23 insertions(+), 25 deletions(-)
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index da37535..5a678f3 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1640,7 +1640,7 @@ static int expand_zero_clusters_in_l1(BlockDriverState
*bs, uint64_t *l1_table,
for (i = 0; i < l1_size; i++) {
uint64_t l2_offset = l1_table[i] & L1E_OFFSET_MASK;
bool l2_dirty = false;
-uint16_t l2_refcount;
+uint64_t l2_refcount;
if (!l2_offset) {
/* unallocated */
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index b3aed9c..095ff9b 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -29,7 +29,7 @@
static int64_t alloc_clusters_noref(BlockDriverState *bs, uint64_t size);
static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs,
-int64_t offset, int64_t length, uint16_t addend,
+int64_t offset, int64_t length, uint64_t addend,
bool decrease, enum qcow2_discard_type type);
@@ -91,7 +91,7 @@ static int load_refcount_block(BlockDriverState *bs,
* *refcount. Returns 0 on success and -errno on failure.
*/
int qcow2_get_refcount(BlockDriverState *bs, int64_t cluster_index,
- uint16_t *refcount)
+ uint64_t *refcount)
{
BDRVQcowState *s = bs->opaque;
uint64_t refcount_table_index, block_index;
@@ -537,7 +537,7 @@ found:
static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs,
int64_t offset,
int64_t length,
- uint16_t addend,
+ uint64_t addend,
bool decrease,
enum qcow2_discard_type
type)
{
@@ -549,8 +549,8 @@ static int QEMU_WARN_UNUSED_RESULT
update_refcount(BlockDriverState *bs,
#ifdef DEBUG_ALLOC2
fprintf(stderr, "update_refcount: offset=%" PRId64 " size=%" PRId64
-" addend=%" PRId16 " decrease=%d\n", offset, length,
-(int16_t)addend, decrease);
+" addend=%" PRId64 " decrease=%d\n", offset, length,
+(int64_t)addend, decrease);
#endif
if (length < 0) {
return -EINVAL;
@@ -569,7 +569,7 @@ static int QEMU_WARN_UNUSED_RESULT
update_refcount(BlockDriverState *bs,
cluster_offset += s->cluster_size)
{
int block_index;
-uint16_t refcount;
+uint64_t refcount;
int64_t cluster_index = cluster_offset >> s->cluster_bits;
int64_t table_index = cluster_index >> s->refcount_block_bits;
@@ -596,9 +596,9 @@ static int QEMU_WARN_UNUSED_RESULT
update_refcount(BlockDriverState *bs,
block_index = cluster_index & (s->refcount_block_size - 1);
refcount = be16_to_cpu(refcount_block[block_index]);
-if ((uint16_t)(refcount + addend) > s->refcount_max ||
-(!decrease && (uint16_t)(refcount + addend) < refcount) ||
-( decrease && (uint16_t)(refcount + addend) > refcount))
+if (refcount + addend > s->refcount_max ||
+(!decrease && refcount + addend < refcount) ||
+( decrease && refcount + addend > refcount))
{
ret = -EINVAL;
goto fail;
@@ -656,7 +656,7 @@ fail:
*/
int qcow2_update_cluster_refcount(BlockDriverState *bs,
int64_t cluster_index,
- uint16_t addend, bool decrease,
+ uint64_t addend, bool decrease,
enum qcow2_discard_type type)
{
BDRVQcowState *s = bs->opaque;
@@ -682,8 +682,7 @@ int qcow2_update_cluster_refcount(BlockDriverState *bs,
static int64_t alloc_clusters_noref(BlockDriverState *bs, uint64_t size)
{
BDRVQcowState *s = bs->opaque;
-uint64_t i, nb_clusters;
-uint16_t refcount;
+uint64_t i, nb_clusters, refcount;
int ret;
nb_clusters = size_to_clusters(s, size);
@@ -741,9 +740,8 @@ int qcow2_alloc_clusters_at(BlockDriverState *bs, uint64_t
offset,
int nb_clusters)
{
BDRVQcowState *s = bs->opaque;
-uint64_t cluster_index;
+uint64_t cluster_index, refcount;
uint64_t i;
-uint16_t refcount;
int ret;
assert(nb_clusters >= 0);
@@ -897,11 +895,10 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
int64_t l1_table_offset, int l1_size, int addend)
{
BDRVQcowState *s = bs->op
