| Message ID | 20190301191545.8728-8-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | bitmaps: add inconsistent bit | expand |
On 3/1/19 1:15 PM, 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 <jsnow@redhat.com> > --- > block/qcow2-bitmap.c | 103 ++++++++++++++++++++++--------------------- > 1 file changed, 53 insertions(+), 50 deletions(-) > > @@ -962,35 +963,39 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp) > } > > QSIMPLEQ_FOREACH(bm, bm_list, entry) { > + BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp); > + if (bitmap == NULL) { > + goto fail; > } > + if (bm->flags & BME_FLAG_IN_USE) { > + bdrv_dirty_bitmap_set_inconsistent(bitmap); If you take my suggestion of an assertion in 1/7, then this line... > } else { > - g_slist_foreach(created_dirty_bitmaps, set_readonly_helper, > - (gpointer)true); > + /* NB: updated flags only get written if can_write(bs) is true. */ > + bm->flags |= BME_FLAG_IN_USE; > + needs_update = true; > } > + if (!(bm->flags & BME_FLAG_AUTO)) { > + bdrv_disable_dirty_bitmap(bitmap); > + } > + bdrv_dirty_bitmap_set_persistance(bitmap, true); ...and this line need to swap order. Also, can we have a preliminary patch to s/persistance/persistence/ and fix our typo? > @@ -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); Nice - you're fixingthemissing spaces. > - 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)) { > + 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); Reviewed-by: Eric Blake <eblake@redhat.com>
01.03.2019 22:15, 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. I think, s/delete it/delete the bitmap/, as "it" is the bit after "un-set 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. hmm, and inconsitent bitmaps don't have this flag, isn't it? > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > block/qcow2-bitmap.c | 103 ++++++++++++++++++++++--------------------- > 1 file changed, 53 insertions(+), 50 deletions(-) > > diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c > index 3ee524da4b..c3b210ede1 100644 > --- a/block/qcow2-bitmap.c > +++ b/block/qcow2-bitmap.c [..] > @@ -962,35 +963,39 @@ 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; > - } > - > - if (!(bm->flags & BME_FLAG_AUTO)) { > - bdrv_disable_dirty_bitmap(bitmap); > - } > - bdrv_dirty_bitmap_set_persistance(bitmap, true); > - bm->flags |= BME_FLAG_IN_USE; > - created_dirty_bitmaps = > - g_slist_append(created_dirty_bitmaps, bitmap); > + BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp); > + if (bitmap == NULL) { > + goto fail; > } > - } > > - if (created_dirty_bitmaps != NULL) { > - if (can_write(bs)) { > - /* in_use flags must be updated */ > - int ret = update_ext_header_and_dir_in_place(bs, bm_list); > - if (ret < 0) { > - error_setg_errno(errp, -ret, "Can't update bitmap directory"); > - goto fail; > - } > - header_updated = true; > + if (bm->flags & BME_FLAG_IN_USE) { > + bdrv_dirty_bitmap_set_inconsistent(bitmap); > } else { > - g_slist_foreach(created_dirty_bitmaps, set_readonly_helper, > - (gpointer)true); > + /* NB: updated flags only get written if can_write(bs) is true. */ > + bm->flags |= BME_FLAG_IN_USE; > + needs_update = true; > } > + if (!(bm->flags & BME_FLAG_AUTO)) { > + bdrv_disable_dirty_bitmap(bitmap); > + } > + bdrv_dirty_bitmap_set_persistance(bitmap, true); > + created_dirty_bitmaps = > + g_slist_append(created_dirty_bitmaps, bitmap); > + } > + > + if (needs_update && can_write(bs)) { > + /* in_use flags must be updated */ > + int ret = update_ext_header_and_dir_in_place(bs, bm_list); > + if (ret < 0) { > + error_setg_errno(errp, -ret, "Can't update bitmap directory"); > + goto fail; > + } > + header_updated = true; > + } > + > + if (!can_write(bs)) { > + g_slist_foreach(created_dirty_bitmaps, set_readonly_helper, > + (gpointer)true); hmmm.. I think, inconsistent bitmaps should have readonly flag always set or always unset, independently on can_write, as we are not going to write something related to inconsistent bitmaps. And I think, better is always unset, to have inconsistent bitmaps as something unrelated to readonly. Readonly are bitmaps, which are not marked IN_USE on open for some reason.. I understand, that inconsistent bitmaps are some kind of readonly too, as we can't write to them.. But we can't read too anyway, and we don't interfere with reopen logic at all. So again, better is don't mark inconsistent bitmaps as readonly. > } > > g_slist_free(created_dirty_bitmaps); > @@ -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)) { > + 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); > + 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; > } > >
On 3/6/19 9:26 AM, Vladimir Sementsov-Ogievskiy wrote: > 01.03.2019 22:15, 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. > > I think, s/delete it/delete the bitmap/, as "it" is the bit after "un-set it". > You are absolutely correct. Don't let anyone fool you into thinking that Americans know English. >> >> 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. > > hmm, and inconsitent bitmaps don't have this flag, isn't it? > I didn't make any effort to treat them differently -- the readonly flag only gets applied when the image is actually read only. I felt it was conceptually simpler that way. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> block/qcow2-bitmap.c | 103 ++++++++++++++++++++++--------------------- >> 1 file changed, 53 insertions(+), 50 deletions(-) >> >> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c >> index 3ee524da4b..c3b210ede1 100644 >> --- a/block/qcow2-bitmap.c >> +++ b/block/qcow2-bitmap.c > > [..] > >> @@ -962,35 +963,39 @@ 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; >> - } >> - >> - if (!(bm->flags & BME_FLAG_AUTO)) { >> - bdrv_disable_dirty_bitmap(bitmap); >> - } >> - bdrv_dirty_bitmap_set_persistance(bitmap, true); >> - bm->flags |= BME_FLAG_IN_USE; >> - created_dirty_bitmaps = >> - g_slist_append(created_dirty_bitmaps, bitmap); >> + BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp); >> + if (bitmap == NULL) { >> + goto fail; >> } >> - } >> >> - if (created_dirty_bitmaps != NULL) { >> - if (can_write(bs)) { >> - /* in_use flags must be updated */ >> - int ret = update_ext_header_and_dir_in_place(bs, bm_list); >> - if (ret < 0) { >> - error_setg_errno(errp, -ret, "Can't update bitmap directory"); >> - goto fail; >> - } >> - header_updated = true; >> + if (bm->flags & BME_FLAG_IN_USE) { >> + bdrv_dirty_bitmap_set_inconsistent(bitmap); >> } else { >> - g_slist_foreach(created_dirty_bitmaps, set_readonly_helper, >> - (gpointer)true); >> + /* NB: updated flags only get written if can_write(bs) is true. */ >> + bm->flags |= BME_FLAG_IN_USE; >> + needs_update = true; >> } >> + if (!(bm->flags & BME_FLAG_AUTO)) { >> + bdrv_disable_dirty_bitmap(bitmap); >> + } >> + bdrv_dirty_bitmap_set_persistance(bitmap, true); >> + created_dirty_bitmaps = >> + g_slist_append(created_dirty_bitmaps, bitmap); >> + } >> + >> + if (needs_update && can_write(bs)) { >> + /* in_use flags must be updated */ >> + int ret = update_ext_header_and_dir_in_place(bs, bm_list); >> + if (ret < 0) { >> + error_setg_errno(errp, -ret, "Can't update bitmap directory"); >> + goto fail; >> + } >> + header_updated = true; >> + } >> + >> + if (!can_write(bs)) { >> + g_slist_foreach(created_dirty_bitmaps, set_readonly_helper, >> + (gpointer)true); > > hmmm.. > > I think, inconsistent bitmaps should have readonly flag always set or always unset, independently on can_write, > as we are not going to write something related to inconsistent bitmaps. > We'll never change metadata for inconsistent bitmaps, no. We might delete them which causes a metadata change, though -- and right now the readonly check stops that in blockdev.c before we attempt (and fail) the operation. It does have at least a light usage. It's not a crucial feature, but I think the error message is nicer. > And I think, better is always unset, to have inconsistent bitmaps as something unrelated to readonly. > The way I think of it is as unrelated, which is why I set readonly and inconsistent orthogonally. > Readonly are bitmaps, which are not marked IN_USE on open for some reason.. > I tend to think of readonly bitmaps as persistent bitmaps attached to storage we are unable to write back to, so we must prevent their modification. I suppose the IN_USE viewpoint works too; "This is a bitmap that we have not marked as IN_USE so it must not be used." > I understand, that inconsistent bitmaps are some kind of readonly too, as we can't write to them.. > But we can't read too anyway, and we don't interfere with reopen logic at all. So again, better is > don't mark inconsistent bitmaps as readonly. > Hm, so in your model; we just NEVER set readonly for persistent bitmaps, which incurs some changes at load time, but allows the reload_rw function to go completely unmodified. That's not unreasonable in terms of SLOC, but semantically I am not sure which approach is better. I'm leaning towards keeping this as written for now, but... well. Any thoughts, Eric? Would you like to flip a coin? --js
On 3/6/19 3:46 PM, John Snow wrote: >> I think, inconsistent bitmaps should have readonly flag always set or always unset, independently on can_write, >> as we are not going to write something related to inconsistent bitmaps. >> > > We'll never change metadata for inconsistent bitmaps, no. We might > delete them which causes a metadata change, though -- and right now the > readonly check stops that in blockdev.c before we attempt (and fail) the > operation. It does have at least a light usage. > > It's not a crucial feature, but I think the error message is nicer. > >> And I think, better is always unset, to have inconsistent bitmaps as something unrelated to readonly. >> > > The way I think of it is as unrelated, which is why I set readonly and > inconsistent orthogonally. > >> Readonly are bitmaps, which are not marked IN_USE on open for some reason.. >> > > I tend to think of readonly bitmaps as persistent bitmaps attached to > storage we are unable to write back to, so we must prevent their > modification. I like that wording (keeping read-only and inconsistent orthogonal), > > I suppose the IN_USE viewpoint works too; "This is a bitmap that we have > not marked as IN_USE so it must not be used." > Yes, it does work, but it is a bit more of a stretch, so I'm not quite as sure about using it. >> I understand, that inconsistent bitmaps are some kind of readonly too, as we can't write to them.. >> But we can't read too anyway, and we don't interfere with reopen logic at all. So again, better is >> don't mark inconsistent bitmaps as readonly. >> > > Hm, so in your model; we just NEVER set readonly for persistent bitmaps, > which incurs some changes at load time, but allows the reload_rw > function to go completely unmodified. > > That's not unreasonable in terms of SLOC, but semantically I am not sure > which approach is better. I'm leaning towards keeping this as written > for now, but... well. Any thoughts, Eric? Would you like to flip a coin? No strong preferences, other than let's get it in before soft freeze, to make sure we don't miss out on it entirely due to waiting for the coin flip :)
On 3/7/19 11:37 AM, Eric Blake wrote: > On 3/6/19 3:46 PM, John Snow wrote: > >>> I think, inconsistent bitmaps should have readonly flag always set or always unset, independently on can_write, >>> as we are not going to write something related to inconsistent bitmaps. >>> >> >> We'll never change metadata for inconsistent bitmaps, no. We might >> delete them which causes a metadata change, though -- and right now the >> readonly check stops that in blockdev.c before we attempt (and fail) the >> operation. It does have at least a light usage. >> >> It's not a crucial feature, but I think the error message is nicer. >> >>> And I think, better is always unset, to have inconsistent bitmaps as something unrelated to readonly. >>> >> >> The way I think of it is as unrelated, which is why I set readonly and >> inconsistent orthogonally. >> >>> Readonly are bitmaps, which are not marked IN_USE on open for some reason.. >>> >> >> I tend to think of readonly bitmaps as persistent bitmaps attached to >> storage we are unable to write back to, so we must prevent their >> modification. > > I like that wording (keeping read-only and inconsistent orthogonal), > >> >> I suppose the IN_USE viewpoint works too; "This is a bitmap that we have >> not marked as IN_USE so it must not be used." >> > > Yes, it does work, but it is a bit more of a stretch, so I'm not quite > as sure about using it. > >>> I understand, that inconsistent bitmaps are some kind of readonly too, as we can't write to them.. >>> But we can't read too anyway, and we don't interfere with reopen logic at all. So again, better is >>> don't mark inconsistent bitmaps as readonly. >>> >> >> Hm, so in your model; we just NEVER set readonly for persistent bitmaps, >> which incurs some changes at load time, but allows the reload_rw >> function to go completely unmodified. >> >> That's not unreasonable in terms of SLOC, but semantically I am not sure >> which approach is better. I'm leaning towards keeping this as written >> for now, but... well. Any thoughts, Eric? Would you like to flip a coin? > > No strong preferences, other than let's get it in before soft freeze, to > make sure we don't miss out on it entirely due to waiting for the coin > flip :) > > OK. I have a slight preference for keeping the flag meanings orthogonal and logically consistent, because I believe it keeps the code less surprising to anyone who isn't Eric, Vlad, or myself. So I think I will stage this as-is, and if it poses a problem I will accept counter-proposals during freeze to shore it up if necessary. --js
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index 3ee524da4b..c3b210ede1 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,35 +963,39 @@ 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; - } - - if (!(bm->flags & BME_FLAG_AUTO)) { - bdrv_disable_dirty_bitmap(bitmap); - } - bdrv_dirty_bitmap_set_persistance(bitmap, true); - bm->flags |= BME_FLAG_IN_USE; - created_dirty_bitmaps = - g_slist_append(created_dirty_bitmaps, bitmap); + BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp); + if (bitmap == NULL) { + goto fail; } - } - if (created_dirty_bitmaps != NULL) { - if (can_write(bs)) { - /* in_use flags must be updated */ - int ret = update_ext_header_and_dir_in_place(bs, bm_list); - if (ret < 0) { - error_setg_errno(errp, -ret, "Can't update bitmap directory"); - goto fail; - } - header_updated = true; + if (bm->flags & BME_FLAG_IN_USE) { + bdrv_dirty_bitmap_set_inconsistent(bitmap); } else { - g_slist_foreach(created_dirty_bitmaps, set_readonly_helper, - (gpointer)true); + /* NB: updated flags only get written if can_write(bs) is true. */ + bm->flags |= BME_FLAG_IN_USE; + needs_update = true; } + if (!(bm->flags & BME_FLAG_AUTO)) { + bdrv_disable_dirty_bitmap(bitmap); + } + bdrv_dirty_bitmap_set_persistance(bitmap, true); + created_dirty_bitmaps = + g_slist_append(created_dirty_bitmaps, bitmap); + } + + if (needs_update && can_write(bs)) { + /* in_use flags must be updated */ + int ret = update_ext_header_and_dir_in_place(bs, bm_list); + if (ret < 0) { + error_setg_errno(errp, -ret, "Can't update bitmap directory"); + goto fail; + } + header_updated = true; + } + + if (!can_write(bs)) { + g_slist_foreach(created_dirty_bitmaps, set_readonly_helper, + (gpointer)true); } g_slist_free(created_dirty_bitmaps); @@ -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)) { + 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); + 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; }
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 <jsnow@redhat.com> --- block/qcow2-bitmap.c | 103 ++++++++++++++++++++++--------------------- 1 file changed, 53 insertions(+), 50 deletions(-)