* [PATCH v2 01/10] qcow2-refcount: improve style of check_refcounts_l2()
2021-05-05 6:59 [PATCH v2 00/10] qcow2 check: check some reserved bits and subcluster bitmaps Vladimir Sementsov-Ogievskiy
@ 2021-05-05 6:59 ` Vladimir Sementsov-Ogievskiy
2021-05-05 6:59 ` [PATCH v2 02/10] qcow2: compressed read: simplify cluster descriptor passing Vladimir Sementsov-Ogievskiy
` (8 subsequent siblings)
9 siblings, 0 replies; 16+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-05-05 6:59 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, mreitz, kwolf, vsementsov, den, ktkhai, eblake, berto
- don't use same name for size in bytes and in entries
- use g_autofree for l2_table
- add whitespace
- fix block comment style
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block/qcow2-refcount.c | 47 +++++++++++++++++++++---------------------
1 file changed, 24 insertions(+), 23 deletions(-)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 8e649b008e..2734338625 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1601,23 +1601,22 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
int flags, BdrvCheckMode fix, bool active)
{
BDRVQcow2State *s = bs->opaque;
- uint64_t *l2_table, l2_entry;
+ uint64_t l2_entry;
uint64_t next_contiguous_offset = 0;
- int i, l2_size, nb_csectors, ret;
+ int i, nb_csectors, ret;
+ size_t l2_size_bytes = s->l2_size * l2_entry_size(s);
+ g_autofree uint64_t *l2_table = g_malloc(l2_size_bytes);
/* Read L2 table from disk */
- l2_size = s->l2_size * l2_entry_size(s);
- l2_table = g_malloc(l2_size);
-
- ret = bdrv_pread(bs->file, l2_offset, l2_table, l2_size);
+ ret = bdrv_pread(bs->file, l2_offset, l2_table, l2_size_bytes);
if (ret < 0) {
fprintf(stderr, "ERROR: I/O error in check_refcounts_l2\n");
res->check_errors++;
- goto fail;
+ return ret;
}
/* Do the actual checks */
- for(i = 0; i < s->l2_size; i++) {
+ for (i = 0; i < s->l2_size; i++) {
l2_entry = get_l2_entry(s, l2_table, i);
switch (qcow2_get_cluster_type(bs, l2_entry)) {
@@ -1647,14 +1646,15 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
l2_entry & QCOW2_COMPRESSED_SECTOR_MASK,
nb_csectors * QCOW2_COMPRESSED_SECTOR_SIZE);
if (ret < 0) {
- goto fail;
+ return ret;
}
if (flags & CHECK_FRAG_INFO) {
res->bfi.allocated_clusters++;
res->bfi.compressed_clusters++;
- /* Compressed clusters are fragmented by nature. Since they
+ /*
+ * Compressed clusters are fragmented by nature. Since they
* take up sub-sector space but we only have sector granularity
* I/O we need to re-read the same sectors even for adjacent
* compressed clusters.
@@ -1700,9 +1700,11 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
if (ret < 0) {
fprintf(stderr, "ERROR: Overlap check failed\n");
res->check_errors++;
- /* Something is seriously wrong, so abort checking
- * this L2 table */
- goto fail;
+ /*
+ * Something is seriously wrong, so abort checking
+ * this L2 table.
+ */
+ return ret;
}
ret = bdrv_pwrite_sync(bs->file, l2e_offset,
@@ -1712,13 +1714,17 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
fprintf(stderr, "ERROR: Failed to overwrite L2 "
"table entry: %s\n", strerror(-ret));
res->check_errors++;
- /* Do not abort, continue checking the rest of this
- * L2 table's entries */
+ /*
+ * 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) */
+ /*
+ * Skip marking the cluster as used
+ * (it is unused now).
+ */
continue;
}
}
@@ -1743,7 +1749,7 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
refcount_table_size,
offset, s->cluster_size);
if (ret < 0) {
- goto fail;
+ return ret;
}
}
break;
@@ -1758,12 +1764,7 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
}
}
- g_free(l2_table);
return 0;
-
-fail:
- g_free(l2_table);
- return ret;
}
/*
--
2.29.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v2 02/10] qcow2: compressed read: simplify cluster descriptor passing
2021-05-05 6:59 [PATCH v2 00/10] qcow2 check: check some reserved bits and subcluster bitmaps Vladimir Sementsov-Ogievskiy
2021-05-05 6:59 ` [PATCH v2 01/10] qcow2-refcount: improve style of check_refcounts_l2() Vladimir Sementsov-Ogievskiy
@ 2021-05-05 6:59 ` Vladimir Sementsov-Ogievskiy
2021-05-10 15:36 ` Alberto Garcia
2021-05-05 6:59 ` [PATCH v2 03/10] qcow2: introduce qcow2_parse_compressed_l2_entry() helper Vladimir Sementsov-Ogievskiy
` (7 subsequent siblings)
9 siblings, 1 reply; 16+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-05-05 6:59 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, mreitz, kwolf, vsementsov, den, ktkhai, eblake, berto
Let's pass the whole L2 entry and not bother with
L2E_COMPRESSED_OFFSET_SIZE_MASK.
It also helps further refactoring that adds generic
qcow2_parse_compressed_l2_entry() helper.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block/qcow2.h | 1 -
block/qcow2-cluster.c | 5 ++---
block/qcow2.c | 12 +++++++-----
3 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index 0fe5f74ed3..42a0058ab7 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -588,7 +588,6 @@ typedef enum QCow2MetadataOverlap {
#define L1E_OFFSET_MASK 0x00fffffffffffe00ULL
#define L2E_OFFSET_MASK 0x00fffffffffffe00ULL
-#define L2E_COMPRESSED_OFFSET_SIZE_MASK 0x3fffffffffffffffULL
#define REFT_OFFSET_MASK 0xfffffffffffffe00ULL
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index bd0597842f..04735ee439 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -556,8 +556,7 @@ static int coroutine_fn do_perform_cow_write(BlockDriverState *bs,
* offset needs to be aligned to a cluster boundary.
*
* If the cluster is unallocated then *host_offset will be 0.
- * If the cluster is compressed then *host_offset will contain the
- * complete compressed cluster descriptor.
+ * If the cluster is compressed then *host_offset will contain the l2 entry.
*
* On entry, *bytes is the maximum number of contiguous bytes starting at
* offset that we are interested in.
@@ -660,7 +659,7 @@ int qcow2_get_host_offset(BlockDriverState *bs, uint64_t offset,
ret = -EIO;
goto fail;
}
- *host_offset = l2_entry & L2E_COMPRESSED_OFFSET_SIZE_MASK;
+ *host_offset = l2_entry;
break;
case QCOW2_SUBCLUSTER_ZERO_PLAIN:
case QCOW2_SUBCLUSTER_UNALLOCATED_PLAIN:
diff --git a/block/qcow2.c b/block/qcow2.c
index 9727ae8fe3..746ae85b89 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -74,7 +74,7 @@ typedef struct {
static int coroutine_fn
qcow2_co_preadv_compressed(BlockDriverState *bs,
- uint64_t cluster_descriptor,
+ uint64_t l2_entry,
uint64_t offset,
uint64_t bytes,
QEMUIOVector *qiov,
@@ -2177,7 +2177,7 @@ typedef struct Qcow2AioTask {
BlockDriverState *bs;
QCow2SubclusterType subcluster_type; /* only for read */
- uint64_t host_offset; /* or full descriptor in compressed clusters */
+ uint64_t host_offset; /* or l2_entry for compressed read */
uint64_t offset;
uint64_t bytes;
QEMUIOVector *qiov;
@@ -4665,7 +4665,7 @@ qcow2_co_pwritev_compressed_part(BlockDriverState *bs,
static int coroutine_fn
qcow2_co_preadv_compressed(BlockDriverState *bs,
- uint64_t cluster_descriptor,
+ uint64_t l2_entry,
uint64_t offset,
uint64_t bytes,
QEMUIOVector *qiov,
@@ -4677,8 +4677,10 @@ qcow2_co_preadv_compressed(BlockDriverState *bs,
uint8_t *buf, *out_buf;
int offset_in_cluster = offset_into_cluster(s, offset);
- coffset = cluster_descriptor & s->cluster_offset_mask;
- nb_csectors = ((cluster_descriptor >> s->csize_shift) & s->csize_mask) + 1;
+ assert(qcow2_get_cluster_type(bs, l2_entry) == QCOW2_CLUSTER_COMPRESSED);
+
+ coffset = l2_entry & s->cluster_offset_mask;
+ nb_csectors = ((l2_entry >> s->csize_shift) & s->csize_mask) + 1;
csize = nb_csectors * QCOW2_COMPRESSED_SECTOR_SIZE -
(coffset & ~QCOW2_COMPRESSED_SECTOR_MASK);
--
2.29.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v2 02/10] qcow2: compressed read: simplify cluster descriptor passing
2021-05-05 6:59 ` [PATCH v2 02/10] qcow2: compressed read: simplify cluster descriptor passing Vladimir Sementsov-Ogievskiy
@ 2021-05-10 15:36 ` Alberto Garcia
0 siblings, 0 replies; 16+ messages in thread
From: Alberto Garcia @ 2021-05-10 15:36 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, vsementsov, qemu-devel, mreitz, ktkhai, den
On Wed 05 May 2021 08:59:47 AM CEST, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> Let's pass the whole L2 entry and not bother with
> L2E_COMPRESSED_OFFSET_SIZE_MASK.
>
> It also helps further refactoring that adds generic
> qcow2_parse_compressed_l2_entry() helper.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 03/10] qcow2: introduce qcow2_parse_compressed_l2_entry() helper
2021-05-05 6:59 [PATCH v2 00/10] qcow2 check: check some reserved bits and subcluster bitmaps Vladimir Sementsov-Ogievskiy
2021-05-05 6:59 ` [PATCH v2 01/10] qcow2-refcount: improve style of check_refcounts_l2() Vladimir Sementsov-Ogievskiy
2021-05-05 6:59 ` [PATCH v2 02/10] qcow2: compressed read: simplify cluster descriptor passing Vladimir Sementsov-Ogievskiy
@ 2021-05-05 6:59 ` Vladimir Sementsov-Ogievskiy
2021-05-05 6:59 ` [PATCH v2 04/10] qcow2-refcount: introduce fix_l2_entry_by_zero() Vladimir Sementsov-Ogievskiy
` (6 subsequent siblings)
9 siblings, 0 replies; 16+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-05-05 6:59 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, mreitz, kwolf, vsementsov, den, ktkhai, eblake, berto
Add helper to parse compressed l2_entry and use it everywhere instead
of open-coding.
Note, that in most places we move to precise coffset/csize instead of
sector-aligned. Still it should work good enough for updating
refcounts.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block/qcow2.h | 3 ++-
block/qcow2-cluster.c | 15 +++++++++++++++
block/qcow2-refcount.c | 36 +++++++++++++++++-------------------
block/qcow2.c | 9 ++-------
4 files changed, 36 insertions(+), 27 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index 42a0058ab7..c0e1e83796 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -110,7 +110,6 @@
/* Defined in the qcow2 spec (compressed cluster descriptor) */
#define QCOW2_COMPRESSED_SECTOR_SIZE 512U
-#define QCOW2_COMPRESSED_SECTOR_MASK (~(QCOW2_COMPRESSED_SECTOR_SIZE - 1ULL))
/* Must be at least 2 to cover COW */
#define MIN_L2_CACHE_SIZE 2 /* cache entries */
@@ -913,6 +912,8 @@ int qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
uint64_t offset,
int compressed_size,
uint64_t *host_offset);
+void qcow2_parse_compressed_l2_entry(BlockDriverState *bs, uint64_t l2_entry,
+ uint64_t *coffset, int *csize);
int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m);
void qcow2_alloc_cluster_abort(BlockDriverState *bs, QCowL2Meta *m);
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 04735ee439..70d0570a33 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -2462,3 +2462,18 @@ fail:
g_free(l1_table);
return ret;
}
+
+void qcow2_parse_compressed_l2_entry(BlockDriverState *bs, uint64_t l2_entry,
+ uint64_t *coffset, int *csize)
+{
+ BDRVQcow2State *s = bs->opaque;
+ int nb_csectors;
+
+ assert(qcow2_get_cluster_type(bs, l2_entry) == QCOW2_CLUSTER_COMPRESSED);
+
+ *coffset = l2_entry & s->cluster_offset_mask;
+
+ nb_csectors = ((l2_entry >> s->csize_shift) & s->csize_mask) + 1;
+ *csize = nb_csectors * QCOW2_COMPRESSED_SECTOR_SIZE -
+ (*coffset & (QCOW2_COMPRESSED_SECTOR_SIZE - 1));
+}
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 2734338625..66cbb94ef9 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1177,11 +1177,11 @@ void qcow2_free_any_cluster(BlockDriverState *bs, uint64_t l2_entry,
switch (ctype) {
case QCOW2_CLUSTER_COMPRESSED:
{
- int64_t offset = (l2_entry & s->cluster_offset_mask)
- & QCOW2_COMPRESSED_SECTOR_MASK;
- int size = QCOW2_COMPRESSED_SECTOR_SIZE *
- (((l2_entry >> s->csize_shift) & s->csize_mask) + 1);
- qcow2_free_clusters(bs, offset, size, type);
+ uint64_t coffset;
+ int csize;
+
+ qcow2_parse_compressed_l2_entry(bs, l2_entry, &coffset, &csize);
+ qcow2_free_clusters(bs, coffset, csize, type);
}
break;
case QCOW2_CLUSTER_NORMAL:
@@ -1247,7 +1247,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
bool l1_allocated = false;
int64_t old_entry, old_l2_offset;
unsigned slice, slice_size2, n_slices;
- int i, j, l1_modified = 0, nb_csectors;
+ int i, j, l1_modified = 0;
int ret;
assert(addend >= -1 && addend <= 1);
@@ -1318,14 +1318,14 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
switch (qcow2_get_cluster_type(bs, entry)) {
case QCOW2_CLUSTER_COMPRESSED:
- nb_csectors = ((entry >> s->csize_shift) &
- s->csize_mask) + 1;
if (addend != 0) {
- uint64_t coffset = (entry & s->cluster_offset_mask)
- & QCOW2_COMPRESSED_SECTOR_MASK;
+ uint64_t coffset;
+ int csize;
+
+ qcow2_parse_compressed_l2_entry(bs, entry,
+ &coffset, &csize);
ret = update_refcount(
- bs, coffset,
- nb_csectors * QCOW2_COMPRESSED_SECTOR_SIZE,
+ bs, coffset, csize,
abs(addend), addend < 0,
QCOW2_DISCARD_SNAPSHOT);
if (ret < 0) {
@@ -1603,7 +1603,7 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
BDRVQcow2State *s = bs->opaque;
uint64_t l2_entry;
uint64_t next_contiguous_offset = 0;
- int i, nb_csectors, ret;
+ int i, ret;
size_t l2_size_bytes = s->l2_size * l2_entry_size(s);
g_autofree uint64_t *l2_table = g_malloc(l2_size_bytes);
@@ -1617,6 +1617,8 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
/* Do the actual checks */
for (i = 0; i < s->l2_size; i++) {
+ uint64_t coffset;
+ int csize;
l2_entry = get_l2_entry(s, l2_table, i);
switch (qcow2_get_cluster_type(bs, l2_entry)) {
@@ -1638,13 +1640,9 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
}
/* Mark cluster as used */
- nb_csectors = ((l2_entry >> s->csize_shift) &
- s->csize_mask) + 1;
- l2_entry &= s->cluster_offset_mask;
+ qcow2_parse_compressed_l2_entry(bs, l2_entry, &coffset, &csize);
ret = qcow2_inc_refcounts_imrt(
- bs, res, refcount_table, refcount_table_size,
- l2_entry & QCOW2_COMPRESSED_SECTOR_MASK,
- nb_csectors * QCOW2_COMPRESSED_SECTOR_SIZE);
+ bs, res, refcount_table, refcount_table_size, coffset, csize);
if (ret < 0) {
return ret;
}
diff --git a/block/qcow2.c b/block/qcow2.c
index 746ae85b89..418779a43b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4672,17 +4672,12 @@ qcow2_co_preadv_compressed(BlockDriverState *bs,
size_t qiov_offset)
{
BDRVQcow2State *s = bs->opaque;
- int ret = 0, csize, nb_csectors;
+ int ret = 0, csize;
uint64_t coffset;
uint8_t *buf, *out_buf;
int offset_in_cluster = offset_into_cluster(s, offset);
- assert(qcow2_get_cluster_type(bs, l2_entry) == QCOW2_CLUSTER_COMPRESSED);
-
- coffset = l2_entry & s->cluster_offset_mask;
- nb_csectors = ((l2_entry >> s->csize_shift) & s->csize_mask) + 1;
- csize = nb_csectors * QCOW2_COMPRESSED_SECTOR_SIZE -
- (coffset & ~QCOW2_COMPRESSED_SECTOR_MASK);
+ qcow2_parse_compressed_l2_entry(bs, l2_entry, &coffset, &csize);
buf = g_try_malloc(csize);
if (!buf) {
--
2.29.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v2 04/10] qcow2-refcount: introduce fix_l2_entry_by_zero()
2021-05-05 6:59 [PATCH v2 00/10] qcow2 check: check some reserved bits and subcluster bitmaps Vladimir Sementsov-Ogievskiy
` (2 preceding siblings ...)
2021-05-05 6:59 ` [PATCH v2 03/10] qcow2: introduce qcow2_parse_compressed_l2_entry() helper Vladimir Sementsov-Ogievskiy
@ 2021-05-05 6:59 ` Vladimir Sementsov-Ogievskiy
2021-05-05 6:59 ` [PATCH v2 05/10] qcow2-refcount: fix_l2_entry_by_zero(): also zero L2 entry bitmap Vladimir Sementsov-Ogievskiy
` (5 subsequent siblings)
9 siblings, 0 replies; 16+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-05-05 6:59 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, mreitz, kwolf, vsementsov, den, ktkhai, eblake, berto
Split fix_l2_entry_by_zero() out of check_refcounts_l2() to be
reused in further patch.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block/qcow2-refcount.c | 87 +++++++++++++++++++++++++++++-------------
1 file changed, 60 insertions(+), 27 deletions(-)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 66cbb94ef9..184b96ad63 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1587,6 +1587,54 @@ enum {
CHECK_FRAG_INFO = 0x2, /* update BlockFragInfo counters */
};
+/*
+ * Fix L2 entry by making it QCOW2_CLUSTER_ZERO_PLAIN.
+ *
+ * This function decrements res->corruptions on success, so the caller is
+ * responsible to increment res->corruptions prior to the call.
+ *
+ * On failure in-memory @l2_table may be modified.
+ */
+static int fix_l2_entry_by_zero(BlockDriverState *bs, BdrvCheckResult *res,
+ uint64_t l2_offset,
+ uint64_t *l2_table, int l2_index, bool active,
+ bool *metadata_overlap)
+{
+ BDRVQcow2State *s = bs->opaque;
+ int ret;
+ int idx = l2_index * (l2_entry_size(s) / sizeof(uint64_t));
+ uint64_t l2e_offset = l2_offset + (uint64_t)l2_index * l2_entry_size(s);
+ int ign = active ? QCOW2_OL_ACTIVE_L2 : QCOW2_OL_INACTIVE_L2;
+ uint64_t l2_entry = has_subclusters(s) ? 0 : QCOW_OFLAG_ZERO;
+
+ set_l2_entry(s, l2_table, l2_index, l2_entry);
+ ret = qcow2_pre_write_overlap_check(bs, ign, l2e_offset, l2_entry_size(s),
+ false);
+ if (metadata_overlap) {
+ *metadata_overlap = ret < 0;
+ }
+ if (ret < 0) {
+ fprintf(stderr, "ERROR: Overlap check failed\n");
+ goto fail;
+ }
+
+ ret = bdrv_pwrite_sync(bs->file, l2e_offset, &l2_table[idx],
+ l2_entry_size(s));
+ if (ret < 0) {
+ fprintf(stderr, "ERROR: Failed to overwrite L2 "
+ "table entry: %s\n", strerror(-ret));
+ goto fail;
+ }
+
+ res->corruptions--;
+ res->corruptions_fixed++;
+ return 0;
+
+fail:
+ res->check_errors++;
+ return ret;
+}
+
/*
* Increases the refcount in the given refcount table for the all clusters
* referenced in the L2 table. While doing so, performs some checks on L2
@@ -1606,6 +1654,7 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
int i, ret;
size_t l2_size_bytes = s->l2_size * l2_entry_size(s);
g_autofree uint64_t *l2_table = g_malloc(l2_size_bytes);
+ bool metadata_overlap;
/* Read L2 table from disk */
ret = bdrv_pread(bs->file, l2_offset, l2_table, l2_size_bytes);
@@ -1685,19 +1734,10 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR",
offset);
if (fix & BDRV_FIX_ERRORS) {
- int idx = i * (l2_entry_size(s) / sizeof(uint64_t));
- uint64_t l2e_offset =
- l2_offset + (uint64_t)i * l2_entry_size(s);
- int ign = active ? QCOW2_OL_ACTIVE_L2 :
- QCOW2_OL_INACTIVE_L2;
-
- l2_entry = has_subclusters(s) ? 0 : QCOW_OFLAG_ZERO;
- set_l2_entry(s, l2_table, i, l2_entry);
- ret = qcow2_pre_write_overlap_check(bs, ign,
- l2e_offset, l2_entry_size(s), false);
- if (ret < 0) {
- fprintf(stderr, "ERROR: Overlap check failed\n");
- res->check_errors++;
+ ret = fix_l2_entry_by_zero(bs, res, l2_offset,
+ l2_table, i, active,
+ &metadata_overlap);
+ if (metadata_overlap) {
/*
* Something is seriously wrong, so abort checking
* this L2 table.
@@ -1705,26 +1745,19 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
return ret;
}
- ret = bdrv_pwrite_sync(bs->file, l2e_offset,
- &l2_table[idx],
- l2_entry_size(s));
- if (ret < 0) {
- fprintf(stderr, "ERROR: Failed to overwrite L2 "
- "table entry: %s\n", strerror(-ret));
- res->check_errors++;
- /*
- * Do not abort, continue checking the rest of this
- * L2 table's entries.
- */
- } else {
- res->corruptions--;
- res->corruptions_fixed++;
+ if (ret == 0) {
/*
* Skip marking the cluster as used
* (it is unused now).
*/
continue;
}
+
+ /*
+ * Failed to fix.
+ * Do not abort, continue checking the rest of this
+ * L2 table's entries.
+ */
}
} else {
fprintf(stderr, "ERROR offset=%" PRIx64 ": Data cluster is "
--
2.29.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v2 05/10] qcow2-refcount: fix_l2_entry_by_zero(): also zero L2 entry bitmap
2021-05-05 6:59 [PATCH v2 00/10] qcow2 check: check some reserved bits and subcluster bitmaps Vladimir Sementsov-Ogievskiy
` (3 preceding siblings ...)
2021-05-05 6:59 ` [PATCH v2 04/10] qcow2-refcount: introduce fix_l2_entry_by_zero() Vladimir Sementsov-Ogievskiy
@ 2021-05-05 6:59 ` Vladimir Sementsov-Ogievskiy
2021-05-05 6:59 ` [PATCH v2 06/10] qcow2-refcount: check_refcounts_l2(): check l2_bitmap Vladimir Sementsov-Ogievskiy
` (4 subsequent siblings)
9 siblings, 0 replies; 16+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-05-05 6:59 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, mreitz, kwolf, vsementsov, den, ktkhai, eblake, berto
We'll reuse the function to fix wrong L2 entry bitmap. Support it now.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block/qcow2-refcount.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 184b96ad63..f48c5e1b5d 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1588,7 +1588,8 @@ enum {
};
/*
- * Fix L2 entry by making it QCOW2_CLUSTER_ZERO_PLAIN.
+ * Fix L2 entry by making it QCOW2_CLUSTER_ZERO_PLAIN (or making all its present
+ * subclusters QCOW2_SUBCLUSTER_ZERO_PLAIN).
*
* This function decrements res->corruptions on success, so the caller is
* responsible to increment res->corruptions prior to the call.
@@ -1605,9 +1606,20 @@ static int fix_l2_entry_by_zero(BlockDriverState *bs, BdrvCheckResult *res,
int idx = l2_index * (l2_entry_size(s) / sizeof(uint64_t));
uint64_t l2e_offset = l2_offset + (uint64_t)l2_index * l2_entry_size(s);
int ign = active ? QCOW2_OL_ACTIVE_L2 : QCOW2_OL_INACTIVE_L2;
- uint64_t l2_entry = has_subclusters(s) ? 0 : QCOW_OFLAG_ZERO;
- set_l2_entry(s, l2_table, l2_index, l2_entry);
+ if (has_subclusters(s)) {
+ uint64_t l2_bitmap = get_l2_bitmap(s, l2_table, l2_index);
+
+ /* Allocated subclusters become zero */
+ l2_bitmap |= l2_bitmap << 32;
+ l2_bitmap &= QCOW_L2_BITMAP_ALL_ZEROES;
+
+ set_l2_bitmap(s, l2_table, l2_index, l2_bitmap);
+ set_l2_entry(s, l2_table, l2_index, 0);
+ } else {
+ set_l2_entry(s, l2_table, l2_index, QCOW_OFLAG_ZERO);
+ }
+
ret = qcow2_pre_write_overlap_check(bs, ign, l2e_offset, l2_entry_size(s),
false);
if (metadata_overlap) {
--
2.29.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v2 06/10] qcow2-refcount: check_refcounts_l2(): check l2_bitmap
2021-05-05 6:59 [PATCH v2 00/10] qcow2 check: check some reserved bits and subcluster bitmaps Vladimir Sementsov-Ogievskiy
` (4 preceding siblings ...)
2021-05-05 6:59 ` [PATCH v2 05/10] qcow2-refcount: fix_l2_entry_by_zero(): also zero L2 entry bitmap Vladimir Sementsov-Ogievskiy
@ 2021-05-05 6:59 ` Vladimir Sementsov-Ogievskiy
2021-05-11 16:49 ` Kirill Tkhai
2021-05-05 6:59 ` [PATCH v2 07/10] qcow2-refcount: check_refcounts_l2(): check reserved bits Vladimir Sementsov-Ogievskiy
` (3 subsequent siblings)
9 siblings, 1 reply; 16+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-05-05 6:59 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, mreitz, kwolf, vsementsov, den, ktkhai, eblake, berto
Check subcluster bitmap of the l2 entry for different types of
clusters:
- for compressed it must be zero
- for allocated check consistency of two parts of the bitmap
- for unallocated all subclusters should be unallocated
(or zero-plain)
For unallocated clusters we can safely fix the entry by making it
zero-plain.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block/qcow2-refcount.c | 30 +++++++++++++++++++++++++++++-
1 file changed, 29 insertions(+), 1 deletion(-)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index f48c5e1b5d..062ec48a15 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1681,6 +1681,7 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
uint64_t coffset;
int csize;
l2_entry = get_l2_entry(s, l2_table, i);
+ uint64_t l2_bitmap = get_l2_bitmap(s, l2_table, i);
switch (qcow2_get_cluster_type(bs, l2_entry)) {
case QCOW2_CLUSTER_COMPRESSED:
@@ -1700,6 +1701,14 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
break;
}
+ if (l2_bitmap) {
+ fprintf(stderr, "ERROR compressed cluster %d with non-zero "
+ "subcluster allocation bitmap, entry=0x%" PRIx64 "\n",
+ i, l2_entry);
+ res->corruptions++;
+ break;
+ }
+
/* Mark cluster as used */
qcow2_parse_compressed_l2_entry(bs, l2_entry, &coffset, &csize);
ret = qcow2_inc_refcounts_imrt(
@@ -1727,13 +1736,19 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
{
uint64_t offset = l2_entry & L2E_OFFSET_MASK;
+ if ((l2_bitmap >> 32) & l2_bitmap) {
+ res->corruptions++;
+ fprintf(stderr, "ERROR offset=%" PRIx64 ": Allocated "
+ "cluster has corrupted subcluster allocation bitmap\n",
+ offset);
+ }
+
/* Correct offsets are cluster aligned */
if (offset_into_cluster(s, offset)) {
bool contains_data;
res->corruptions++;
if (has_subclusters(s)) {
- uint64_t l2_bitmap = get_l2_bitmap(s, l2_table, i);
contains_data = (l2_bitmap & QCOW_L2_BITMAP_ALL_ALLOC);
} else {
contains_data = !(l2_entry & QCOW_OFLAG_ZERO);
@@ -1800,6 +1815,19 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
case QCOW2_CLUSTER_ZERO_PLAIN:
case QCOW2_CLUSTER_UNALLOCATED:
+ if (l2_bitmap & QCOW_L2_BITMAP_ALL_ALLOC) {
+ res->corruptions++;
+ fprintf(stderr, "%s: Unallocated "
+ "cluster has non-zero subcluster allocation map\n",
+ fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR");
+ if (fix & BDRV_FIX_ERRORS) {
+ ret = fix_l2_entry_by_zero(bs, res, l2_offset, l2_table, i,
+ active, &metadata_overlap);
+ if (metadata_overlap) {
+ return ret;
+ }
+ }
+ }
break;
default:
--
2.29.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v2 06/10] qcow2-refcount: check_refcounts_l2(): check l2_bitmap
2021-05-05 6:59 ` [PATCH v2 06/10] qcow2-refcount: check_refcounts_l2(): check l2_bitmap Vladimir Sementsov-Ogievskiy
@ 2021-05-11 16:49 ` Kirill Tkhai
0 siblings, 0 replies; 16+ messages in thread
From: Kirill Tkhai @ 2021-05-11 16:49 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: qemu-devel, mreitz, kwolf, den, eblake, berto
On 05.05.2021 09:59, Vladimir Sementsov-Ogievskiy wrote:
> Check subcluster bitmap of the l2 entry for different types of
> clusters:
>
> - for compressed it must be zero
> - for allocated check consistency of two parts of the bitmap
> - for unallocated all subclusters should be unallocated
> (or zero-plain)
>
> For unallocated clusters we can safely fix the entry by making it
> zero-plain.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
Tested-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> ---
> block/qcow2-refcount.c | 30 +++++++++++++++++++++++++++++-
> 1 file changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index f48c5e1b5d..062ec48a15 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -1681,6 +1681,7 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
> uint64_t coffset;
> int csize;
> l2_entry = get_l2_entry(s, l2_table, i);
> + uint64_t l2_bitmap = get_l2_bitmap(s, l2_table, i);
>
> switch (qcow2_get_cluster_type(bs, l2_entry)) {
> case QCOW2_CLUSTER_COMPRESSED:
> @@ -1700,6 +1701,14 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
> break;
> }
>
> + if (l2_bitmap) {
> + fprintf(stderr, "ERROR compressed cluster %d with non-zero "
> + "subcluster allocation bitmap, entry=0x%" PRIx64 "\n",
> + i, l2_entry);
> + res->corruptions++;
> + break;
> + }
> +
> /* Mark cluster as used */
> qcow2_parse_compressed_l2_entry(bs, l2_entry, &coffset, &csize);
> ret = qcow2_inc_refcounts_imrt(
> @@ -1727,13 +1736,19 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
> {
> uint64_t offset = l2_entry & L2E_OFFSET_MASK;
>
> + if ((l2_bitmap >> 32) & l2_bitmap) {
> + res->corruptions++;
> + fprintf(stderr, "ERROR offset=%" PRIx64 ": Allocated "
> + "cluster has corrupted subcluster allocation bitmap\n",
> + offset);
> + }
> +
> /* Correct offsets are cluster aligned */
> if (offset_into_cluster(s, offset)) {
> bool contains_data;
> res->corruptions++;
>
> if (has_subclusters(s)) {
> - uint64_t l2_bitmap = get_l2_bitmap(s, l2_table, i);
> contains_data = (l2_bitmap & QCOW_L2_BITMAP_ALL_ALLOC);
> } else {
> contains_data = !(l2_entry & QCOW_OFLAG_ZERO);
> @@ -1800,6 +1815,19 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
>
> case QCOW2_CLUSTER_ZERO_PLAIN:
> case QCOW2_CLUSTER_UNALLOCATED:
> + if (l2_bitmap & QCOW_L2_BITMAP_ALL_ALLOC) {
> + res->corruptions++;
> + fprintf(stderr, "%s: Unallocated "
> + "cluster has non-zero subcluster allocation map\n",
> + fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR");
> + if (fix & BDRV_FIX_ERRORS) {
> + ret = fix_l2_entry_by_zero(bs, res, l2_offset, l2_table, i,
> + active, &metadata_overlap);
> + if (metadata_overlap) {
> + return ret;
> + }
> + }
> + }
> break;
>
> default:
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 07/10] qcow2-refcount: check_refcounts_l2(): check reserved bits
2021-05-05 6:59 [PATCH v2 00/10] qcow2 check: check some reserved bits and subcluster bitmaps Vladimir Sementsov-Ogievskiy
` (5 preceding siblings ...)
2021-05-05 6:59 ` [PATCH v2 06/10] qcow2-refcount: check_refcounts_l2(): check l2_bitmap Vladimir Sementsov-Ogievskiy
@ 2021-05-05 6:59 ` Vladimir Sementsov-Ogievskiy
2021-05-11 16:54 ` Kirill Tkhai
2021-05-05 6:59 ` [PATCH v2 08/10] qcow2-refcount: improve style of check_refcounts_l1() Vladimir Sementsov-Ogievskiy
` (2 subsequent siblings)
9 siblings, 1 reply; 16+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-05-05 6:59 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, mreitz, kwolf, vsementsov, den, ktkhai, eblake, berto
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block/qcow2.h | 1 +
block/qcow2-refcount.c | 12 +++++++++++-
2 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index c0e1e83796..b8b1093b61 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -587,6 +587,7 @@ typedef enum QCow2MetadataOverlap {
#define L1E_OFFSET_MASK 0x00fffffffffffe00ULL
#define L2E_OFFSET_MASK 0x00fffffffffffe00ULL
+#define L2E_STD_RESERVED_MASK 0x3f000000000001feULL
#define REFT_OFFSET_MASK 0xfffffffffffffe00ULL
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 062ec48a15..47cc82449b 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1682,8 +1682,18 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
int csize;
l2_entry = get_l2_entry(s, l2_table, i);
uint64_t l2_bitmap = get_l2_bitmap(s, l2_table, i);
+ QCow2ClusterType type = qcow2_get_cluster_type(bs, l2_entry);
- switch (qcow2_get_cluster_type(bs, l2_entry)) {
+ if (type != QCOW2_CLUSTER_COMPRESSED) {
+ /* Check reserved bits of Standard Cluster Descriptor */
+ if (l2_entry & L2E_STD_RESERVED_MASK) {
+ fprintf(stderr, "ERROR found l2 entry with reserved bits set: "
+ "%" PRIx64, l2_entry);
+ res->corruptions++;
+ }
+ }
+
+ switch (type) {
case QCOW2_CLUSTER_COMPRESSED:
/* Compressed clusters don't have QCOW_OFLAG_COPIED */
if (l2_entry & QCOW_OFLAG_COPIED) {
--
2.29.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v2 07/10] qcow2-refcount: check_refcounts_l2(): check reserved bits
2021-05-05 6:59 ` [PATCH v2 07/10] qcow2-refcount: check_refcounts_l2(): check reserved bits Vladimir Sementsov-Ogievskiy
@ 2021-05-11 16:54 ` Kirill Tkhai
0 siblings, 0 replies; 16+ messages in thread
From: Kirill Tkhai @ 2021-05-11 16:54 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: qemu-devel, mreitz, kwolf, den, eblake, berto
On 05.05.2021 09:59, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
> block/qcow2.h | 1 +
> block/qcow2-refcount.c | 12 +++++++++++-
> 2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/block/qcow2.h b/block/qcow2.h
> index c0e1e83796..b8b1093b61 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -587,6 +587,7 @@ typedef enum QCow2MetadataOverlap {
>
> #define L1E_OFFSET_MASK 0x00fffffffffffe00ULL
> #define L2E_OFFSET_MASK 0x00fffffffffffe00ULL
> +#define L2E_STD_RESERVED_MASK 0x3f000000000001feULL
>
> #define REFT_OFFSET_MASK 0xfffffffffffffe00ULL
>
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 062ec48a15..47cc82449b 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -1682,8 +1682,18 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
> int csize;
> l2_entry = get_l2_entry(s, l2_table, i);
> uint64_t l2_bitmap = get_l2_bitmap(s, l2_table, i);
> + QCow2ClusterType type = qcow2_get_cluster_type(bs, l2_entry);
>
> - switch (qcow2_get_cluster_type(bs, l2_entry)) {
> + if (type != QCOW2_CLUSTER_COMPRESSED) {
> + /* Check reserved bits of Standard Cluster Descriptor */
> + if (l2_entry & L2E_STD_RESERVED_MASK) {
> + fprintf(stderr, "ERROR found l2 entry with reserved bits set: "
> + "%" PRIx64, l2_entry);
'\n' is missed.
The rest is OK for me.
Tested-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> + res->corruptions++;
> + }
> + }
> +
> + switch (type) {
> case QCOW2_CLUSTER_COMPRESSED:
> /* Compressed clusters don't have QCOW_OFLAG_COPIED */
> if (l2_entry & QCOW_OFLAG_COPIED) {
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 08/10] qcow2-refcount: improve style of check_refcounts_l1()
2021-05-05 6:59 [PATCH v2 00/10] qcow2 check: check some reserved bits and subcluster bitmaps Vladimir Sementsov-Ogievskiy
` (6 preceding siblings ...)
2021-05-05 6:59 ` [PATCH v2 07/10] qcow2-refcount: check_refcounts_l2(): check reserved bits Vladimir Sementsov-Ogievskiy
@ 2021-05-05 6:59 ` Vladimir Sementsov-Ogievskiy
2021-05-05 6:59 ` [PATCH v2 09/10] qcow2-refcount: check_refcounts_l1(): check reserved bits Vladimir Sementsov-Ogievskiy
2021-05-05 6:59 ` [PATCH v2 10/10] qcow2-refcount: check_refblocks(): add separate message for reserved Vladimir Sementsov-Ogievskiy
9 siblings, 0 replies; 16+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-05-05 6:59 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, mreitz, kwolf, vsementsov, den, ktkhai, eblake, berto
- use g_autofree for l1_table
- better name for size in bytes variable
- reduce code blocks nesting
- whitespaces, braces, newlines
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/qcow2-refcount.c | 98 +++++++++++++++++++++---------------------
1 file changed, 50 insertions(+), 48 deletions(-)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 47cc82449b..69294a94fe 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1864,71 +1864,73 @@ static int check_refcounts_l1(BlockDriverState *bs,
int flags, BdrvCheckMode fix, bool active)
{
BDRVQcow2State *s = bs->opaque;
- uint64_t *l1_table = NULL, l2_offset, l1_size2;
+ size_t l1_size_bytes = l1_size * L1E_SIZE;
+ g_autofree uint64_t *l1_table = NULL;
+ uint64_t l2_offset;
int i, ret;
- l1_size2 = l1_size * L1E_SIZE;
+ if (!l1_size) {
+ return 0;
+ }
/* Mark L1 table as used */
ret = qcow2_inc_refcounts_imrt(bs, res, refcount_table, refcount_table_size,
- l1_table_offset, l1_size2);
+ l1_table_offset, l1_size_bytes);
if (ret < 0) {
- goto fail;
+ return ret;
+ }
+
+ l1_table = g_try_malloc(l1_size_bytes);
+ if (l1_table == NULL) {
+ res->check_errors++;
+ return -ENOMEM;
}
/* Read L1 table entries from disk */
- if (l1_size2 > 0) {
- l1_table = g_try_malloc(l1_size2);
- if (l1_table == NULL) {
- ret = -ENOMEM;
- res->check_errors++;
- goto fail;
- }
- ret = bdrv_pread(bs->file, l1_table_offset, l1_table, l1_size2);
- if (ret < 0) {
- fprintf(stderr, "ERROR: I/O error in check_refcounts_l1\n");
- res->check_errors++;
- goto fail;
- }
- for(i = 0;i < l1_size; i++)
- be64_to_cpus(&l1_table[i]);
+ ret = bdrv_pread(bs->file, l1_table_offset, l1_table, l1_size_bytes);
+ if (ret < 0) {
+ fprintf(stderr, "ERROR: I/O error in check_refcounts_l1\n");
+ res->check_errors++;
+ return ret;
+ }
+
+ for (i = 0; i < l1_size; i++) {
+ be64_to_cpus(&l1_table[i]);
}
/* Do the actual checks */
- for(i = 0; i < l1_size; i++) {
- l2_offset = l1_table[i];
- if (l2_offset) {
- /* Mark L2 table as used */
- l2_offset &= L1E_OFFSET_MASK;
- ret = qcow2_inc_refcounts_imrt(bs, res,
- refcount_table, refcount_table_size,
- l2_offset, s->cluster_size);
- if (ret < 0) {
- goto fail;
- }
+ for (i = 0; i < l1_size; i++) {
+ if (!l1_table[i]) {
+ continue;
+ }
- /* L2 tables are cluster aligned */
- if (offset_into_cluster(s, l2_offset)) {
- fprintf(stderr, "ERROR l2_offset=%" PRIx64 ": Table is not "
- "cluster aligned; L1 entry corrupted\n", l2_offset);
- res->corruptions++;
- }
+ l2_offset = l1_table[i] & L1E_OFFSET_MASK;
- /* Process and check L2 entries */
- ret = check_refcounts_l2(bs, res, refcount_table,
- refcount_table_size, l2_offset, flags,
- fix, active);
- if (ret < 0) {
- goto fail;
- }
+ /* Mark L2 table as used */
+ ret = qcow2_inc_refcounts_imrt(bs, res,
+ refcount_table, refcount_table_size,
+ l2_offset, s->cluster_size);
+ if (ret < 0) {
+ return ret;
+ }
+
+ /* L2 tables are cluster aligned */
+ if (offset_into_cluster(s, l2_offset)) {
+ fprintf(stderr, "ERROR l2_offset=%" PRIx64 ": Table is not "
+ "cluster aligned; L1 entry corrupted\n", l2_offset);
+ res->corruptions++;
+ }
+
+ /* Process and check L2 entries */
+ ret = check_refcounts_l2(bs, res, refcount_table,
+ refcount_table_size, l2_offset, flags,
+ fix, active);
+ if (ret < 0) {
+ return ret;
}
}
- g_free(l1_table);
- return 0;
-fail:
- g_free(l1_table);
- return ret;
+ return 0;
}
/*
--
2.29.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH v2 09/10] qcow2-refcount: check_refcounts_l1(): check reserved bits
2021-05-05 6:59 [PATCH v2 00/10] qcow2 check: check some reserved bits and subcluster bitmaps Vladimir Sementsov-Ogievskiy
` (7 preceding siblings ...)
2021-05-05 6:59 ` [PATCH v2 08/10] qcow2-refcount: improve style of check_refcounts_l1() Vladimir Sementsov-Ogievskiy
@ 2021-05-05 6:59 ` Vladimir Sementsov-Ogievskiy
2021-05-11 16:55 ` Kirill Tkhai
2021-05-05 6:59 ` [PATCH v2 10/10] qcow2-refcount: check_refblocks(): add separate message for reserved Vladimir Sementsov-Ogievskiy
9 siblings, 1 reply; 16+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-05-05 6:59 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, mreitz, kwolf, vsementsov, den, ktkhai, eblake, berto
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block/qcow2.h | 1 +
block/qcow2-refcount.c | 6 ++++++
2 files changed, 7 insertions(+)
diff --git a/block/qcow2.h b/block/qcow2.h
index b8b1093b61..58fd7f1678 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -586,6 +586,7 @@ typedef enum QCow2MetadataOverlap {
(QCOW2_OL_CACHED | QCOW2_OL_INACTIVE_L2)
#define L1E_OFFSET_MASK 0x00fffffffffffe00ULL
+#define L1E_RESERVED_MASK 0x7f000000000001ffULL
#define L2E_OFFSET_MASK 0x00fffffffffffe00ULL
#define L2E_STD_RESERVED_MASK 0x3f000000000001feULL
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 69294a94fe..15c4f6b075 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1904,6 +1904,12 @@ static int check_refcounts_l1(BlockDriverState *bs,
continue;
}
+ if (l1_table[i] & L1E_RESERVED_MASK) {
+ fprintf(stderr, "ERROR found L1 entry with reserved bits set: "
+ "%" PRIx64, l1_table[i]);
+ res->corruptions++;
+ }
+
l2_offset = l1_table[i] & L1E_OFFSET_MASK;
/* Mark L2 table as used */
--
2.29.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v2 09/10] qcow2-refcount: check_refcounts_l1(): check reserved bits
2021-05-05 6:59 ` [PATCH v2 09/10] qcow2-refcount: check_refcounts_l1(): check reserved bits Vladimir Sementsov-Ogievskiy
@ 2021-05-11 16:55 ` Kirill Tkhai
0 siblings, 0 replies; 16+ messages in thread
From: Kirill Tkhai @ 2021-05-11 16:55 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: qemu-devel, mreitz, kwolf, den, eblake, berto
On 05.05.2021 09:59, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
> block/qcow2.h | 1 +
> block/qcow2-refcount.c | 6 ++++++
> 2 files changed, 7 insertions(+)
>
> diff --git a/block/qcow2.h b/block/qcow2.h
> index b8b1093b61..58fd7f1678 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -586,6 +586,7 @@ typedef enum QCow2MetadataOverlap {
> (QCOW2_OL_CACHED | QCOW2_OL_INACTIVE_L2)
>
> #define L1E_OFFSET_MASK 0x00fffffffffffe00ULL
> +#define L1E_RESERVED_MASK 0x7f000000000001ffULL
> #define L2E_OFFSET_MASK 0x00fffffffffffe00ULL
> #define L2E_STD_RESERVED_MASK 0x3f000000000001feULL
>
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 69294a94fe..15c4f6b075 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -1904,6 +1904,12 @@ static int check_refcounts_l1(BlockDriverState *bs,
> continue;
> }
>
> + if (l1_table[i] & L1E_RESERVED_MASK) {
> + fprintf(stderr, "ERROR found L1 entry with reserved bits set: "
> + "%" PRIx64, l1_table[i]);
'\n' is missed.
The rest is OK for me.
Tested-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> + res->corruptions++;
> + }
> +
> l2_offset = l1_table[i] & L1E_OFFSET_MASK;
>
> /* Mark L2 table as used */
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 10/10] qcow2-refcount: check_refblocks(): add separate message for reserved
2021-05-05 6:59 [PATCH v2 00/10] qcow2 check: check some reserved bits and subcluster bitmaps Vladimir Sementsov-Ogievskiy
` (8 preceding siblings ...)
2021-05-05 6:59 ` [PATCH v2 09/10] qcow2-refcount: check_refcounts_l1(): check reserved bits Vladimir Sementsov-Ogievskiy
@ 2021-05-05 6:59 ` Vladimir Sementsov-Ogievskiy
2021-05-11 17:09 ` Kirill Tkhai
9 siblings, 1 reply; 16+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-05-05 6:59 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, mreitz, kwolf, vsementsov, den, ktkhai, eblake, berto
Split checking for reserved bits out of aligned offset check.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block/qcow2.h | 1 +
block/qcow2-refcount.c | 10 +++++++++-
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index 58fd7f1678..fd48a89d45 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -591,6 +591,7 @@ typedef enum QCow2MetadataOverlap {
#define L2E_STD_RESERVED_MASK 0x3f000000000001feULL
#define REFT_OFFSET_MASK 0xfffffffffffffe00ULL
+#define REFT_RESERVED_MASK 0x1ffULL
#define INV_OFFSET (-1ULL)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 15c4f6b075..472a7026db 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -2091,9 +2091,17 @@ static int check_refblocks(BlockDriverState *bs, BdrvCheckResult *res,
for(i = 0; i < s->refcount_table_size; i++) {
uint64_t offset, cluster;
- offset = s->refcount_table[i];
+ offset = s->refcount_table[i] & REFT_OFFSET_MASK;
cluster = offset >> s->cluster_bits;
+ if (s->refcount_table[i] & REFT_RESERVED_MASK) {
+ fprintf(stderr, "ERROR refcount table entry %" PRId64 " has "
+ "reserved bits set\n", i);
+ res->corruptions++;
+ *rebuild = true;
+ continue;
+ }
+
/* Refcount blocks are cluster aligned */
if (offset_into_cluster(s, offset)) {
fprintf(stderr, "ERROR refcount block %" PRId64 " is not "
--
2.29.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v2 10/10] qcow2-refcount: check_refblocks(): add separate message for reserved
2021-05-05 6:59 ` [PATCH v2 10/10] qcow2-refcount: check_refblocks(): add separate message for reserved Vladimir Sementsov-Ogievskiy
@ 2021-05-11 17:09 ` Kirill Tkhai
0 siblings, 0 replies; 16+ messages in thread
From: Kirill Tkhai @ 2021-05-11 17:09 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: qemu-devel, mreitz, kwolf, den, eblake, berto
On 05.05.2021 09:59, Vladimir Sementsov-Ogievskiy wrote:
> Split checking for reserved bits out of aligned offset check.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
Tested-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> ---
> block/qcow2.h | 1 +
> block/qcow2-refcount.c | 10 +++++++++-
> 2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 58fd7f1678..fd48a89d45 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -591,6 +591,7 @@ typedef enum QCow2MetadataOverlap {
> #define L2E_STD_RESERVED_MASK 0x3f000000000001feULL
>
> #define REFT_OFFSET_MASK 0xfffffffffffffe00ULL
> +#define REFT_RESERVED_MASK 0x1ffULL
>
> #define INV_OFFSET (-1ULL)
>
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 15c4f6b075..472a7026db 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -2091,9 +2091,17 @@ static int check_refblocks(BlockDriverState *bs, BdrvCheckResult *res,
>
> for(i = 0; i < s->refcount_table_size; i++) {
> uint64_t offset, cluster;
> - offset = s->refcount_table[i];
> + offset = s->refcount_table[i] & REFT_OFFSET_MASK;
> cluster = offset >> s->cluster_bits;
>
> + if (s->refcount_table[i] & REFT_RESERVED_MASK) {
> + fprintf(stderr, "ERROR refcount table entry %" PRId64 " has "
> + "reserved bits set\n", i);
> + res->corruptions++;
> + *rebuild = true;
> + continue;
> + }
> +
> /* Refcount blocks are cluster aligned */
> if (offset_into_cluster(s, offset)) {
> fprintf(stderr, "ERROR refcount block %" PRId64 " is not "
>
^ permalink raw reply [flat|nested] 16+ messages in thread