A v2 version of the patch. First of all a commit to move qcow2_check_bitmaps_refcounts lower into the code in order to make sure we can access update_ext_header_and_dir. Next I've moved the repair functionality into the existing qcow2_check_bitmaps_refcounts function. Thanks Jean-Louis Jean-Louis Dupond (2): qcow2: move qcow2_check_bitmaps_refcounts func qcow2: add functionality to repair bitmaps block/qcow2-bitmap.c | 194 +++++++++++++++++++++++++---------------- block/qcow2-refcount.c | 3 +- block/qcow2.c | 15 +++- block/qcow2.h | 3 +- 4 files changed, 132 insertions(+), 83 deletions(-) -- 2.54.0 Move the qcow2_check_bitmaps_refcounts function in order to be able to call update_ext_header_and_dir from this function in the following patch. Signed-off-by: Jean-Louis Dupond <jean-louis@dupond.be> --- block/qcow2-bitmap.c | 154 +++++++++++++++++++++---------------------- 1 file changed, 77 insertions(+), 77 deletions(-) diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index XXXXXXX..XXXXXXX 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -XXX,XX +XXX,XX @@ fail: return NULL; } -int coroutine_fn -qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res, - void **refcount_table, - int64_t *refcount_table_size) -{ - int ret; - BDRVQcow2State *s = bs->opaque; - Qcow2BitmapList *bm_list; - Qcow2Bitmap *bm; - - if (s->nb_bitmaps == 0) { - return 0; - } - - ret = qcow2_inc_refcounts_imrt(bs, res, refcount_table, refcount_table_size, - s->bitmap_directory_offset, - s->bitmap_directory_size); - if (ret < 0) { - return ret; - } - - bm_list = bitmap_list_load(bs, s->bitmap_directory_offset, - s->bitmap_directory_size, NULL); - if (bm_list == NULL) { - res->corruptions++; - return -EINVAL; - } - - QSIMPLEQ_FOREACH(bm, bm_list, entry) { - uint64_t *bitmap_table = NULL; - int i; - - ret = qcow2_inc_refcounts_imrt(bs, res, - refcount_table, refcount_table_size, - bm->table.offset, - bm->table.size * BME_TABLE_ENTRY_SIZE); - if (ret < 0) { - goto out; - } - - ret = bitmap_table_load(bs, &bm->table, &bitmap_table); - if (ret < 0) { - res->corruptions++; - goto out; - } - - for (i = 0; i < bm->table.size; ++i) { - uint64_t entry = bitmap_table[i]; - uint64_t offset = entry & BME_TABLE_ENTRY_OFFSET_MASK; - - if (check_table_entry(entry, s->cluster_size) < 0) { - res->corruptions++; - continue; - } - - if (offset == 0) { - continue; - } - - ret = qcow2_inc_refcounts_imrt(bs, res, - refcount_table, refcount_table_size, - offset, s->cluster_size); - if (ret < 0) { - g_free(bitmap_table); - goto out; - } - } - - g_free(bitmap_table); - } - -out: - bitmap_list_free(bm_list); - - return ret; -} - /* bitmap_list_store * Store bitmap list to qcow2 image as a bitmap directory. * Everything is checked. @@ -XXX,XX +XXX,XX @@ uint64_t qcow2_get_persistent_dirty_bitmap_size(BlockDriverState *in_bs, return bitmaps_size; } + +int coroutine_fn +qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res, + void **refcount_table, + int64_t *refcount_table_size) +{ + int ret; + BDRVQcow2State *s = bs->opaque; + Qcow2BitmapList *bm_list; + Qcow2Bitmap *bm; + + if (s->nb_bitmaps == 0) { + return 0; + } + + ret = qcow2_inc_refcounts_imrt(bs, res, refcount_table, refcount_table_size, + s->bitmap_directory_offset, + s->bitmap_directory_size); + if (ret < 0) { + return ret; + } + + bm_list = bitmap_list_load(bs, s->bitmap_directory_offset, + s->bitmap_directory_size, NULL); + if (bm_list == NULL) { + res->corruptions++; + return -EINVAL; + } + + QSIMPLEQ_FOREACH(bm, bm_list, entry) { + uint64_t *bitmap_table = NULL; + int i; + + ret = qcow2_inc_refcounts_imrt(bs, res, + refcount_table, refcount_table_size, + bm->table.offset, + bm->table.size * BME_TABLE_ENTRY_SIZE); + if (ret < 0) { + goto out; + } + + ret = bitmap_table_load(bs, &bm->table, &bitmap_table); + if (ret < 0) { + res->corruptions++; + goto out; + } + + for (i = 0; i < bm->table.size; ++i) { + uint64_t entry = bitmap_table[i]; + uint64_t offset = entry & BME_TABLE_ENTRY_OFFSET_MASK; + + if (check_table_entry(entry, s->cluster_size) < 0) { + res->corruptions++; + continue; + } + + if (offset == 0) { + continue; + } + + ret = qcow2_inc_refcounts_imrt(bs, res, + refcount_table, refcount_table_size, + offset, s->cluster_size); + if (ret < 0) { + g_free(bitmap_table); + goto out; + } + } + + g_free(bitmap_table); + } + +out: + bitmap_list_free(bm_list); + + return ret; +} -- 2.54.0 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 constraints So 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 argument qemu-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 65536 refcount=1 reference=0 Repairing cluster 98304 refcount=1 reference=0 Repairing cluster 131072 refcount=1 reference=0 Repairing cluster 163840 refcount=1 reference=0 Repairing cluster 196612 refcount=1 reference=0 Repairing cluster 229377 refcount=1 reference=0 Repairing cluster 262146 refcount=1 reference=0 Repairing cluster 294915 refcount=1 reference=0 Repairing cluster 327684 refcount=1 reference=0 Repairing cluster 360449 refcount=1 reference=0 Repairing cluster 393218 refcount=1 reference=0 Repairing cluster 425987 refcount=1 reference=0 Repairing cluster 458756 refcount=1 reference=0 Repairing cluster 491521 refcount=1 reference=0 Repairing cluster 524289 refcount=1 reference=0 Repairing cluster 557058 refcount=1 reference=0 Repairing cluster 589827 refcount=1 reference=0 Repairing cluster 622596 refcount=1 reference=0 Repairing cluster 655361 refcount=1 reference=0 Repairing cluster 688130 refcount=1 reference=0 Repairing cluster 720899 refcount=1 reference=0 Repairing cluster 753668 refcount=1 reference=0 Repairing cluster 786433 refcount=1 reference=0 Repairing cluster 819202 refcount=1 reference=0 Repairing cluster 851971 refcount=1 reference=0 Repairing cluster 884740 refcount=1 reference=0 Repairing cluster 917505 refcount=1 reference=0 Repairing cluster 950274 refcount=1 reference=0 Repairing cluster 983042 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 clusters Image 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 <jean-louis@dupond.be> --- block/qcow2-bitmap.c | 42 +++++++++++++++++++++++++++++++++++++++++- block/qcow2-refcount.c | 3 ++- block/qcow2.c | 15 +++++++++++---- block/qcow2.h | 3 ++- 4 files changed, 56 insertions(+), 7 deletions(-) diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index XXXXXXX..XXXXXXX 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -XXX,XX +XXX,XX @@ uint64_t qcow2_get_persistent_dirty_bitmap_size(BlockDriverState *in_bs, int coroutine_fn qcow2_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) { @@ -XXX,XX +XXX,XX @@ qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res, return ret; } + if (fix & BDRV_FIX_ERRORS) { + 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, @@ -XXX,XX +XXX,XX @@ 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; } @@ -XXX,XX +XXX,XX @@ qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res, if (check_table_entry(entry, s->cluster_size) < 0) { res->corruptions++; + bitmap_valid = false; continue; } @@ -XXX,XX +XXX,XX @@ qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res, } } + if ((fix & BDRV_FIX_ERRORS) && bitmap_valid) { + valid_bitmaps++; + Qcow2Bitmap *bm_copy = g_new0(Qcow2Bitmap, 1); + *bm_copy = *bm; + bm_copy->name = g_strdup(bm->name); + QSIMPLEQ_INSERT_TAIL(fixed_list, bm_copy, entry); + } + g_free(bitmap_table); } + /* If fixing, update the bitmap directory with the repaired list */ + if ((fix & BDRV_FIX_ERRORS) && s->nb_bitmaps != valid_bitmaps) { + ret = update_ext_header_and_dir(bs, fixed_list); + if (ret >= 0) { + res->corruptions_fixed += s->nb_bitmaps - valid_bitmaps; + } + } + out: + if (fix & BDRV_FIX_ERRORS) { + 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 XXXXXXX..XXXXXXX 100644 --- a/block/qcow2-refcount.c +++ b/block/qcow2-refcount.c @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -XXX,XX +XXX,XX @@ void qcow2_cache_discard(Qcow2Cache *c, void *table); int coroutine_fn GRAPH_RDLOCK qcow2_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, -- 2.54.0 A v3 version of the patch. - Added a forward declaration for update_ext_header_and_dir so the function doesn't need to be moved anymore. - Style fix of comment - Keep s->nb_bitmaps in a variable in order to have correct corruption fixed count - Remove obsolete if in bitmap_list_free Thanks Jean-Louis Jean-Louis Dupond (1): qcow2: add functionality to repair bitmaps 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(-) -- 2.54.0 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 constraints So 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 argument qemu-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 clusters Image 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 <jean-louis@dupond.be> --- 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 XXXXXXX..XXXXXXX 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -XXX,XX +XXX,XX @@ 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); @@ -XXX,XX +XXX,XX @@ fail: int coroutine_fn qcow2_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) { @@ -XXX,XX +XXX,XX @@ qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res, return ret; } + if (fix & BDRV_FIX_ERRORS) { + 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, @@ -XXX,XX +XXX,XX @@ 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; } @@ -XXX,XX +XXX,XX @@ qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res, if (check_table_entry(entry, s->cluster_size) < 0) { res->corruptions++; + bitmap_valid = false; continue; } @@ -XXX,XX +XXX,XX @@ qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res, } } + if ((fix & BDRV_FIX_ERRORS) && bitmap_valid) { + valid_bitmaps++; + Qcow2Bitmap *bm_copy = g_new0(Qcow2Bitmap, 1); + *bm_copy = *bm; + bm_copy->name = g_strdup(bm->name); + QSIMPLEQ_INSERT_TAIL(fixed_list, bm_copy, entry); + } + 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 XXXXXXX..XXXXXXX 100644 --- a/block/qcow2-refcount.c +++ b/block/qcow2-refcount.c @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -XXX,XX +XXX,XX @@ void qcow2_cache_discard(Qcow2Cache *c, void *table); int coroutine_fn GRAPH_RDLOCK qcow2_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, -- 2.54.0