[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 1/6] block: maintain persistent disabled bitm
From: |
John Snow |
Subject: |
Re: [Qemu-devel] [PATCH v2 1/6] block: maintain persistent disabled bitmaps |
Date: |
Fri, 19 Jan 2018 18:43:15 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 |
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 <address@hidden>
> ---
> 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 <address@hidden>
- Re: [Qemu-devel] [PATCH v2 4/6] qmp: transaction support for block-dirty-bitmap-enable/disable, (continued)
[Qemu-devel] [PATCH v2 6/6] qapi: add disabled parameter to block-dirty-bitmap-add, Vladimir Sementsov-Ogievskiy, 2018/01/16
[Qemu-devel] [PATCH v2 5/6] qapi: add block-dirty-bitmap-merge, Vladimir Sementsov-Ogievskiy, 2018/01/16
[Qemu-devel] [PATCH v2 2/6] block/dirty-bitmap: add lock to bdrv_enable/disable_dirty_bitmap, Vladimir Sementsov-Ogievskiy, 2018/01/16
[Qemu-devel] [PATCH v2 3/6] qapi: add block-dirty-bitmap-enable/disable, Vladimir Sementsov-Ogievskiy, 2018/01/16
[Qemu-devel] [PATCH v2 1/6] block: maintain persistent disabled bitmaps, Vladimir Sementsov-Ogievskiy, 2018/01/16
- Re: [Qemu-devel] [PATCH v2 1/6] block: maintain persistent disabled bitmaps,
John Snow <=
Re: [Qemu-devel] [PATCH v2 0/6] qmp dirty bitmap API, John Snow, 2018/01/19