On 22/05/2026 13:29, Vladimir Sementsov-Ogievskiy wrote:
I opted for the current behaviour as bitmask compare is also performant and avoids extra variable. Next to that all code in qcow2.c also use 'flags & BDRV_O_CHECK' style if. So :)On 22.05.26 11:51, Jean-Louis Dupond wrote:In some cases, it might happen that bitmaps become corrupt. For example when adding/removing bitmaps on a live image. Of course this should not happen, but in case this happens, the image is corrupt and even cannot be opened anymore. You'll get something like the following:qemu-img: Could not open 'disk.qcow2': Bitmap '' doesn't satisfy the constraintsSo the image becomes useless, and cannot be repaired. This while in fact only (one) bitmap entry is corrupt, and the rest of the data is just intact. This commit adds a way to fix this corruption, by just replacing the bitmap list in the qcow2 image with the valid bitmaps, and dropping the bitmaps that are corrupt. $ qemu-img check disk.qcow2 qemu-img: Check failed: Invalid argumentqemu-img: Lost persistent bitmaps during inactivation of node '#block147': Bitmap '' doesn't satisfy the constraints$ qemu-img check -r all disk.qcow2 qcow2_free_clusters failed: Invalid argument Leaked cluster 3 refcount=1 reference=0 Leaked cluster 26 refcount=1 reference=0 ERROR cluster 983214 refcount=0 reference=1 Rebuilding refcount structure Repairing cluster 1 refcount=1 reference=0 Repairing cluster 2 refcount=1 reference=0 Repairing cluster 32768 refcount=1 reference=0 .... Repairing cluster 983214 refcount=1 reference=0 The following inconsistencies were found and repaired: 2 leaked clusters 3 corruptions Double checking the fixed image now... No errors were found on the image.983056/1048576 = 93.75% allocated, 0.05% fragmented, 0.00% compressed clustersImage end offset: 64437878784 And the image is valid again! Worst case you lose all bitmaps, but at least the image and the data itself is useable again. Signed-off-by: Jean-Louis Dupond <[email protected]> --- block/qcow2-bitmap.c | 43 +++++++++++++++++++++++++++++++++++++++++- block/qcow2-refcount.c | 3 ++- block/qcow2.c | 16 ++++++++++++---- block/qcow2.h | 3 ++- 4 files changed, 58 insertions(+), 7 deletions(-) diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index 256ec99878..ab2f7fd83b 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -100,6 +100,9 @@ typedef enum BitmapType { BT_DIRTY_TRACKING_BITMAP = 1 } BitmapType; +static int GRAPH_RDLOCK+update_ext_header_and_dir(BlockDriverState *bs, Qcow2BitmapList *bm_list);+ static inline bool can_write(BlockDriverState *bs) {return !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);@@ -655,11 +658,14 @@ fail: int coroutine_fnqcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,void **refcount_table, - int64_t *refcount_table_size) + int64_t *refcount_table_size, + BdrvCheckMode fix) { int ret; BDRVQcow2State *s = bs->opaque; Qcow2BitmapList *bm_list; + Qcow2BitmapList *fixed_list = NULL; + int valid_bitmaps = 0; Qcow2Bitmap *bm; if (s->nb_bitmaps == 0) {@@ -673,16 +679,29 @@ qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,return ret; } + if (fix & BDRV_FIX_ERRORS) {Maybe, worth making a variable for it, and use it here and below. bool fix_errors = fix & BDRV_FIX_ERRORS;
But I'm open for both...
+ fixed_list = bitmap_list_new(); + } + bm_list = bitmap_list_load(bs, s->bitmap_directory_offset, s->bitmap_directory_size, NULL); if (bm_list == NULL) { res->corruptions++; + + if (fix & BDRV_FIX_ERRORS) { + ret = update_ext_header_and_dir(bs, fixed_list); + if (ret >= 0) { + res->corruptions_fixed++; + } + goto out; + } return -EINVAL; } QSIMPLEQ_FOREACH(bm, bm_list, entry) { uint64_t *bitmap_table = NULL; int i; + bool bitmap_valid = true; ret = qcow2_inc_refcounts_imrt(bs, res,refcount_table, refcount_table_size, @@ -695,6 +714,9 @@ qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,ret = bitmap_table_load(bs, &bm->table, &bitmap_table); if (ret < 0) { res->corruptions++; + if (fix & BDRV_FIX_ERRORS) { + continue; + } goto out; }@@ -704,6 +726,7 @@ qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,if (check_table_entry(entry, s->cluster_size) < 0) { res->corruptions++; + bitmap_valid = false; continue; }@@ -720,10 +743,28 @@ qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,} } + if ((fix & BDRV_FIX_ERRORS) && bitmap_valid) { + valid_bitmaps++; + Qcow2Bitmap *bm_copy = g_new0(Qcow2Bitmap, 1);Also, from docs/devel/style.rst: Declarations ============ Mixed declarations (interleaving statements and declarations withinblocks) are generally not allowed; declarations should be at the beginningof blocks. So, Qcow2Bitmap *bm_copy = g_new0(Qcow2Bitmap, 1); should be the first line of the block.
Will Fix!
+ *bm_copy = *bm; + bm_copy->name = g_strdup(bm->name); + QSIMPLEQ_INSERT_TAIL(fixed_list, bm_copy, entry);Hmm, and another thought (sorry I didn't figure it out in the previous iteration):
No problem!
Why do we need even this half-copied additional list?Instead, we can remove broken elements from original bm_list (when fix & BDRV_FIX_ERRORS),and reuse it for update_ext_header_and_dir().
I looked for the best way to handle this.Thought that creating a new list with valid items would be easiest, but changing the approach to remove corrupt items from the existing list.
+ } + g_free(bitmap_table); }+ /* If fixing, update the bitmap directory with the repaired list */+ if ((fix & BDRV_FIX_ERRORS) && s->nb_bitmaps != valid_bitmaps) { + uint32_t initial_bitmaps = s->nb_bitmaps; + ret = update_ext_header_and_dir(bs, fixed_list); + if (ret >= 0) { + res->corruptions_fixed += initial_bitmaps - valid_bitmaps; + } + } + out: + bitmap_list_free(fixed_list); bitmap_list_free(bm_list); return ret; diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c index 6512cda407..df7f6d2f23 100644 --- a/block/qcow2-refcount.c +++ b/block/qcow2-refcount.c@@ -2305,7 +2305,8 @@ calculate_refcounts(BlockDriverState *bs, BdrvCheckResult *res,} /* bitmaps */- ret = qcow2_check_bitmaps_refcounts(bs, res, refcount_table, nb_clusters);+ ret = qcow2_check_bitmaps_refcounts(bs, res, refcount_table, + nb_clusters, fix); if (ret < 0) { return ret; } diff --git a/block/qcow2.c b/block/qcow2.c index 81fd299b4c..adfb7ee37a 100644 --- a/block/qcow2.c +++ b/block/qcow2.c@@ -1927,11 +1927,19 @@ qcow2_do_open(BlockDriverState *bs, QDict *options, int flags,if (!(bdrv_get_flags(bs) & BDRV_O_INACTIVE)) {/* It's case 1, 2 or 3.2. Or 3.1 which is BUG in management layer. */bool header_updated; - if (!qcow2_load_dirty_bitmaps(bs, &header_updated, errp)) { - ret = -EINVAL; - goto fail; + Error *local_err = NULL;+ if (!qcow2_load_dirty_bitmaps(bs, &header_updated, &local_err)) {+ /* + * Allow this to fail in check mode + * because otherwise we can't open the image at all. + */ + if (!(flags & BDRV_O_CHECK)) { + ret = -EINVAL; + error_propagate(errp, local_err); + goto fail; + } + error_free(local_err); } - update_header = update_header && !header_updated; } diff --git a/block/qcow2.h b/block/qcow2.h index 192a45d596..2d9c6929a7 100644 --- a/block/qcow2.h +++ b/block/qcow2.h@@ -1034,7 +1034,8 @@ void qcow2_cache_discard(Qcow2Cache *c, void *table);int coroutine_fn GRAPH_RDLOCKqcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,void **refcount_table, - int64_t *refcount_table_size); + int64_t *refcount_table_size, + BdrvCheckMode fix); bool coroutine_fn GRAPH_RDLOCK qcow2_load_dirty_bitmaps(BlockDriverState *bs, bool *header_updated,
I'll post a v4 with the fixes. Thanks Jean-Louis
