* [Qemu-devel] [PATCH v4 1/5] qcow2-refcount: fix check_oflag_copied
2018-12-14 13:42 [Qemu-devel] [PATCH v4 0/5] qcow2 check improvements part I Vladimir Sementsov-Ogievskiy
@ 2018-12-14 13:42 ` Vladimir Sementsov-Ogievskiy
2019-02-27 12:02 ` Max Reitz
2018-12-14 13:42 ` [Qemu-devel] [PATCH v4 2/5] qcow2-refcount: avoid eating RAM Vladimir Sementsov-Ogievskiy
` (4 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-12-14 13:42 UTC (permalink / raw)
To: qemu-block, qemu-devel; +Cc: mreitz, kwolf, vsementsov, den
Increase corruptions_fixed only after successful fix.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/qcow2-refcount.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 1c63ac244a..b717fdecc6 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1822,7 +1822,7 @@ static int check_oflag_copied(BlockDriverState *bs, BdrvCheckResult *res,
for (i = 0; i < s->l1_size; i++) {
uint64_t l1_entry = s->l1_table[i];
uint64_t l2_offset = l1_entry & L1E_OFFSET_MASK;
- bool l2_dirty = false;
+ int l2_dirty = 0;
if (!l2_offset) {
continue;
@@ -1884,8 +1884,7 @@ static int check_oflag_copied(BlockDriverState *bs, BdrvCheckResult *res,
l2_table[j] = cpu_to_be64(refcount == 1
? l2_entry | QCOW_OFLAG_COPIED
: l2_entry & ~QCOW_OFLAG_COPIED);
- l2_dirty = true;
- res->corruptions_fixed++;
+ l2_dirty++;
} else {
res->corruptions++;
}
@@ -1893,7 +1892,7 @@ static int check_oflag_copied(BlockDriverState *bs, BdrvCheckResult *res,
}
}
- if (l2_dirty) {
+ if (l2_dirty > 0) {
ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_ACTIVE_L2,
l2_offset, s->cluster_size);
if (ret < 0) {
@@ -1911,6 +1910,7 @@ static int check_oflag_copied(BlockDriverState *bs, BdrvCheckResult *res,
res->check_errors++;
goto fail;
}
+ res->corruptions_fixed += l2_dirty;
}
}
--
2.18.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* [Qemu-devel] [PATCH v4 2/5] qcow2-refcount: avoid eating RAM
2018-12-14 13:42 [Qemu-devel] [PATCH v4 0/5] qcow2 check improvements part I Vladimir Sementsov-Ogievskiy
2018-12-14 13:42 ` [Qemu-devel] [PATCH v4 1/5] qcow2-refcount: fix check_oflag_copied Vladimir Sementsov-Ogievskiy
@ 2018-12-14 13:42 ` Vladimir Sementsov-Ogievskiy
2019-02-27 12:26 ` Max Reitz
2019-02-27 14:43 ` Eric Blake
2018-12-14 13:42 ` [Qemu-devel] [PATCH v4 3/5] qcow2-refcount: check_refcounts_l2: reduce ignored overlaps Vladimir Sementsov-Ogievskiy
` (3 subsequent siblings)
5 siblings, 2 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-12-14 13:42 UTC (permalink / raw)
To: qemu-block, qemu-devel; +Cc: mreitz, kwolf, vsementsov, den
qcow2_inc_refcounts_imrt() (through realloc_refcount_array()) can eat
an unpredictable amount of memory on corrupted table entries, which are
referencing regions far beyond the end of file.
Prevent this, by skipping such regions from further processing.
Interesting that iotest 138 checks exactly the behavior which we fix
here. So, change the test appropriately.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/qcow2-refcount.c | 18 ++++++++++++++++++
tests/qemu-iotests/138 | 12 +++++-------
tests/qemu-iotests/138.out | 5 ++++-
3 files changed, 27 insertions(+), 8 deletions(-)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index b717fdecc6..c41c1fbe00 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1505,12 +1505,30 @@ int qcow2_inc_refcounts_imrt(BlockDriverState *bs, BdrvCheckResult *res,
{
BDRVQcow2State *s = bs->opaque;
uint64_t start, last, cluster_offset, k, refcount;
+ int64_t file_len;
int ret;
if (size <= 0) {
return 0;
}
+ file_len = bdrv_getlength(bs->file->bs);
+ if (file_len < 0) {
+ return file_len;
+ }
+
+ /* Last cluster of qcow2 image may be semi-allocated, so it's may be OK to
+ * reference some space after file end but it should be less than one
+ * cluster.
+ */
+ if (offset + size - file_len >= s->cluster_size) {
+ fprintf(stderr, "ERROR: counting reference for region exceeding the "
+ "end of the file by one cluster or more: offset 0x%" PRIx64
+ " size 0x%" PRIx64 "\n", offset, size);
+ res->corruptions++;
+ return 0;
+ }
+
start = start_of_cluster(s, offset);
last = start_of_cluster(s, offset + size - 1);
for(cluster_offset = start; cluster_offset <= last;
diff --git a/tests/qemu-iotests/138 b/tests/qemu-iotests/138
index eccbcae3a6..12e20762e5 100755
--- a/tests/qemu-iotests/138
+++ b/tests/qemu-iotests/138
@@ -54,15 +54,13 @@ $QEMU_IO -c 'write 0 512' "$TEST_IMG" | _filter_qemu_io
# Put the data cluster at a multiple of 2 TB, resulting in the image apparently
# having a multiple of 2^32 clusters
# (To be more specific: It is at 32 PB)
-poke_file "$TEST_IMG" 2048 "\x80\x80\x00\x00\x00\x00\x00\x00"
+poke_file "$TEST_IMG" $((2048 + 8)) "\x00\x80\x00\x00\x00\x00\x00\x00"
# An offset of 32 PB results in qemu-img check having to allocate an in-memory
-# refcount table of 128 TB (16 bit refcounts, 512 byte clusters).
-# This should be generally too much for any system and thus fail.
-# What this test is checking is that the qcow2 driver actually tries to allocate
-# such a large amount of memory (and is consequently aborting) instead of having
-# truncated the cluster count somewhere (which would result in much less memory
-# being allocated and then a segfault occurring).
+# refcount table of 128 TB (16 bit refcounts, 512 byte clusters), if qemu-img
+# don't check that referenced data cluster is far beyond the end of file.
+# But starting from 4.0, qemu-img does this check, and instead of "Cannot
+# allocate memory", we have an error showing that l2 entry is invalid.
_check_test_img
# success, all done
diff --git a/tests/qemu-iotests/138.out b/tests/qemu-iotests/138.out
index 3fe911f85a..aca7d47a80 100644
--- a/tests/qemu-iotests/138.out
+++ b/tests/qemu-iotests/138.out
@@ -5,5 +5,8 @@ QA output created by 138
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=512
wrote 512/512 bytes at offset 0
512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-qemu-img: Check failed: Cannot allocate memory
+ERROR: counting reference for region exceeding the end of the file by one cluster or more: offset 0x80000000000000 size 0x200
+
+1 errors were found on the image.
+Data may be corrupted, or further writes to the image may corrupt it.
*** done
--
2.18.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH v4 2/5] qcow2-refcount: avoid eating RAM
2018-12-14 13:42 ` [Qemu-devel] [PATCH v4 2/5] qcow2-refcount: avoid eating RAM Vladimir Sementsov-Ogievskiy
@ 2019-02-27 12:26 ` Max Reitz
2019-02-27 14:43 ` Eric Blake
1 sibling, 0 replies; 15+ messages in thread
From: Max Reitz @ 2019-02-27 12:26 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block, qemu-devel; +Cc: kwolf, den
[-- Attachment #1: Type: text/plain, Size: 4724 bytes --]
On 14.12.18 14:42, Vladimir Sementsov-Ogievskiy wrote:
> qcow2_inc_refcounts_imrt() (through realloc_refcount_array()) can eat
> an unpredictable amount of memory on corrupted table entries, which are
> referencing regions far beyond the end of file.
>
> Prevent this, by skipping such regions from further processing.
Is it actually wrong to reference clusters beyond the EOF?
Hmm... The spec says everywhere "offset into the image file" (except
for L2 entries pointing to data clusters, but let's say that's just a
mistake). This implies that offsets have to be within the confines of
the file, OK.
> Interesting that iotest 138 checks exactly the behavior which we fix
> here. So, change the test appropriately.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> block/qcow2-refcount.c | 18 ++++++++++++++++++
> tests/qemu-iotests/138 | 12 +++++-------
> tests/qemu-iotests/138.out | 5 ++++-
> 3 files changed, 27 insertions(+), 8 deletions(-)
>
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index b717fdecc6..c41c1fbe00 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -1505,12 +1505,30 @@ int qcow2_inc_refcounts_imrt(BlockDriverState *bs, BdrvCheckResult *res,
> {
> BDRVQcow2State *s = bs->opaque;
> uint64_t start, last, cluster_offset, k, refcount;
> + int64_t file_len;
> int ret;
>
> if (size <= 0) {
> return 0;
> }
>
> + file_len = bdrv_getlength(bs->file->bs);
I just had to delete a paragraph where I was questioning the use of
bdrv_getlength() here again, because I remembered I did that before and
decided to give in...
> + if (file_len < 0) {
> + return file_len;
> + }
> +
> + /* Last cluster of qcow2 image may be semi-allocated, so it's may be OK to
With s/it's/it/:
Reviewed-by: Max Reitz <mreitz@redhat.com>
> + * reference some space after file end but it should be less than one
> + * cluster.
> + */
> + if (offset + size - file_len >= s->cluster_size) {
> + fprintf(stderr, "ERROR: counting reference for region exceeding the "
> + "end of the file by one cluster or more: offset 0x%" PRIx64
> + " size 0x%" PRIx64 "\n", offset, size);
> + res->corruptions++;
> + return 0;
> + }
> +
> start = start_of_cluster(s, offset);
> last = start_of_cluster(s, offset + size - 1);
> for(cluster_offset = start; cluster_offset <= last;
> diff --git a/tests/qemu-iotests/138 b/tests/qemu-iotests/138
> index eccbcae3a6..12e20762e5 100755
> --- a/tests/qemu-iotests/138
> +++ b/tests/qemu-iotests/138
> @@ -54,15 +54,13 @@ $QEMU_IO -c 'write 0 512' "$TEST_IMG" | _filter_qemu_io
> # Put the data cluster at a multiple of 2 TB, resulting in the image apparently
> # having a multiple of 2^32 clusters
> # (To be more specific: It is at 32 PB)
> -poke_file "$TEST_IMG" 2048 "\x80\x80\x00\x00\x00\x00\x00\x00"
> +poke_file "$TEST_IMG" $((2048 + 8)) "\x00\x80\x00\x00\x00\x00\x00\x00"
>
> # An offset of 32 PB results in qemu-img check having to allocate an in-memory
> -# refcount table of 128 TB (16 bit refcounts, 512 byte clusters).
> -# This should be generally too much for any system and thus fail.
> -# What this test is checking is that the qcow2 driver actually tries to allocate
> -# such a large amount of memory (and is consequently aborting) instead of having
> -# truncated the cluster count somewhere (which would result in much less memory
> -# being allocated and then a segfault occurring).
> +# refcount table of 128 TB (16 bit refcounts, 512 byte clusters), if qemu-img
> +# don't check that referenced data cluster is far beyond the end of file.
> +# But starting from 4.0, qemu-img does this check, and instead of "Cannot
> +# allocate memory", we have an error showing that l2 entry is invalid.
> _check_test_img
>
> # success, all done
> diff --git a/tests/qemu-iotests/138.out b/tests/qemu-iotests/138.out
> index 3fe911f85a..aca7d47a80 100644
> --- a/tests/qemu-iotests/138.out
> +++ b/tests/qemu-iotests/138.out
> @@ -5,5 +5,8 @@ QA output created by 138
> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=512
> wrote 512/512 bytes at offset 0
> 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> -qemu-img: Check failed: Cannot allocate memory
> +ERROR: counting reference for region exceeding the end of the file by one cluster or more: offset 0x80000000000000 size 0x200
> +
> +1 errors were found on the image.
> +Data may be corrupted, or further writes to the image may corrupt it.
> *** done
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH v4 2/5] qcow2-refcount: avoid eating RAM
2018-12-14 13:42 ` [Qemu-devel] [PATCH v4 2/5] qcow2-refcount: avoid eating RAM Vladimir Sementsov-Ogievskiy
2019-02-27 12:26 ` Max Reitz
@ 2019-02-27 14:43 ` Eric Blake
1 sibling, 0 replies; 15+ messages in thread
From: Eric Blake @ 2019-02-27 14:43 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block, qemu-devel; +Cc: kwolf, den, mreitz
On 12/14/18 7:42 AM, Vladimir Sementsov-Ogievskiy wrote:
> qcow2_inc_refcounts_imrt() (through realloc_refcount_array()) can eat
> an unpredictable amount of memory on corrupted table entries, which are
> referencing regions far beyond the end of file.
>
> Prevent this, by skipping such regions from further processing.
>
> Interesting that iotest 138 checks exactly the behavior which we fix
> here. So, change the test appropriately.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> + /* Last cluster of qcow2 image may be semi-allocated, so it's may be OK to
> + * reference some space after file end but it should be less than one
> + * cluster.
Are we guaranteed that this is always the case, even when incrementally
building up an image. That is, if we get a power failure in the middle
of allocating both a new L2 table and a larger refcount table, can we
see an image which temporary references 2 or more clusters beyond the
end of the file that it was previously using? Or are we guaranteed that
the way we update the refcount table will never see more than one
cluster beyond the end of the file for any other reference? I guess
where I'm going with this is a question of whether we should permit a
finite overrun to account for multiple pieces of metadata all being in
flight at once, and only reject the obvious overruns that are definitely
beyond that sane limit, and whether 1 cluster is too small for that sane
limit.
> + */
> + if (offset + size - file_len >= s->cluster_size) {
> + fprintf(stderr, "ERROR: counting reference for region exceeding the "
> + "end of the file by one cluster or more: offset 0x%" PRIx64
> + " size 0x%" PRIx64 "\n", offset, size);
Should we be using something smarter than fprintf() here?
Grammar suggestion:
ERROR: reference to a region too far beyond the end of the file:
(which is shorter than what you wrote, and also has the nice property of
allowing us to change our mind on whether it is 1 cluster, 2, 10, or
some other finite limit as our heuristic of when we consider the image
corrupt without having to reword it).
> +++ b/tests/qemu-iotests/138
> @@ -54,15 +54,13 @@ $QEMU_IO -c 'write 0 512' "$TEST_IMG" | _filter_qemu_io
> # Put the data cluster at a multiple of 2 TB, resulting in the image apparently
> # having a multiple of 2^32 clusters
> # (To be more specific: It is at 32 PB)
> -poke_file "$TEST_IMG" 2048 "\x80\x80\x00\x00\x00\x00\x00\x00"
> +poke_file "$TEST_IMG" $((2048 + 8)) "\x00\x80\x00\x00\x00\x00\x00\x00"
>
> # An offset of 32 PB results in qemu-img check having to allocate an in-memory
> -# refcount table of 128 TB (16 bit refcounts, 512 byte clusters).
> -# This should be generally too much for any system and thus fail.
> -# What this test is checking is that the qcow2 driver actually tries to allocate
> -# such a large amount of memory (and is consequently aborting) instead of having
> -# truncated the cluster count somewhere (which would result in much less memory
> -# being allocated and then a segfault occurring).
> +# refcount table of 128 TB (16 bit refcounts, 512 byte clusters), if qemu-img
> +# don't check that referenced data cluster is far beyond the end of file.
> +# But starting from 4.0, qemu-img does this check, and instead of "Cannot
> +# allocate memory", we have an error showing that l2 entry is invalid.
> _check_test_img
In other words, we are still gracefully invalidating the file, but now
because of a heuristic rather than a memory allocation failure.
I might word the comment differently:
An offset of 32 PB would result in qemu-img check having to allocate an
in-memory refcount table of 128 TB (16 bit refcounts, 512 byte
clusters). It is unlikely that a system has this much memory, so the
test ensures that qemu-img either gracefully fails an allocation (prior
to 4.0) or flags the image as invalid (the reference points to a cluster
too far beyond the actual file), rather than silently allocating a
truncated memory amount or dumping core.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH v4 3/5] qcow2-refcount: check_refcounts_l2: reduce ignored overlaps
2018-12-14 13:42 [Qemu-devel] [PATCH v4 0/5] qcow2 check improvements part I Vladimir Sementsov-Ogievskiy
2018-12-14 13:42 ` [Qemu-devel] [PATCH v4 1/5] qcow2-refcount: fix check_oflag_copied Vladimir Sementsov-Ogievskiy
2018-12-14 13:42 ` [Qemu-devel] [PATCH v4 2/5] qcow2-refcount: avoid eating RAM Vladimir Sementsov-Ogievskiy
@ 2018-12-14 13:42 ` Vladimir Sementsov-Ogievskiy
2018-12-14 13:42 ` [Qemu-devel] [PATCH v4 4/5] qcow2-refcount: check_refcounts_l2: don't count fixed cluster as allocated Vladimir Sementsov-Ogievskiy
` (2 subsequent siblings)
5 siblings, 0 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-12-14 13:42 UTC (permalink / raw)
To: qemu-block, qemu-devel; +Cc: mreitz, kwolf, vsementsov, den
Reduce number of structures ignored in overlap check: when checking
active table ignore active tables, when checking inactive table ignore
inactive ones.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block/qcow2-refcount.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index c41c1fbe00..e76085d9aa 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1575,7 +1575,7 @@ enum {
static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
void **refcount_table,
int64_t *refcount_table_size, int64_t l2_offset,
- int flags, BdrvCheckMode fix)
+ int flags, BdrvCheckMode fix, bool active)
{
BDRVQcow2State *s = bs->opaque;
uint64_t *l2_table, l2_entry;
@@ -1659,11 +1659,12 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
if (fix & BDRV_FIX_ERRORS) {
uint64_t l2e_offset =
l2_offset + (uint64_t)i * sizeof(uint64_t);
+ int ign = active ? QCOW2_OL_ACTIVE_L2 :
+ QCOW2_OL_INACTIVE_L2;
l2_entry = QCOW_OFLAG_ZERO;
l2_table[i] = cpu_to_be64(l2_entry);
- ret = qcow2_pre_write_overlap_check(bs,
- QCOW2_OL_ACTIVE_L2 | QCOW2_OL_INACTIVE_L2,
+ ret = qcow2_pre_write_overlap_check(bs, ign,
l2e_offset, sizeof(uint64_t));
if (ret < 0) {
fprintf(stderr, "ERROR: Overlap check failed\n");
@@ -1737,7 +1738,7 @@ static int check_refcounts_l1(BlockDriverState *bs,
void **refcount_table,
int64_t *refcount_table_size,
int64_t l1_table_offset, int l1_size,
- int flags, BdrvCheckMode fix)
+ int flags, BdrvCheckMode fix, bool active)
{
BDRVQcow2State *s = bs->opaque;
uint64_t *l1_table = NULL, l2_offset, l1_size2;
@@ -1793,7 +1794,7 @@ static int check_refcounts_l1(BlockDriverState *bs,
/* Process and check L2 entries */
ret = check_refcounts_l2(bs, res, refcount_table,
refcount_table_size, l2_offset, flags,
- fix);
+ fix, active);
if (ret < 0) {
goto fail;
}
@@ -2079,7 +2080,7 @@ static int calculate_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
/* current L1 table */
ret = check_refcounts_l1(bs, res, refcount_table, nb_clusters,
s->l1_table_offset, s->l1_size, CHECK_FRAG_INFO,
- fix);
+ fix, true);
if (ret < 0) {
return ret;
}
@@ -2102,7 +2103,8 @@ static int calculate_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
continue;
}
ret = check_refcounts_l1(bs, res, refcount_table, nb_clusters,
- sn->l1_table_offset, sn->l1_size, 0, fix);
+ sn->l1_table_offset, sn->l1_size, 0, fix,
+ false);
if (ret < 0) {
return ret;
}
--
2.18.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* [Qemu-devel] [PATCH v4 4/5] qcow2-refcount: check_refcounts_l2: don't count fixed cluster as allocated
2018-12-14 13:42 [Qemu-devel] [PATCH v4 0/5] qcow2 check improvements part I Vladimir Sementsov-Ogievskiy
` (2 preceding siblings ...)
2018-12-14 13:42 ` [Qemu-devel] [PATCH v4 3/5] qcow2-refcount: check_refcounts_l2: reduce ignored overlaps Vladimir Sementsov-Ogievskiy
@ 2018-12-14 13:42 ` Vladimir Sementsov-Ogievskiy
2019-02-27 12:32 ` Max Reitz
2018-12-14 13:42 ` [Qemu-devel] [PATCH v4 5/5] qcow2-refcount: don't mask corruptions under internal errors Vladimir Sementsov-Ogievskiy
2019-01-10 15:28 ` [Qemu-devel] [PATCH v4 0/5] qcow2 check improvements part I Vladimir Sementsov-Ogievskiy
5 siblings, 1 reply; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-12-14 13:42 UTC (permalink / raw)
To: qemu-block, qemu-devel; +Cc: mreitz, kwolf, vsementsov, den
Do not count a cluster which is fixed to be ZERO as allocated.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/qcow2-refcount.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index e76085d9aa..8da0e91dd3 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1637,15 +1637,6 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
{
uint64_t offset = l2_entry & L2E_OFFSET_MASK;
- if (flags & CHECK_FRAG_INFO) {
- res->bfi.allocated_clusters++;
- if (next_contiguous_offset &&
- offset != next_contiguous_offset) {
- res->bfi.fragmented_clusters++;
- }
- next_contiguous_offset = offset + s->cluster_size;
- }
-
/* Correct offsets are cluster aligned */
if (offset_into_cluster(s, offset)) {
if (qcow2_get_cluster_type(l2_entry) ==
@@ -1698,6 +1689,15 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
}
}
+ if (flags & CHECK_FRAG_INFO) {
+ res->bfi.allocated_clusters++;
+ if (next_contiguous_offset &&
+ offset != next_contiguous_offset) {
+ res->bfi.fragmented_clusters++;
+ }
+ next_contiguous_offset = offset + s->cluster_size;
+ }
+
/* Mark cluster as used */
ret = qcow2_inc_refcounts_imrt(bs, res,
refcount_table, refcount_table_size,
--
2.18.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* [Qemu-devel] [PATCH v4 5/5] qcow2-refcount: don't mask corruptions under internal errors
2018-12-14 13:42 [Qemu-devel] [PATCH v4 0/5] qcow2 check improvements part I Vladimir Sementsov-Ogievskiy
` (3 preceding siblings ...)
2018-12-14 13:42 ` [Qemu-devel] [PATCH v4 4/5] qcow2-refcount: check_refcounts_l2: don't count fixed cluster as allocated Vladimir Sementsov-Ogievskiy
@ 2018-12-14 13:42 ` Vladimir Sementsov-Ogievskiy
2019-02-27 12:42 ` Max Reitz
2019-01-10 15:28 ` [Qemu-devel] [PATCH v4 0/5] qcow2 check improvements part I Vladimir Sementsov-Ogievskiy
5 siblings, 1 reply; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-12-14 13:42 UTC (permalink / raw)
To: qemu-block, qemu-devel; +Cc: mreitz, kwolf, vsementsov, den
No reasons for not reporting found corruptions as corruptions in case
of some internal errors, especially in case of just failed to fix l2
entry (and in this case, missed corruptions may influence comparing
logic, when we calculate difference between corruptions fields of two
results)
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/qcow2-refcount.c | 31 ++++++++++++++-----------------
1 file changed, 14 insertions(+), 17 deletions(-)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 8da0e91dd3..b0e006e628 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1639,6 +1639,8 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
/* Correct offsets are cluster aligned */
if (offset_into_cluster(s, offset)) {
+ res->corruptions++;
+
if (qcow2_get_cluster_type(l2_entry) ==
QCOW2_CLUSTER_ZERO_ALLOC)
{
@@ -1674,18 +1676,16 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
/* Do not abort, continue checking the rest of this
* L2 table's entries */
} else {
+ res->corruptions--;
res->corruptions_fixed++;
/* Skip marking the cluster as used
* (it is unused now) */
continue;
}
- } else {
- res->corruptions++;
}
} else {
fprintf(stderr, "ERROR offset=%" PRIx64 ": Data cluster is "
"not properly aligned; L2 entry corrupted.\n", offset);
- res->corruptions++;
}
}
@@ -1854,6 +1854,7 @@ static int check_oflag_copied(BlockDriverState *bs, BdrvCheckResult *res,
continue;
}
if ((refcount == 1) != ((l1_entry & QCOW_OFLAG_COPIED) != 0)) {
+ res->corruptions++;
fprintf(stderr, "%s OFLAG_COPIED L2 cluster: l1_index=%d "
"l1_entry=%" PRIx64 " refcount=%" PRIu64 "\n",
repair ? "Repairing" : "ERROR", i, l1_entry, refcount);
@@ -1866,9 +1867,8 @@ static int check_oflag_copied(BlockDriverState *bs, BdrvCheckResult *res,
res->check_errors++;
goto fail;
}
+ res->corruptions--;
res->corruptions_fixed++;
- } else {
- res->corruptions++;
}
}
@@ -1899,19 +1899,16 @@ static int check_oflag_copied(BlockDriverState *bs, BdrvCheckResult *res,
fprintf(stderr, "%s OFLAG_COPIED data cluster: "
"l2_entry=%" PRIx64 " refcount=%" PRIu64 "\n",
repair ? "Repairing" : "ERROR", l2_entry, refcount);
- if (repair) {
- l2_table[j] = cpu_to_be64(refcount == 1
- ? l2_entry | QCOW_OFLAG_COPIED
- : l2_entry & ~QCOW_OFLAG_COPIED);
- l2_dirty++;
- } else {
- res->corruptions++;
- }
+ l2_table[j] = cpu_to_be64(refcount == 1
+ ? l2_entry | QCOW_OFLAG_COPIED
+ : l2_entry & ~QCOW_OFLAG_COPIED);
+ l2_dirty++;
}
}
}
- if (l2_dirty > 0) {
+ res->corruptions += l2_dirty;
+ if (repair && l2_dirty > 0) {
ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_ACTIVE_L2,
l2_offset, s->cluster_size);
if (ret < 0) {
@@ -1929,6 +1926,7 @@ static int check_oflag_copied(BlockDriverState *bs, BdrvCheckResult *res,
res->check_errors++;
goto fail;
}
+ res->corruptions -= l2_dirty;
res->corruptions_fixed += l2_dirty;
}
}
@@ -1967,6 +1965,7 @@ static int check_refblocks(BlockDriverState *bs, BdrvCheckResult *res,
}
if (cluster >= *nb_clusters) {
+ res->corruptions++;
fprintf(stderr, "%s refcount block %" PRId64 " is outside image\n",
fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
@@ -2006,6 +2005,7 @@ static int check_refblocks(BlockDriverState *bs, BdrvCheckResult *res,
goto resize_fail;
}
+ res->corruptions--;
res->corruptions_fixed++;
ret = qcow2_inc_refcounts_imrt(bs, res,
refcount_table, nb_clusters,
@@ -2019,12 +2019,9 @@ static int check_refblocks(BlockDriverState *bs, BdrvCheckResult *res,
continue;
resize_fail:
- res->corruptions++;
*rebuild = true;
fprintf(stderr, "ERROR could not resize image: %s\n",
strerror(-ret));
- } else {
- res->corruptions++;
}
continue;
}
--
2.18.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH v4 5/5] qcow2-refcount: don't mask corruptions under internal errors
2018-12-14 13:42 ` [Qemu-devel] [PATCH v4 5/5] qcow2-refcount: don't mask corruptions under internal errors Vladimir Sementsov-Ogievskiy
@ 2019-02-27 12:42 ` Max Reitz
2019-02-27 12:47 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 15+ messages in thread
From: Max Reitz @ 2019-02-27 12:42 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block, qemu-devel; +Cc: kwolf, den
[-- Attachment #1: Type: text/plain, Size: 1866 bytes --]
On 14.12.18 14:42, Vladimir Sementsov-Ogievskiy wrote:
> No reasons for not reporting found corruptions as corruptions in case
> of some internal errors, especially in case of just failed to fix l2
> entry (and in this case, missed corruptions may influence comparing
> logic, when we calculate difference between corruptions fields of two
> results)
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> block/qcow2-refcount.c | 31 ++++++++++++++-----------------
> 1 file changed, 14 insertions(+), 17 deletions(-)
>
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 8da0e91dd3..b0e006e628 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
[...]
> @@ -1899,19 +1899,16 @@ static int check_oflag_copied(BlockDriverState *bs, BdrvCheckResult *res,
> fprintf(stderr, "%s OFLAG_COPIED data cluster: "
> "l2_entry=%" PRIx64 " refcount=%" PRIu64 "\n",
> repair ? "Repairing" : "ERROR", l2_entry, refcount);
> - if (repair) {
> - l2_table[j] = cpu_to_be64(refcount == 1
> - ? l2_entry | QCOW_OFLAG_COPIED
> - : l2_entry & ~QCOW_OFLAG_COPIED);
> - l2_dirty++;
> - } else {
> - res->corruptions++;
> - }
> + l2_table[j] = cpu_to_be64(refcount == 1
> + ? l2_entry | QCOW_OFLAG_COPIED
> + : l2_entry & ~QCOW_OFLAG_COPIED);
> + l2_dirty++;
I found the old logic to be easier to understand, actually. It made it
clear that if !repair, the L2 table is not going to be touched.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH v4 5/5] qcow2-refcount: don't mask corruptions under internal errors
2019-02-27 12:42 ` Max Reitz
@ 2019-02-27 12:47 ` Vladimir Sementsov-Ogievskiy
2019-02-27 12:48 ` Max Reitz
0 siblings, 1 reply; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-02-27 12:47 UTC (permalink / raw)
To: Max Reitz, qemu-block@nongnu.org, qemu-devel@nongnu.org
Cc: kwolf@redhat.com, Denis Lunev
27.02.2019 15:42, Max Reitz wrote:
> On 14.12.18 14:42, Vladimir Sementsov-Ogievskiy wrote:
>> No reasons for not reporting found corruptions as corruptions in case
>> of some internal errors, especially in case of just failed to fix l2
>> entry (and in this case, missed corruptions may influence comparing
>> logic, when we calculate difference between corruptions fields of two
>> results)
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>> block/qcow2-refcount.c | 31 ++++++++++++++-----------------
>> 1 file changed, 14 insertions(+), 17 deletions(-)
>>
>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>> index 8da0e91dd3..b0e006e628 100644
>> --- a/block/qcow2-refcount.c
>> +++ b/block/qcow2-refcount.c
>
> [...]
>
>> @@ -1899,19 +1899,16 @@ static int check_oflag_copied(BlockDriverState *bs, BdrvCheckResult *res,
>> fprintf(stderr, "%s OFLAG_COPIED data cluster: "
>> "l2_entry=%" PRIx64 " refcount=%" PRIu64 "\n",
>> repair ? "Repairing" : "ERROR", l2_entry, refcount);
>> - if (repair) {
>> - l2_table[j] = cpu_to_be64(refcount == 1
>> - ? l2_entry | QCOW_OFLAG_COPIED
>> - : l2_entry & ~QCOW_OFLAG_COPIED);
>> - l2_dirty++;
>> - } else {
>> - res->corruptions++;
>> - }
>> + l2_table[j] = cpu_to_be64(refcount == 1
>> + ? l2_entry | QCOW_OFLAG_COPIED
>> + : l2_entry & ~QCOW_OFLAG_COPIED);
>> + l2_dirty++;
>
> I found the old logic to be easier to understand, actually. It made it
> clear that if !repair, the L2 table is not going to be touched.
>
> Max
>
so,
if (repair) {
keep as is
}
res->corruptions++;
ok?
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH v4 5/5] qcow2-refcount: don't mask corruptions under internal errors
2019-02-27 12:47 ` Vladimir Sementsov-Ogievskiy
@ 2019-02-27 12:48 ` Max Reitz
0 siblings, 0 replies; 15+ messages in thread
From: Max Reitz @ 2019-02-27 12:48 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block@nongnu.org,
qemu-devel@nongnu.org
Cc: kwolf@redhat.com, Denis Lunev
[-- Attachment #1: Type: text/plain, Size: 2256 bytes --]
On 27.02.19 13:47, Vladimir Sementsov-Ogievskiy wrote:
> 27.02.2019 15:42, Max Reitz wrote:
>> On 14.12.18 14:42, Vladimir Sementsov-Ogievskiy wrote:
>>> No reasons for not reporting found corruptions as corruptions in case
>>> of some internal errors, especially in case of just failed to fix l2
>>> entry (and in this case, missed corruptions may influence comparing
>>> logic, when we calculate difference between corruptions fields of two
>>> results)
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>> block/qcow2-refcount.c | 31 ++++++++++++++-----------------
>>> 1 file changed, 14 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>>> index 8da0e91dd3..b0e006e628 100644
>>> --- a/block/qcow2-refcount.c
>>> +++ b/block/qcow2-refcount.c
>>
>> [...]
>>
>>> @@ -1899,19 +1899,16 @@ static int check_oflag_copied(BlockDriverState *bs, BdrvCheckResult *res,
>>> fprintf(stderr, "%s OFLAG_COPIED data cluster: "
>>> "l2_entry=%" PRIx64 " refcount=%" PRIu64 "\n",
>>> repair ? "Repairing" : "ERROR", l2_entry, refcount);
>>> - if (repair) {
>>> - l2_table[j] = cpu_to_be64(refcount == 1
>>> - ? l2_entry | QCOW_OFLAG_COPIED
>>> - : l2_entry & ~QCOW_OFLAG_COPIED);
>>> - l2_dirty++;
>>> - } else {
>>> - res->corruptions++;
>>> - }
>>> + l2_table[j] = cpu_to_be64(refcount == 1
>>> + ? l2_entry | QCOW_OFLAG_COPIED
>>> + : l2_entry & ~QCOW_OFLAG_COPIED);
>>> + l2_dirty++;
>>
>> I found the old logic to be easier to understand, actually. It made it
>> clear that if !repair, the L2 table is not going to be touched.
>>
>> Max
>>
>
> so,
>
> if (repair) {
> keep as is
> }
> res->corruptions++;
>
> ok?
Yep, that works for me. Or above the fprintf() as you did it in other
places in this patch.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v4 0/5] qcow2 check improvements part I
2018-12-14 13:42 [Qemu-devel] [PATCH v4 0/5] qcow2 check improvements part I Vladimir Sementsov-Ogievskiy
` (4 preceding siblings ...)
2018-12-14 13:42 ` [Qemu-devel] [PATCH v4 5/5] qcow2-refcount: don't mask corruptions under internal errors Vladimir Sementsov-Ogievskiy
@ 2019-01-10 15:28 ` Vladimir Sementsov-Ogievskiy
2019-02-27 10:07 ` Vladimir Sementsov-Ogievskiy
5 siblings, 1 reply; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-01-10 15:28 UTC (permalink / raw)
To: qemu-block@nongnu.org, qemu-devel@nongnu.org
Cc: mreitz@redhat.com, kwolf@redhat.com, Denis Lunev
ping
14.12.2018 16:42, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
>
> These series are derived from "[PATCH v3 0/8] qcow2 check improvements"
> (https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg02284.html)
>
> Some patches are new, some patches are not included and are postponed to
> part II.
>
> So, here:
> 01,03 - unchanged
> 02: handle broken iotest
> 04-05: new
>
> Vladimir Sementsov-Ogievskiy (5):
> qcow2-refcount: fix check_oflag_copied
> qcow2-refcount: avoid eating RAM
> qcow2-refcount: check_refcounts_l2: reduce ignored overlaps
> qcow2-refcount: check_refcounts_l2: don't count fixed cluster as
> allocated
> qcow2-refcount: don't mask corruptions under internal errors
>
> block/qcow2-refcount.c | 87 +++++++++++++++++++++++---------------
> tests/qemu-iotests/138 | 12 +++---
> tests/qemu-iotests/138.out | 5 ++-
> 3 files changed, 61 insertions(+), 43 deletions(-)
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [Qemu-devel] [PATCH v4 0/5] qcow2 check improvements part I
2019-01-10 15:28 ` [Qemu-devel] [PATCH v4 0/5] qcow2 check improvements part I Vladimir Sementsov-Ogievskiy
@ 2019-02-27 10:07 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 15+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-02-27 10:07 UTC (permalink / raw)
To: qemu-block@nongnu.org, qemu-devel@nongnu.org
Cc: mreitz@redhat.com, kwolf@redhat.com, Denis Lunev
ping
10.01.2019 18:28, Vladimir Sementsov-Ogievskiy wrote:
> ping
>
> 14.12.2018 16:42, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> These series are derived from "[PATCH v3 0/8] qcow2 check improvements"
>> (https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg02284.html)
>>
>> Some patches are new, some patches are not included and are postponed to
>> part II.
>>
>> So, here:
>> 01,03 - unchanged
>> 02: handle broken iotest
>> 04-05: new
>>
>> Vladimir Sementsov-Ogievskiy (5):
>> qcow2-refcount: fix check_oflag_copied
>> qcow2-refcount: avoid eating RAM
>> qcow2-refcount: check_refcounts_l2: reduce ignored overlaps
>> qcow2-refcount: check_refcounts_l2: don't count fixed cluster as
>> allocated
>> qcow2-refcount: don't mask corruptions under internal errors
>>
>> block/qcow2-refcount.c | 87 +++++++++++++++++++++++---------------
>> tests/qemu-iotests/138 | 12 +++---
>> tests/qemu-iotests/138.out | 5 ++-
>> 3 files changed, 61 insertions(+), 43 deletions(-)
>>
>
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 15+ messages in thread