On 01/16/2018 07:54 AM, Vladimir Sementsov-Ogievskiy wrote: > To maintain load/store disabled bitmap there is new approach: > > - deprecate @autoload flag of block-dirty-bitmap-add, make it ignored > - store enabled bitmaps as "auto" to qcow2 > - store disabled bitmaps without "auto" flag to qcow2 > - on qcow2 open load "auto" bitmaps as enabled and others > as disabled (except in_use bitmaps) > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> > --- > qapi/block-core.json | 6 +++--- > block/qcow2.h | 2 +- > include/block/dirty-bitmap.h | 1 - > block/dirty-bitmap.c | 18 ------------------ > block/qcow2-bitmap.c | 12 +++++++----- > block/qcow2.c | 2 +- > blockdev.c | 10 ++-------- > 7 files changed, 14 insertions(+), 37 deletions(-) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index ab96e348e6..827254db22 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -1593,9 +1593,9 @@ > # Qcow2 disks support persistent bitmaps. Default is false for > # block-dirty-bitmap-add. (Since: 2.10) > # > -# @autoload: the bitmap will be automatically loaded when the image it is > stored > -# in is opened. This flag may only be specified for persistent > -# bitmaps. Default is false for block-dirty-bitmap-add. (Since: > 2.10) > +# @autoload: ignored and deprecated since 2.12. > +# Currently, all dirty tracking bitmaps are loaded from Qcow2 on > +# open.
Hmm, so we're going to say that *all* persistent bitmaps are loaded into memory, but they may-or-may-not-be enabled, is that the approach we're taking now? > # > # Since: 2.4 > ## > diff --git a/block/qcow2.h b/block/qcow2.h > index 782a206ecb..a3e29276fc 100644 > --- a/block/qcow2.h > +++ b/block/qcow2.h > @@ -672,7 +672,7 @@ void qcow2_cache_discard(BlockDriverState *bs, Qcow2Cache > *c, void *table); > int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res, > void **refcount_table, > int64_t *refcount_table_size); > -bool qcow2_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error > **errp); > +bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp); > int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp); > void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error > **errp); > int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp); > diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h > index 3579a7597c..144e77a879 100644 > --- a/include/block/dirty-bitmap.h > +++ b/include/block/dirty-bitmap.h > @@ -65,7 +65,6 @@ void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap > *bitmap, > void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap); > > void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap, bool value); > -void bdrv_dirty_bitmap_set_autoload(BdrvDirtyBitmap *bitmap, bool autoload); > void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap, > bool persistent); > > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c > index bd04e991b1..3777be1985 100644 > --- a/block/dirty-bitmap.c > +++ b/block/dirty-bitmap.c > @@ -52,8 +52,6 @@ struct BdrvDirtyBitmap { > Such operations must fail and both the > image > and this bitmap must remain unchanged > while > this flag is set. */ > - bool autoload; /* For persistent bitmaps: bitmap must be > - autoloaded on image opening */ > bool persistent; /* bitmap must be saved to owner disk image > */ > QLIST_ENTRY(BdrvDirtyBitmap) list; > }; > @@ -104,7 +102,6 @@ void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap) > g_free(bitmap->name); > bitmap->name = NULL; > bitmap->persistent = false; > - bitmap->autoload = false; > } > > /* Called with BQL taken. */ > @@ -261,8 +258,6 @@ BdrvDirtyBitmap > *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, > bitmap->successor = NULL; > successor->persistent = bitmap->persistent; > bitmap->persistent = false; > - successor->autoload = bitmap->autoload; > - bitmap->autoload = false; > bdrv_release_dirty_bitmap(bs, bitmap); > > return successor; > @@ -667,19 +662,6 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs) > } > > /* Called with BQL taken. */ > -void bdrv_dirty_bitmap_set_autoload(BdrvDirtyBitmap *bitmap, bool autoload) > -{ > - qemu_mutex_lock(bitmap->mutex); > - bitmap->autoload = autoload; > - qemu_mutex_unlock(bitmap->mutex); > -} > - > -bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap) > -{ > - return bitmap->autoload; > -} > - > -/* Called with BQL taken. */ > void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap, bool > persistent) > { > qemu_mutex_lock(bitmap->mutex); > diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c > index f45e46cfbd..ae14464de6 100644 > --- a/block/qcow2-bitmap.c > +++ b/block/qcow2-bitmap.c > @@ -933,14 +933,14 @@ static void set_readonly_helper(gpointer bitmap, > gpointer value) > bdrv_dirty_bitmap_set_readonly(bitmap, (bool)value); > } > > -/* qcow2_load_autoloading_dirty_bitmaps() > +/* qcow2_load_dirty_bitmaps() > * Return value is a hint for caller: true means that the Qcow2 header was > * updated. (false doesn't mean that the header should be updated by the > * caller, it just means that updating was not needed or the image cannot be > * written to). > * On failure the function returns false. > */ > -bool qcow2_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error **errp) > +bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp) > { > BDRVQcow2State *s = bs->opaque; > Qcow2BitmapList *bm_list; > @@ -960,14 +960,16 @@ bool > qcow2_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error **errp) > } > > QSIMPLEQ_FOREACH(bm, bm_list, entry) { > - if ((bm->flags & BME_FLAG_AUTO) && !(bm->flags & BME_FLAG_IN_USE)) { > + 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); > + } So we're re-using this as the enabled flag, basically. > bdrv_dirty_bitmap_set_persistance(bitmap, true); > - bdrv_dirty_bitmap_set_autoload(bitmap, true); > bm->flags |= BME_FLAG_IN_USE; > created_dirty_bitmaps = > g_slist_append(created_dirty_bitmaps, bitmap); > @@ -1369,7 +1371,7 @@ void > qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) > bm->table.size = 0; > QSIMPLEQ_INSERT_TAIL(&drop_tables, tb, entry); > } > - bm->flags = bdrv_dirty_bitmap_get_autoload(bitmap) ? BME_FLAG_AUTO : > 0; > + bm->flags = bdrv_dirty_bitmap_enabled(bitmap) ? BME_FLAG_AUTO : 0; > bm->granularity_bits = ctz32(bdrv_dirty_bitmap_granularity(bitmap)); > bm->dirty_bitmap = bitmap; > } > diff --git a/block/qcow2.c b/block/qcow2.c > index 92cb9f9bfa..93c3a97cfe 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -1441,7 +1441,7 @@ static int qcow2_do_open(BlockDriverState *bs, QDict > *options, int flags, > s->autoclear_features &= QCOW2_AUTOCLEAR_MASK; > } > > - if (qcow2_load_autoloading_dirty_bitmaps(bs, &local_err)) { > + if (qcow2_load_dirty_bitmaps(bs, &local_err)) { > update_header = false; > } > if (local_err != NULL) { > diff --git a/blockdev.c b/blockdev.c > index 56a6b24a0b..8068cbd606 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -2738,14 +2738,9 @@ void qmp_block_dirty_bitmap_add(const char *node, > const char *name, > if (!has_persistent) { > persistent = false; > } > - if (!has_autoload) { > - autoload = false; > - } > > - if (has_autoload && !persistent) { > - error_setg(errp, "Autoload flag must be used only for persistent " > - "bitmaps"); > - return; > + if (has_autoload) { > + warn_report("Autoload option is deprected and its value is ignored"); "deprecated" > } > > if (persistent && > @@ -2760,7 +2755,6 @@ void qmp_block_dirty_bitmap_add(const char *node, const > char *name, > } > > bdrv_dirty_bitmap_set_persistance(bitmap, persistent); > - bdrv_dirty_bitmap_set_autoload(bitmap, autoload); > } > > void qmp_block_dirty_bitmap_remove(const char *node, const char *name, > Checks out mechanically; I'm not sure yet if we ought to re-use BME_FLAG_AUTO as the enabled flag. I'll get back to that :) With spelling error fixed: Reviewed-by: John Snow <js...@redhat.com>