23.02.2019 3:22, John Snow wrote: > Set the inconsistent bit on load instead of rejecting such bitmaps. > There is no way to un-set it; the only option is to delete it. > > Obvervations: > - bitmap loading does not need to update the header for in_use bitmaps. > - inconsistent bitmaps don't need to have their data loaded; they're > glorified corruption sentinels. > - bitmap saving does not need to save inconsistent bitmaps back to disk. > - bitmap reopening DOES need to drop the readonly flag from inconsistent > bitmaps to allow reopening of qcow2 files with non-qemu-owned bitmaps > being eventually flushed back to disk. > > Signed-off-by: John Snow <js...@redhat.com> > --- > block/qcow2-bitmap.c | 77 +++++++++++++++++++++++--------------------- > 1 file changed, 40 insertions(+), 37 deletions(-) > > diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c > index 3ee524da4b..d1cc11da88 100644 > --- a/block/qcow2-bitmap.c > +++ b/block/qcow2-bitmap.c > @@ -343,9 +343,15 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs, > uint32_t granularity; > BdrvDirtyBitmap *bitmap = NULL; > > + granularity = 1U << bm->granularity_bits; > + bitmap = bdrv_create_dirty_bitmap(bs, granularity, bm->name, errp); > + if (bitmap == NULL) { > + goto fail; > + } > + > if (bm->flags & BME_FLAG_IN_USE) { > - error_setg(errp, "Bitmap '%s' is in use", bm->name); > - goto fail; > + /* Data is unusable, skip loading it */ > + return bitmap; > } > > ret = bitmap_table_load(bs, &bm->table, &bitmap_table); > @@ -356,12 +362,6 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs, > goto fail; > } > > - granularity = 1U << bm->granularity_bits; > - bitmap = bdrv_create_dirty_bitmap(bs, granularity, bm->name, errp); > - if (bitmap == NULL) { > - goto fail; > - } > - > ret = load_bitmap_data(bs, bitmap_table, bm->table.size, bitmap); > if (ret < 0) { > error_setg_errno(errp, -ret, "Could not read bitmap '%s' from > image", > @@ -949,6 +949,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error > **errp) > Qcow2Bitmap *bm; > GSList *created_dirty_bitmaps = NULL; > bool header_updated = false; > + bool needs_update = false; > > if (s->nb_bitmaps == 0) { > /* No bitmaps - nothing to do */ > @@ -962,23 +963,27 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, > Error **errp) > } > > QSIMPLEQ_FOREACH(bm, bm_list, entry) { > - if (!(bm->flags & BME_FLAG_IN_USE)) { > - BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp); > - if (bitmap == NULL) { > - goto fail; > - } > + BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp); > + if (bitmap == NULL) { > + goto fail; > + } > > - if (!(bm->flags & BME_FLAG_AUTO)) { > - bdrv_disable_dirty_bitmap(bitmap); > - } > - bdrv_dirty_bitmap_set_persistance(bitmap, true); > + if (bm->flags & BME_FLAG_IN_USE) { > + bdrv_dirty_bitmap_set_inconsistent(bitmap); > + } else { > + /* NB: updated flags only get written if can_write(bs) is true. > */ > bm->flags |= BME_FLAG_IN_USE; > - created_dirty_bitmaps = > - g_slist_append(created_dirty_bitmaps, bitmap); > + needs_update = true; > } > + if (!(bm->flags & BME_FLAG_AUTO)) { > + bdrv_disable_dirty_bitmap(bitmap); > + } > + bdrv_dirty_bitmap_set_persistance(bitmap, true);
We define persistent bitmaps as which we are going to store.. But we are not going to store inconsistent bitmaps. So inconsistent bitmaps are not persistent. > + created_dirty_bitmaps = > + g_slist_append(created_dirty_bitmaps, bitmap); > } > > - if (created_dirty_bitmaps != NULL) { > + if (needs_update) { created_dirty_bitmaps list needed only for setting them readonly, if failed write that they are IN_USE. So, instead of creating additional variable, better is just not include IN_USE bitmaps to this list. And it may be renamed like set_in_use_list. > if (can_write(bs)) { > /* in_use flags must be updated */ > int ret = update_ext_header_and_dir_in_place(bs, bm_list); > @@ -1112,23 +1117,21 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState > *bs, bool *header_updated, > } > > QSIMPLEQ_FOREACH(bm, bm_list, entry) { > - if (!(bm->flags & BME_FLAG_IN_USE)) { > - BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name); > - if (bitmap == NULL) { > - continue; > - } > - > - if (!bdrv_dirty_bitmap_readonly(bitmap)) { > - error_setg(errp, "Bitmap %s is not readonly but not marked" > - "'IN_USE' in the image. Something went > wrong," > - "all the bitmaps may be corrupted", > bm->name); > - ret = -EINVAL; > - goto out; > - } > + BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name); > + if (bitmap == NULL) { > + continue; > + } > > - bm->flags |= BME_FLAG_IN_USE; > - ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap); > + if (!bdrv_dirty_bitmap_readonly(bitmap)) { so, we'll fail, as we didn't marked inconsistetn bitmaps as readonly above. Should we? Or we may check here inconsistance directly. > + error_setg(errp, "Bitmap %s was loaded prior to rw-reopen, but > was " > + "not marked as readonly. This is a bug, something > went " > + "wrong. All of the bitmaps may be corrupted", > bm->name); Hmm, your wording covers invalid case when inconsistent bitmap is not IN_USE, and sounds better for user, though may be less precise (as it may be not one reopen in a sequence..). Ok for me. > + ret = -EINVAL; > + goto out; > } > + > + bm->flags |= BME_FLAG_IN_USE; > + ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap); > } > > if (ro_dirty_bitmaps != NULL) { > @@ -1424,8 +1427,8 @@ void > qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) > Qcow2Bitmap *bm; > > if (!bdrv_dirty_bitmap_get_persistance(bitmap) || > - bdrv_dirty_bitmap_readonly(bitmap)) > - { > + bdrv_dirty_bitmap_readonly(bitmap) || > + bdrv_dirty_bitmap_inconsistent(bitmap)) { > continue; > } > > -- Best regards, Vladimir