* [Qemu-devel] [PATCH 00/13] Dirty bitmap changes for migration/persistence work
@ 2016-01-04 10:27 Fam Zheng
2016-01-04 10:27 ` [Qemu-devel] [PATCH 01/13] backup: Use Bitmap to replace "s->bitmap" Fam Zheng
` (13 more replies)
0 siblings, 14 replies; 36+ messages in thread
From: Fam Zheng @ 2016-01-04 10:27 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Vladimir Sementsov-Ogievskiy, jsnow, qemu-block
Two major features are added to block dirty bitmap (and underlying HBitmap) in
this series: meta bitmap and serialization, together with all other supportive
patches.
Both operations are common in dirty bitmap migration and persistence: they need
to find whether and which part of the dirty bitmap in question has changed with
meta dirty bitmap, and they need to write it to the target with serialization.
Fam Zheng (11):
backup: Use Bitmap to replace "s->bitmap"
typedefs: Add BdrvDirtyBitmap and HBitmapIter
block: Move block dirty bitmap code to separate files
block: Remove unused typedef of BlockDriverDirtyHandler
block: Hide HBitmap in block dirty bitmap interface
HBitmap: Introduce "meta" bitmap to track bit changes
tests: Add test code for meta bitmap
block: Support meta dirty bitmap
block: Add two dirty bitmap getters
block: Assert that bdrv_release_dirty_bitmap succeeded
tests: Add test code for hbitmap serialization
Vladimir Sementsov-Ogievskiy (2):
hbitmap: serialization
block: BdrvDirtyBitmap serialization interface
block.c | 339 -----------------------------
block/Makefile.objs | 2 +-
block/backup.c | 25 ++-
block/dirty-bitmap.c | 491 +++++++++++++++++++++++++++++++++++++++++++
block/mirror.c | 14 +-
include/block/block.h | 39 +---
include/block/dirty-bitmap.h | 70 ++++++
include/qemu/hbitmap.h | 84 ++++++++
include/qemu/typedefs.h | 3 +
tests/test-hbitmap.c | 252 ++++++++++++++++++++++
util/hbitmap.c | 197 +++++++++++++++--
11 files changed, 1106 insertions(+), 410 deletions(-)
create mode 100644 block/dirty-bitmap.c
create mode 100644 include/block/dirty-bitmap.h
--
2.4.3
^ permalink raw reply [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH 01/13] backup: Use Bitmap to replace "s->bitmap"
2016-01-04 10:27 [Qemu-devel] [PATCH 00/13] Dirty bitmap changes for migration/persistence work Fam Zheng
@ 2016-01-04 10:27 ` Fam Zheng
2016-01-04 10:27 ` [Qemu-devel] [PATCH 02/13] typedefs: Add BdrvDirtyBitmap and HBitmapIter Fam Zheng
` (12 subsequent siblings)
13 siblings, 0 replies; 36+ messages in thread
From: Fam Zheng @ 2016-01-04 10:27 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Vladimir Sementsov-Ogievskiy, jsnow, qemu-block
"s->bitmap" tracks done sectors, we only check bit states without using any
iterator which HBitmap is good for. Switch to "Bitmap" which is simpler and
more memory efficient.
Meanwhile, rename it to done_bitmap, to reflect the intention.
Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
block/backup.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 705bb77..56ddec0 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -22,6 +22,7 @@
#include "qapi/qmp/qerror.h"
#include "qemu/ratelimit.h"
#include "sysemu/block-backend.h"
+#include "qemu/bitmap.h"
#define BACKUP_CLUSTER_BITS 16
#define BACKUP_CLUSTER_SIZE (1 << BACKUP_CLUSTER_BITS)
@@ -47,7 +48,7 @@ typedef struct BackupBlockJob {
BlockdevOnError on_target_error;
CoRwlock flush_rwlock;
uint64_t sectors_read;
- HBitmap *bitmap;
+ unsigned long *done_bitmap;
QLIST_HEAD(, CowRequest) inflight_reqs;
} BackupBlockJob;
@@ -113,7 +114,7 @@ static int coroutine_fn backup_do_cow(BlockDriverState *bs,
cow_request_begin(&cow_request, job, start, end);
for (; start < end; start++) {
- if (hbitmap_get(job->bitmap, start)) {
+ if (test_bit(start, job->done_bitmap)) {
trace_backup_do_cow_skip(job, start);
continue; /* already copied */
}
@@ -164,7 +165,7 @@ static int coroutine_fn backup_do_cow(BlockDriverState *bs,
goto out;
}
- hbitmap_set(job->bitmap, start, 1);
+ set_bit(start, job->done_bitmap);
/* Publish progress, guest I/O counts as progress too. Note that the
* offset field is an opaque progress value, it is not a disk offset.
@@ -394,7 +395,7 @@ static void coroutine_fn backup_run(void *opaque)
start = 0;
end = DIV_ROUND_UP(job->common.len, BACKUP_CLUSTER_SIZE);
- job->bitmap = hbitmap_alloc(end, 0);
+ job->done_bitmap = bitmap_new(end);
bdrv_set_enable_write_cache(target, true);
if (target->blk) {
@@ -475,7 +476,7 @@ static void coroutine_fn backup_run(void *opaque)
/* wait until pending backup_do_cow() calls have completed */
qemu_co_rwlock_wrlock(&job->flush_rwlock);
qemu_co_rwlock_unlock(&job->flush_rwlock);
- hbitmap_free(job->bitmap);
+ g_free(job->done_bitmap);
if (target->blk) {
blk_iostatus_disable(target->blk);
--
2.4.3
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH 02/13] typedefs: Add BdrvDirtyBitmap and HBitmapIter
2016-01-04 10:27 [Qemu-devel] [PATCH 00/13] Dirty bitmap changes for migration/persistence work Fam Zheng
2016-01-04 10:27 ` [Qemu-devel] [PATCH 01/13] backup: Use Bitmap to replace "s->bitmap" Fam Zheng
@ 2016-01-04 10:27 ` Fam Zheng
2016-01-05 22:14 ` John Snow
2016-01-04 10:27 ` [Qemu-devel] [PATCH 03/13] block: Move block dirty bitmap code to separate files Fam Zheng
` (11 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Fam Zheng @ 2016-01-04 10:27 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Vladimir Sementsov-Ogievskiy, jsnow, qemu-block
Following patches to refactor and move block dirty bitmap code could use this.
Signed-off-by: Fam Zheng <famz@redhat.com>
---
include/qemu/typedefs.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 78fe6e8..e83934e 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -10,6 +10,7 @@ typedef struct AddressSpace AddressSpace;
typedef struct AioContext AioContext;
typedef struct AllwinnerAHCIState AllwinnerAHCIState;
typedef struct AudioState AudioState;
+typedef struct BdrvDirtyBitmap BdrvDirtyBitmap;
typedef struct BlockBackend BlockBackend;
typedef struct BlockBackendRootState BlockBackendRootState;
typedef struct BlockDriverState BlockDriverState;
@@ -28,6 +29,7 @@ typedef struct EventNotifier EventNotifier;
typedef struct FWCfgIoState FWCfgIoState;
typedef struct FWCfgMemState FWCfgMemState;
typedef struct FWCfgState FWCfgState;
+typedef struct HBitmapIter HBitmapIter;
typedef struct HCIInfo HCIInfo;
typedef struct I2CBus I2CBus;
typedef struct I2SCodec I2SCodec;
--
2.4.3
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH 03/13] block: Move block dirty bitmap code to separate files
2016-01-04 10:27 [Qemu-devel] [PATCH 00/13] Dirty bitmap changes for migration/persistence work Fam Zheng
2016-01-04 10:27 ` [Qemu-devel] [PATCH 01/13] backup: Use Bitmap to replace "s->bitmap" Fam Zheng
2016-01-04 10:27 ` [Qemu-devel] [PATCH 02/13] typedefs: Add BdrvDirtyBitmap and HBitmapIter Fam Zheng
@ 2016-01-04 10:27 ` Fam Zheng
2016-01-05 22:32 ` John Snow
2016-01-04 10:27 ` [Qemu-devel] [PATCH 04/13] block: Remove unused typedef of BlockDriverDirtyHandler Fam Zheng
` (10 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Fam Zheng @ 2016-01-04 10:27 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Vladimir Sementsov-Ogievskiy, jsnow, qemu-block
The only change is making bdrv_dirty_bitmap_truncate public. It is used in
block.c.
Signed-off-by: Fam Zheng <famz@redhat.com>
---
block.c | 339 ---------------------------------------
block/Makefile.objs | 2 +-
block/dirty-bitmap.c | 366 +++++++++++++++++++++++++++++++++++++++++++
include/block/block.h | 37 +----
include/block/dirty-bitmap.h | 42 +++++
5 files changed, 410 insertions(+), 376 deletions(-)
create mode 100644 block/dirty-bitmap.c
create mode 100644 include/block/dirty-bitmap.h
diff --git a/block.c b/block.c
index 411edbf..b544190 100644
--- a/block.c
+++ b/block.c
@@ -55,23 +55,6 @@
#include <windows.h>
#endif
-/**
- * A BdrvDirtyBitmap can be in three possible states:
- * (1) successor is NULL and disabled is false: full r/w mode
- * (2) successor is NULL and disabled is true: read only mode ("disabled")
- * (3) successor is set: frozen mode.
- * A frozen bitmap cannot be renamed, deleted, anonymized, cleared, set,
- * or enabled. A frozen bitmap can only abdicate() or reclaim().
- */
-struct BdrvDirtyBitmap {
- HBitmap *bitmap; /* Dirty sector bitmap implementation */
- BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */
- char *name; /* Optional non-empty unique ID */
- int64_t size; /* Size of the bitmap (Number of sectors) */
- bool disabled; /* Bitmap is read-only */
- QLIST_ENTRY(BdrvDirtyBitmap) list;
-};
-
#define NOT_DONE 0x7fffffff /* used while emulated sync operation in progress */
struct BdrvStates bdrv_states = QTAILQ_HEAD_INITIALIZER(bdrv_states);
@@ -87,7 +70,6 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
BlockDriverState *parent,
const BdrvChildRole *child_role, Error **errp);
-static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
/* If non-zero, use only whitelisted block drivers */
static int use_bdrv_whitelist;
@@ -3375,327 +3357,6 @@ void bdrv_lock_medium(BlockDriverState *bs, bool locked)
}
}
-BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, const char *name)
-{
- BdrvDirtyBitmap *bm;
-
- assert(name);
- QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
- if (bm->name && !strcmp(name, bm->name)) {
- return bm;
- }
- }
- return NULL;
-}
-
-void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap)
-{
- assert(!bdrv_dirty_bitmap_frozen(bitmap));
- g_free(bitmap->name);
- bitmap->name = NULL;
-}
-
-BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
- uint32_t granularity,
- const char *name,
- Error **errp)
-{
- int64_t bitmap_size;
- BdrvDirtyBitmap *bitmap;
- uint32_t sector_granularity;
-
- assert((granularity & (granularity - 1)) == 0);
-
- if (name && bdrv_find_dirty_bitmap(bs, name)) {
- error_setg(errp, "Bitmap already exists: %s", name);
- return NULL;
- }
- sector_granularity = granularity >> BDRV_SECTOR_BITS;
- assert(sector_granularity);
- bitmap_size = bdrv_nb_sectors(bs);
- if (bitmap_size < 0) {
- error_setg_errno(errp, -bitmap_size, "could not get length of device");
- errno = -bitmap_size;
- return NULL;
- }
- bitmap = g_new0(BdrvDirtyBitmap, 1);
- bitmap->bitmap = hbitmap_alloc(bitmap_size, ctz32(sector_granularity));
- bitmap->size = bitmap_size;
- bitmap->name = g_strdup(name);
- bitmap->disabled = false;
- QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
- return bitmap;
-}
-
-bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
-{
- return bitmap->successor;
-}
-
-bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
-{
- return !(bitmap->disabled || bitmap->successor);
-}
-
-DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap)
-{
- if (bdrv_dirty_bitmap_frozen(bitmap)) {
- return DIRTY_BITMAP_STATUS_FROZEN;
- } else if (!bdrv_dirty_bitmap_enabled(bitmap)) {
- return DIRTY_BITMAP_STATUS_DISABLED;
- } else {
- return DIRTY_BITMAP_STATUS_ACTIVE;
- }
-}
-
-/**
- * Create a successor bitmap destined to replace this bitmap after an operation.
- * Requires that the bitmap is not frozen and has no successor.
- */
-int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
- BdrvDirtyBitmap *bitmap, Error **errp)
-{
- uint64_t granularity;
- BdrvDirtyBitmap *child;
-
- if (bdrv_dirty_bitmap_frozen(bitmap)) {
- error_setg(errp, "Cannot create a successor for a bitmap that is "
- "currently frozen");
- return -1;
- }
- assert(!bitmap->successor);
-
- /* Create an anonymous successor */
- granularity = bdrv_dirty_bitmap_granularity(bitmap);
- child = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
- if (!child) {
- return -1;
- }
-
- /* Successor will be on or off based on our current state. */
- child->disabled = bitmap->disabled;
-
- /* Install the successor and freeze the parent */
- bitmap->successor = child;
- return 0;
-}
-
-/**
- * For a bitmap with a successor, yield our name to the successor,
- * delete the old bitmap, and return a handle to the new bitmap.
- */
-BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
- BdrvDirtyBitmap *bitmap,
- Error **errp)
-{
- char *name;
- BdrvDirtyBitmap *successor = bitmap->successor;
-
- if (successor == NULL) {
- error_setg(errp, "Cannot relinquish control if "
- "there's no successor present");
- return NULL;
- }
-
- name = bitmap->name;
- bitmap->name = NULL;
- successor->name = name;
- bitmap->successor = NULL;
- bdrv_release_dirty_bitmap(bs, bitmap);
-
- return successor;
-}
-
-/**
- * In cases of failure where we can no longer safely delete the parent,
- * we may wish to re-join the parent and child/successor.
- * The merged parent will be un-frozen, but not explicitly re-enabled.
- */
-BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
- BdrvDirtyBitmap *parent,
- Error **errp)
-{
- BdrvDirtyBitmap *successor = parent->successor;
-
- if (!successor) {
- error_setg(errp, "Cannot reclaim a successor when none is present");
- return NULL;
- }
-
- if (!hbitmap_merge(parent->bitmap, successor->bitmap)) {
- error_setg(errp, "Merging of parent and successor bitmap failed");
- return NULL;
- }
- bdrv_release_dirty_bitmap(bs, successor);
- parent->successor = NULL;
-
- return parent;
-}
-
-/**
- * Truncates _all_ bitmaps attached to a BDS.
- */
-static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
-{
- BdrvDirtyBitmap *bitmap;
- uint64_t size = bdrv_nb_sectors(bs);
-
- QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
- assert(!bdrv_dirty_bitmap_frozen(bitmap));
- hbitmap_truncate(bitmap->bitmap, size);
- bitmap->size = size;
- }
-}
-
-void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
-{
- BdrvDirtyBitmap *bm, *next;
- QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
- if (bm == bitmap) {
- assert(!bdrv_dirty_bitmap_frozen(bm));
- QLIST_REMOVE(bitmap, list);
- hbitmap_free(bitmap->bitmap);
- g_free(bitmap->name);
- g_free(bitmap);
- return;
- }
- }
-}
-
-void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
-{
- assert(!bdrv_dirty_bitmap_frozen(bitmap));
- bitmap->disabled = true;
-}
-
-void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
-{
- assert(!bdrv_dirty_bitmap_frozen(bitmap));
- bitmap->disabled = false;
-}
-
-BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
-{
- BdrvDirtyBitmap *bm;
- BlockDirtyInfoList *list = NULL;
- BlockDirtyInfoList **plist = &list;
-
- QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
- BlockDirtyInfo *info = g_new0(BlockDirtyInfo, 1);
- BlockDirtyInfoList *entry = g_new0(BlockDirtyInfoList, 1);
- info->count = bdrv_get_dirty_count(bm);
- info->granularity = bdrv_dirty_bitmap_granularity(bm);
- info->has_name = !!bm->name;
- info->name = g_strdup(bm->name);
- info->status = bdrv_dirty_bitmap_status(bm);
- entry->value = info;
- *plist = entry;
- plist = &entry->next;
- }
-
- return list;
-}
-
-int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector)
-{
- if (bitmap) {
- return hbitmap_get(bitmap->bitmap, sector);
- } else {
- return 0;
- }
-}
-
-/**
- * Chooses a default granularity based on the existing cluster size,
- * but clamped between [4K, 64K]. Defaults to 64K in the case that there
- * is no cluster size information available.
- */
-uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs)
-{
- BlockDriverInfo bdi;
- uint32_t granularity;
-
- if (bdrv_get_info(bs, &bdi) >= 0 && bdi.cluster_size > 0) {
- granularity = MAX(4096, bdi.cluster_size);
- granularity = MIN(65536, granularity);
- } else {
- granularity = 65536;
- }
-
- return granularity;
-}
-
-uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap)
-{
- return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
-}
-
-void bdrv_dirty_iter_init(BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
-{
- hbitmap_iter_init(hbi, bitmap->bitmap, 0);
-}
-
-void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
- int64_t cur_sector, int nr_sectors)
-{
- assert(bdrv_dirty_bitmap_enabled(bitmap));
- hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
-}
-
-void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
- int64_t cur_sector, int nr_sectors)
-{
- assert(bdrv_dirty_bitmap_enabled(bitmap));
- hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
-}
-
-void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
-{
- assert(bdrv_dirty_bitmap_enabled(bitmap));
- if (!out) {
- hbitmap_reset_all(bitmap->bitmap);
- } else {
- HBitmap *backup = bitmap->bitmap;
- bitmap->bitmap = hbitmap_alloc(bitmap->size,
- hbitmap_granularity(backup));
- *out = backup;
- }
-}
-
-void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in)
-{
- HBitmap *tmp = bitmap->bitmap;
- assert(bdrv_dirty_bitmap_enabled(bitmap));
- bitmap->bitmap = in;
- hbitmap_free(tmp);
-}
-
-void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
- int nr_sectors)
-{
- BdrvDirtyBitmap *bitmap;
- QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
- if (!bdrv_dirty_bitmap_enabled(bitmap)) {
- continue;
- }
- hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
- }
-}
-
-/**
- * Advance an HBitmapIter to an arbitrary offset.
- */
-void bdrv_set_dirty_iter(HBitmapIter *hbi, int64_t offset)
-{
- assert(hbi->hb);
- hbitmap_iter_init(hbi, hbi->hb, offset);
-}
-
-int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
-{
- return hbitmap_count(bitmap->bitmap);
-}
-
/* Get a reference to bs */
void bdrv_ref(BlockDriverState *bs)
{
diff --git a/block/Makefile.objs b/block/Makefile.objs
index 58ef2ef..cdd8655 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -20,7 +20,7 @@ block-obj-$(CONFIG_RBD) += rbd.o
block-obj-$(CONFIG_GLUSTERFS) += gluster.o
block-obj-$(CONFIG_ARCHIPELAGO) += archipelago.o
block-obj-$(CONFIG_LIBSSH2) += ssh.o
-block-obj-y += accounting.o
+block-obj-y += accounting.o dirty-bitmap.o
block-obj-y += write-threshold.o
common-obj-y += stream.o
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
new file mode 100644
index 0000000..7924c38
--- /dev/null
+++ b/block/dirty-bitmap.c
@@ -0,0 +1,366 @@
+/*
+ * Block Dirty Bitmap
+ *
+ * Copyright (c) 2016 Red Hat. Inc
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+#include "config-host.h"
+#include "qemu-common.h"
+#include "trace.h"
+#include "block/block_int.h"
+#include "block/blockjob.h"
+
+/**
+ * A BdrvDirtyBitmap can be in three possible states:
+ * (1) successor is NULL and disabled is false: full r/w mode
+ * (2) successor is NULL and disabled is true: read only mode ("disabled")
+ * (3) successor is set: frozen mode.
+ * A frozen bitmap cannot be renamed, deleted, anonymized, cleared, set,
+ * or enabled. A frozen bitmap can only abdicate() or reclaim().
+ */
+struct BdrvDirtyBitmap {
+ HBitmap *bitmap; /* Dirty sector bitmap implementation */
+ BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */
+ char *name; /* Optional non-empty unique ID */
+ int64_t size; /* Size of the bitmap (Number of sectors) */
+ bool disabled; /* Bitmap is read-only */
+ QLIST_ENTRY(BdrvDirtyBitmap) list;
+};
+
+BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, const char *name)
+{
+ BdrvDirtyBitmap *bm;
+
+ assert(name);
+ QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
+ if (bm->name && !strcmp(name, bm->name)) {
+ return bm;
+ }
+ }
+ return NULL;
+}
+
+void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap)
+{
+ assert(!bdrv_dirty_bitmap_frozen(bitmap));
+ g_free(bitmap->name);
+ bitmap->name = NULL;
+}
+
+BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
+ uint32_t granularity,
+ const char *name,
+ Error **errp)
+{
+ int64_t bitmap_size;
+ BdrvDirtyBitmap *bitmap;
+ uint32_t sector_granularity;
+
+ assert((granularity & (granularity - 1)) == 0);
+
+ if (name && bdrv_find_dirty_bitmap(bs, name)) {
+ error_setg(errp, "Bitmap already exists: %s", name);
+ return NULL;
+ }
+ sector_granularity = granularity >> BDRV_SECTOR_BITS;
+ assert(sector_granularity);
+ bitmap_size = bdrv_nb_sectors(bs);
+ if (bitmap_size < 0) {
+ error_setg_errno(errp, -bitmap_size, "could not get length of device");
+ errno = -bitmap_size;
+ return NULL;
+ }
+ bitmap = g_new0(BdrvDirtyBitmap, 1);
+ bitmap->bitmap = hbitmap_alloc(bitmap_size, ctz32(sector_granularity));
+ bitmap->size = bitmap_size;
+ bitmap->name = g_strdup(name);
+ bitmap->disabled = false;
+ QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
+ return bitmap;
+}
+
+bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
+{
+ return bitmap->successor;
+}
+
+bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
+{
+ return !(bitmap->disabled || bitmap->successor);
+}
+
+DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap)
+{
+ if (bdrv_dirty_bitmap_frozen(bitmap)) {
+ return DIRTY_BITMAP_STATUS_FROZEN;
+ } else if (!bdrv_dirty_bitmap_enabled(bitmap)) {
+ return DIRTY_BITMAP_STATUS_DISABLED;
+ } else {
+ return DIRTY_BITMAP_STATUS_ACTIVE;
+ }
+}
+
+/**
+ * Create a successor bitmap destined to replace this bitmap after an operation.
+ * Requires that the bitmap is not frozen and has no successor.
+ */
+int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap, Error **errp)
+{
+ uint64_t granularity;
+ BdrvDirtyBitmap *child;
+
+ if (bdrv_dirty_bitmap_frozen(bitmap)) {
+ error_setg(errp, "Cannot create a successor for a bitmap that is "
+ "currently frozen");
+ return -1;
+ }
+ assert(!bitmap->successor);
+
+ /* Create an anonymous successor */
+ granularity = bdrv_dirty_bitmap_granularity(bitmap);
+ child = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
+ if (!child) {
+ return -1;
+ }
+
+ /* Successor will be on or off based on our current state. */
+ child->disabled = bitmap->disabled;
+
+ /* Install the successor and freeze the parent */
+ bitmap->successor = child;
+ return 0;
+}
+
+/**
+ * For a bitmap with a successor, yield our name to the successor,
+ * delete the old bitmap, and return a handle to the new bitmap.
+ */
+BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap,
+ Error **errp)
+{
+ char *name;
+ BdrvDirtyBitmap *successor = bitmap->successor;
+
+ if (successor == NULL) {
+ error_setg(errp, "Cannot relinquish control if "
+ "there's no successor present");
+ return NULL;
+ }
+
+ name = bitmap->name;
+ bitmap->name = NULL;
+ successor->name = name;
+ bitmap->successor = NULL;
+ bdrv_release_dirty_bitmap(bs, bitmap);
+
+ return successor;
+}
+
+/**
+ * In cases of failure where we can no longer safely delete the parent,
+ * we may wish to re-join the parent and child/successor.
+ * The merged parent will be un-frozen, but not explicitly re-enabled.
+ */
+BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
+ BdrvDirtyBitmap *parent,
+ Error **errp)
+{
+ BdrvDirtyBitmap *successor = parent->successor;
+
+ if (!successor) {
+ error_setg(errp, "Cannot reclaim a successor when none is present");
+ return NULL;
+ }
+
+ if (!hbitmap_merge(parent->bitmap, successor->bitmap)) {
+ error_setg(errp, "Merging of parent and successor bitmap failed");
+ return NULL;
+ }
+ bdrv_release_dirty_bitmap(bs, successor);
+ parent->successor = NULL;
+
+ return parent;
+}
+
+/**
+ * Truncates _all_ bitmaps attached to a BDS.
+ */
+void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
+{
+ BdrvDirtyBitmap *bitmap;
+ uint64_t size = bdrv_nb_sectors(bs);
+
+ QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
+ assert(!bdrv_dirty_bitmap_frozen(bitmap));
+ hbitmap_truncate(bitmap->bitmap, size);
+ bitmap->size = size;
+ }
+}
+
+void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+{
+ BdrvDirtyBitmap *bm, *next;
+ QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
+ if (bm == bitmap) {
+ assert(!bdrv_dirty_bitmap_frozen(bm));
+ QLIST_REMOVE(bitmap, list);
+ hbitmap_free(bitmap->bitmap);
+ g_free(bitmap->name);
+ g_free(bitmap);
+ return;
+ }
+ }
+}
+
+void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
+{
+ assert(!bdrv_dirty_bitmap_frozen(bitmap));
+ bitmap->disabled = true;
+}
+
+void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
+{
+ assert(!bdrv_dirty_bitmap_frozen(bitmap));
+ bitmap->disabled = false;
+}
+
+BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
+{
+ BdrvDirtyBitmap *bm;
+ BlockDirtyInfoList *list = NULL;
+ BlockDirtyInfoList **plist = &list;
+
+ QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
+ BlockDirtyInfo *info = g_new0(BlockDirtyInfo, 1);
+ BlockDirtyInfoList *entry = g_new0(BlockDirtyInfoList, 1);
+ info->count = bdrv_get_dirty_count(bm);
+ info->granularity = bdrv_dirty_bitmap_granularity(bm);
+ info->has_name = !!bm->name;
+ info->name = g_strdup(bm->name);
+ info->status = bdrv_dirty_bitmap_status(bm);
+ entry->value = info;
+ *plist = entry;
+ plist = &entry->next;
+ }
+
+ return list;
+}
+
+int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector)
+{
+ if (bitmap) {
+ return hbitmap_get(bitmap->bitmap, sector);
+ } else {
+ return 0;
+ }
+}
+
+/**
+ * Chooses a default granularity based on the existing cluster size,
+ * but clamped between [4K, 64K]. Defaults to 64K in the case that there
+ * is no cluster size information available.
+ */
+uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs)
+{
+ BlockDriverInfo bdi;
+ uint32_t granularity;
+
+ if (bdrv_get_info(bs, &bdi) >= 0 && bdi.cluster_size > 0) {
+ granularity = MAX(4096, bdi.cluster_size);
+ granularity = MIN(65536, granularity);
+ } else {
+ granularity = 65536;
+ }
+
+ return granularity;
+}
+
+uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap)
+{
+ return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
+}
+
+void bdrv_dirty_iter_init(BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
+{
+ hbitmap_iter_init(hbi, bitmap->bitmap, 0);
+}
+
+void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
+ int64_t cur_sector, int nr_sectors)
+{
+ assert(bdrv_dirty_bitmap_enabled(bitmap));
+ hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
+}
+
+void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
+ int64_t cur_sector, int nr_sectors)
+{
+ assert(bdrv_dirty_bitmap_enabled(bitmap));
+ hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
+}
+
+void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
+{
+ assert(bdrv_dirty_bitmap_enabled(bitmap));
+ if (!out) {
+ hbitmap_reset_all(bitmap->bitmap);
+ } else {
+ HBitmap *backup = bitmap->bitmap;
+ bitmap->bitmap = hbitmap_alloc(bitmap->size,
+ hbitmap_granularity(backup));
+ *out = backup;
+ }
+}
+
+void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in)
+{
+ HBitmap *tmp = bitmap->bitmap;
+ assert(bdrv_dirty_bitmap_enabled(bitmap));
+ bitmap->bitmap = in;
+ hbitmap_free(tmp);
+}
+
+void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
+ int nr_sectors)
+{
+ BdrvDirtyBitmap *bitmap;
+ QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
+ if (!bdrv_dirty_bitmap_enabled(bitmap)) {
+ continue;
+ }
+ hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
+ }
+}
+
+/**
+ * Advance an HBitmapIter to an arbitrary offset.
+ */
+void bdrv_set_dirty_iter(HBitmapIter *hbi, int64_t offset)
+{
+ assert(hbi->hb);
+ hbitmap_iter_init(hbi, hbi->hb, offset);
+}
+
+int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
+{
+ return hbitmap_count(bitmap->bitmap);
+}
diff --git a/include/block/block.h b/include/block/block.h
index db8e096..97e9b5e 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -6,6 +6,7 @@
#include "qemu/option.h"
#include "qemu/coroutine.h"
#include "block/accounting.h"
+#include "block/dirty-bitmap.h"
#include "qapi/qmp/qobject.h"
#include "qapi-types.h"
@@ -471,42 +472,6 @@ void *qemu_try_blockalign(BlockDriverState *bs, size_t size);
void *qemu_try_blockalign0(BlockDriverState *bs, size_t size);
bool bdrv_qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov);
-struct HBitmapIter;
-typedef struct BdrvDirtyBitmap BdrvDirtyBitmap;
-BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
- uint32_t granularity,
- const char *name,
- Error **errp);
-int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
- BdrvDirtyBitmap *bitmap,
- Error **errp);
-BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
- BdrvDirtyBitmap *bitmap,
- Error **errp);
-BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
- BdrvDirtyBitmap *bitmap,
- Error **errp);
-BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
- const char *name);
-void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap);
-void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
-void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
-void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
-BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
-uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
-uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap);
-bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
-bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap);
-DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap);
-int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector);
-void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
- int64_t cur_sector, int nr_sectors);
-void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
- int64_t cur_sector, int nr_sectors);
-void bdrv_dirty_iter_init(BdrvDirtyBitmap *bitmap, struct HBitmapIter *hbi);
-void bdrv_set_dirty_iter(struct HBitmapIter *hbi, int64_t offset);
-int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
-
void bdrv_enable_copy_on_read(BlockDriverState *bs);
void bdrv_disable_copy_on_read(BlockDriverState *bs);
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
new file mode 100644
index 0000000..6175cf3
--- /dev/null
+++ b/include/block/dirty-bitmap.h
@@ -0,0 +1,42 @@
+#ifndef BLOCK_DIRTY_BITMAP_H
+#define BLOCK_DIRTY_BITMAP_H
+
+#include "qemu-common.h"
+
+typedef struct BdrvDirtyBitmap BdrvDirtyBitmap;
+BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
+ uint32_t granularity,
+ const char *name,
+ Error **errp);
+int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap,
+ Error **errp);
+BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap,
+ Error **errp);
+BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap,
+ Error **errp);
+BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
+ const char *name);
+void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap);
+void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
+void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
+void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
+BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
+uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
+uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap);
+bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
+bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap);
+DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap);
+int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector);
+void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
+ int64_t cur_sector, int nr_sectors);
+void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
+ int64_t cur_sector, int nr_sectors);
+void bdrv_dirty_iter_init(BdrvDirtyBitmap *bitmap, struct HBitmapIter *hbi);
+void bdrv_set_dirty_iter(struct HBitmapIter *hbi, int64_t offset);
+int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
+void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
+
+#endif
--
2.4.3
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH 04/13] block: Remove unused typedef of BlockDriverDirtyHandler
2016-01-04 10:27 [Qemu-devel] [PATCH 00/13] Dirty bitmap changes for migration/persistence work Fam Zheng
` (2 preceding siblings ...)
2016-01-04 10:27 ` [Qemu-devel] [PATCH 03/13] block: Move block dirty bitmap code to separate files Fam Zheng
@ 2016-01-04 10:27 ` Fam Zheng
2016-01-05 22:35 ` John Snow
2016-01-04 10:27 ` [Qemu-devel] [PATCH 05/13] block: Hide HBitmap in block dirty bitmap interface Fam Zheng
` (9 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Fam Zheng @ 2016-01-04 10:27 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Vladimir Sementsov-Ogievskiy, jsnow, qemu-block
Signed-off-by: Fam Zheng <famz@redhat.com>
---
include/block/block.h | 2 --
1 file changed, 2 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 97e9b5e..0f42964 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -320,8 +320,6 @@ BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs,
const char *node_name, Error **errp);
/* async block I/O */
-typedef void BlockDriverDirtyHandler(BlockDriverState *bs, int64_t sector,
- int sector_num);
BlockAIOCB *bdrv_aio_readv(BlockDriverState *bs, int64_t sector_num,
QEMUIOVector *iov, int nb_sectors,
BlockCompletionFunc *cb, void *opaque);
--
2.4.3
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH 05/13] block: Hide HBitmap in block dirty bitmap interface
2016-01-04 10:27 [Qemu-devel] [PATCH 00/13] Dirty bitmap changes for migration/persistence work Fam Zheng
` (3 preceding siblings ...)
2016-01-04 10:27 ` [Qemu-devel] [PATCH 04/13] block: Remove unused typedef of BlockDriverDirtyHandler Fam Zheng
@ 2016-01-04 10:27 ` Fam Zheng
2016-01-05 23:01 ` John Snow
2016-01-04 10:27 ` [Qemu-devel] [PATCH 06/13] HBitmap: Introduce "meta" bitmap to track bit changes Fam Zheng
` (8 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Fam Zheng @ 2016-01-04 10:27 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Vladimir Sementsov-Ogievskiy, jsnow, qemu-block
HBitmap is an implementation detail of block dirty bitmap that should be hidden
from users. Introduce a BdrvDirtyBitmapIter to encapsulate the underlying
HBitmapIter.
A small difference in the interface is, before, an HBitmapIter is initialized
in place, now the new BdrvDirtyBitmapIter must be dynamically allocated because
the structure definition is in block.c.
Two current users are converted too.
Signed-off-by: Fam Zheng <famz@redhat.com>
---
block/backup.c | 14 ++++++++------
block/dirty-bitmap.c | 36 +++++++++++++++++++++++++++++++-----
block/mirror.c | 14 ++++++++------
include/block/dirty-bitmap.h | 7 +++++--
include/qemu/typedefs.h | 1 +
5 files changed, 53 insertions(+), 19 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 56ddec0..2388039 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -326,14 +326,14 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
int64_t end;
int64_t last_cluster = -1;
BlockDriverState *bs = job->common.bs;
- HBitmapIter hbi;
+ BdrvDirtyBitmapIter *dbi;
granularity = bdrv_dirty_bitmap_granularity(job->sync_bitmap);
clusters_per_iter = MAX((granularity / BACKUP_CLUSTER_SIZE), 1);
- bdrv_dirty_iter_init(job->sync_bitmap, &hbi);
+ dbi = bdrv_dirty_iter_new(job->sync_bitmap, 0);
/* Find the next dirty sector(s) */
- while ((sector = hbitmap_iter_next(&hbi)) != -1) {
+ while ((sector = bdrv_dirty_iter_next(dbi)) != -1) {
cluster = sector / BACKUP_SECTORS_PER_CLUSTER;
/* Fake progress updates for any clusters we skipped */
@@ -345,7 +345,7 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
for (end = cluster + clusters_per_iter; cluster < end; cluster++) {
do {
if (yield_and_check(job)) {
- return ret;
+ goto out;
}
ret = backup_do_cow(bs, cluster * BACKUP_SECTORS_PER_CLUSTER,
BACKUP_SECTORS_PER_CLUSTER, &error_is_read,
@@ -353,7 +353,7 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
if ((ret < 0) &&
backup_error_action(job, error_is_read, -ret) ==
BLOCK_ERROR_ACTION_REPORT) {
- return ret;
+ goto out;
}
} while (ret < 0);
}
@@ -361,7 +361,7 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
/* If the bitmap granularity is smaller than the backup granularity,
* we need to advance the iterator pointer to the next cluster. */
if (granularity < BACKUP_CLUSTER_SIZE) {
- bdrv_set_dirty_iter(&hbi, cluster * BACKUP_SECTORS_PER_CLUSTER);
+ bdrv_set_dirty_iter(dbi, cluster * BACKUP_SECTORS_PER_CLUSTER);
}
last_cluster = cluster - 1;
@@ -373,6 +373,8 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
job->common.offset += ((end - last_cluster - 1) * BACKUP_CLUSTER_SIZE);
}
+out:
+ bdrv_dirty_iter_free(dbi);
return ret;
}
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 7924c38..53cf88d 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -41,9 +41,15 @@ struct BdrvDirtyBitmap {
char *name; /* Optional non-empty unique ID */
int64_t size; /* Size of the bitmap (Number of sectors) */
bool disabled; /* Bitmap is read-only */
+ int active_iterators; /* How many iterators are active */
QLIST_ENTRY(BdrvDirtyBitmap) list;
};
+struct BdrvDirtyBitmapIter {
+ HBitmapIter hbi;
+ BdrvDirtyBitmap *bitmap;
+};
+
BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, const char *name)
{
BdrvDirtyBitmap *bm;
@@ -221,6 +227,7 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
BdrvDirtyBitmap *bm, *next;
QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
if (bm == bitmap) {
+ assert(!bitmap->active_iterators);
assert(!bdrv_dirty_bitmap_frozen(bm));
QLIST_REMOVE(bitmap, list);
hbitmap_free(bitmap->bitmap);
@@ -299,9 +306,29 @@ uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap)
return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
}
-void bdrv_dirty_iter_init(BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
+BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap,
+ uint64_t first_sector)
{
- hbitmap_iter_init(hbi, bitmap->bitmap, 0);
+ BdrvDirtyBitmapIter *iter = g_new(BdrvDirtyBitmapIter, 1);
+ hbitmap_iter_init(&iter->hbi, bitmap->bitmap, first_sector);
+ iter->bitmap = bitmap;
+ bitmap->active_iterators++;
+ return iter;
+}
+
+void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter)
+{
+ if (!iter) {
+ return;
+ }
+ assert(iter->bitmap->active_iterators > 0);
+ iter->bitmap->active_iterators--;
+ g_free(iter);
+}
+
+int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter)
+{
+ return hbitmap_iter_next(&iter->hbi);
}
void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
@@ -354,10 +381,9 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
/**
* Advance an HBitmapIter to an arbitrary offset.
*/
-void bdrv_set_dirty_iter(HBitmapIter *hbi, int64_t offset)
+void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *iter, int64_t sector_num)
{
- assert(hbi->hb);
- hbitmap_iter_init(hbi, hbi->hb, offset);
+ hbitmap_iter_init(&iter->hbi, iter->bitmap->bitmap, sector_num);
}
int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
diff --git a/block/mirror.c b/block/mirror.c
index f201f2b..70ca844 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -52,7 +52,7 @@ typedef struct MirrorBlockJob {
int64_t bdev_length;
unsigned long *cow_bitmap;
BdrvDirtyBitmap *dirty_bitmap;
- HBitmapIter hbi;
+ BdrvDirtyBitmapIter *dbi;
uint8_t *buf;
QSIMPLEQ_HEAD(, MirrorBuffer) buf_free;
int buf_free_count;
@@ -170,10 +170,11 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
max_iov = MIN(source->bl.max_iov, s->target->bl.max_iov);
- s->sector_num = hbitmap_iter_next(&s->hbi);
+ s->sector_num = bdrv_dirty_iter_next(s->dbi);
if (s->sector_num < 0) {
- bdrv_dirty_iter_init(s->dirty_bitmap, &s->hbi);
- s->sector_num = hbitmap_iter_next(&s->hbi);
+ bdrv_dirty_iter_free(s->dbi);
+ s->dbi = bdrv_dirty_iter_new(s->dirty_bitmap, 0);
+ s->sector_num = bdrv_dirty_iter_next(s->dbi);
trace_mirror_restart_iter(s, bdrv_get_dirty_count(s->dirty_bitmap));
assert(s->sector_num >= 0);
}
@@ -291,7 +292,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
*/
if (next_sector > hbitmap_next_sector
&& bdrv_get_dirty(source, s->dirty_bitmap, next_sector)) {
- hbitmap_next_sector = hbitmap_iter_next(&s->hbi);
+ hbitmap_next_sector = bdrv_dirty_iter_next(s->dbi);
}
next_sector += sectors_per_chunk;
@@ -502,7 +503,7 @@ static void coroutine_fn mirror_run(void *opaque)
}
}
- bdrv_dirty_iter_init(s->dirty_bitmap, &s->hbi);
+ s->dbi = bdrv_dirty_iter_new(s->dirty_bitmap, 0);
for (;;) {
uint64_t delay_ns = 0;
int64_t cnt;
@@ -615,6 +616,7 @@ immediate_exit:
qemu_vfree(s->buf);
g_free(s->cow_bitmap);
g_free(s->in_flight_bitmap);
+ bdrv_dirty_iter_free(s->dbi);
bdrv_release_dirty_bitmap(bs, s->dirty_bitmap);
if (s->target->blk) {
blk_iostatus_disable(s->target->blk);
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 6175cf3..16bb15a 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -34,8 +34,11 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors);
void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors);
-void bdrv_dirty_iter_init(BdrvDirtyBitmap *bitmap, struct HBitmapIter *hbi);
-void bdrv_set_dirty_iter(struct HBitmapIter *hbi, int64_t offset);
+BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap,
+ uint64_t first_sector);
+void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter);
+int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter);
+void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t sector_num);
int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index e83934e..0e9efcd 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -11,6 +11,7 @@ typedef struct AioContext AioContext;
typedef struct AllwinnerAHCIState AllwinnerAHCIState;
typedef struct AudioState AudioState;
typedef struct BdrvDirtyBitmap BdrvDirtyBitmap;
+typedef struct BdrvDirtyBitmapIter BdrvDirtyBitmapIter;
typedef struct BlockBackend BlockBackend;
typedef struct BlockBackendRootState BlockBackendRootState;
typedef struct BlockDriverState BlockDriverState;
--
2.4.3
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH 06/13] HBitmap: Introduce "meta" bitmap to track bit changes
2016-01-04 10:27 [Qemu-devel] [PATCH 00/13] Dirty bitmap changes for migration/persistence work Fam Zheng
` (4 preceding siblings ...)
2016-01-04 10:27 ` [Qemu-devel] [PATCH 05/13] block: Hide HBitmap in block dirty bitmap interface Fam Zheng
@ 2016-01-04 10:27 ` Fam Zheng
2016-01-06 0:09 ` John Snow
2016-01-11 15:40 ` Vladimir Sementsov-Ogievskiy
2016-01-04 10:27 ` [Qemu-devel] [PATCH 07/13] tests: Add test code for meta bitmap Fam Zheng
` (7 subsequent siblings)
13 siblings, 2 replies; 36+ messages in thread
From: Fam Zheng @ 2016-01-04 10:27 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Vladimir Sementsov-Ogievskiy, jsnow, qemu-block
Upon each bit toggle, the corresponding bit in the meta bitmap will be
set.
Signed-off-by: Fam Zheng <famz@redhat.com>
---
include/qemu/hbitmap.h | 8 +++++++
util/hbitmap.c | 61 +++++++++++++++++++++++++++++++++++++-------------
2 files changed, 54 insertions(+), 15 deletions(-)
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index bb94a00..ed672e7 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -181,6 +181,14 @@ void hbitmap_iter_init(HBitmapIter *hbi, const HBitmap *hb, uint64_t first);
*/
unsigned long hbitmap_iter_skip_words(HBitmapIter *hbi);
+/* hbitmap_create_meta
+ * Create a "meta" hbitmap to track dirtiness of the bits in this HBitmap.
+ *
+ * @hb: The HBitmap to operate on.
+ * @chunk_size: How many bits in @hb does one bit in the meta track.
+ */
+HBitmap *hbitmap_create_meta(HBitmap *hb, int chunk_size);
+
/**
* hbitmap_iter_next:
* @hbi: HBitmapIter to operate on.
diff --git a/util/hbitmap.c b/util/hbitmap.c
index 50b888f..55d3182 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -81,6 +81,9 @@ struct HBitmap {
*/
int granularity;
+ /* A meta dirty bitmap to track the dirtiness of bits in this HBitmap. */
+ HBitmap *meta;
+
/* A number of progressively less coarse bitmaps (i.e. level 0 is the
* coarsest). Each bit in level N represents a word in level N+1 that
* has a set bit, except the last level where each bit represents the
@@ -212,25 +215,27 @@ static uint64_t hb_count_between(HBitmap *hb, uint64_t start, uint64_t last)
}
/* Setting starts at the last layer and propagates up if an element
- * changes from zero to non-zero.
+ * changes.
*/
static inline bool hb_set_elem(unsigned long *elem, uint64_t start, uint64_t last)
{
unsigned long mask;
- bool changed;
+ unsigned long old;
assert((last >> BITS_PER_LEVEL) == (start >> BITS_PER_LEVEL));
assert(start <= last);
mask = 2UL << (last & (BITS_PER_LONG - 1));
mask -= 1UL << (start & (BITS_PER_LONG - 1));
- changed = (*elem == 0);
+ old = *elem;
*elem |= mask;
- return changed;
+ return old != *elem;
}
-/* The recursive workhorse (the depth is limited to HBITMAP_LEVELS)... */
-static void hb_set_between(HBitmap *hb, int level, uint64_t start, uint64_t last)
+/* The recursive workhorse (the depth is limited to HBITMAP_LEVELS)...
+ * Returns true if at least one bit is changed. */
+static bool hb_set_between(HBitmap *hb, int level, uint64_t start,
+ uint64_t last)
{
size_t pos = start >> BITS_PER_LEVEL;
size_t lastpos = last >> BITS_PER_LEVEL;
@@ -259,22 +264,27 @@ static void hb_set_between(HBitmap *hb, int level, uint64_t start, uint64_t last
if (level > 0 && changed) {
hb_set_between(hb, level - 1, pos, lastpos);
}
+ return changed;
}
void hbitmap_set(HBitmap *hb, uint64_t start, uint64_t count)
{
/* Compute range in the last layer. */
+ uint64_t first, n;
uint64_t last = start + count - 1;
trace_hbitmap_set(hb, start, count,
start >> hb->granularity, last >> hb->granularity);
- start >>= hb->granularity;
+ first = start >> hb->granularity;
last >>= hb->granularity;
- count = last - start + 1;
+ n = last - first + 1;
- hb->count += count - hb_count_between(hb, start, last);
- hb_set_between(hb, HBITMAP_LEVELS - 1, start, last);
+ hb->count += n - hb_count_between(hb, first, last);
+ if (hb_set_between(hb, HBITMAP_LEVELS - 1, first, last) &&
+ hb->meta) {
+ hbitmap_set(hb->meta, start, count);
+ }
}
/* Resetting works the other way round: propagate up if the new
@@ -295,8 +305,10 @@ static inline bool hb_reset_elem(unsigned long *elem, uint64_t start, uint64_t l
return blanked;
}
-/* The recursive workhorse (the depth is limited to HBITMAP_LEVELS)... */
-static void hb_reset_between(HBitmap *hb, int level, uint64_t start, uint64_t last)
+/* The recursive workhorse (the depth is limited to HBITMAP_LEVELS)...
+ * Returns true if at least one bit is changed. */
+static bool hb_reset_between(HBitmap *hb, int level, uint64_t start,
+ uint64_t last)
{
size_t pos = start >> BITS_PER_LEVEL;
size_t lastpos = last >> BITS_PER_LEVEL;
@@ -339,21 +351,28 @@ static void hb_reset_between(HBitmap *hb, int level, uint64_t start, uint64_t la
if (level > 0 && changed) {
hb_reset_between(hb, level - 1, pos, lastpos);
}
+
+ return changed;
+
}
void hbitmap_reset(HBitmap *hb, uint64_t start, uint64_t count)
{
/* Compute range in the last layer. */
+ uint64_t first;
uint64_t last = start + count - 1;
trace_hbitmap_reset(hb, start, count,
start >> hb->granularity, last >> hb->granularity);
- start >>= hb->granularity;
+ first = start >> hb->granularity;
last >>= hb->granularity;
- hb->count -= hb_count_between(hb, start, last);
- hb_reset_between(hb, HBITMAP_LEVELS - 1, start, last);
+ hb->count -= hb_count_between(hb, first, last);
+ if (hb_reset_between(hb, HBITMAP_LEVELS - 1, first, last) &&
+ hb->meta) {
+ hbitmap_set(hb->meta, start, count);
+ }
}
void hbitmap_reset_all(HBitmap *hb)
@@ -384,6 +403,9 @@ void hbitmap_free(HBitmap *hb)
for (i = HBITMAP_LEVELS; i-- > 0; ) {
g_free(hb->levels[i]);
}
+ if (hb->meta) {
+ hbitmap_free(hb->meta);
+ }
g_free(hb);
}
@@ -493,3 +515,12 @@ bool hbitmap_merge(HBitmap *a, const HBitmap *b)
return true;
}
+
+HBitmap *hbitmap_create_meta(HBitmap *hb, int chunk_size)
+{
+ assert(!(chunk_size & (chunk_size - 1)));
+ assert(!hb->meta);
+ hb->meta = hbitmap_alloc(hb->size << hb->granularity,
+ hb->granularity + ctz32(chunk_size));
+ return hb->meta;
+}
--
2.4.3
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH 07/13] tests: Add test code for meta bitmap
2016-01-04 10:27 [Qemu-devel] [PATCH 00/13] Dirty bitmap changes for migration/persistence work Fam Zheng
` (5 preceding siblings ...)
2016-01-04 10:27 ` [Qemu-devel] [PATCH 06/13] HBitmap: Introduce "meta" bitmap to track bit changes Fam Zheng
@ 2016-01-04 10:27 ` Fam Zheng
2016-01-06 20:46 ` John Snow
2016-01-04 10:27 ` [Qemu-devel] [PATCH 08/13] block: Support meta dirty bitmap Fam Zheng
` (6 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Fam Zheng @ 2016-01-04 10:27 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Vladimir Sementsov-Ogievskiy, jsnow, qemu-block
Signed-off-by: Fam Zheng <famz@redhat.com>
---
tests/test-hbitmap.c | 113 +++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 113 insertions(+)
diff --git a/tests/test-hbitmap.c b/tests/test-hbitmap.c
index abcea0c..19136e7 100644
--- a/tests/test-hbitmap.c
+++ b/tests/test-hbitmap.c
@@ -14,6 +14,7 @@
#include <string.h>
#include <sys/types.h>
#include "qemu/hbitmap.h"
+#include "block/block.h"
#define LOG_BITS_PER_LONG (BITS_PER_LONG == 32 ? 5 : 6)
@@ -23,6 +24,7 @@
typedef struct TestHBitmapData {
HBitmap *hb;
+ HBitmap *meta;
unsigned long *bits;
size_t size;
size_t old_size;
@@ -94,6 +96,14 @@ static void hbitmap_test_init(TestHBitmapData *data,
}
}
+static void hbitmap_test_init_meta(TestHBitmapData *data,
+ uint64_t size, int granularity,
+ int meta_chunk)
+{
+ hbitmap_test_init(data, size, granularity);
+ data->meta = hbitmap_create_meta(data->hb, meta_chunk);
+}
+
static inline size_t hbitmap_test_array_size(size_t bits)
{
size_t n = (bits + BITS_PER_LONG - 1) / BITS_PER_LONG;
@@ -637,6 +647,103 @@ static void test_hbitmap_truncate_shrink_large(TestHBitmapData *data,
hbitmap_test_truncate(data, size, -diff, 0);
}
+static void hbitmap_check_meta(TestHBitmapData *data,
+ int64_t start, int count)
+{
+ int64_t i;
+
+ for (i = 0; i < data->size; i++) {
+ if (i >= start && i < start + count) {
+ g_assert(hbitmap_get(data->meta, i));
+ } else {
+ g_assert(!hbitmap_get(data->meta, i));
+ }
+ }
+}
+
+static void hbitmap_test_meta(TestHBitmapData *data,
+ int64_t start, int count,
+ int64_t check_start, int check_count)
+{
+ hbitmap_reset_all(data->hb);
+ hbitmap_reset_all(data->meta);
+
+ /* Test "unset" -> "unset" will not update meta. */
+ hbitmap_reset(data->hb, start, count);
+ hbitmap_check_meta(data, 0, 0);
+
+ /* Test "unset" -> "set" will update meta */
+ hbitmap_set(data->hb, start, count);
+ hbitmap_check_meta(data, check_start, check_count);
+
+ /* Test "set" -> "set" will not update meta */
+ hbitmap_reset_all(data->meta);
+ hbitmap_set(data->hb, start, count);
+ hbitmap_check_meta(data, 0, 0);
+
+ /* Test "set" -> "unset" will update meta */
+ hbitmap_reset_all(data->meta);
+ hbitmap_reset(data->hb, start, count);
+ hbitmap_check_meta(data, check_start, check_count);
+}
+
+static void hbitmap_test_meta_do(TestHBitmapData *data, int chunk_size)
+{
+ uint64_t size = chunk_size * 100;
+ hbitmap_test_init_meta(data, size, 0, chunk_size);
+
+ hbitmap_test_meta(data, 0, 1, 0, chunk_size);
+ hbitmap_test_meta(data, 0, chunk_size, 0, chunk_size);
+ hbitmap_test_meta(data, chunk_size - 1, 1, 0, chunk_size);
+ hbitmap_test_meta(data, chunk_size - 1, 2, 0, chunk_size * 2);
+ hbitmap_test_meta(data, chunk_size - 1, chunk_size + 1, 0, chunk_size * 2);
+ hbitmap_test_meta(data, chunk_size - 1, chunk_size + 2, 0, chunk_size * 3);
+ hbitmap_test_meta(data, 7 * chunk_size - 1, chunk_size + 2,
+ 6 * chunk_size, chunk_size * 3);
+ hbitmap_test_meta(data, size - 1, 1, size - chunk_size, chunk_size);
+ hbitmap_test_meta(data, 0, size, 0, size);
+}
+
+static void test_hbitmap_meta_byte(TestHBitmapData *data, const void *unused)
+{
+ hbitmap_test_meta_do(data, BITS_PER_BYTE);
+}
+
+static void test_hbitmap_meta_word(TestHBitmapData *data, const void *unused)
+{
+ hbitmap_test_meta_do(data, BITS_PER_LONG);
+}
+
+static void test_hbitmap_meta_sector(TestHBitmapData *data, const void *unused)
+{
+ hbitmap_test_meta_do(data, BDRV_SECTOR_SIZE * BITS_PER_BYTE);
+}
+
+/**
+ * Create an HBitmap and test set/unset.
+ */
+static void test_hbitmap_meta_one(TestHBitmapData *data, const void *unused)
+{
+ int i;
+ int64_t offsets[] = {
+ 0, 1, L1 - 1, L1, L1 + 1, L2 - 1, L2, L2 + 1, L3 - 1, L3, L3 + 1
+ };
+
+ hbitmap_test_init_meta(data, L3 * 2, 0, 1);
+ for (i = 0; i < ARRAY_SIZE(offsets); i++) {
+ hbitmap_test_meta(data, offsets[i], 1, offsets[i], 1);
+ hbitmap_test_meta(data, offsets[i], L1, offsets[i], L1);
+ hbitmap_test_meta(data, offsets[i], L2, offsets[i], L2);
+ }
+}
+
+static void test_hbitmap_meta_zero(TestHBitmapData *data, const void *unused)
+{
+ hbitmap_test_init_meta(data, 0, 0, 1);
+
+ hbitmap_check_meta(data, 0, 0);
+}
+
static void hbitmap_test_add(const char *testpath,
void (*test_func)(TestHBitmapData *data, const void *user_data))
{
@@ -686,6 +793,12 @@ int main(int argc, char **argv)
test_hbitmap_truncate_grow_large);
hbitmap_test_add("/hbitmap/truncate/shrink/large",
test_hbitmap_truncate_shrink_large);
+
+ hbitmap_test_add("/hbitmap/meta/zero", test_hbitmap_meta_zero);
+ hbitmap_test_add("/hbitmap/meta/one", test_hbitmap_meta_one);
+ hbitmap_test_add("/hbitmap/meta/byte", test_hbitmap_meta_byte);
+ hbitmap_test_add("/hbitmap/meta/word", test_hbitmap_meta_word);
+ hbitmap_test_add("/hbitmap/meta/sector", test_hbitmap_meta_sector);
g_test_run();
return 0;
--
2.4.3
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH 08/13] block: Support meta dirty bitmap
2016-01-04 10:27 [Qemu-devel] [PATCH 00/13] Dirty bitmap changes for migration/persistence work Fam Zheng
` (6 preceding siblings ...)
2016-01-04 10:27 ` [Qemu-devel] [PATCH 07/13] tests: Add test code for meta bitmap Fam Zheng
@ 2016-01-04 10:27 ` Fam Zheng
2016-01-07 19:30 ` John Snow
2016-01-04 10:27 ` [Qemu-devel] [PATCH 09/13] block: Add two dirty bitmap getters Fam Zheng
` (5 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Fam Zheng @ 2016-01-04 10:27 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Vladimir Sementsov-Ogievskiy, jsnow, qemu-block
The added group of operations enables tracking of the changed bits in
the dirty bitmap.
Signed-off-by: Fam Zheng <famz@redhat.com>
---
block/dirty-bitmap.c | 51 ++++++++++++++++++++++++++++++++++++++++++++
include/block/dirty-bitmap.h | 9 ++++++++
2 files changed, 60 insertions(+)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 53cf88d..4314659 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -37,6 +37,7 @@
*/
struct BdrvDirtyBitmap {
HBitmap *bitmap; /* Dirty sector bitmap implementation */
+ HBitmap *meta; /* Meta dirty bitmap */
BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */
char *name; /* Optional non-empty unique ID */
int64_t size; /* Size of the bitmap (Number of sectors) */
@@ -102,6 +103,56 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
return bitmap;
}
+/* bdrv_create_meta_dirty_bitmap
+ *
+ * Create a meta dirty bitmap that tracks the changes of bits in @bitmap. I.e.
+ * when a dirty status bit in @bitmap is changed (either from reset to set or
+ * the other way around), its respective meta dirty bitmap bit will be marked
+ * dirty as well.
+ *
+ * @bitmap: the block dirty bitmap for which to create a meta dirty bitmap.
+ * @granularity: how many bytes of bitmap data does each bit in the meta bitmap
+ * track.
+ */
+void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap,
+ int granularity)
+{
+ assert(!bitmap->meta);
+ bitmap->meta = hbitmap_create_meta(bitmap->bitmap,
+ BDRV_SECTOR_SIZE * BITS_PER_BYTE);
+}
+
+void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap)
+{
+ assert(bitmap->meta);
+ hbitmap_free(bitmap->meta);
+ bitmap->meta = NULL;
+}
+
+int bdrv_dirty_bitmap_get_meta(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap, int64_t sector,
+ int nb_sectors)
+{
+ uint64_t i;
+ int gran = bdrv_dirty_bitmap_granularity(bitmap) >> BDRV_SECTOR_BITS;
+
+ /* To optimize: we can make hbitmap to internally check the range in a
+ * coarse level, or at least do it word by word. */
+ for (i = sector; i < sector + nb_sectors; i += gran) {
+ if (hbitmap_get(bitmap->meta, i)) {
+ return true;
+ }
+ }
+ return false;
+}
+
+void bdrv_dirty_bitmap_reset_meta(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap, int64_t sector,
+ int nb_sectors)
+{
+ hbitmap_reset(bitmap->meta, sector, nb_sectors);
+}
+
bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
{
return bitmap->successor;
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 16bb15a..0715220 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -8,6 +8,9 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
uint32_t granularity,
const char *name,
Error **errp);
+void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap,
+ int granularity);
+void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap);
int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap,
Error **errp);
@@ -34,6 +37,12 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors);
void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors);
+int bdrv_dirty_bitmap_get_meta(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap, int64_t sector,
+ int nb_sectors);
+void bdrv_dirty_bitmap_reset_meta(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap, int64_t sector,
+ int nb_sectors);
BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap,
uint64_t first_sector);
void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter);
--
2.4.3
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH 09/13] block: Add two dirty bitmap getters
2016-01-04 10:27 [Qemu-devel] [PATCH 00/13] Dirty bitmap changes for migration/persistence work Fam Zheng
` (7 preceding siblings ...)
2016-01-04 10:27 ` [Qemu-devel] [PATCH 08/13] block: Support meta dirty bitmap Fam Zheng
@ 2016-01-04 10:27 ` Fam Zheng
2016-01-07 19:35 ` John Snow
2016-01-04 10:27 ` [Qemu-devel] [PATCH 10/13] block: Assert that bdrv_release_dirty_bitmap succeeded Fam Zheng
` (4 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Fam Zheng @ 2016-01-04 10:27 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Vladimir Sementsov-Ogievskiy, jsnow, qemu-block
For dirty bitmap users to get the size and the name of a
BdrvDirtyBitmap.
Signed-off-by: Fam Zheng <famz@redhat.com>
---
block/dirty-bitmap.c | 10 ++++++++++
include/block/dirty-bitmap.h | 2 ++
2 files changed, 12 insertions(+)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 4314659..9cac794 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -153,6 +153,16 @@ void bdrv_dirty_bitmap_reset_meta(BlockDriverState *bs,
hbitmap_reset(bitmap->meta, sector, nb_sectors);
}
+int64_t bdrv_dirty_bitmap_size(BdrvDirtyBitmap *bitmap)
+{
+ return bitmap->size;
+}
+
+const char *bdrv_dirty_bitmap_name(BdrvDirtyBitmap *bitmap)
+{
+ return bitmap->name;
+}
+
bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
{
return bitmap->successor;
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 0715220..e36efb6 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -31,6 +31,8 @@ uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap);
bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap);
+const char *bdrv_dirty_bitmap_name(BdrvDirtyBitmap *bitmap);
+int64_t bdrv_dirty_bitmap_size(BdrvDirtyBitmap *bitmap);
DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap);
int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector);
void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
--
2.4.3
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH 10/13] block: Assert that bdrv_release_dirty_bitmap succeeded
2016-01-04 10:27 [Qemu-devel] [PATCH 00/13] Dirty bitmap changes for migration/persistence work Fam Zheng
` (8 preceding siblings ...)
2016-01-04 10:27 ` [Qemu-devel] [PATCH 09/13] block: Add two dirty bitmap getters Fam Zheng
@ 2016-01-04 10:27 ` Fam Zheng
2016-01-07 19:38 ` John Snow
2016-01-04 10:27 ` [Qemu-devel] [PATCH 11/13] hbitmap: serialization Fam Zheng
` (3 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Fam Zheng @ 2016-01-04 10:27 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Vladimir Sementsov-Ogievskiy, jsnow, qemu-block
This makes sure we don't leak a dirty bitmap in any case.
Signed-off-by: Fam Zheng <famz@redhat.com>
---
block/dirty-bitmap.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 9cac794..60ee965 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -297,6 +297,7 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
return;
}
}
+ abort();
}
void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
--
2.4.3
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH 11/13] hbitmap: serialization
2016-01-04 10:27 [Qemu-devel] [PATCH 00/13] Dirty bitmap changes for migration/persistence work Fam Zheng
` (9 preceding siblings ...)
2016-01-04 10:27 ` [Qemu-devel] [PATCH 10/13] block: Assert that bdrv_release_dirty_bitmap succeeded Fam Zheng
@ 2016-01-04 10:27 ` Fam Zheng
2016-01-07 21:11 ` John Snow
` (2 more replies)
2016-01-04 10:27 ` [Qemu-devel] [PATCH 12/13] block: BdrvDirtyBitmap serialization interface Fam Zheng
` (2 subsequent siblings)
13 siblings, 3 replies; 36+ messages in thread
From: Fam Zheng @ 2016-01-04 10:27 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Vladimir Sementsov-Ogievskiy, jsnow, qemu-block
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Functions to serialize / deserialize(restore) HBitmap. HBitmap should be
saved to linear sequence of bits independently of endianness and bitmap
array element (unsigned long) size. Therefore Little Endian is chosen.
These functions are appropriate for dirty bitmap migration, restoring
the bitmap in several steps is available. To save performance, every
step writes only the last level of the bitmap. All other levels are
restored by hbitmap_deserialize_finish() as a last step of restoring.
So, HBitmap is inconsistent while restoring.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
[Fix left shift operand to 1UL; add "finish" parameter. - Fam]
Signed-off-by: Fam Zheng <famz@redhat.com>
---
include/qemu/hbitmap.h | 76 +++++++++++++++++++++++++++
util/hbitmap.c | 136 +++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 212 insertions(+)
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index ed672e7..3414113 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -149,6 +149,82 @@ void hbitmap_reset_all(HBitmap *hb);
bool hbitmap_get(const HBitmap *hb, uint64_t item);
/**
+ * hbitmap_data_size:
+ * @hb: HBitmap to operate on.
+ * @count: Number of bits
+ *
+ * Grunularity of serialization chunks, used by other serializetion functions.
+ * For every chunk:
+ * 1. Chunk start should be aligned to this granularity.
+ * 2. Chunk size should be aligned too, except for last chunk (for which
+ * start + count == hb->size)
+ */
+uint64_t hbitmap_serialization_granularity(const HBitmap *hb);
+
+/**
+ * hbitmap_data_size:
+ * @hb: HBitmap to operate on.
+ * @count: Number of bits
+ *
+ * Return amount of bytes hbitmap_(de)serialize_part needs
+ */
+uint64_t hbitmap_serialization_size(const HBitmap *hb,
+ uint64_t start, uint64_t count);
+
+/**
+ * hbitmap_serialize_part
+ * @hb: HBitmap to operate on.
+ * @buf: Buffer to store serialized bitmap.
+ * @start: First bit to store.
+ * @count: Number of bits to store.
+ *
+ * Stores HBitmap data corresponding to given region. The format of saved data
+ * is linear sequence of bits, so it can be used by hbitmap_deserialize_part
+ * independently of endianness and size of HBitmap level array elements
+ */
+void hbitmap_serialize_part(const HBitmap *hb, uint8_t *buf,
+ uint64_t start, uint64_t count);
+
+/**
+ * hbitmap_deserialize_part
+ * @hb: HBitmap to operate on.
+ * @buf: Buffer to restore bitmap data from.
+ * @start: First bit to restore.
+ * @count: Number of bits to restore.
+ * @finish: Whether to call hbitmap_deserialize_finish automatically.
+ *
+ * Restores HBitmap data corresponding to given region. The format is the same
+ * as for hbitmap_serialize_part.
+ *
+ * if @finish is false, caller must call hbitmap_serialize_finish before using
+ * the bitmap.
+ */
+void hbitmap_deserialize_part(HBitmap *hb, uint8_t *buf,
+ uint64_t start, uint64_t count,
+ bool finish);
+
+/**
+ * hbitmap_deserialize_zeroes
+ * @hb: HBitmap to operate on.
+ * @start: First bit to restore.
+ * @count: Number of bits to restore.
+ * @finish: Whether to call hbitmap_deserialize_finish automatically.
+ *
+ * Same as hbitmap_serialize_part, but fills the bitmap with zeroes.
+ */
+void hbitmap_deserialize_zeroes(HBitmap *hb, uint64_t start, uint64_t count,
+ bool finish);
+
+/**
+ * hbitmap_deserialize_finish
+ * @hb: HBitmap to operate on.
+ *
+ * Repair HBitmap after calling hbitmap_deserialize_data. Actually, all HBitmap
+ * layers are restored here.
+ */
+void hbitmap_deserialize_finish(HBitmap *hb);
+
+/**
* hbitmap_free:
* @hb: HBitmap to operate on.
*
diff --git a/util/hbitmap.c b/util/hbitmap.c
index 55d3182..ab8cd81 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -397,6 +397,142 @@ bool hbitmap_get(const HBitmap *hb, uint64_t item)
return (hb->levels[HBITMAP_LEVELS - 1][pos >> BITS_PER_LEVEL] & bit) != 0;
}
+uint64_t hbitmap_serialization_granularity(const HBitmap *hb)
+{
+ return BITS_PER_LONG << hb->granularity;
+}
+
+/* serilization chunk start should be aligned to serialization granularity.
+ * Serilization chunk size scould be aligned to serialization granularity too,
+ * except for last chunk.
+ */
+static void serialization_chunk(const HBitmap *hb,
+ uint64_t start, uint64_t count,
+ unsigned long **first_el, size_t *el_count)
+{
+ uint64_t last = start + count - 1;
+ uint64_t gran = hbitmap_serialization_granularity(hb);
+
+ assert(((start + count - 1) >> hb->granularity) < hb->size);
+ assert((start & (gran - 1)) == 0);
+ if ((last >> hb->granularity) != hb->size - 1) {
+ assert((count & (gran - 1)) == 0);
+ }
+
+ start = (start >> hb->granularity) >> BITS_PER_LEVEL;
+ last = (last >> hb->granularity) >> BITS_PER_LEVEL;
+
+ *first_el = &hb->levels[HBITMAP_LEVELS - 1][start];
+ *el_count = last - start + 1;
+}
+
+uint64_t hbitmap_serialization_size(const HBitmap *hb,
+ uint64_t start, uint64_t count)
+{
+ uint64_t el_count;
+ unsigned long *cur;
+
+ if (!count) {
+ return 0;
+ }
+ serialization_chunk(hb, start, count, &cur, &el_count);
+
+ return el_count * sizeof(unsigned long);
+}
+
+void hbitmap_serialize_part(const HBitmap *hb, uint8_t *buf,
+ uint64_t start, uint64_t count)
+{
+ uint64_t el_count;
+ unsigned long *cur, *end;
+
+ if (!count) {
+ return;
+ }
+ serialization_chunk(hb, start, count, &cur, &el_count);
+ end = cur + el_count;
+
+ while (cur != end) {
+ unsigned long el =
+ (BITS_PER_LONG == 32 ? cpu_to_le32(*cur) : cpu_to_le64(*cur));
+
+ memcpy(buf, &el, sizeof(el));
+ buf += sizeof(el);
+ cur++;
+ }
+}
+
+void hbitmap_deserialize_part(HBitmap *hb, uint8_t *buf,
+ uint64_t start, uint64_t count,
+ bool finish)
+{
+ uint64_t el_count;
+ unsigned long *cur, *end;
+
+ if (!count) {
+ return;
+ }
+ serialization_chunk(hb, start, count, &cur, &el_count);
+ end = cur + el_count;
+
+ while (cur != end) {
+ memcpy(cur, buf, sizeof(*cur));
+
+ if (BITS_PER_LONG == 32) {
+ cpu_to_le32s((uint32_t *)cur);
+ } else {
+ cpu_to_le64s((uint64_t *)cur);
+ }
+
+ buf += sizeof(unsigned long);
+ cur++;
+ }
+ if (finish) {
+ hbitmap_deserialize_finish(hb);
+ }
+}
+
+void hbitmap_deserialize_zeroes(HBitmap *hb, uint64_t start, uint64_t count,
+ bool finish)
+{
+ uint64_t el_count;
+ unsigned long *first;
+
+ if (!count) {
+ return;
+ }
+ serialization_chunk(hb, start, count, &first, &el_count);
+
+ memset(first, 0, el_count * sizeof(unsigned long));
+ if (finish) {
+ hbitmap_deserialize_finish(hb);
+ }
+}
+
+void hbitmap_deserialize_finish(HBitmap *bitmap)
+{
+ int64_t i, size, prev_size;
+ int lev;
+
+ /* restore levels starting from penultimate to zero level, assuming
+ * that the last level is ok */
+ size = MAX((bitmap->size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
+ for (lev = HBITMAP_LEVELS - 1; lev-- > 0; ) {
+ prev_size = size;
+ size = MAX((size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
+ memset(bitmap->levels[lev], 0, size * sizeof(unsigned long));
+
+ for (i = 0; i < prev_size; ++i) {
+ if (bitmap->levels[lev + 1][i]) {
+ bitmap->levels[lev][i >> BITS_PER_LEVEL] |=
+ 1UL << (i & (BITS_PER_LONG - 1));
+ }
+ }
+ }
+
+ bitmap->levels[0][0] |= 1UL << (BITS_PER_LONG - 1);
+}
+
void hbitmap_free(HBitmap *hb)
{
unsigned i;
--
2.4.3
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH 12/13] block: BdrvDirtyBitmap serialization interface
2016-01-04 10:27 [Qemu-devel] [PATCH 00/13] Dirty bitmap changes for migration/persistence work Fam Zheng
` (10 preceding siblings ...)
2016-01-04 10:27 ` [Qemu-devel] [PATCH 11/13] hbitmap: serialization Fam Zheng
@ 2016-01-04 10:27 ` Fam Zheng
2016-01-04 10:27 ` [Qemu-devel] [PATCH 13/13] tests: Add test code for hbitmap serialization Fam Zheng
2016-01-07 21:32 ` [Qemu-devel] [PATCH 00/13] Dirty bitmap changes for migration/persistence work John Snow
13 siblings, 0 replies; 36+ messages in thread
From: Fam Zheng @ 2016-01-04 10:27 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Vladimir Sementsov-Ogievskiy, jsnow, qemu-block
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Several functions to provide necessary access to BdrvDirtyBitmap for
block-migration.c
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
[Add the "finish" parameters. - Fam]
Signed-off-by: Fam Zheng <famz@redhat.com>
---
block/dirty-bitmap.c | 37 +++++++++++++++++++++++++++++++++++++
include/block/dirty-bitmap.h | 14 ++++++++++++++
2 files changed, 51 insertions(+)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 60ee965..d524549 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -428,6 +428,43 @@ void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in)
hbitmap_free(tmp);
}
+uint64_t bdrv_dirty_bitmap_serialization_size(const BdrvDirtyBitmap *bitmap,
+ uint64_t start, uint64_t count)
+{
+ return hbitmap_serialization_size(bitmap->bitmap, start, count);
+}
+
+uint64_t bdrv_dirty_bitmap_serialization_align(const BdrvDirtyBitmap *bitmap)
+{
+ return hbitmap_serialization_granularity(bitmap->bitmap);
+}
+
+void bdrv_dirty_bitmap_serialize_part(const BdrvDirtyBitmap *bitmap,
+ uint8_t *buf, uint64_t start,
+ uint64_t count)
+{
+ hbitmap_serialize_part(bitmap->bitmap, buf, start, count);
+}
+
+void bdrv_dirty_bitmap_deserialize_part(BdrvDirtyBitmap *bitmap,
+ uint8_t *buf, uint64_t start,
+ uint64_t count, bool finish)
+{
+ hbitmap_deserialize_part(bitmap->bitmap, buf, start, count, finish);
+}
+
+void bdrv_dirty_bitmap_deserialize_zeroes(BdrvDirtyBitmap *bitmap,
+ uint64_t start, uint64_t count,
+ bool finish)
+{
+ hbitmap_deserialize_zeroes(bitmap->bitmap, start, count, finish);
+}
+
+void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap)
+{
+ hbitmap_deserialize_finish(bitmap->bitmap);
+}
+
void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
int nr_sectors)
{
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index e36efb6..d4a8bf1 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -53,4 +53,18 @@ void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t sector_num);
int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
+uint64_t bdrv_dirty_bitmap_serialization_size(const BdrvDirtyBitmap *bitmap,
+ uint64_t start, uint64_t count);
+uint64_t bdrv_dirty_bitmap_serialization_align(const BdrvDirtyBitmap *bitmap);
+void bdrv_dirty_bitmap_serialize_part(const BdrvDirtyBitmap *bitmap,
+ uint8_t *buf, uint64_t start,
+ uint64_t count);
+void bdrv_dirty_bitmap_deserialize_part(BdrvDirtyBitmap *bitmap,
+ uint8_t *buf, uint64_t start,
+ uint64_t count, bool finish);
+void bdrv_dirty_bitmap_deserialize_zeroes(BdrvDirtyBitmap *bitmap,
+ uint64_t start, uint64_t count,
+ bool finish);
+void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
+
#endif
--
2.4.3
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH 13/13] tests: Add test code for hbitmap serialization
2016-01-04 10:27 [Qemu-devel] [PATCH 00/13] Dirty bitmap changes for migration/persistence work Fam Zheng
` (11 preceding siblings ...)
2016-01-04 10:27 ` [Qemu-devel] [PATCH 12/13] block: BdrvDirtyBitmap serialization interface Fam Zheng
@ 2016-01-04 10:27 ` Fam Zheng
2016-01-07 21:32 ` [Qemu-devel] [PATCH 00/13] Dirty bitmap changes for migration/persistence work John Snow
13 siblings, 0 replies; 36+ messages in thread
From: Fam Zheng @ 2016-01-04 10:27 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Vladimir Sementsov-Ogievskiy, jsnow, qemu-block
Signed-off-by: Fam Zheng <famz@redhat.com>
---
tests/test-hbitmap.c | 139 +++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 139 insertions(+)
diff --git a/tests/test-hbitmap.c b/tests/test-hbitmap.c
index 19136e7..4f61961 100644
--- a/tests/test-hbitmap.c
+++ b/tests/test-hbitmap.c
@@ -14,6 +14,7 @@
#include <string.h>
#include <sys/types.h>
#include "qemu/hbitmap.h"
+#include "qemu/bitmap.h"
#include "block/block.h"
#define LOG_BITS_PER_LONG (BITS_PER_LONG == 32 ? 5 : 6)
@@ -737,6 +738,16 @@ static void test_hbitmap_meta_one(TestHBitmapData *data, const void *unused)
}
}
+static void test_hbitmap_serialize_granularity(TestHBitmapData *data,
+ const void *unused)
+{
+ int r;
+
+ hbitmap_test_init(data, L3 * 2, 3);
+ r = hbitmap_serialization_granularity(data->hb);
+ g_assert_cmpint(r, ==, BITS_PER_LONG << 3);
+}
+
static void test_hbitmap_meta_zero(TestHBitmapData *data, const void *unused)
{
hbitmap_test_init_meta(data, 0, 0, 1);
@@ -744,6 +755,125 @@ static void test_hbitmap_meta_zero(TestHBitmapData *data, const void *unused)
hbitmap_check_meta(data, 0, 0);
}
+static void hbitmap_test_serialize_range(TestHBitmapData *data,
+ uint8_t *buf, size_t buf_size,
+ uint64_t pos, uint64_t count)
+{
+ size_t i;
+
+ assert(hbitmap_granularity(data->hb) == 0);
+ hbitmap_reset_all(data->hb);
+ memset(buf, 0, buf_size);
+ if (count) {
+ hbitmap_set(data->hb, pos, count);
+ }
+ hbitmap_serialize_part(data->hb, buf, 0, data->size);
+ for (i = 0; i < data->size; i++) {
+ int is_set = test_bit(i, (unsigned long *)buf);
+ if (i >= pos && i < pos + count) {
+ g_assert(is_set);
+ } else {
+ g_assert(!is_set);
+ }
+ }
+ hbitmap_reset_all(data->hb);
+ hbitmap_deserialize_part(data->hb, buf, 0, data->size, true);
+
+ for (i = 0; i < data->size; i++) {
+ int is_set = hbitmap_get(data->hb, i);
+ if (i >= pos && i < pos + count) {
+ g_assert(is_set);
+ } else {
+ g_assert(!is_set);
+ }
+ }
+}
+
+static void test_hbitmap_serialize_basic(TestHBitmapData *data,
+ const void *unused)
+{
+ int i, j;
+ size_t buf_size;
+ uint8_t *buf;
+ uint64_t positions[] = { 0, 1, L1 - 1, L1, L2 - 1, L2, L2 + 1, L3 - 1 };
+ int num_positions = sizeof(positions) / sizeof(positions[0]);
+
+ hbitmap_test_init(data, L3, 0);
+ buf_size = hbitmap_serialization_size(data->hb, 0, data->size);
+ buf = g_malloc0(buf_size);
+
+ for (i = 0; i < num_positions; i++) {
+ for (j = 0; j < num_positions; j++) {
+ hbitmap_test_serialize_range(data, buf, buf_size,
+ positions[i],
+ MIN(positions[j], L3 - positions[i]));
+ }
+ }
+
+ g_free(buf);
+}
+
+static void test_hbitmap_serialize_part(TestHBitmapData *data,
+ const void *unused)
+{
+ int i, j, k;
+ size_t buf_size;
+ uint8_t *buf;
+ uint64_t positions[] = { 0, 1, L1 - 1, L1, L2 - 1, L2, L2 + 1, L3 - 1 };
+ int num_positions = sizeof(positions) / sizeof(positions[0]);
+
+ hbitmap_test_init(data, L3, 0);
+ buf_size = L2;
+ buf = g_malloc0(buf_size);
+
+ for (i = 0; i < num_positions; i++) {
+ hbitmap_set(data->hb, positions[i], 1);
+ }
+
+ for (i = 0; i < data->size; i += buf_size) {
+ hbitmap_serialize_part(data->hb, buf, i, buf_size);
+ for (j = 0; j < buf_size; j++) {
+ bool should_set = false;
+ for (k = 0; k < num_positions; k++) {
+ if (positions[k] == j + i) {
+ should_set = true;
+ break;
+ }
+ }
+ g_assert_cmpint(should_set, ==, test_bit(j, (unsigned long *)buf));
+ }
+ }
+
+ g_free(buf);
+}
+
+static void test_hbitmap_serialize_zeroes(TestHBitmapData *data,
+ const void *unused)
+{
+ int i;
+ HBitmapIter iter;
+ int64_t next;
+ uint64_t positions[] = { 0, L1, L2, L3 - L1};
+ int num_positions = sizeof(positions) / sizeof(positions[0]);
+
+ hbitmap_test_init(data, L3, 0);
+
+ for (i = 0; i < num_positions; i++) {
+ hbitmap_set(data->hb, positions[i], L1);
+ }
+
+ for (i = 0; i < num_positions; i++) {
+ hbitmap_deserialize_zeroes(data->hb, positions[i], L1, true);
+ hbitmap_iter_init(&iter, data->hb, 0);
+ next = hbitmap_iter_next(&iter);
+ if (i == num_positions - 1) {
+ g_assert_cmpint(next, ==, -1);
+ } else {
+ g_assert_cmpint(next, ==, positions[i + 1]);
+ }
+ }
+}
+
static void hbitmap_test_add(const char *testpath,
void (*test_func)(TestHBitmapData *data, const void *user_data))
{
@@ -799,6 +929,15 @@ int main(int argc, char **argv)
hbitmap_test_add("/hbitmap/meta/byte", test_hbitmap_meta_byte);
hbitmap_test_add("/hbitmap/meta/word", test_hbitmap_meta_word);
hbitmap_test_add("/hbitmap/meta/sector", test_hbitmap_meta_sector);
+
+ hbitmap_test_add("/hbitmap/serialize/granularity",
+ test_hbitmap_serialize_granularity);
+ hbitmap_test_add("/hbitmap/serialize/basic",
+ test_hbitmap_serialize_basic);
+ hbitmap_test_add("/hbitmap/serialize/part",
+ test_hbitmap_serialize_part);
+ hbitmap_test_add("/hbitmap/serialize/zeroes",
+ test_hbitmap_serialize_zeroes);
g_test_run();
return 0;
--
2.4.3
^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 02/13] typedefs: Add BdrvDirtyBitmap and HBitmapIter
2016-01-04 10:27 ` [Qemu-devel] [PATCH 02/13] typedefs: Add BdrvDirtyBitmap and HBitmapIter Fam Zheng
@ 2016-01-05 22:14 ` John Snow
2016-01-08 2:13 ` Fam Zheng
0 siblings, 1 reply; 36+ messages in thread
From: John Snow @ 2016-01-05 22:14 UTC (permalink / raw)
To: Fam Zheng, qemu-devel
Cc: Kevin Wolf, Jeff Cody, Vladimir Sementsov-Ogievskiy, qemu-block
On 01/04/2016 05:27 AM, Fam Zheng wrote:
> Following patches to refactor and move block dirty bitmap code could use this.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> include/qemu/typedefs.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> index 78fe6e8..e83934e 100644
> --- a/include/qemu/typedefs.h
> +++ b/include/qemu/typedefs.h
> @@ -10,6 +10,7 @@ typedef struct AddressSpace AddressSpace;
> typedef struct AioContext AioContext;
> typedef struct AllwinnerAHCIState AllwinnerAHCIState;
> typedef struct AudioState AudioState;
> +typedef struct BdrvDirtyBitmap BdrvDirtyBitmap;
> typedef struct BlockBackend BlockBackend;
> typedef struct BlockBackendRootState BlockBackendRootState;
> typedef struct BlockDriverState BlockDriverState;
> @@ -28,6 +29,7 @@ typedef struct EventNotifier EventNotifier;
> typedef struct FWCfgIoState FWCfgIoState;
> typedef struct FWCfgMemState FWCfgMemState;
> typedef struct FWCfgState FWCfgState;
> +typedef struct HBitmapIter HBitmapIter;
> typedef struct HCIInfo HCIInfo;
> typedef struct I2CBus I2CBus;
> typedef struct I2SCodec I2SCodec;
>
Should the existing typedefs be removed?
>> include/block/block.h:typedef struct BdrvDirtyBitmap BdrvDirtyBitmap;
>> include/block/block.h:struct HBitmapIter;
>> include/qemu/hbitmap.h:typedef struct HBitmapIter HBitmapIter;
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 03/13] block: Move block dirty bitmap code to separate files
2016-01-04 10:27 ` [Qemu-devel] [PATCH 03/13] block: Move block dirty bitmap code to separate files Fam Zheng
@ 2016-01-05 22:32 ` John Snow
0 siblings, 0 replies; 36+ messages in thread
From: John Snow @ 2016-01-05 22:32 UTC (permalink / raw)
To: Fam Zheng, qemu-devel
Cc: Kevin Wolf, Jeff Cody, Vladimir Sementsov-Ogievskiy, qemu-block
On 01/04/2016 05:27 AM, Fam Zheng wrote:
> The only change is making bdrv_dirty_bitmap_truncate public. It is used in
> block.c.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> block.c | 339 ---------------------------------------
> block/Makefile.objs | 2 +-
> block/dirty-bitmap.c | 366 +++++++++++++++++++++++++++++++++++++++++++
> include/block/block.h | 37 +----
> include/block/dirty-bitmap.h | 42 +++++
> 5 files changed, 410 insertions(+), 376 deletions(-)
> create mode 100644 block/dirty-bitmap.c
> create mode 100644 include/block/dirty-bitmap.h
>
> diff --git a/block.c b/block.c
> index 411edbf..b544190 100644
> --- a/block.c
> +++ b/block.c
> @@ -55,23 +55,6 @@
> #include <windows.h>
> #endif
>
> -/**
> - * A BdrvDirtyBitmap can be in three possible states:
> - * (1) successor is NULL and disabled is false: full r/w mode
> - * (2) successor is NULL and disabled is true: read only mode ("disabled")
> - * (3) successor is set: frozen mode.
> - * A frozen bitmap cannot be renamed, deleted, anonymized, cleared, set,
> - * or enabled. A frozen bitmap can only abdicate() or reclaim().
> - */
> -struct BdrvDirtyBitmap {
> - HBitmap *bitmap; /* Dirty sector bitmap implementation */
> - BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */
> - char *name; /* Optional non-empty unique ID */
> - int64_t size; /* Size of the bitmap (Number of sectors) */
> - bool disabled; /* Bitmap is read-only */
> - QLIST_ENTRY(BdrvDirtyBitmap) list;
> -};
> -
> #define NOT_DONE 0x7fffffff /* used while emulated sync operation in progress */
>
> struct BdrvStates bdrv_states = QTAILQ_HEAD_INITIALIZER(bdrv_states);
> @@ -87,7 +70,6 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
> BlockDriverState *parent,
> const BdrvChildRole *child_role, Error **errp);
>
> -static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
> /* If non-zero, use only whitelisted block drivers */
> static int use_bdrv_whitelist;
>
> @@ -3375,327 +3357,6 @@ void bdrv_lock_medium(BlockDriverState *bs, bool locked)
> }
> }
>
> -BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, const char *name)
> -{
> - BdrvDirtyBitmap *bm;
> -
> - assert(name);
> - QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
> - if (bm->name && !strcmp(name, bm->name)) {
> - return bm;
> - }
> - }
> - return NULL;
> -}
> -
> -void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap)
> -{
> - assert(!bdrv_dirty_bitmap_frozen(bitmap));
> - g_free(bitmap->name);https://www.facebook.com/nano.nago
> - bitmap->name = NULL;
> -}
> -
> -BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
> - uint32_t granularity,
> - const char *name,
> - Error **errp)
> -{
> - int64_t bitmap_size;
> - BdrvDirtyBitmap *bitmap;
> - uint32_t sector_granularity;
> -
> - assert((granularity & (granularity - 1)) == 0);
> -
> - if (name && bdrv_find_dirty_bitmap(bs, name)) {
> - error_setg(errp, "Bitmap already exists: %s", name);
> - return NULL;
> - }
> - sector_granularity = granularity >> BDRV_SECTOR_BITS;
> - assert(sector_granularity);
> - bitmap_size = bdrv_nb_sectors(bs);
> - if (bitmap_size < 0) {
> - error_setg_errno(errp, -bitmap_size, "could not get length of device");
> - errno = -bitmap_size;
> - return NULL;
> - }
> - bitmap = g_new0(BdrvDirtyBitmap, 1);
> - bitmap->bitmap = hbitmap_alloc(bitmap_size, ctz32(sector_granularity));
> - bitmap->size = bitmap_size;
> - bitmap->name = g_strdup(name);
> - bitmap->disabled = false;
> - QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
> - return bitmap;
> -}
> -
> -bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
> -{
> - return bitmap->successor;
> -}
> -
> -bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
> -{
> - return !(bitmap->disabled || bitmap->successor);
> -}
> -
> -DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap)
> -{
> - if (bdrv_dirty_bitmap_frozen(bitmap)) {
> - return DIRTY_BITMAP_STATUS_FROZEN;
> - } else if (!bdrv_dirty_bitmap_enabled(bitmap)) {
> - return DIRTY_BITMAP_STATUS_DISABLED;
> - } else {
> - return DIRTY_BITMAP_STATUS_ACTIVE;
> - }
> -}
> -
> -/**
> - * Create a successor bitmap destined to replace this bitmap after an operation.
> - * Requires that the bitmap is not frozen and has no successor.
> - */
> -int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
> - BdrvDirtyBitmap *bitmap, Error **errp)
> -{
> - uint64_t granularity;
> - BdrvDirtyBitmap *child;
> -
> - if (bdrv_dirty_bitmap_frozen(bitmap)) {
> - error_setg(errp, "Cannot create a successor for a bitmap that is "
> - "currently frozen");
> - return -1;
> - }
> - assert(!bitmap->successor);
> -
> - /* Create an anonymous successor */
> - granularity = bdrv_dirty_bitmap_granularity(bitmap);
> - child = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
> - if (!child) {
> - return -1;
> - }
> -
> - /* Successor will be on or off based on our current state. */
> - child->disabled = bitmap->disabled;
> -
> - /* Install the successor and freeze the parent */
> - bitmap->successor = child;
> - return 0;
> -}
> -
> -/**
> - * For a bitmap with a successor, yield our name to the successor,
> - * delete the old bitmap, and return a handle to the new bitmap.
> - */
> -BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
> - BdrvDirtyBitmap *bitmap,
> - Error **errp)
> -{
> - char *name;
> - BdrvDirtyBitmap *successor = bitmap->successor;
> -
> - if (successor == NULL) {
> - error_setg(errp, "Cannot relinquish control if "
> - "there's no successor present");
> - return NULL;
> - }
> -
> - name = bitmap->name;
> - bitmap->name = NULL;
> - successor->name = name;
> - bitmap->successor = NULL;
> - bdrv_release_dirty_bitmap(bs, bitmap);
> -
> - return successor;
> -}
> -
> -/**
> - * In cases of failure where we can no longer safely delete the parent,
> - * we may wish to re-join the parent and child/successor.
> - * The merged parent will be un-frozen, but not explicitly re-enabled.
> - */
> -BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
> - BdrvDirtyBitmap *parent,
> - Error **errp)
> -{
> - BdrvDirtyBitmap *successor = parent->successor;
> -
> - if (!successor) {
> - error_setg(errp, "Cannot reclaim a successor when none is present");
> - return NULL;
> - }
> -
> - if (!hbitmap_merge(parent->bitmap, successor->bitmap)) {
> - error_setg(errp, "Merging of parent and successor bitmap failed");
> - return NULL;
> - }
> - bdrv_release_dirty_bitmap(bs, successor);
> - parent->successor = NULL;
> -
> - return parent;
> -}
> -
> -/**
> - * Truncates _all_ bitmaps attached to a BDS.
> - */
> -static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
> -{
> - BdrvDirtyBitmap *bitmap;
> - uint64_t size = bdrv_nb_sectors(bs);
> -
> - QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
> - assert(!bdrv_dirty_bitmap_frozen(bitmap));
> - hbitmap_truncate(bitmap->bitmap, size);
> - bitmap->size = size;
> - }
> -}
> -
> -void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
> -{
> - BdrvDirtyBitmap *bm, *next;
> - QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
> - if (bm == bitmap) {
> - assert(!bdrv_dirty_bitmap_frozen(bm));
> - QLIST_REMOVE(bitmap, list);
> - hbitmap_free(bitmap->bitmap);
> - g_free(bitmap->name);
> - g_free(bitmap);
> - return;
> - }
> - }
> -}
> -
> -void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
> -{
> - assert(!bdrv_dirty_bitmap_frozen(bitmap));
> - bitmap->disabled = true;
> -}
> -
> -void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
> -{
> - assert(!bdrv_dirty_bitmap_frozen(bitmap));
> - bitmap->disabled = false;
> -}
> -
> -BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
> -{
> - BdrvDirtyBitmap *bm;
> - BlockDirtyInfoList *list = NULL;
> - BlockDirtyInfoList **plist = &list;
> -
> - QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
> - BlockDirtyInfo *info = g_new0(BlockDirtyInfo, 1);
> - BlockDirtyInfoList *entry = g_new0(BlockDirtyInfoList, 1);
> - info->count = bdrv_get_dirty_count(bm);
> - info->granularity = bdrv_dirty_bitmap_granularity(bm);
> - info->has_name = !!bm->name;
> - info->name = g_strdup(bm->name);
> - info->status = bdrv_dirty_bitmap_status(bm);
> - entry->value = info;
> - *plist = entry;
> - plist = &entry->next;
> - }
> -
> - return list;
> -}
> -
> -int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector)
> -{
> - if (bitmap) {
> - return hbitmap_get(bitmap->bitmap, sector);
> - } else {
> - return 0;
> - }
> -}
> -
> -/**
> - * Chooses a default granularity based on the existing cluster size,
> - * but clamped between [4K, 64K]. Defaults to 64K in the case that there
> - * is no cluster size information available.
> - */
> -uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs)
> -{
> - BlockDriverInfo bdi;
> - uint32_t granularity;
> -
> - if (bdrv_get_info(bs, &bdi) >= 0 && bdi.cluster_size > 0) {
> - granularity = MAX(4096, bdi.cluster_size);
> - granularity = MIN(65536, granularity);
> - } else {
> - granularity = 65536;
> - }
> -
> - return granularity;
> -}
> -
> -uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap)
> -{
> - return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
> -}
> -
> -void bdrv_dirty_iter_init(BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
> -{
> - hbitmap_iter_init(hbi, bitmap->bitmap, 0);
> -}
> -
> -void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> - int64_t cur_sector, int nr_sectors)
> -{
> - assert(bdrv_dirty_bitmap_enabled(bitmap));
> - hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
> -}
> -
> -void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> - int64_t cur_sector, int nr_sectors)
> -{
> - assert(bdrv_dirty_bitmap_enabled(bitmap));
> - hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
> -}
> -
> -void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
> -{
> - assert(bdrv_dirty_bitmap_enabled(bitmap));
> - if (!out) {
> - hbitmap_reset_all(bitmap->bitmap);
> - } else {
> - HBitmap *backup = bitmap->bitmap;
> - bitmap->bitmap = hbitmap_alloc(bitmap->size,
> - hbitmap_granularity(backup));
> - *out = backup;
> - }
> -}
> -
> -void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in)
> -{
> - HBitmap *tmp = bitmap->bitmap;
> - assert(bdrv_dirty_bitmap_enabled(bitmap));
> - bitmap->bitmap = in;
> - hbitmap_free(tmp);
> -}
> -
> -void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
> - int nr_sectors)
> -{
> - BdrvDirtyBitmap *bitmap;
> - QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
> - if (!bdrv_dirty_bitmap_enabled(bitmap)) {
> - continue;
> - }
> - hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
> - }
> -}
> -
> -/**
> - * Advance an HBitmapIter to an arbitrary offset.
> - */
> -void bdrv_set_dirty_iter(HBitmapIter *hbi, int64_t offset)
> -{
> - assert(hbi->hb);
> - hbitmap_iter_init(hbi, hbi->hb, offset);
> -}
> -
> -int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
> -{
> - return hbitmap_count(bitmap->bitmap);
> -}
> -
> /* Get a reference to bs */
> void bdrv_ref(BlockDriverState *bs)
> {
> diff --git a/block/Makefile.objs b/block/Makefile.objs
> index 58ef2ef..cdd8655 100644
> --- a/block/Makefile.objs
> +++ b/block/Makefile.objs
> @@ -20,7 +20,7 @@ block-obj-$(CONFIG_RBD) += rbd.o
> block-obj-$(CONFIG_GLUSTERFS) += gluster.o
> block-obj-$(CONFIG_ARCHIPELAGO) += archipelago.o
> block-obj-$(CONFIG_LIBSSH2) += ssh.o
> -block-obj-y += accounting.o
> +block-obj-y += accounting.o dirty-bitmap.o
> block-obj-y += write-threshold.o
>
> common-obj-y += stream.o
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> new file mode 100644
> index 0000000..7924c38
> --- /dev/null
> +++ b/block/dirty-bitmap.c
> @@ -0,0 +1,366 @@
> +/*
> + * Block Dirty Bitmap
> + *
> + * Copyright (c) 2016 Red Hat. Inc
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +#include "config-host.h"
> +#include "qemu-common.h"
> +#include "trace.h"
> +#include "block/block_int.h"
> +#include "block/blockjob.h"
> +
> +/**
> + * A BdrvDirtyBitmap can be in three possible states:
> + * (1) successor is NULL and disabled is false: full r/w mode
> + * (2) successor is NULL and disabled is true: read only mode ("disabled")
> + * (3) successor is set: frozen mode.
> + * A frozen bitmap cannot be renamed, deleted, anonymized, cleared, set,
> + * or enabled. A frozen bitmap can only abdicate() or reclaim().
> + */
> +struct BdrvDirtyBitmap {
> + HBitmap *bitmap; /* Dirty sector bitmap implementation */
> + BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */
> + char *name; /* Optional non-empty unique ID */
> + int64_t size; /* Size of the bitmap (Number of sectors) */
> + bool disabled; /* Bitmap is read-only */
> + QLIST_ENTRY(BdrvDirtyBitmap) list;
> +};
> +
> +BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, const char *name)
> +{
> + BdrvDirtyBitmap *bm;
> +
> + assert(name);
> + QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
> + if (bm->name && !strcmp(name, bm->name)) {
> + return bm;
> + }
> + }
> + return NULL;
> +}
> +
> +void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap)
> +{
> + assert(!bdrv_dirty_bitmap_frozen(bitmap));
> + g_free(bitmap->name);
> + bitmap->name = NULL;
> +}
> +
> +BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
> + uint32_t granularity,
> + const char *name,
> + Error **errp)
> +{
> + int64_t bitmap_size;
> + BdrvDirtyBitmap *bitmap;
> + uint32_t sector_granularity;
> +
> + assert((granularity & (granularity - 1)) == 0);
> +
> + if (name && bdrv_find_dirty_bitmap(bs, name)) {
> + error_setg(errp, "Bitmap already exists: %s", name);
> + return NULL;
> + }
> + sector_granularity = granularity >> BDRV_SECTOR_BITS;
> + assert(sector_granularity);
> + bitmap_size = bdrv_nb_sectors(bs);
> + if (bitmap_size < 0) {
> + error_setg_errno(errp, -bitmap_size, "could not get length of device");
> + errno = -bitmap_size;
> + return NULL;
> + }
> + bitmap = g_new0(BdrvDirtyBitmap, 1);
> + bitmap->bitmap = hbitmap_alloc(bitmap_size, ctz32(sector_granularity));
> + bitmap->size = bitmap_size;
> + bitmap->name = g_strdup(name);
> + bitmap->disabled = false;
> + QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
> + return bitmap;
> +}
> +
> +bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
> +{
> + return bitmap->successor;
> +}
> +
> +bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
> +{
> + return !(bitmap->disabled || bitmap->successor);
> +}
> +
> +DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap)
> +{
> + if (bdrv_dirty_bitmap_frozen(bitmap)) {
> + return DIRTY_BITMAP_STATUS_FROZEN;
> + } else if (!bdrv_dirty_bitmap_enabled(bitmap)) {
> + return DIRTY_BITMAP_STATUS_DISABLED;
> + } else {
> + return DIRTY_BITMAP_STATUS_ACTIVE;
> + }
> +}
> +
> +/**
> + * Create a successor bitmap destined to replace this bitmap after an operation.
> + * Requires that the bitmap is not frozen and has no successor.
> + */
> +int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
> + BdrvDirtyBitmap *bitmap, Error **errp)
> +{
> + uint64_t granularity;
> + BdrvDirtyBitmap *child;
> +
> + if (bdrv_dirty_bitmap_frozen(bitmap)) {
> + error_setg(errp, "Cannot create a successor for a bitmap that is "
> + "currently frozen");
> + return -1;
> + }
> + assert(!bitmap->successor);
> +
> + /* Create an anonymous successor */
> + granularity = bdrv_dirty_bitmap_granularity(bitmap);
> + child = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
> + if (!child) {
> + return -1;
> + }
> +
> + /* Successor will be on or off based on our current state. */
> + child->disabled = bitmap->disabled;
> +
> + /* Install the successor and freeze the parent */
> + bitmap->successor = child;
> + return 0;
> +}
> +
> +/**
> + * For a bitmap with a successor, yield our name to the successor,
> + * delete the old bitmap, and return a handle to the new bitmap.
> + */
> +BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
> + BdrvDirtyBitmap *bitmap,
> + Error **errp)
> +{
> + char *name;
> + BdrvDirtyBitmap *successor = bitmap->successor;
> +
> + if (successor == NULL) {
> + error_setg(errp, "Cannot relinquish control if "
> + "there's no successor present");
> + return NULL;
> + }
> +
> + name = bitmap->name;
> + bitmap->name = NULL;
> + successor->name = name;
> + bitmap->successor = NULL;
> + bdrv_release_dirty_bitmap(bs, bitmap);
> +
> + return successor;
> +}
> +
> +/**
> + * In cases of failure where we can no longer safely delete the parent,
> + * we may wish to re-join the parent and child/successor.
> + * The merged parent will be un-frozen, but not explicitly re-enabled.
> + */
> +BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
> + BdrvDirtyBitmap *parent,
> + Error **errp)
> +{
> + BdrvDirtyBitmap *successor = parent->successor;
> +
> + if (!successor) {
> + error_setg(errp, "Cannot reclaim a successor when none is present");
> + return NULL;
> + }
> +
> + if (!hbitmap_merge(parent->bitmap, successor->bitmap)) {
> + error_setg(errp, "Merging of parent and successor bitmap failed");
> + return NULL;
> + }
> + bdrv_release_dirty_bitmap(bs, successor);
> + parent->successor = NULL;
> +
> + return parent;
> +}
> +
> +/**
> + * Truncates _all_ bitmaps attached to a BDS.
> + */
> +void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
> +{
> + BdrvDirtyBitmap *bitmap;
> + uint64_t size = bdrv_nb_sectors(bs);
> +
> + QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
> + assert(!bdrv_dirty_bitmap_frozen(bitmap));
> + hbitmap_truncate(bitmap->bitmap, size);
> + bitmap->size = size;
> + }
> +}
> +
> +void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
> +{
> + BdrvDirtyBitmap *bm, *next;
> + QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
> + if (bm == bitmap) {
> + assert(!bdrv_dirty_bitmap_frozen(bm));
> + QLIST_REMOVE(bitmap, list);
> + hbitmap_free(bitmap->bitmap);
> + g_free(bitmap->name);
> + g_free(bitmap);
> + return;
> + }
> + }
> +}
> +
> +void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
> +{
> + assert(!bdrv_dirty_bitmap_frozen(bitmap));
> + bitmap->disabled = true;
> +}
> +
> +void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
> +{
> + assert(!bdrv_dirty_bitmap_frozen(bitmap));
> + bitmap->disabled = false;
> +}
> +
> +BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
> +{
> + BdrvDirtyBitmap *bm;
> + BlockDirtyInfoList *list = NULL;
> + BlockDirtyInfoList **plist = &list;
> +
> + QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
> + BlockDirtyInfo *info = g_new0(BlockDirtyInfo, 1);
> + BlockDirtyInfoList *entry = g_new0(BlockDirtyInfoList, 1);
> + info->count = bdrv_get_dirty_count(bm);
> + info->granularity = bdrv_dirty_bitmap_granularity(bm);
> + info->has_name = !!bm->name;
> + info->name = g_strdup(bm->name);
> + info->status = bdrv_dirty_bitmap_status(bm);
> + entry->value = info;
> + *plist = entry;
> + plist = &entry->next;
> + }
> +
> + return list;
> +}
> +
> +int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector)
> +{
> + if (bitmap) {
> + return hbitmap_get(bitmap->bitmap, sector);
> + } else {
> + return 0;
> + }
> +}
> +
> +/**
> + * Chooses a default granularity based on the existing cluster size,
> + * but clamped between [4K, 64K]. Defaults to 64K in the case that there
> + * is no cluster size information available.
> + */
> +uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs)
> +{
> + BlockDriverInfo bdi;
> + uint32_t granularity;
> +
> + if (bdrv_get_info(bs, &bdi) >= 0 && bdi.cluster_size > 0) {
> + granularity = MAX(4096, bdi.cluster_size);
> + granularity = MIN(65536, granularity);
> + } else {
> + granularity = 65536;
> + }
> +
> + return granularity;
> +}
> +
> +uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap)
> +{
> + return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
> +}
> +
> +void bdrv_dirty_iter_init(BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
> +{
> + hbitmap_iter_init(hbi, bitmap->bitmap, 0);
> +}
> +
> +void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> + int64_t cur_sector, int nr_sectors)
> +{
> + assert(bdrv_dirty_bitmap_enabled(bitmap));
> + hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
> +}
> +
> +void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> + int64_t cur_sector, int nr_sectors)
> +{
> + assert(bdrv_dirty_bitmap_enabled(bitmap));
> + hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
> +}
> +
> +void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
> +{
> + assert(bdrv_dirty_bitmap_enabled(bitmap));
> + if (!out) {
> + hbitmap_reset_all(bitmap->bitmap);
> + } else {
> + HBitmap *backup = bitmap->bitmap;
> + bitmap->bitmap = hbitmap_alloc(bitmap->size,
> + hbitmap_granularity(backup));
> + *out = backup;
> + }
> +}
> +
> +void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in)
> +{
> + HBitmap *tmp = bitmap->bitmap;
> + assert(bdrv_dirty_bitmap_enabled(bitmap));
> + bitmap->bitmap = in;
> + hbitmap_free(tmp);
> +}
> +
> +void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
> + int nr_sectors)
> +{
> + BdrvDirtyBitmap *bitmap;
> + QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
> + if (!bdrv_dirty_bitmap_enabled(bitmap)) {
> + continue;
> + }
> + hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
> + }
> +}
> +
> +/**
> + * Advance an HBitmapIter to an arbitrary offset.
> + */
> +void bdrv_set_dirty_iter(HBitmapIter *hbi, int64_t offset)
> +{
> + assert(hbi->hb);
> + hbitmap_iter_init(hbi, hbi->hb, offset);
> +}
> +
> +int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
> +{
> + return hbitmap_count(bitmap->bitmap);
> +}
> diff --git a/include/block/block.h b/include/block/block.h
> index db8e096..97e9b5e 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -6,6 +6,7 @@
> #include "qemu/option.h"
> #include "qemu/coroutine.h"
> #include "block/accounting.h"
> +#include "block/dirty-bitmap.h"
> #include "qapi/qmp/qobject.h"
> #include "qapi-types.h"
>
> @@ -471,42 +472,6 @@ void *qemu_try_blockalign(BlockDriverState *bs, size_t size);
> void *qemu_try_blockalign0(BlockDriverState *bs, size_t size);
> bool bdrv_qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov);
>
> -struct HBitmapIter;
> -typedef struct BdrvDirtyBitmap BdrvDirtyBitmap;
> -BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
> - uint32_t granularity,
> - const char *name,
> - Error **errp);
> -int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
> - BdrvDirtyBitmap *bitmap,
> - Error **errp);
> -BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
> - BdrvDirtyBitmap *bitmap,
> - Error **errp);
> -BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
> - BdrvDirtyBitmap *bitmap,
> - Error **errp);
> -BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
> - const char *name);
> -void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap);
> -void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
> -void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
> -void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
> -BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
> -uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
> -uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap);
> -bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
> -bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap);
> -DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap);
> -int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector);
> -void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> - int64_t cur_sector, int nr_sectors);
> -void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> - int64_t cur_sector, int nr_sectors);
> -void bdrv_dirty_iter_init(BdrvDirtyBitmap *bitmap, struct HBitmapIter *hbi);
> -void bdrv_set_dirty_iter(struct HBitmapIter *hbi, int64_t offset);
> -int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
> -
> void bdrv_enable_copy_on_read(BlockDriverState *bs);
> void bdrv_disable_copy_on_read(BlockDriverState *bs);
>
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> new file mode 100644
> index 0000000..6175cf3
> --- /dev/null
> +++ b/include/block/dirty-bitmap.h
> @@ -0,0 +1,42 @@
> +#ifndef BLOCK_DIRTY_BITMAP_H
> +#define BLOCK_DIRTY_BITMAP_H
> +
> +#include "qemu-common.h"
> +
> +typedef struct BdrvDirtyBitmap BdrvDirtyBitmap;
> +BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
> + uint32_t granularity,
> + const char *name,
> + Error **errp);
> +int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
> + BdrvDirtyBitmap *bitmap,
> + Error **errp);
> +BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
> + BdrvDirtyBitmap *bitmap,
> + Error **errp);
> +BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
> + BdrvDirtyBitmap *bitmap,
> + Error **errp);
> +BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
> + const char *name);
> +void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap);
> +void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
> +void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
> +void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
> +BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
> +uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
> +uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap);
> +bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
> +bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap);
> +DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap);
> +int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector);
> +void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> + int64_t cur_sector, int nr_sectors);
> +void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> + int64_t cur_sector, int nr_sectors);
> +void bdrv_dirty_iter_init(BdrvDirtyBitmap *bitmap, struct HBitmapIter *hbi);
> +void bdrv_set_dirty_iter(struct HBitmapIter *hbi, int64_t offset);
> +int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
> +void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
> +
> +#endif
>
yes!!!
Reviewed-by: John Snow <jsnow@redhat.com>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 04/13] block: Remove unused typedef of BlockDriverDirtyHandler
2016-01-04 10:27 ` [Qemu-devel] [PATCH 04/13] block: Remove unused typedef of BlockDriverDirtyHandler Fam Zheng
@ 2016-01-05 22:35 ` John Snow
0 siblings, 0 replies; 36+ messages in thread
From: John Snow @ 2016-01-05 22:35 UTC (permalink / raw)
To: Fam Zheng, qemu-devel
Cc: Kevin Wolf, Jeff Cody, Vladimir Sementsov-Ogievskiy, qemu-block
On 01/04/2016 05:27 AM, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> include/block/block.h | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/include/block/block.h b/include/block/block.h
> index 97e9b5e..0f42964 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -320,8 +320,6 @@ BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs,
> const char *node_name, Error **errp);
>
> /* async block I/O */
> -typedef void BlockDriverDirtyHandler(BlockDriverState *bs, int64_t sector,
> - int sector_num);
> BlockAIOCB *bdrv_aio_readv(BlockDriverState *bs, int64_t sector_num,
> QEMUIOVector *iov, int nb_sectors,
> BlockCompletionFunc *cb, void *opaque);
>
Can be pulled independently of this series.
Reviewed-by: John Snow <jsnow@redhat.com>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 05/13] block: Hide HBitmap in block dirty bitmap interface
2016-01-04 10:27 ` [Qemu-devel] [PATCH 05/13] block: Hide HBitmap in block dirty bitmap interface Fam Zheng
@ 2016-01-05 23:01 ` John Snow
2016-01-20 5:09 ` Fam Zheng
0 siblings, 1 reply; 36+ messages in thread
From: John Snow @ 2016-01-05 23:01 UTC (permalink / raw)
To: Fam Zheng, qemu-devel
Cc: Kevin Wolf, Jeff Cody, Vladimir Sementsov-Ogievskiy, qemu-block
Should we skip adding the typedef for HBitmapIter if we're just going to
use this instead?
On 01/04/2016 05:27 AM, Fam Zheng wrote:
> HBitmap is an implementation detail of block dirty bitmap that should be hidden
> from users. Introduce a BdrvDirtyBitmapIter to encapsulate the underlying
> HBitmapIter.
>
> A small difference in the interface is, before, an HBitmapIter is initialized
> in place, now the new BdrvDirtyBitmapIter must be dynamically allocated because
> the structure definition is in block.c.
>
> Two current users are converted too.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> block/backup.c | 14 ++++++++------
> block/dirty-bitmap.c | 36 +++++++++++++++++++++++++++++++-----
> block/mirror.c | 14 ++++++++------
> include/block/dirty-bitmap.h | 7 +++++--
> include/qemu/typedefs.h | 1 +
> 5 files changed, 53 insertions(+), 19 deletions(-)
>
> diff --git a/block/backup.c b/block/backup.c
> index 56ddec0..2388039 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -326,14 +326,14 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
> int64_t end;
> int64_t last_cluster = -1;
> BlockDriverState *bs = job->common.bs;
> - HBitmapIter hbi;
> + BdrvDirtyBitmapIter *dbi;
>
> granularity = bdrv_dirty_bitmap_granularity(job->sync_bitmap);
> clusters_per_iter = MAX((granularity / BACKUP_CLUSTER_SIZE), 1);
> - bdrv_dirty_iter_init(job->sync_bitmap, &hbi);
> + dbi = bdrv_dirty_iter_new(job->sync_bitmap, 0);
>
> /* Find the next dirty sector(s) */
> - while ((sector = hbitmap_iter_next(&hbi)) != -1) {
> + while ((sector = bdrv_dirty_iter_next(dbi)) != -1) {
> cluster = sector / BACKUP_SECTORS_PER_CLUSTER;
>
> /* Fake progress updates for any clusters we skipped */
> @@ -345,7 +345,7 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
> for (end = cluster + clusters_per_iter; cluster < end; cluster++) {
> do {
> if (yield_and_check(job)) {
> - return ret;
> + goto out;
> }
> ret = backup_do_cow(bs, cluster * BACKUP_SECTORS_PER_CLUSTER,
> BACKUP_SECTORS_PER_CLUSTER, &error_is_read,
> @@ -353,7 +353,7 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
> if ((ret < 0) &&
> backup_error_action(job, error_is_read, -ret) ==
> BLOCK_ERROR_ACTION_REPORT) {
> - return ret;
> + goto out;
> }
> } while (ret < 0);
> }
> @@ -361,7 +361,7 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
> /* If the bitmap granularity is smaller than the backup granularity,
> * we need to advance the iterator pointer to the next cluster. */
> if (granularity < BACKUP_CLUSTER_SIZE) {
> - bdrv_set_dirty_iter(&hbi, cluster * BACKUP_SECTORS_PER_CLUSTER);
> + bdrv_set_dirty_iter(dbi, cluster * BACKUP_SECTORS_PER_CLUSTER);
> }
>
> last_cluster = cluster - 1;
> @@ -373,6 +373,8 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
> job->common.offset += ((end - last_cluster - 1) * BACKUP_CLUSTER_SIZE);
> }
>
> +out:
> + bdrv_dirty_iter_free(dbi);
> return ret;
> }
>
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 7924c38..53cf88d 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -41,9 +41,15 @@ struct BdrvDirtyBitmap {
> char *name; /* Optional non-empty unique ID */
> int64_t size; /* Size of the bitmap (Number of sectors) */
> bool disabled; /* Bitmap is read-only */
> + int active_iterators; /* How many iterators are active */
> QLIST_ENTRY(BdrvDirtyBitmap) list;
> };
>
> +struct BdrvDirtyBitmapIter {
> + HBitmapIter hbi;
> + BdrvDirtyBitmap *bitmap;
> +};
> +
> BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, const char *name)
> {
> BdrvDirtyBitmap *bm;
> @@ -221,6 +227,7 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
> BdrvDirtyBitmap *bm, *next;
> QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
> if (bm == bitmap) {
> + assert(!bitmap->active_iterators);
Should we add any assertions into the truncate function, too?
> assert(!bdrv_dirty_bitmap_frozen(bm));
> QLIST_REMOVE(bitmap, list);
> hbitmap_free(bitmap->bitmap);
> @@ -299,9 +306,29 @@ uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap)
> return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
> }
>
> -void bdrv_dirty_iter_init(BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
> +BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap,
> + uint64_t first_sector)
> {
> - hbitmap_iter_init(hbi, bitmap->bitmap, 0);
> + BdrvDirtyBitmapIter *iter = g_new(BdrvDirtyBitmapIter, 1);
> + hbitmap_iter_init(&iter->hbi, bitmap->bitmap, first_sector);
> + iter->bitmap = bitmap;
> + bitmap->active_iterators++;
> + return iter;
> +}
> +
> +void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter)
> +{
> + if (!iter) {
> + return;
> + }
> + assert(iter->bitmap->active_iterators > 0);
> + iter->bitmap->active_iterators--;
> + g_free(iter);
> +}
> +
> +int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter)
> +{
> + return hbitmap_iter_next(&iter->hbi);
> }
>
> void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> @@ -354,10 +381,9 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
> /**
> * Advance an HBitmapIter to an arbitrary offset.
> */
> -void bdrv_set_dirty_iter(HBitmapIter *hbi, int64_t offset)
> +void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *iter, int64_t sector_num)
> {
> - assert(hbi->hb);
> - hbitmap_iter_init(hbi, hbi->hb, offset);
> + hbitmap_iter_init(&iter->hbi, iter->bitmap->bitmap, sector_num);
> }
>
> int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
> diff --git a/block/mirror.c b/block/mirror.c
> index f201f2b..70ca844 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -52,7 +52,7 @@ typedef struct MirrorBlockJob {
> int64_t bdev_length;
> unsigned long *cow_bitmap;
> BdrvDirtyBitmap *dirty_bitmap;
> - HBitmapIter hbi;
> + BdrvDirtyBitmapIter *dbi;
> uint8_t *buf;
> QSIMPLEQ_HEAD(, MirrorBuffer) buf_free;
> int buf_free_count;
> @@ -170,10 +170,11 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
>
> max_iov = MIN(source->bl.max_iov, s->target->bl.max_iov);
>
> - s->sector_num = hbitmap_iter_next(&s->hbi);
> + s->sector_num = bdrv_dirty_iter_next(s->dbi);
> if (s->sector_num < 0) {
> - bdrv_dirty_iter_init(s->dirty_bitmap, &s->hbi);
> - s->sector_num = hbitmap_iter_next(&s->hbi);
> + bdrv_dirty_iter_free(s->dbi);
> + s->dbi = bdrv_dirty_iter_new(s->dirty_bitmap, 0);
> + s->sector_num = bdrv_dirty_iter_next(s->dbi);
> trace_mirror_restart_iter(s, bdrv_get_dirty_count(s->dirty_bitmap));
> assert(s->sector_num >= 0);
> }
> @@ -291,7 +292,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
> */
> if (next_sector > hbitmap_next_sector
> && bdrv_get_dirty(source, s->dirty_bitmap, next_sector)) {
> - hbitmap_next_sector = hbitmap_iter_next(&s->hbi);
> + hbitmap_next_sector = bdrv_dirty_iter_next(s->dbi);
> }
>
> next_sector += sectors_per_chunk;
> @@ -502,7 +503,7 @@ static void coroutine_fn mirror_run(void *opaque)
> }
> }
>
> - bdrv_dirty_iter_init(s->dirty_bitmap, &s->hbi);
> + s->dbi = bdrv_dirty_iter_new(s->dirty_bitmap, 0);
> for (;;) {
> uint64_t delay_ns = 0;
> int64_t cnt;
> @@ -615,6 +616,7 @@ immediate_exit:
> qemu_vfree(s->buf);
> g_free(s->cow_bitmap);
> g_free(s->in_flight_bitmap);
> + bdrv_dirty_iter_free(s->dbi);
> bdrv_release_dirty_bitmap(bs, s->dirty_bitmap);
> if (s->target->blk) {
> blk_iostatus_disable(s->target->blk);
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 6175cf3..16bb15a 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -34,8 +34,11 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> int64_t cur_sector, int nr_sectors);
> void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> int64_t cur_sector, int nr_sectors);
> -void bdrv_dirty_iter_init(BdrvDirtyBitmap *bitmap, struct HBitmapIter *hbi);
> -void bdrv_set_dirty_iter(struct HBitmapIter *hbi, int64_t offset);
> +BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap,
> + uint64_t first_sector);
> +void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter);
> +int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter);
> +void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t sector_num);
> int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
> void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
>
> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> index e83934e..0e9efcd 100644
> --- a/include/qemu/typedefs.h
> +++ b/include/qemu/typedefs.h
> @@ -11,6 +11,7 @@ typedef struct AioContext AioContext;
> typedef struct AllwinnerAHCIState AllwinnerAHCIState;
> typedef struct AudioState AudioState;
> typedef struct BdrvDirtyBitmap BdrvDirtyBitmap;
> +typedef struct BdrvDirtyBitmapIter BdrvDirtyBitmapIter;
> typedef struct BlockBackend BlockBackend;
> typedef struct BlockBackendRootState BlockBackendRootState;
> typedef struct BlockDriverState BlockDriverState;
>
Regardless of answers to above questions:
Reviewed-by: John Snow <jsnow@redhat.com>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 06/13] HBitmap: Introduce "meta" bitmap to track bit changes
2016-01-04 10:27 ` [Qemu-devel] [PATCH 06/13] HBitmap: Introduce "meta" bitmap to track bit changes Fam Zheng
@ 2016-01-06 0:09 ` John Snow
2016-01-11 15:40 ` Vladimir Sementsov-Ogievskiy
1 sibling, 0 replies; 36+ messages in thread
From: John Snow @ 2016-01-06 0:09 UTC (permalink / raw)
To: Fam Zheng, qemu-devel
Cc: Kevin Wolf, Jeff Cody, Vladimir Sementsov-Ogievskiy, qemu-block
On 01/04/2016 05:27 AM, Fam Zheng wrote:
> Upon each bit toggle, the corresponding bit in the meta bitmap will be
> set.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> include/qemu/hbitmap.h | 8 +++++++
> util/hbitmap.c | 61 +++++++++++++++++++++++++++++++++++++-------------
> 2 files changed, 54 insertions(+), 15 deletions(-)
>
> diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
> index bb94a00..ed672e7 100644
> --- a/include/qemu/hbitmap.h
> +++ b/include/qemu/hbitmap.h
> @@ -181,6 +181,14 @@ void hbitmap_iter_init(HBitmapIter *hbi, const HBitmap *hb, uint64_t first);
> */
> unsigned long hbitmap_iter_skip_words(HBitmapIter *hbi);
>
> +/* hbitmap_create_meta
> + * Create a "meta" hbitmap to track dirtiness of the bits in this HBitmap.
> + *
> + * @hb: The HBitmap to operate on.
> + * @chunk_size: How many bits in @hb does one bit in the meta track.
> + */
> +HBitmap *hbitmap_create_meta(HBitmap *hb, int chunk_size);
> +
> /**
> * hbitmap_iter_next:
> * @hbi: HBitmapIter to operate on.
> diff --git a/util/hbitmap.c b/util/hbitmap.c
> index 50b888f..55d3182 100644
> --- a/util/hbitmap.c
> +++ b/util/hbitmap.c
> @@ -81,6 +81,9 @@ struct HBitmap {
> */
> int granularity;
>
> + /* A meta dirty bitmap to track the dirtiness of bits in this HBitmap. */
> + HBitmap *meta;
> +
> /* A number of progressively less coarse bitmaps (i.e. level 0 is the
> * coarsest). Each bit in level N represents a word in level N+1 that
> * has a set bit, except the last level where each bit represents the
> @@ -212,25 +215,27 @@ static uint64_t hb_count_between(HBitmap *hb, uint64_t start, uint64_t last)
> }
>
> /* Setting starts at the last layer and propagates up if an element
> - * changes from zero to non-zero.
> + * changes.
> */
Isn't this comment wrong anyway? hb_set_elem does not propagate upward
by itself.
> static inline bool hb_set_elem(unsigned long *elem, uint64_t start, uint64_t last)
> {
> unsigned long mask;
> - bool changed;
> + unsigned long old;
>
> assert((last >> BITS_PER_LEVEL) == (start >> BITS_PER_LEVEL));
> assert(start <= last);
>
> mask = 2UL << (last & (BITS_PER_LONG - 1));
> mask -= 1UL << (start & (BITS_PER_LONG - 1));
> - changed = (*elem == 0);
> + old = *elem;
> *elem |= mask;
> - return changed;
> + return old != *elem;
> }
>
> -/* The recursive workhorse (the depth is limited to HBITMAP_LEVELS)... */
> -static void hb_set_between(HBitmap *hb, int level, uint64_t start, uint64_t last)
> +/* The recursive workhorse (the depth is limited to HBITMAP_LEVELS)...
> + * Returns true if at least one bit is changed. */
> +static bool hb_set_between(HBitmap *hb, int level, uint64_t start,
> + uint64_t last)
> {
> size_t pos = start >> BITS_PER_LEVEL;
> size_t lastpos = last >> BITS_PER_LEVEL;
> @@ -259,22 +264,27 @@ static void hb_set_between(HBitmap *hb, int level, uint64_t start, uint64_t last
> if (level > 0 && changed) {
> hb_set_between(hb, level - 1, pos, lastpos);
> }
> + return changed;
> }
>
> void hbitmap_set(HBitmap *hb, uint64_t start, uint64_t count)
> {
> /* Compute range in the last layer. */
> + uint64_t first, n;
> uint64_t last = start + count - 1;
>
> trace_hbitmap_set(hb, start, count,
> start >> hb->granularity, last >> hb->granularity);
>
> - start >>= hb->granularity;
> + first = start >> hb->granularity;
> last >>= hb->granularity;
> - count = last - start + 1;
> + n = last - first + 1;
>
> - hb->count += count - hb_count_between(hb, start, last);
> - hb_set_between(hb, HBITMAP_LEVELS - 1, start, last);
> + hb->count += n - hb_count_between(hb, first, last);
> + if (hb_set_between(hb, HBITMAP_LEVELS - 1, first, last) &&
> + hb->meta) {
> + hbitmap_set(hb->meta, start, count);
> + }
> }
>
> /* Resetting works the other way round: propagate up if the new
> @@ -295,8 +305,10 @@ static inline bool hb_reset_elem(unsigned long *elem, uint64_t start, uint64_t l
> return blanked;
> }
>
> -/* The recursive workhorse (the depth is limited to HBITMAP_LEVELS)... */
> -static void hb_reset_between(HBitmap *hb, int level, uint64_t start, uint64_t last)
> +/* The recursive workhorse (the depth is limited to HBITMAP_LEVELS)...
> + * Returns true if at least one bit is changed. */
> +static bool hb_reset_between(HBitmap *hb, int level, uint64_t start,
> + uint64_t last)
> {
> size_t pos = start >> BITS_PER_LEVEL;
> size_t lastpos = last >> BITS_PER_LEVEL;
> @@ -339,21 +351,28 @@ static void hb_reset_between(HBitmap *hb, int level, uint64_t start, uint64_t la
> if (level > 0 && changed) {
> hb_reset_between(hb, level - 1, pos, lastpos);
> }
> +
> + return changed;
> +
> }
>
> void hbitmap_reset(HBitmap *hb, uint64_t start, uint64_t count)
> {
> /* Compute range in the last layer. */
> + uint64_t first;
> uint64_t last = start + count - 1;
>
> trace_hbitmap_reset(hb, start, count,
> start >> hb->granularity, last >> hb->granularity);
>
> - start >>= hb->granularity;
> + first = start >> hb->granularity;
> last >>= hb->granularity;
>
> - hb->count -= hb_count_between(hb, start, last);
> - hb_reset_between(hb, HBITMAP_LEVELS - 1, start, last);
> + hb->count -= hb_count_between(hb, first, last);
> + if (hb_reset_between(hb, HBITMAP_LEVELS - 1, first, last) &&
> + hb->meta) {
> + hbitmap_set(hb->meta, start, count);
> + }
> }
>
> void hbitmap_reset_all(HBitmap *hb)
> @@ -384,6 +403,9 @@ void hbitmap_free(HBitmap *hb)
> for (i = HBITMAP_LEVELS; i-- > 0; ) {
> g_free(hb->levels[i]);
> }
> + if (hb->meta) {
> + hbitmap_free(hb->meta);
> + }
> g_free(hb);
> }
>
> @@ -493,3 +515,12 @@ bool hbitmap_merge(HBitmap *a, const HBitmap *b)
>
> return true;
> }
> +
> +HBitmap *hbitmap_create_meta(HBitmap *hb, int chunk_size)
> +{
> + assert(!(chunk_size & (chunk_size - 1)));
> + assert(!hb->meta);
> + hb->meta = hbitmap_alloc(hb->size << hb->granularity,
> + hb->granularity + ctz32(chunk_size));
> + return hb->meta;
> +}
>
I am a little skeptical of returning handles to internal state, but it's
the easiest way to re-use all of the existing HBitmap infrastructure to
iterate over the meta bitmap, so I guess this is fine.
Should we also add an hbitmap_destroy_meta for when we're done with it?
Regardless;
Reviewed-by: John Snow <jsnow@redhat.com>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 07/13] tests: Add test code for meta bitmap
2016-01-04 10:27 ` [Qemu-devel] [PATCH 07/13] tests: Add test code for meta bitmap Fam Zheng
@ 2016-01-06 20:46 ` John Snow
0 siblings, 0 replies; 36+ messages in thread
From: John Snow @ 2016-01-06 20:46 UTC (permalink / raw)
To: Fam Zheng, qemu-devel
Cc: Kevin Wolf, Jeff Cody, Vladimir Sementsov-Ogievskiy, qemu-block
On 01/04/2016 05:27 AM, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> tests/test-hbitmap.c | 113 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 113 insertions(+)
>
> diff --git a/tests/test-hbitmap.c b/tests/test-hbitmap.c
> index abcea0c..19136e7 100644
> --- a/tests/test-hbitmap.c
> +++ b/tests/test-hbitmap.c
> @@ -14,6 +14,7 @@
> #include <string.h>
> #include <sys/types.h>
> #include "qemu/hbitmap.h"
> +#include "block/block.h"
>
> #define LOG_BITS_PER_LONG (BITS_PER_LONG == 32 ? 5 : 6)
>
> @@ -23,6 +24,7 @@
>
> typedef struct TestHBitmapData {
> HBitmap *hb;
> + HBitmap *meta;
> unsigned long *bits;
> size_t size;
> size_t old_size;
> @@ -94,6 +96,14 @@ static void hbitmap_test_init(TestHBitmapData *data,
> }
> }
>
> +static void hbitmap_test_init_meta(TestHBitmapData *data,
> + uint64_t size, int granularity,
> + int meta_chunk)
> +{
> + hbitmap_test_init(data, size, granularity);
> + data->meta = hbitmap_create_meta(data->hb, meta_chunk);
> +}
> +
> static inline size_t hbitmap_test_array_size(size_t bits)
> {
> size_t n = (bits + BITS_PER_LONG - 1) / BITS_PER_LONG;
> @@ -637,6 +647,103 @@ static void test_hbitmap_truncate_shrink_large(TestHBitmapData *data,
> hbitmap_test_truncate(data, size, -diff, 0);
> }
>
> +static void hbitmap_check_meta(TestHBitmapData *data,
> + int64_t start, int count)
> +{
> + int64_t i;
> +
> + for (i = 0; i < data->size; i++) {
> + if (i >= start && i < start + count) {
> + g_assert(hbitmap_get(data->meta, i));
> + } else {
> + g_assert(!hbitmap_get(data->meta, i));
> + }
> + }
> +}
> +
> +static void hbitmap_test_meta(TestHBitmapData *data,
> + int64_t start, int count,
> + int64_t check_start, int check_count)
> +{
> + hbitmap_reset_all(data->hb);
> + hbitmap_reset_all(data->meta);
> +
> + /* Test "unset" -> "unset" will not update meta. */
> + hbitmap_reset(data->hb, start, count);
> + hbitmap_check_meta(data, 0, 0);
> +
> + /* Test "unset" -> "set" will update meta */
> + hbitmap_set(data->hb, start, count);
> + hbitmap_check_meta(data, check_start, check_count);
> +
> + /* Test "set" -> "set" will not update meta */
> + hbitmap_reset_all(data->meta);
> + hbitmap_set(data->hb, start, count);
> + hbitmap_check_meta(data, 0, 0);
> +
> + /* Test "set" -> "unset" will update meta */
> + hbitmap_reset_all(data->meta);
> + hbitmap_reset(data->hb, start, count);
> + hbitmap_check_meta(data, check_start, check_count);
> +}
> +
> +static void hbitmap_test_meta_do(TestHBitmapData *data, int chunk_size)
> +{
> + uint64_t size = chunk_size * 100;
> + hbitmap_test_init_meta(data, size, 0, chunk_size);
> +
> + hbitmap_test_meta(data, 0, 1, 0, chunk_size);
> + hbitmap_test_meta(data, 0, chunk_size, 0, chunk_size);
> + hbitmap_test_meta(data, chunk_size - 1, 1, 0, chunk_size);
> + hbitmap_test_meta(data, chunk_size - 1, 2, 0, chunk_size * 2);
> + hbitmap_test_meta(data, chunk_size - 1, chunk_size + 1, 0, chunk_size * 2);
> + hbitmap_test_meta(data, chunk_size - 1, chunk_size + 2, 0, chunk_size * 3);
> + hbitmap_test_meta(data, 7 * chunk_size - 1, chunk_size + 2,
> + 6 * chunk_size, chunk_size * 3);
> + hbitmap_test_meta(data, size - 1, 1, size - chunk_size, chunk_size);
> + hbitmap_test_meta(data, 0, size, 0, size);
> +}
> +
> +static void test_hbitmap_meta_byte(TestHBitmapData *data, const void *unused)
> +{
> + hbitmap_test_meta_do(data, BITS_PER_BYTE);
> +}
> +
> +static void test_hbitmap_meta_word(TestHBitmapData *data, const void *unused)
> +{
> + hbitmap_test_meta_do(data, BITS_PER_LONG);
> +}
> +
> +static void test_hbitmap_meta_sector(TestHBitmapData *data, const void *unused)
> +{
> + hbitmap_test_meta_do(data, BDRV_SECTOR_SIZE * BITS_PER_BYTE);
> +}
> +
> +/**
> + * Create an HBitmap and test set/unset.
> + */
> +static void test_hbitmap_meta_one(TestHBitmapData *data, const void *unused)
> +{
> + int i;
> + int64_t offsets[] = {
> + 0, 1, L1 - 1, L1, L1 + 1, L2 - 1, L2, L2 + 1, L3 - 1, L3, L3 + 1
> + };
> +
> + hbitmap_test_init_meta(data, L3 * 2, 0, 1);
> + for (i = 0; i < ARRAY_SIZE(offsets); i++) {
> + hbitmap_test_meta(data, offsets[i], 1, offsets[i], 1);
> + hbitmap_test_meta(data, offsets[i], L1, offsets[i], L1);
> + hbitmap_test_meta(data, offsets[i], L2, offsets[i], L2);
> + }
> +}
> +
> +static void test_hbitmap_meta_zero(TestHBitmapData *data, const void *unused)
> +{
> + hbitmap_test_init_meta(data, 0, 0, 1);
> +
> + hbitmap_check_meta(data, 0, 0);
> +}
> +
> static void hbitmap_test_add(const char *testpath,
> void (*test_func)(TestHBitmapData *data, const void *user_data))
> {
> @@ -686,6 +793,12 @@ int main(int argc, char **argv)
> test_hbitmap_truncate_grow_large);
> hbitmap_test_add("/hbitmap/truncate/shrink/large",
> test_hbitmap_truncate_shrink_large);
> +
> + hbitmap_test_add("/hbitmap/meta/zero", test_hbitmap_meta_zero);
> + hbitmap_test_add("/hbitmap/meta/one", test_hbitmap_meta_one);
> + hbitmap_test_add("/hbitmap/meta/byte", test_hbitmap_meta_byte);
> + hbitmap_test_add("/hbitmap/meta/word", test_hbitmap_meta_word);
> + hbitmap_test_add("/hbitmap/meta/sector", test_hbitmap_meta_sector);
> g_test_run();
>
> return 0;
>
looks quite thorough!
Reviewed-by: John Snow <jsnow@redhat.com>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 08/13] block: Support meta dirty bitmap
2016-01-04 10:27 ` [Qemu-devel] [PATCH 08/13] block: Support meta dirty bitmap Fam Zheng
@ 2016-01-07 19:30 ` John Snow
2016-01-20 6:07 ` Fam Zheng
0 siblings, 1 reply; 36+ messages in thread
From: John Snow @ 2016-01-07 19:30 UTC (permalink / raw)
To: Fam Zheng, qemu-devel
Cc: Kevin Wolf, Jeff Cody, Vladimir Sementsov-Ogievskiy, qemu-block
On 01/04/2016 05:27 AM, Fam Zheng wrote:
> The added group of operations enables tracking of the changed bits in
> the dirty bitmap.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> block/dirty-bitmap.c | 51 ++++++++++++++++++++++++++++++++++++++++++++
> include/block/dirty-bitmap.h | 9 ++++++++
> 2 files changed, 60 insertions(+)
>
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 53cf88d..4314659 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -37,6 +37,7 @@
> */
> struct BdrvDirtyBitmap {
> HBitmap *bitmap; /* Dirty sector bitmap implementation */
> + HBitmap *meta; /* Meta dirty bitmap */
IMO, this gets a little strange -- if I understand correctly, you're
using this meta pointer as a cache for the meta bitmap contained within
"bitmap," and not actually creating a new "standalone" HBitmap.
Since it has the same type as the prior "bitmap" member, though, it
makes it look like they're both the same kind of object ... when in
fact, one is the child of the other.
It's probably fine, but I was momentarily confused. I don't have a
better suggestion.
> BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */
> char *name; /* Optional non-empty unique ID */
> int64_t size; /* Size of the bitmap (Number of sectors) */
> @@ -102,6 +103,56 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
> return bitmap;
> }
>
> +/* bdrv_create_meta_dirty_bitmap
> + *
> + * Create a meta dirty bitmap that tracks the changes of bits in @bitmap. I.e.
> + * when a dirty status bit in @bitmap is changed (either from reset to set or
> + * the other way around), its respective meta dirty bitmap bit will be marked
> + * dirty as well.
> + *
> + * @bitmap: the block dirty bitmap for which to create a meta dirty bitmap.
> + * @granularity: how many bytes of bitmap data does each bit in the meta bitmap
> + * track.
> + */
> +void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> + int granularity)
> +{
> + assert(!bitmap->meta);
> + bitmap->meta = hbitmap_create_meta(bitmap->bitmap,
> + BDRV_SECTOR_SIZE * BITS_PER_BYTE);
> +}
> +
> +void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap)
> +{
> + assert(bitmap->meta);
> + hbitmap_free(bitmap->meta);
This leaves a dangling pointer inside the Hbitmap, no?
> + bitmap->meta = NULL;
> +}
> +
> +int bdrv_dirty_bitmap_get_meta(BlockDriverState *bs,
> + BdrvDirtyBitmap *bitmap, int64_t sector,
> + int nb_sectors)
> +{
> + uint64_t i;
> + int gran = bdrv_dirty_bitmap_granularity(bitmap) >> BDRV_SECTOR_BITS;
> +
> + /* To optimize: we can make hbitmap to internally check the range in a
> + * coarse level, or at least do it word by word. */
> + for (i = sector; i < sector + nb_sectors; i += gran) {
> + if (hbitmap_get(bitmap->meta, i)) {
> + return true;
> + }
> + }
> + return false;
> +}
> +
In essence get_meta() is a greedy algorithm that simply returns true if
anything is set between [sector, sector + nb_sectors], yes?
Is this more useful than just using an iterator directly on the
meta-bitmap?
I haven't finished reading but, I imagine that:
- If we need to check to see what is dirty specifically, we can just use
the iterator. If the iterator doesn't return anything, we know it's
empty. If it does return, we know exactly what's dirty.
- If we need to explicitly check for emptiness in general, we can use
the internal popcount.
I'm not sure when a 'dirty range bool' will be explicitly useful all by
itself, but maybe that becomes obvious later.
> +void bdrv_dirty_bitmap_reset_meta(BlockDriverState *bs,
> + BdrvDirtyBitmap *bitmap, int64_t sector,
> + int nb_sectors)
> +{
> + hbitmap_reset(bitmap->meta, sector, nb_sectors);
> +}
> +
> bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
> {
> return bitmap->successor;
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 16bb15a..0715220 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -8,6 +8,9 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
> uint32_t granularity,
> const char *name,
> Error **errp);
> +void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> + int granularity);
> +void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap);
> int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
> BdrvDirtyBitmap *bitmap,
> Error **errp);
> @@ -34,6 +37,12 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> int64_t cur_sector, int nr_sectors);
> void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> int64_t cur_sector, int nr_sectors);
> +int bdrv_dirty_bitmap_get_meta(BlockDriverState *bs,
> + BdrvDirtyBitmap *bitmap, int64_t sector,
> + int nb_sectors);
> +void bdrv_dirty_bitmap_reset_meta(BlockDriverState *bs,
> + BdrvDirtyBitmap *bitmap, int64_t sector,
> + int nb_sectors);
> BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap,
> uint64_t first_sector);
> void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter);
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 09/13] block: Add two dirty bitmap getters
2016-01-04 10:27 ` [Qemu-devel] [PATCH 09/13] block: Add two dirty bitmap getters Fam Zheng
@ 2016-01-07 19:35 ` John Snow
0 siblings, 0 replies; 36+ messages in thread
From: John Snow @ 2016-01-07 19:35 UTC (permalink / raw)
To: Fam Zheng, qemu-devel
Cc: Kevin Wolf, Jeff Cody, Vladimir Sementsov-Ogievskiy, qemu-block
On 01/04/2016 05:27 AM, Fam Zheng wrote:
> For dirty bitmap users to get the size and the name of a
> BdrvDirtyBitmap.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> block/dirty-bitmap.c | 10 ++++++++++
> include/block/dirty-bitmap.h | 2 ++
> 2 files changed, 12 insertions(+)
>
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 4314659..9cac794 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -153,6 +153,16 @@ void bdrv_dirty_bitmap_reset_meta(BlockDriverState *bs,
> hbitmap_reset(bitmap->meta, sector, nb_sectors);
> }
>
> +int64_t bdrv_dirty_bitmap_size(BdrvDirtyBitmap *bitmap)
> +{
> + return bitmap->size;
> +}
> +
> +const char *bdrv_dirty_bitmap_name(BdrvDirtyBitmap *bitmap)
> +{
> + return bitmap->name;
> +}
> +
> bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
> {
> return bitmap->successor;
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 0715220..e36efb6 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -31,6 +31,8 @@ uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
> uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap);
> bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
> bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap);
> +const char *bdrv_dirty_bitmap_name(BdrvDirtyBitmap *bitmap);
> +int64_t bdrv_dirty_bitmap_size(BdrvDirtyBitmap *bitmap);
> DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap);
> int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector);
> void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>
Reviewed-by: John Snow <jsnow@redhat.com>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 10/13] block: Assert that bdrv_release_dirty_bitmap succeeded
2016-01-04 10:27 ` [Qemu-devel] [PATCH 10/13] block: Assert that bdrv_release_dirty_bitmap succeeded Fam Zheng
@ 2016-01-07 19:38 ` John Snow
0 siblings, 0 replies; 36+ messages in thread
From: John Snow @ 2016-01-07 19:38 UTC (permalink / raw)
To: Fam Zheng, qemu-devel
Cc: Kevin Wolf, Jeff Cody, Vladimir Sementsov-Ogievskiy, qemu-block
On 01/04/2016 05:27 AM, Fam Zheng wrote:
> This makes sure we don't leak a dirty bitmap in any case.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> block/dirty-bitmap.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 9cac794..60ee965 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -297,6 +297,7 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
> return;
> }
> }
> + abort();
> }
>
> void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>
Before this patch: release can be called on a (BDS, BITMAP) pair in
cases where the bitmap does not belong to the BDS and this function just
ignored it.
After this patch: We abort() in cases where we get this pairing incorrect.
I don't think there is any reason to ever get this wrong on a call to
this function, so this is alright.
Reviewed-by: John Snow <jsnow@redhat.com>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 11/13] hbitmap: serialization
2016-01-04 10:27 ` [Qemu-devel] [PATCH 11/13] hbitmap: serialization Fam Zheng
@ 2016-01-07 21:11 ` John Snow
2016-01-11 15:12 ` Vladimir Sementsov-Ogievskiy
2016-01-11 14:48 ` Vladimir Sementsov-Ogievskiy
2016-01-11 15:19 ` Vladimir Sementsov-Ogievskiy
2 siblings, 1 reply; 36+ messages in thread
From: John Snow @ 2016-01-07 21:11 UTC (permalink / raw)
To: Fam Zheng, qemu-devel
Cc: Kevin Wolf, Jeff Cody, Vladimir Sementsov-Ogievskiy, qemu-block
On 01/04/2016 05:27 AM, Fam Zheng wrote:
> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> Functions to serialize / deserialize(restore) HBitmap. HBitmap should be
> saved to linear sequence of bits independently of endianness and bitmap
> array element (unsigned long) size. Therefore Little Endian is chosen.
>
> These functions are appropriate for dirty bitmap migration, restoring
> the bitmap in several steps is available. To save performance, every
> step writes only the last level of the bitmap. All other levels are
> restored by hbitmap_deserialize_finish() as a last step of restoring.
> So, HBitmap is inconsistent while restoring.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> [Fix left shift operand to 1UL; add "finish" parameter. - Fam]
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> include/qemu/hbitmap.h | 76 +++++++++++++++++++++++++++
> util/hbitmap.c | 136 +++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 212 insertions(+)
>
> diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
> index ed672e7..3414113 100644
> --- a/include/qemu/hbitmap.h
> +++ b/include/qemu/hbitmap.h
> @@ -149,6 +149,82 @@ void hbitmap_reset_all(HBitmap *hb);
> bool hbitmap_get(const HBitmap *hb, uint64_t item);
>
> /**
> + * hbitmap_data_size:
> + * @hb: HBitmap to operate on.
> + * @count: Number of bits
> + *
> + * Grunularity of serialization chunks, used by other serializetion functions.
> + * For every chunk:
> + * 1. Chunk start should be aligned to this granularity.
> + * 2. Chunk size should be aligned too, except for last chunk (for which
> + * start + count == hb->size)
> + */
Whoops, this is the documentation for the function below. Looks like you
updated this one instead of the one adjacent to the prototype, too.
> +uint64_t hbitmap_serialization_granularity(const HBitmap *hb);
> +
> +/**
> + * hbitmap_data_size:
> + * @hb: HBitmap to operate on.
> + * @count: Number of bits
> + *
> + * Return amount of bytes hbitmap_(de)serialize_part needs
> + */
> +uint64_t hbitmap_serialization_size(const HBitmap *hb,
> + uint64_t start, uint64_t count);
> +
> +/**
> + * hbitmap_serialize_part
> + * @hb: HBitmap to operate on.
> + * @buf: Buffer to store serialized bitmap.
> + * @start: First bit to store.
> + * @count: Number of bits to store.
> + *
> + * Stores HBitmap data corresponding to given region. The format of saved data
> + * is linear sequence of bits, so it can be used by hbitmap_deserialize_part
> + * independently of endianness and size of HBitmap level array elements
> + */
I suppose that the buffer needs to be pre-allocated and "count/8" bytes
long; but rather specifically it needs to be hbitmap_serialization_size
bytes long, probably. Worth mentioning in the notes for @count since
we're already writing docs?
> +void hbitmap_serialize_part(const HBitmap *hb, uint8_t *buf,
> + uint64_t start, uint64_t count);
> +
> +/**
> + * hbitmap_deserialize_part
> + * @hb: HBitmap to operate on.
> + * @buf: Buffer to restore bitmap data from.
> + * @start: First bit to restore.
> + * @count: Number of bits to restore.
> + * @finish: Whether to call hbitmap_deserialize_finish automatically.
> + *
> + * Restores HBitmap data corresponding to given region. The format is the same
> + * as for hbitmap_serialize_part.
> + *
> + * if @finish is false, caller must call hbitmap_serialize_finish before using
> + * the bitmap.
> + */
> +void hbitmap_deserialize_part(HBitmap *hb, uint8_t *buf,
> + uint64_t start, uint64_t count,
> + bool finish);
> +
> +/**
> + * hbitmap_deserialize_zeroes
> + * @hb: HBitmap to operate on.
> + * @start: First bit to restore.
> + * @count: Number of bits to restore.
> + * @finish: Whether to call hbitmap_deserialize_finish automatically.
> + *
> + * Same as hbitmap_serialize_part, but fills the bitmap with zeroes.
> + */
Fills the bitmap with zeroes?
> +void hbitmap_deserialize_zeroes(HBitmap *hb, uint64_t start, uint64_t count,
> + bool finish);
> +
> +/**
> + * hbitmap_deserialize_finish
> + * @hb: HBitmap to operate on.
> + *
> + * Repair HBitmap after calling hbitmap_deserialize_data. Actually, all HBitmap
> + * layers are restored here.
> + */
> +void hbitmap_deserialize_finish(HBitmap *hb);
> +
> +/**
> * hbitmap_free:
> * @hb: HBitmap to operate on.
> *
> diff --git a/util/hbitmap.c b/util/hbitmap.c
> index 55d3182..ab8cd81 100644
> --- a/util/hbitmap.c
> +++ b/util/hbitmap.c
> @@ -397,6 +397,142 @@ bool hbitmap_get(const HBitmap *hb, uint64_t item)
> return (hb->levels[HBITMAP_LEVELS - 1][pos >> BITS_PER_LEVEL] & bit) != 0;
> }
>
> +uint64_t hbitmap_serialization_granularity(const HBitmap *hb)
> +{
> + return BITS_PER_LONG << hb->granularity;
> +}
> +
> +/* serilization chunk start should be aligned to serialization granularity.
> + * Serilization chunk size scould be aligned to serialization granularity too,
> + * except for last chunk.
> + */
SeriAlization misspelled at the beginning of both lines. (missing the 'a')
> +static void serialization_chunk(const HBitmap *hb,
> + uint64_t start, uint64_t count,
> + unsigned long **first_el, size_t *el_count)
> +{
> + uint64_t last = start + count - 1;
> + uint64_t gran = hbitmap_serialization_granularity(hb);
> +
> + assert(((start + count - 1) >> hb->granularity) < hb->size);
> + assert((start & (gran - 1)) == 0);
> + if ((last >> hb->granularity) != hb->size - 1) {
> + assert((count & (gran - 1)) == 0);
> + }
> +
> + start = (start >> hb->granularity) >> BITS_PER_LEVEL;
> + last = (last >> hb->granularity) >> BITS_PER_LEVEL;
> +
> + *first_el = &hb->levels[HBITMAP_LEVELS - 1][start];
> + *el_count = last - start + 1;
> +}
> +
> +uint64_t hbitmap_serialization_size(const HBitmap *hb,
> + uint64_t start, uint64_t count)
> +{
> + uint64_t el_count;
> + unsigned long *cur;
> +
> + if (!count) {
> + return 0;
> + }
> + serialization_chunk(hb, start, count, &cur, &el_count);
Seems like a waste just to get the size.
> +
> + return el_count * sizeof(unsigned long);
> +}
> +
> +void hbitmap_serialize_part(const HBitmap *hb, uint8_t *buf,
> + uint64_t start, uint64_t count)
> +{
> + uint64_t el_count;
> + unsigned long *cur, *end;
> +
> + if (!count) {
> + return;
> + }
> + serialization_chunk(hb, start, count, &cur, &el_count);
> + end = cur + el_count;
> +
> + while (cur != end) {
> + unsigned long el =
> + (BITS_PER_LONG == 32 ? cpu_to_le32(*cur) : cpu_to_le64(*cur));
> +
> + memcpy(buf, &el, sizeof(el));
> + buf += sizeof(el);
> + cur++;
> + }
> +}
> +
> +void hbitmap_deserialize_part(HBitmap *hb, uint8_t *buf,
> + uint64_t start, uint64_t count,
> + bool finish)
> +{
> + uint64_t el_count;
> + unsigned long *cur, *end;
> +
> + if (!count) {
> + return;
> + }
> + serialization_chunk(hb, start, count, &cur, &el_count);
> + end = cur + el_count;
> +
> + while (cur != end) {
> + memcpy(cur, buf, sizeof(*cur));
> +
> + if (BITS_PER_LONG == 32) {
> + cpu_to_le32s((uint32_t *)cur);
> + } else {
> + cpu_to_le64s((uint64_t *)cur);
> + }
> +
> + buf += sizeof(unsigned long);
> + cur++;
> + }
> + if (finish) {
> + hbitmap_deserialize_finish(hb);
> + }
> +}
> +
> +void hbitmap_deserialize_zeroes(HBitmap *hb, uint64_t start, uint64_t count,
> + bool finish)
> +{
> + uint64_t el_count;
> + unsigned long *first;
> +
> + if (!count) {
> + return;
> + }
> + serialization_chunk(hb, start, count, &first, &el_count);
> +
I guess this is just to get the el_count, primarily, because we just
bowl over it just below.
Shouldn't this just use the serialization size function up above, anyway?
> + memset(first, 0, el_count * sizeof(unsigned long));
> + if (finish) {
> + hbitmap_deserialize_finish(hb);
> + }
> +}
> +
> +void hbitmap_deserialize_finish(HBitmap *bitmap)
> +{
> + int64_t i, size, prev_size;
> + int lev;
> +
> + /* restore levels starting from penultimate to zero level, assuming
> + * that the last level is ok */
> + size = MAX((bitmap->size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
> + for (lev = HBITMAP_LEVELS - 1; lev-- > 0; ) {
> + prev_size = size;
> + size = MAX((size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
> + memset(bitmap->levels[lev], 0, size * sizeof(unsigned long));
> +
> + for (i = 0; i < prev_size; ++i) {
> + if (bitmap->levels[lev + 1][i]) {
> + bitmap->levels[lev][i >> BITS_PER_LEVEL] |=
> + 1UL << (i & (BITS_PER_LONG - 1));
> + }
> + }
> + }
> +
> + bitmap->levels[0][0] |= 1UL << (BITS_PER_LONG - 1);
> +}
> +
> void hbitmap_free(HBitmap *hb)
> {
> unsigned i;
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 00/13] Dirty bitmap changes for migration/persistence work
2016-01-04 10:27 [Qemu-devel] [PATCH 00/13] Dirty bitmap changes for migration/persistence work Fam Zheng
` (12 preceding siblings ...)
2016-01-04 10:27 ` [Qemu-devel] [PATCH 13/13] tests: Add test code for hbitmap serialization Fam Zheng
@ 2016-01-07 21:32 ` John Snow
2016-01-08 0:29 ` Fam Zheng
13 siblings, 1 reply; 36+ messages in thread
From: John Snow @ 2016-01-07 21:32 UTC (permalink / raw)
To: Fam Zheng, qemu-devel
Cc: Kevin Wolf, Jeff Cody, Vladimir Sementsov-Ogievskiy, qemu-block
On 01/04/2016 05:27 AM, Fam Zheng wrote:
> Two major features are added to block dirty bitmap (and underlying HBitmap) in
> this series: meta bitmap and serialization, together with all other supportive
> patches.
>
> Both operations are common in dirty bitmap migration and persistence: they need
> to find whether and which part of the dirty bitmap in question has changed with
> meta dirty bitmap, and they need to write it to the target with serialization.
>
> Fam Zheng (11):
> backup: Use Bitmap to replace "s->bitmap"
> typedefs: Add BdrvDirtyBitmap and HBitmapIter
> block: Move block dirty bitmap code to separate files
> block: Remove unused typedef of BlockDriverDirtyHandler
> block: Hide HBitmap in block dirty bitmap interface
> HBitmap: Introduce "meta" bitmap to track bit changes
> tests: Add test code for meta bitmap
> block: Support meta dirty bitmap
> block: Add two dirty bitmap getters
> block: Assert that bdrv_release_dirty_bitmap succeeded
> tests: Add test code for hbitmap serialization
>
> Vladimir Sementsov-Ogievskiy (2):
> hbitmap: serialization
> block: BdrvDirtyBitmap serialization interface
>
> block.c | 339 -----------------------------
> block/Makefile.objs | 2 +-
> block/backup.c | 25 ++-
> block/dirty-bitmap.c | 491 +++++++++++++++++++++++++++++++++++++++++++
> block/mirror.c | 14 +-
> include/block/block.h | 39 +---
> include/block/dirty-bitmap.h | 70 ++++++
> include/qemu/hbitmap.h | 84 ++++++++
> include/qemu/typedefs.h | 3 +
> tests/test-hbitmap.c | 252 ++++++++++++++++++++++
> util/hbitmap.c | 197 +++++++++++++++--
> 11 files changed, 1106 insertions(+), 410 deletions(-)
> create mode 100644 block/dirty-bitmap.c
> create mode 100644 include/block/dirty-bitmap.h
>
That's all I've got for now. Happy New Year!
What's your roadmap for these patches? This, then QBM?
--js
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 00/13] Dirty bitmap changes for migration/persistence work
2016-01-07 21:32 ` [Qemu-devel] [PATCH 00/13] Dirty bitmap changes for migration/persistence work John Snow
@ 2016-01-08 0:29 ` Fam Zheng
0 siblings, 0 replies; 36+ messages in thread
From: Fam Zheng @ 2016-01-08 0:29 UTC (permalink / raw)
To: John Snow
Cc: Kevin Wolf, Jeff Cody, Vladimir Sementsov-Ogievskiy, qemu-devel,
qemu-block
On Thu, 01/07 16:32, John Snow wrote:
>
>
> On 01/04/2016 05:27 AM, Fam Zheng wrote:
> > Two major features are added to block dirty bitmap (and underlying HBitmap) in
> > this series: meta bitmap and serialization, together with all other supportive
> > patches.
> >
> > Both operations are common in dirty bitmap migration and persistence: they need
> > to find whether and which part of the dirty bitmap in question has changed with
> > meta dirty bitmap, and they need to write it to the target with serialization.
> >
> > Fam Zheng (11):
> > backup: Use Bitmap to replace "s->bitmap"
> > typedefs: Add BdrvDirtyBitmap and HBitmapIter
> > block: Move block dirty bitmap code to separate files
> > block: Remove unused typedef of BlockDriverDirtyHandler
> > block: Hide HBitmap in block dirty bitmap interface
> > HBitmap: Introduce "meta" bitmap to track bit changes
> > tests: Add test code for meta bitmap
> > block: Support meta dirty bitmap
> > block: Add two dirty bitmap getters
> > block: Assert that bdrv_release_dirty_bitmap succeeded
> > tests: Add test code for hbitmap serialization
> >
> > Vladimir Sementsov-Ogievskiy (2):
> > hbitmap: serialization
> > block: BdrvDirtyBitmap serialization interface
> >
> > block.c | 339 -----------------------------
> > block/Makefile.objs | 2 +-
> > block/backup.c | 25 ++-
> > block/dirty-bitmap.c | 491 +++++++++++++++++++++++++++++++++++++++++++
> > block/mirror.c | 14 +-
> > include/block/block.h | 39 +---
> > include/block/dirty-bitmap.h | 70 ++++++
> > include/qemu/hbitmap.h | 84 ++++++++
> > include/qemu/typedefs.h | 3 +
> > tests/test-hbitmap.c | 252 ++++++++++++++++++++++
> > util/hbitmap.c | 197 +++++++++++++++--
> > 11 files changed, 1106 insertions(+), 410 deletions(-)
> > create mode 100644 block/dirty-bitmap.c
> > create mode 100644 include/block/dirty-bitmap.h
> >
>
> That's all I've got for now. Happy New Year!
>
> What's your roadmap for these patches? This, then QBM?
Yes, QBM is on the way, right after this.
Fam
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 02/13] typedefs: Add BdrvDirtyBitmap and HBitmapIter
2016-01-05 22:14 ` John Snow
@ 2016-01-08 2:13 ` Fam Zheng
0 siblings, 0 replies; 36+ messages in thread
From: Fam Zheng @ 2016-01-08 2:13 UTC (permalink / raw)
To: John Snow
Cc: Kevin Wolf, Jeff Cody, Vladimir Sementsov-Ogievskiy, qemu-devel,
qemu-block
On Tue, 01/05 17:14, John Snow wrote:
>
>
> On 01/04/2016 05:27 AM, Fam Zheng wrote:
> > Following patches to refactor and move block dirty bitmap code could use this.
> >
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> > include/qemu/typedefs.h | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> > index 78fe6e8..e83934e 100644
> > --- a/include/qemu/typedefs.h
> > +++ b/include/qemu/typedefs.h
> > @@ -10,6 +10,7 @@ typedef struct AddressSpace AddressSpace;
> > typedef struct AioContext AioContext;
> > typedef struct AllwinnerAHCIState AllwinnerAHCIState;
> > typedef struct AudioState AudioState;
> > +typedef struct BdrvDirtyBitmap BdrvDirtyBitmap;
> > typedef struct BlockBackend BlockBackend;
> > typedef struct BlockBackendRootState BlockBackendRootState;
> > typedef struct BlockDriverState BlockDriverState;
> > @@ -28,6 +29,7 @@ typedef struct EventNotifier EventNotifier;
> > typedef struct FWCfgIoState FWCfgIoState;
> > typedef struct FWCfgMemState FWCfgMemState;
> > typedef struct FWCfgState FWCfgState;
> > +typedef struct HBitmapIter HBitmapIter;
> > typedef struct HCIInfo HCIInfo;
> > typedef struct I2CBus I2CBus;
> > typedef struct I2SCodec I2SCodec;
> >
>
> Should the existing typedefs be removed?
>
> >> include/block/block.h:typedef struct BdrvDirtyBitmap BdrvDirtyBitmap;
> >> include/block/block.h:struct HBitmapIter;
> >> include/qemu/hbitmap.h:typedef struct HBitmapIter HBitmapIter;
Yes! Will do.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 11/13] hbitmap: serialization
2016-01-04 10:27 ` [Qemu-devel] [PATCH 11/13] hbitmap: serialization Fam Zheng
2016-01-07 21:11 ` John Snow
@ 2016-01-11 14:48 ` Vladimir Sementsov-Ogievskiy
2016-01-11 15:19 ` Vladimir Sementsov-Ogievskiy
2 siblings, 0 replies; 36+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2016-01-11 14:48 UTC (permalink / raw)
To: Fam Zheng, qemu-devel; +Cc: Kevin Wolf, Jeff Cody, jsnow, qemu-block
On 04.01.2016 13:27, Fam Zheng wrote:
> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> Functions to serialize / deserialize(restore) HBitmap. HBitmap should be
> saved to linear sequence of bits independently of endianness and bitmap
> array element (unsigned long) size. Therefore Little Endian is chosen.
>
> These functions are appropriate for dirty bitmap migration, restoring
> the bitmap in several steps is available. To save performance, every
> step writes only the last level of the bitmap. All other levels are
> restored by hbitmap_deserialize_finish() as a last step of restoring.
> So, HBitmap is inconsistent while restoring.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> [Fix left shift operand to 1UL; add "finish" parameter. - Fam]
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> include/qemu/hbitmap.h | 76 +++++++++++++++++++++++++++
> util/hbitmap.c | 136 +++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 212 insertions(+)
>
> diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
> index ed672e7..3414113 100644
> --- a/include/qemu/hbitmap.h
> +++ b/include/qemu/hbitmap.h
> @@ -149,6 +149,82 @@ void hbitmap_reset_all(HBitmap *hb);
> bool hbitmap_get(const HBitmap *hb, uint64_t item);
>
> /**
> + * hbitmap_data_size:
> + * @hb: HBitmap to operate on.
> + * @count: Number of bits
> + *
> + * Grunularity of serialization chunks, used by other serializetion functions.
> + * For every chunk:
> + * 1. Chunk start should be aligned to this granularity.
> + * 2. Chunk size should be aligned too, except for last chunk (for which
> + * start + count == hb->size)
> + */
> +uint64_t hbitmap_serialization_granularity(const HBitmap *hb);
> +
> +/**
> + * hbitmap_data_size:
> + * @hb: HBitmap to operate on.
> + * @count: Number of bits
> + *
> + * Return amount of bytes hbitmap_(de)serialize_part needs
> + */
> +uint64_t hbitmap_serialization_size(const HBitmap *hb,
> + uint64_t start, uint64_t count);
> +
> +/**
> + * hbitmap_serialize_part
> + * @hb: HBitmap to operate on.
> + * @buf: Buffer to store serialized bitmap.
> + * @start: First bit to store.
> + * @count: Number of bits to store.
> + *
> + * Stores HBitmap data corresponding to given region. The format of saved data
> + * is linear sequence of bits, so it can be used by hbitmap_deserialize_part
> + * independently of endianness and size of HBitmap level array elements
> + */
> +void hbitmap_serialize_part(const HBitmap *hb, uint8_t *buf,
> + uint64_t start, uint64_t count);
> +
> +/**
> + * hbitmap_deserialize_part
> + * @hb: HBitmap to operate on.
> + * @buf: Buffer to restore bitmap data from.
> + * @start: First bit to restore.
> + * @count: Number of bits to restore.
> + * @finish: Whether to call hbitmap_deserialize_finish automatically.
> + *
> + * Restores HBitmap data corresponding to given region. The format is the same
> + * as for hbitmap_serialize_part.
> + *
> + * if @finish is false, caller must call hbitmap_serialize_finish before using
> + * the bitmap.
> + */
> +void hbitmap_deserialize_part(HBitmap *hb, uint8_t *buf,
> + uint64_t start, uint64_t count,
> + bool finish);
> +
> +/**
> + * hbitmap_deserialize_zeroes
> + * @hb: HBitmap to operate on.
> + * @start: First bit to restore.
> + * @count: Number of bits to restore.
> + * @finish: Whether to call hbitmap_deserialize_finish automatically.
> + *
> + * Same as hbitmap_serialize_part, but fills the bitmap with zeroes.
> + */
> +void hbitmap_deserialize_zeroes(HBitmap *hb, uint64_t start, uint64_t count,
> + bool finish);
> +
> +/**
> + * hbitmap_deserialize_finish
> + * @hb: HBitmap to operate on.
> + *
> + * Repair HBitmap after calling hbitmap_deserialize_data. Actually, all HBitmap
> + * layers are restored here.
> + */
> +void hbitmap_deserialize_finish(HBitmap *hb);
> +
> +/**
> * hbitmap_free:
> * @hb: HBitmap to operate on.
> *
> diff --git a/util/hbitmap.c b/util/hbitmap.c
> index 55d3182..ab8cd81 100644
> --- a/util/hbitmap.c
> +++ b/util/hbitmap.c
> @@ -397,6 +397,142 @@ bool hbitmap_get(const HBitmap *hb, uint64_t item)
> return (hb->levels[HBITMAP_LEVELS - 1][pos >> BITS_PER_LEVEL] & bit) != 0;
> }
>
> +uint64_t hbitmap_serialization_granularity(const HBitmap *hb)
> +{
> + return BITS_PER_LONG << hb->granularity;
> +}
> +
> +/* serilization chunk start should be aligned to serialization granularity.
> + * Serilization chunk size scould be aligned to serialization granularity too,
> + * except for last chunk.
> + */
> +static void serialization_chunk(const HBitmap *hb,
> + uint64_t start, uint64_t count,
> + unsigned long **first_el, size_t *el_count)
> +{
> + uint64_t last = start + count - 1;
> + uint64_t gran = hbitmap_serialization_granularity(hb);
> +
> + assert(((start + count - 1) >> hb->granularity) < hb->size);
'last' may be used instead of '(start + count - 1)', and it is better to
swap this line with the next line, to be closer with the similar bound
checking.
> + assert((start & (gran - 1)) == 0);
> + if ((last >> hb->granularity) != hb->size - 1) {
> + assert((count & (gran - 1)) == 0);
> + }
> +
> + start = (start >> hb->granularity) >> BITS_PER_LEVEL;
> + last = (last >> hb->granularity) >> BITS_PER_LEVEL;
> +
> + *first_el = &hb->levels[HBITMAP_LEVELS - 1][start];
> + *el_count = last - start + 1;
> +}
> +
> +uint64_t hbitmap_serialization_size(const HBitmap *hb,
> + uint64_t start, uint64_t count)
> +{
> + uint64_t el_count;
> + unsigned long *cur;
> +
> + if (!count) {
> + return 0;
> + }
> + serialization_chunk(hb, start, count, &cur, &el_count);
> +
> + return el_count * sizeof(unsigned long);
> +}
> +
> +void hbitmap_serialize_part(const HBitmap *hb, uint8_t *buf,
> + uint64_t start, uint64_t count)
> +{
> + uint64_t el_count;
> + unsigned long *cur, *end;
> +
> + if (!count) {
> + return;
> + }
> + serialization_chunk(hb, start, count, &cur, &el_count);
> + end = cur + el_count;
> +
> + while (cur != end) {
> + unsigned long el =
> + (BITS_PER_LONG == 32 ? cpu_to_le32(*cur) : cpu_to_le64(*cur));
> +
> + memcpy(buf, &el, sizeof(el));
> + buf += sizeof(el);
> + cur++;
> + }
> +}
> +
> +void hbitmap_deserialize_part(HBitmap *hb, uint8_t *buf,
> + uint64_t start, uint64_t count,
> + bool finish)
> +{
> + uint64_t el_count;
> + unsigned long *cur, *end;
> +
> + if (!count) {
> + return;
> + }
> + serialization_chunk(hb, start, count, &cur, &el_count);
> + end = cur + el_count;
> +
> + while (cur != end) {
> + memcpy(cur, buf, sizeof(*cur));
> +
> + if (BITS_PER_LONG == 32) {
> + cpu_to_le32s((uint32_t *)cur);
> + } else {
> + cpu_to_le64s((uint64_t *)cur);
le32_to_cpus and le64_to_cpus should be here. Really it doesn't matter,
but sounds better for DEserialization.
> + }
> +
> + buf += sizeof(unsigned long);
> + cur++;
> + }
> + if (finish) {
> + hbitmap_deserialize_finish(hb);
> + }
> +}
> +
> +void hbitmap_deserialize_zeroes(HBitmap *hb, uint64_t start, uint64_t count,
> + bool finish)
> +{
> + uint64_t el_count;
> + unsigned long *first;
> +
> + if (!count) {
> + return;
> + }
> + serialization_chunk(hb, start, count, &first, &el_count);
> +
> + memset(first, 0, el_count * sizeof(unsigned long));
> + if (finish) {
> + hbitmap_deserialize_finish(hb);
> + }
> +}
> +
> +void hbitmap_deserialize_finish(HBitmap *bitmap)
> +{
> + int64_t i, size, prev_size;
> + int lev;
> +
> + /* restore levels starting from penultimate to zero level, assuming
> + * that the last level is ok */
> + size = MAX((bitmap->size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
> + for (lev = HBITMAP_LEVELS - 1; lev-- > 0; ) {
> + prev_size = size;
> + size = MAX((size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
> + memset(bitmap->levels[lev], 0, size * sizeof(unsigned long));
> +
> + for (i = 0; i < prev_size; ++i) {
> + if (bitmap->levels[lev + 1][i]) {
> + bitmap->levels[lev][i >> BITS_PER_LEVEL] |=
> + 1UL << (i & (BITS_PER_LONG - 1));
> + }
> + }
> + }
> +
> + bitmap->levels[0][0] |= 1UL << (BITS_PER_LONG - 1);
> +}
> +
> void hbitmap_free(HBitmap *hb)
> {
> unsigned i;
--
Best regards,
Vladimir
* now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 11/13] hbitmap: serialization
2016-01-07 21:11 ` John Snow
@ 2016-01-11 15:12 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 36+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2016-01-11 15:12 UTC (permalink / raw)
To: John Snow, Fam Zheng, qemu-devel; +Cc: Kevin Wolf, Jeff Cody, qemu-block
On 08.01.2016 00:11, John Snow wrote:
>
> On 01/04/2016 05:27 AM, Fam Zheng wrote:
>> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
>> Functions to serialize / deserialize(restore) HBitmap. HBitmap should be
>> saved to linear sequence of bits independently of endianness and bitmap
>> array element (unsigned long) size. Therefore Little Endian is chosen.
>>
>> These functions are appropriate for dirty bitmap migration, restoring
>> the bitmap in several steps is available. To save performance, every
>> step writes only the last level of the bitmap. All other levels are
>> restored by hbitmap_deserialize_finish() as a last step of restoring.
>> So, HBitmap is inconsistent while restoring.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> [Fix left shift operand to 1UL; add "finish" parameter. - Fam]
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>> ---
>> include/qemu/hbitmap.h | 76 +++++++++++++++++++++++++++
>> util/hbitmap.c | 136 +++++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 212 insertions(+)
>>
>> diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
>> index ed672e7..3414113 100644
>> --- a/include/qemu/hbitmap.h
>> +++ b/include/qemu/hbitmap.h
>> @@ -149,6 +149,82 @@ void hbitmap_reset_all(HBitmap *hb);
>> bool hbitmap_get(const HBitmap *hb, uint64_t item);
>>
>> /**
>> + * hbitmap_data_size:
>> + * @hb: HBitmap to operate on.
>> + * @count: Number of bits
>> + *
>> + * Grunularity of serialization chunks, used by other serializetion functions.
>> + * For every chunk:
>> + * 1. Chunk start should be aligned to this granularity.
>> + * 2. Chunk size should be aligned too, except for last chunk (for which
>> + * start + count == hb->size)
>> + */
> Whoops, this is the documentation for the function below. Looks like you
> updated this one instead of the one adjacent to the prototype, too.
yes.. delete @count line and
s/hbitmap_data_size/hbitmap_serialization_granularity
>
>> +uint64_t hbitmap_serialization_granularity(const HBitmap *hb);
>> +
>> +/**
>> + * hbitmap_data_size:
>> + * @hb: HBitmap to operate on.
>> + * @count: Number of bits
>> + *
>> + * Return amount of bytes hbitmap_(de)serialize_part needs
>> + */
>> +uint64_t hbitmap_serialization_size(const HBitmap *hb,
>> + uint64_t start, uint64_t count);
>> +
>> +/**
>> + * hbitmap_serialize_part
>> + * @hb: HBitmap to operate on.
>> + * @buf: Buffer to store serialized bitmap.
>> + * @start: First bit to store.
>> + * @count: Number of bits to store.
>> + *
>> + * Stores HBitmap data corresponding to given region. The format of saved data
>> + * is linear sequence of bits, so it can be used by hbitmap_deserialize_part
>> + * independently of endianness and size of HBitmap level array elements
>> + */
> I suppose that the buffer needs to be pre-allocated and "count/8" bytes
> long; but rather specifically it needs to be hbitmap_serialization_size
> bytes long, probably. Worth mentioning in the notes for @count since
> we're already writing docs?
Here @count is number of virtual bits. count is in the same dimension as
start, as for all other public functions. An amount of real bits to be
saved is ~ count/granularity.
>
>> +void hbitmap_serialize_part(const HBitmap *hb, uint8_t *buf,
>> + uint64_t start, uint64_t count);
>> +
>> +/**
>> + * hbitmap_deserialize_part
>> + * @hb: HBitmap to operate on.
>> + * @buf: Buffer to restore bitmap data from.
>> + * @start: First bit to restore.
>> + * @count: Number of bits to restore.
>> + * @finish: Whether to call hbitmap_deserialize_finish automatically.
>> + *
>> + * Restores HBitmap data corresponding to given region. The format is the same
>> + * as for hbitmap_serialize_part.
>> + *
>> + * if @finish is false, caller must call hbitmap_serialize_finish before using
>> + * the bitmap.
>> + */
>> +void hbitmap_deserialize_part(HBitmap *hb, uint8_t *buf,
>> + uint64_t start, uint64_t count,
>> + bool finish);
>> +
>> +/**
>> + * hbitmap_deserialize_zeroes
>> + * @hb: HBitmap to operate on.
>> + * @start: First bit to restore.
>> + * @count: Number of bits to restore.
>> + * @finish: Whether to call hbitmap_deserialize_finish automatically.
>> + *
>> + * Same as hbitmap_serialize_part, but fills the bitmap with zeroes.
>> + */
> Fills the bitmap with zeroes?
with finish=1 - yes. with finish=0 - only fills the last level of the
bitmap, and should be used among other deserialize calls with finish=0.
>
>> +void hbitmap_deserialize_zeroes(HBitmap *hb, uint64_t start, uint64_t count,
>> + bool finish);
>> +
>> +/**
>> + * hbitmap_deserialize_finish
>> + * @hb: HBitmap to operate on.
>> + *
>> + * Repair HBitmap after calling hbitmap_deserialize_data. Actually, all HBitmap
>> + * layers are restored here.
>> + */
>> +void hbitmap_deserialize_finish(HBitmap *hb);
>> +
>> +/**
>> * hbitmap_free:
>> * @hb: HBitmap to operate on.
>> *
>> diff --git a/util/hbitmap.c b/util/hbitmap.c
>> index 55d3182..ab8cd81 100644
>> --- a/util/hbitmap.c
>> +++ b/util/hbitmap.c
>> @@ -397,6 +397,142 @@ bool hbitmap_get(const HBitmap *hb, uint64_t item)
>> return (hb->levels[HBITMAP_LEVELS - 1][pos >> BITS_PER_LEVEL] & bit) != 0;
>> }
>>
>> +uint64_t hbitmap_serialization_granularity(const HBitmap *hb)
>> +{
>> + return BITS_PER_LONG << hb->granularity;
>> +}
>> +
>> +/* serilization chunk start should be aligned to serialization granularity.
>> + * Serilization chunk size scould be aligned to serialization granularity too,
>> + * except for last chunk.
>> + */
> SeriAlization misspelled at the beginning of both lines. (missing the 'a')
>
>> +static void serialization_chunk(const HBitmap *hb,
>> + uint64_t start, uint64_t count,
>> + unsigned long **first_el, size_t *el_count)
>> +{
>> + uint64_t last = start + count - 1;
>> + uint64_t gran = hbitmap_serialization_granularity(hb);
>> +
>> + assert(((start + count - 1) >> hb->granularity) < hb->size);
>> + assert((start & (gran - 1)) == 0);
>> + if ((last >> hb->granularity) != hb->size - 1) {
>> + assert((count & (gran - 1)) == 0);
>> + }
>> +
>> + start = (start >> hb->granularity) >> BITS_PER_LEVEL;
>> + last = (last >> hb->granularity) >> BITS_PER_LEVEL;
>> +
>> + *first_el = &hb->levels[HBITMAP_LEVELS - 1][start];
>> + *el_count = last - start + 1;
>> +}
>> +
>> +uint64_t hbitmap_serialization_size(const HBitmap *hb,
>> + uint64_t start, uint64_t count)
>> +{
>> + uint64_t el_count;
>> + unsigned long *cur;
>> +
>> + if (!count) {
>> + return 0;
>> + }
>> + serialization_chunk(hb, start, count, &cur, &el_count);
> Seems like a waste just to get the size.
and check all asserts. If we will provide direct access to chunk
serialization to extern mechanisms (qmp), asserts should be replaced
with normal error handling.
'cur' is useless here... NULL can be used instead of it with appropriate
hanges in serialization_chunk, but I don't thing that this is necessary.
>
>> +
>> + return el_count * sizeof(unsigned long);
>> +}
>> +
>> +void hbitmap_serialize_part(const HBitmap *hb, uint8_t *buf,
>> + uint64_t start, uint64_t count)
>> +{
>> + uint64_t el_count;
>> + unsigned long *cur, *end;
>> +
>> + if (!count) {
>> + return;
>> + }
>> + serialization_chunk(hb, start, count, &cur, &el_count);
>> + end = cur + el_count;
>> +
>> + while (cur != end) {
>> + unsigned long el =
>> + (BITS_PER_LONG == 32 ? cpu_to_le32(*cur) : cpu_to_le64(*cur));
>> +
>> + memcpy(buf, &el, sizeof(el));
>> + buf += sizeof(el);
>> + cur++;
>> + }
>> +}
>> +
>> +void hbitmap_deserialize_part(HBitmap *hb, uint8_t *buf,
>> + uint64_t start, uint64_t count,
>> + bool finish)
>> +{
>> + uint64_t el_count;
>> + unsigned long *cur, *end;
>> +
>> + if (!count) {
>> + return;
>> + }
>> + serialization_chunk(hb, start, count, &cur, &el_count);
>> + end = cur + el_count;
>> +
>> + while (cur != end) {
>> + memcpy(cur, buf, sizeof(*cur));
>> +
>> + if (BITS_PER_LONG == 32) {
>> + cpu_to_le32s((uint32_t *)cur);
>> + } else {
>> + cpu_to_le64s((uint64_t *)cur);
>> + }
>> +
>> + buf += sizeof(unsigned long);
>> + cur++;
>> + }
>> + if (finish) {
>> + hbitmap_deserialize_finish(hb);
>> + }
>> +}
>> +
>> +void hbitmap_deserialize_zeroes(HBitmap *hb, uint64_t start, uint64_t count,
>> + bool finish)
>> +{
>> + uint64_t el_count;
>> + unsigned long *first;
>> +
>> + if (!count) {
>> + return;
>> + }
>> + serialization_chunk(hb, start, count, &first, &el_count);
>> +
> I guess this is just to get the el_count, primarily, because we just
> bowl over it just below.
first is used too.
>
> Shouldn't this just use the serialization size function up above, anyway?
>
>> + memset(first, 0, el_count * sizeof(unsigned long));
>> + if (finish) {
>> + hbitmap_deserialize_finish(hb);
>> + }
>> +}
>> +
>> +void hbitmap_deserialize_finish(HBitmap *bitmap)
>> +{
>> + int64_t i, size, prev_size;
>> + int lev;
>> +
>> + /* restore levels starting from penultimate to zero level, assuming
>> + * that the last level is ok */
>> + size = MAX((bitmap->size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
>> + for (lev = HBITMAP_LEVELS - 1; lev-- > 0; ) {
>> + prev_size = size;
>> + size = MAX((size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
>> + memset(bitmap->levels[lev], 0, size * sizeof(unsigned long));
>> +
>> + for (i = 0; i < prev_size; ++i) {
>> + if (bitmap->levels[lev + 1][i]) {
>> + bitmap->levels[lev][i >> BITS_PER_LEVEL] |=
>> + 1UL << (i & (BITS_PER_LONG - 1));
>> + }
>> + }
>> + }
>> +
>> + bitmap->levels[0][0] |= 1UL << (BITS_PER_LONG - 1);
>> +}
>> +
>> void hbitmap_free(HBitmap *hb)
>> {
>> unsigned i;
>>
--
Best regards,
Vladimir
* now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 11/13] hbitmap: serialization
2016-01-04 10:27 ` [Qemu-devel] [PATCH 11/13] hbitmap: serialization Fam Zheng
2016-01-07 21:11 ` John Snow
2016-01-11 14:48 ` Vladimir Sementsov-Ogievskiy
@ 2016-01-11 15:19 ` Vladimir Sementsov-Ogievskiy
2 siblings, 0 replies; 36+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2016-01-11 15:19 UTC (permalink / raw)
To: Fam Zheng, qemu-devel; +Cc: Kevin Wolf, Jeff Cody, jsnow, qemu-block
On 04.01.2016 13:27, Fam Zheng wrote:
> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> Functions to serialize / deserialize(restore) HBitmap. HBitmap should be
> saved to linear sequence of bits independently of endianness and bitmap
> array element (unsigned long) size. Therefore Little Endian is chosen.
>
> These functions are appropriate for dirty bitmap migration, restoring
> the bitmap in several steps is available. To save performance, every
> step writes only the last level of the bitmap. All other levels are
> restored by hbitmap_deserialize_finish() as a last step of restoring.
> So, HBitmap is inconsistent while restoring.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> [Fix left shift operand to 1UL; add "finish" parameter. - Fam]
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> include/qemu/hbitmap.h | 76 +++++++++++++++++++++++++++
> util/hbitmap.c | 136 +++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 212 insertions(+)
>
> diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
> index ed672e7..3414113 100644
> --- a/include/qemu/hbitmap.h
> +++ b/include/qemu/hbitmap.h
> @@ -149,6 +149,82 @@ void hbitmap_reset_all(HBitmap *hb);
> bool hbitmap_get(const HBitmap *hb, uint64_t item);
>
> /**
> + * hbitmap_data_size:
> + * @hb: HBitmap to operate on.
> + * @count: Number of bits
> + *
> + * Grunularity of serialization chunks, used by other serializetion functions.
> + * For every chunk:
> + * 1. Chunk start should be aligned to this granularity.
> + * 2. Chunk size should be aligned too, except for last chunk (for which
> + * start + count == hb->size)
> + */
> +uint64_t hbitmap_serialization_granularity(const HBitmap *hb);
> +
> +/**
> + * hbitmap_data_size:
> + * @hb: HBitmap to operate on.
> + * @count: Number of bits
> + *
> + * Return amount of bytes hbitmap_(de)serialize_part needs
> + */
> +uint64_t hbitmap_serialization_size(const HBitmap *hb,
> + uint64_t start, uint64_t count);
> +
> +/**
> + * hbitmap_serialize_part
> + * @hb: HBitmap to operate on.
> + * @buf: Buffer to store serialized bitmap.
> + * @start: First bit to store.
> + * @count: Number of bits to store.
> + *
> + * Stores HBitmap data corresponding to given region. The format of saved data
> + * is linear sequence of bits, so it can be used by hbitmap_deserialize_part
> + * independently of endianness and size of HBitmap level array elements
> + */
> +void hbitmap_serialize_part(const HBitmap *hb, uint8_t *buf,
> + uint64_t start, uint64_t count);
> +
> +/**
> + * hbitmap_deserialize_part
> + * @hb: HBitmap to operate on.
> + * @buf: Buffer to restore bitmap data from.
> + * @start: First bit to restore.
> + * @count: Number of bits to restore.
> + * @finish: Whether to call hbitmap_deserialize_finish automatically.
> + *
> + * Restores HBitmap data corresponding to given region. The format is the same
> + * as for hbitmap_serialize_part.
> + *
> + * if @finish is false, caller must call hbitmap_serialize_finish before using
> + * the bitmap.
> + */
> +void hbitmap_deserialize_part(HBitmap *hb, uint8_t *buf,
> + uint64_t start, uint64_t count,
> + bool finish);
> +
> +/**
> + * hbitmap_deserialize_zeroes
> + * @hb: HBitmap to operate on.
> + * @start: First bit to restore.
> + * @count: Number of bits to restore.
> + * @finish: Whether to call hbitmap_deserialize_finish automatically.
> + *
> + * Same as hbitmap_serialize_part, but fills the bitmap with zeroes.
> + */
> +void hbitmap_deserialize_zeroes(HBitmap *hb, uint64_t start, uint64_t count,
> + bool finish);
> +
> +/**
> + * hbitmap_deserialize_finish
> + * @hb: HBitmap to operate on.
> + *
> + * Repair HBitmap after calling hbitmap_deserialize_data. Actually, all HBitmap
> + * layers are restored here.
> + */
> +void hbitmap_deserialize_finish(HBitmap *hb);
> +
> +/**
> * hbitmap_free:
> * @hb: HBitmap to operate on.
> *
> diff --git a/util/hbitmap.c b/util/hbitmap.c
> index 55d3182..ab8cd81 100644
> --- a/util/hbitmap.c
> +++ b/util/hbitmap.c
> @@ -397,6 +397,142 @@ bool hbitmap_get(const HBitmap *hb, uint64_t item)
> return (hb->levels[HBITMAP_LEVELS - 1][pos >> BITS_PER_LEVEL] & bit) != 0;
> }
>
> +uint64_t hbitmap_serialization_granularity(const HBitmap *hb)
> +{
> + return BITS_PER_LONG << hb->granularity;
Isn't it bad, that granularity depends on long size? If we serialize on
32bit machine and deserialize on 64bit one.. This should be carefully
handled. May be, it is easier to replace it with "64 <<
hb->granularity", to be the same on different machines (for same bitmap,
of course)
> +}
> +
> +/* serilization chunk start should be aligned to serialization granularity.
> + * Serilization chunk size scould be aligned to serialization granularity too,
> + * except for last chunk.
> + */
> +static void serialization_chunk(const HBitmap *hb,
> + uint64_t start, uint64_t count,
> + unsigned long **first_el, size_t *el_count)
> +{
> + uint64_t last = start + count - 1;
> + uint64_t gran = hbitmap_serialization_granularity(hb);
> +
> + assert(((start + count - 1) >> hb->granularity) < hb->size);
> + assert((start & (gran - 1)) == 0);
> + if ((last >> hb->granularity) != hb->size - 1) {
> + assert((count & (gran - 1)) == 0);
> + }
> +
> + start = (start >> hb->granularity) >> BITS_PER_LEVEL;
> + last = (last >> hb->granularity) >> BITS_PER_LEVEL;
> +
> + *first_el = &hb->levels[HBITMAP_LEVELS - 1][start];
> + *el_count = last - start + 1;
> +}
> +
> +uint64_t hbitmap_serialization_size(const HBitmap *hb,
> + uint64_t start, uint64_t count)
> +{
> + uint64_t el_count;
> + unsigned long *cur;
> +
> + if (!count) {
> + return 0;
> + }
> + serialization_chunk(hb, start, count, &cur, &el_count);
> +
> + return el_count * sizeof(unsigned long);
> +}
> +
> +void hbitmap_serialize_part(const HBitmap *hb, uint8_t *buf,
> + uint64_t start, uint64_t count)
> +{
> + uint64_t el_count;
> + unsigned long *cur, *end;
> +
> + if (!count) {
> + return;
> + }
> + serialization_chunk(hb, start, count, &cur, &el_count);
> + end = cur + el_count;
> +
> + while (cur != end) {
> + unsigned long el =
> + (BITS_PER_LONG == 32 ? cpu_to_le32(*cur) : cpu_to_le64(*cur));
> +
> + memcpy(buf, &el, sizeof(el));
> + buf += sizeof(el);
> + cur++;
> + }
> +}
> +
> +void hbitmap_deserialize_part(HBitmap *hb, uint8_t *buf,
> + uint64_t start, uint64_t count,
> + bool finish)
> +{
> + uint64_t el_count;
> + unsigned long *cur, *end;
> +
> + if (!count) {
> + return;
> + }
> + serialization_chunk(hb, start, count, &cur, &el_count);
> + end = cur + el_count;
> +
> + while (cur != end) {
> + memcpy(cur, buf, sizeof(*cur));
> +
> + if (BITS_PER_LONG == 32) {
> + cpu_to_le32s((uint32_t *)cur);
> + } else {
> + cpu_to_le64s((uint64_t *)cur);
> + }
> +
> + buf += sizeof(unsigned long);
> + cur++;
> + }
> + if (finish) {
> + hbitmap_deserialize_finish(hb);
> + }
> +}
> +
> +void hbitmap_deserialize_zeroes(HBitmap *hb, uint64_t start, uint64_t count,
> + bool finish)
> +{
> + uint64_t el_count;
> + unsigned long *first;
> +
> + if (!count) {
> + return;
> + }
> + serialization_chunk(hb, start, count, &first, &el_count);
> +
> + memset(first, 0, el_count * sizeof(unsigned long));
> + if (finish) {
> + hbitmap_deserialize_finish(hb);
> + }
> +}
> +
> +void hbitmap_deserialize_finish(HBitmap *bitmap)
> +{
> + int64_t i, size, prev_size;
> + int lev;
> +
> + /* restore levels starting from penultimate to zero level, assuming
> + * that the last level is ok */
> + size = MAX((bitmap->size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
> + for (lev = HBITMAP_LEVELS - 1; lev-- > 0; ) {
> + prev_size = size;
> + size = MAX((size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
> + memset(bitmap->levels[lev], 0, size * sizeof(unsigned long));
> +
> + for (i = 0; i < prev_size; ++i) {
> + if (bitmap->levels[lev + 1][i]) {
> + bitmap->levels[lev][i >> BITS_PER_LEVEL] |=
> + 1UL << (i & (BITS_PER_LONG - 1));
> + }
> + }
> + }
> +
> + bitmap->levels[0][0] |= 1UL << (BITS_PER_LONG - 1);
> +}
> +
> void hbitmap_free(HBitmap *hb)
> {
> unsigned i;
--
Best regards,
Vladimir
* now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 06/13] HBitmap: Introduce "meta" bitmap to track bit changes
2016-01-04 10:27 ` [Qemu-devel] [PATCH 06/13] HBitmap: Introduce "meta" bitmap to track bit changes Fam Zheng
2016-01-06 0:09 ` John Snow
@ 2016-01-11 15:40 ` Vladimir Sementsov-Ogievskiy
2016-01-11 18:56 ` John Snow
1 sibling, 1 reply; 36+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2016-01-11 15:40 UTC (permalink / raw)
To: Fam Zheng, qemu-devel; +Cc: Kevin Wolf, Jeff Cody, jsnow, qemu-block
On 04.01.2016 13:27, Fam Zheng wrote:
> Upon each bit toggle, the corresponding bit in the meta bitmap will be
> set.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> include/qemu/hbitmap.h | 8 +++++++
> util/hbitmap.c | 61 +++++++++++++++++++++++++++++++++++++-------------
> 2 files changed, 54 insertions(+), 15 deletions(-)
>
> diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
> index bb94a00..ed672e7 100644
> --- a/include/qemu/hbitmap.h
> +++ b/include/qemu/hbitmap.h
> @@ -181,6 +181,14 @@ void hbitmap_iter_init(HBitmapIter *hbi, const HBitmap *hb, uint64_t first);
> */
> unsigned long hbitmap_iter_skip_words(HBitmapIter *hbi);
>
> +/* hbitmap_create_meta
> + * Create a "meta" hbitmap to track dirtiness of the bits in this HBitmap.
> + *
> + * @hb: The HBitmap to operate on.
> + * @chunk_size: How many bits in @hb does one bit in the meta track.
> + */
> +HBitmap *hbitmap_create_meta(HBitmap *hb, int chunk_size);
> +
> /**
> * hbitmap_iter_next:
> * @hbi: HBitmapIter to operate on.
> diff --git a/util/hbitmap.c b/util/hbitmap.c
> index 50b888f..55d3182 100644
> --- a/util/hbitmap.c
> +++ b/util/hbitmap.c
> @@ -81,6 +81,9 @@ struct HBitmap {
> */
> int granularity;
>
> + /* A meta dirty bitmap to track the dirtiness of bits in this HBitmap. */
> + HBitmap *meta;
> +
> /* A number of progressively less coarse bitmaps (i.e. level 0 is the
> * coarsest). Each bit in level N represents a word in level N+1 that
> * has a set bit, except the last level where each bit represents the
> @@ -212,25 +215,27 @@ static uint64_t hb_count_between(HBitmap *hb, uint64_t start, uint64_t last)
> }
>
> /* Setting starts at the last layer and propagates up if an element
> - * changes from zero to non-zero.
> + * changes.
> */
> static inline bool hb_set_elem(unsigned long *elem, uint64_t start, uint64_t last)
> {
> unsigned long mask;
> - bool changed;
> + unsigned long old;
>
> assert((last >> BITS_PER_LEVEL) == (start >> BITS_PER_LEVEL));
> assert(start <= last);
>
> mask = 2UL << (last & (BITS_PER_LONG - 1));
> mask -= 1UL << (start & (BITS_PER_LONG - 1));
> - changed = (*elem == 0);
> + old = *elem;
> *elem |= mask;
> - return changed;
> + return old != *elem;
> }
>
> -/* The recursive workhorse (the depth is limited to HBITMAP_LEVELS)... */
> -static void hb_set_between(HBitmap *hb, int level, uint64_t start, uint64_t last)
> +/* The recursive workhorse (the depth is limited to HBITMAP_LEVELS)...
> + * Returns true if at least one bit is changed. */
> +static bool hb_set_between(HBitmap *hb, int level, uint64_t start,
> + uint64_t last)
> {
> size_t pos = start >> BITS_PER_LEVEL;
> size_t lastpos = last >> BITS_PER_LEVEL;
> @@ -259,22 +264,27 @@ static void hb_set_between(HBitmap *hb, int level, uint64_t start, uint64_t last
> if (level > 0 && changed) {
> hb_set_between(hb, level - 1, pos, lastpos);
> }
> + return changed;
> }
>
> void hbitmap_set(HBitmap *hb, uint64_t start, uint64_t count)
> {
> /* Compute range in the last layer. */
> + uint64_t first, n;
> uint64_t last = start + count - 1;
>
> trace_hbitmap_set(hb, start, count,
> start >> hb->granularity, last >> hb->granularity);
>
> - start >>= hb->granularity;
> + first = start >> hb->granularity;
> last >>= hb->granularity;
> - count = last - start + 1;
> + n = last - first + 1;
>
> - hb->count += count - hb_count_between(hb, start, last);
> - hb_set_between(hb, HBITMAP_LEVELS - 1, start, last);
> + hb->count += n - hb_count_between(hb, first, last);
> + if (hb_set_between(hb, HBITMAP_LEVELS - 1, first, last) &&
> + hb->meta) {
I don't know, what optimizer things about it, but definetly
+ if (hb->meta &&
+ hb_set_between(hb, HBITMAP_LEVELS - 1, first, last))
should work faster for most cases, when hb->meta == NULL.
> + hbitmap_set(hb->meta, start, count);
> + }
> }
>
> /* Resetting works the other way round: propagate up if the new
> @@ -295,8 +305,10 @@ static inline bool hb_reset_elem(unsigned long *elem, uint64_t start, uint64_t l
> return blanked;
> }
>
> -/* The recursive workhorse (the depth is limited to HBITMAP_LEVELS)... */
> -static void hb_reset_between(HBitmap *hb, int level, uint64_t start, uint64_t last)
> +/* The recursive workhorse (the depth is limited to HBITMAP_LEVELS)...
> + * Returns true if at least one bit is changed. */
> +static bool hb_reset_between(HBitmap *hb, int level, uint64_t start,
> + uint64_t last)
> {
> size_t pos = start >> BITS_PER_LEVEL;
> size_t lastpos = last >> BITS_PER_LEVEL;
> @@ -339,21 +351,28 @@ static void hb_reset_between(HBitmap *hb, int level, uint64_t start, uint64_t la
> if (level > 0 && changed) {
> hb_reset_between(hb, level - 1, pos, lastpos);
> }
> +
> + return changed;
> +
> }
>
> void hbitmap_reset(HBitmap *hb, uint64_t start, uint64_t count)
> {
> /* Compute range in the last layer. */
> + uint64_t first;
> uint64_t last = start + count - 1;
>
> trace_hbitmap_reset(hb, start, count,
> start >> hb->granularity, last >> hb->granularity);
>
> - start >>= hb->granularity;
> + first = start >> hb->granularity;
> last >>= hb->granularity;
>
> - hb->count -= hb_count_between(hb, start, last);
> - hb_reset_between(hb, HBITMAP_LEVELS - 1, start, last);
> + hb->count -= hb_count_between(hb, first, last);
> + if (hb_reset_between(hb, HBITMAP_LEVELS - 1, first, last) &&
> + hb->meta) {
and here
> + hbitmap_set(hb->meta, start, count);
> + }
> }
>
> void hbitmap_reset_all(HBitmap *hb)
> @@ -384,6 +403,9 @@ void hbitmap_free(HBitmap *hb)
> for (i = HBITMAP_LEVELS; i-- > 0; ) {
> g_free(hb->levels[i]);
> }
> + if (hb->meta) {
> + hbitmap_free(hb->meta);
> + }
hmm, not obvious for me.. why not "the one who creates must than destroy"?
> g_free(hb);
> }
>
> @@ -493,3 +515,12 @@ bool hbitmap_merge(HBitmap *a, const HBitmap *b)
>
> return true;
> }
> +
> +HBitmap *hbitmap_create_meta(HBitmap *hb, int chunk_size)
> +{
> + assert(!(chunk_size & (chunk_size - 1)));
> + assert(!hb->meta);
> + hb->meta = hbitmap_alloc(hb->size << hb->granularity,
> + hb->granularity + ctz32(chunk_size));
> + return hb->meta;
> +}
--
Best regards,
Vladimir
* now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 06/13] HBitmap: Introduce "meta" bitmap to track bit changes
2016-01-11 15:40 ` Vladimir Sementsov-Ogievskiy
@ 2016-01-11 18:56 ` John Snow
2016-01-12 8:25 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 36+ messages in thread
From: John Snow @ 2016-01-11 18:56 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, Fam Zheng, qemu-devel
Cc: Kevin Wolf, Jeff Cody, qemu-block
On 01/11/2016 10:40 AM, Vladimir Sementsov-Ogievskiy wrote:
> On 04.01.2016 13:27, Fam Zheng wrote:
>> Upon each bit toggle, the corresponding bit in the meta bitmap will be
>> set.
>>
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>> ---
>> include/qemu/hbitmap.h | 8 +++++++
>> util/hbitmap.c | 61
>> +++++++++++++++++++++++++++++++++++++-------------
>> 2 files changed, 54 insertions(+), 15 deletions(-)
>>
>> diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
>> index bb94a00..ed672e7 100644
>> --- a/include/qemu/hbitmap.h
>> +++ b/include/qemu/hbitmap.h
>> @@ -181,6 +181,14 @@ void hbitmap_iter_init(HBitmapIter *hbi, const
>> HBitmap *hb, uint64_t first);
>> */
>> unsigned long hbitmap_iter_skip_words(HBitmapIter *hbi);
>> +/* hbitmap_create_meta
>> + * Create a "meta" hbitmap to track dirtiness of the bits in this
>> HBitmap.
>> + *
>> + * @hb: The HBitmap to operate on.
>> + * @chunk_size: How many bits in @hb does one bit in the meta track.
>> + */
>> +HBitmap *hbitmap_create_meta(HBitmap *hb, int chunk_size);
>> +
>> /**
>> * hbitmap_iter_next:
>> * @hbi: HBitmapIter to operate on.
>> diff --git a/util/hbitmap.c b/util/hbitmap.c
>> index 50b888f..55d3182 100644
>> --- a/util/hbitmap.c
>> +++ b/util/hbitmap.c
>> @@ -81,6 +81,9 @@ struct HBitmap {
>> */
>> int granularity;
>> + /* A meta dirty bitmap to track the dirtiness of bits in this
>> HBitmap. */
>> + HBitmap *meta;
>> +
>> /* A number of progressively less coarse bitmaps (i.e. level 0
>> is the
>> * coarsest). Each bit in level N represents a word in level
>> N+1 that
>> * has a set bit, except the last level where each bit
>> represents the
>> @@ -212,25 +215,27 @@ static uint64_t hb_count_between(HBitmap *hb,
>> uint64_t start, uint64_t last)
>> }
>> /* Setting starts at the last layer and propagates up if an element
>> - * changes from zero to non-zero.
>> + * changes.
>> */
>> static inline bool hb_set_elem(unsigned long *elem, uint64_t start,
>> uint64_t last)
>> {
>> unsigned long mask;
>> - bool changed;
>> + unsigned long old;
>> assert((last >> BITS_PER_LEVEL) == (start >> BITS_PER_LEVEL));
>> assert(start <= last);
>> mask = 2UL << (last & (BITS_PER_LONG - 1));
>> mask -= 1UL << (start & (BITS_PER_LONG - 1));
>> - changed = (*elem == 0);
>> + old = *elem;
>> *elem |= mask;
>> - return changed;
>> + return old != *elem;
>> }
>> -/* The recursive workhorse (the depth is limited to
>> HBITMAP_LEVELS)... */
>> -static void hb_set_between(HBitmap *hb, int level, uint64_t start,
>> uint64_t last)
>> +/* The recursive workhorse (the depth is limited to HBITMAP_LEVELS)...
>> + * Returns true if at least one bit is changed. */
>> +static bool hb_set_between(HBitmap *hb, int level, uint64_t start,
>> + uint64_t last)
>> {
>> size_t pos = start >> BITS_PER_LEVEL;
>> size_t lastpos = last >> BITS_PER_LEVEL;
>> @@ -259,22 +264,27 @@ static void hb_set_between(HBitmap *hb, int
>> level, uint64_t start, uint64_t last
>> if (level > 0 && changed) {
>> hb_set_between(hb, level - 1, pos, lastpos);
>> }
>> + return changed;
>> }
>> void hbitmap_set(HBitmap *hb, uint64_t start, uint64_t count)
>> {
>> /* Compute range in the last layer. */
>> + uint64_t first, n;
>> uint64_t last = start + count - 1;
>> trace_hbitmap_set(hb, start, count,
>> start >> hb->granularity, last >>
>> hb->granularity);
>> - start >>= hb->granularity;
>> + first = start >> hb->granularity;
>> last >>= hb->granularity;
>> - count = last - start + 1;
>> + n = last - first + 1;
>> - hb->count += count - hb_count_between(hb, start, last);
>> - hb_set_between(hb, HBITMAP_LEVELS - 1, start, last);
>> + hb->count += n - hb_count_between(hb, first, last);
>> + if (hb_set_between(hb, HBITMAP_LEVELS - 1, first, last) &&
>> + hb->meta) {
>
> I don't know, what optimizer things about it, but definetly
>
> + if (hb->meta &&
> + hb_set_between(hb, HBITMAP_LEVELS - 1, first, last))
>
> should work faster for most cases, when hb->meta == NULL.
>
>
The hb_set_between is first to ensure it always happens.
>> + hbitmap_set(hb->meta, start, count);
>> + }
>> }
>> /* Resetting works the other way round: propagate up if the new
>> @@ -295,8 +305,10 @@ static inline bool hb_reset_elem(unsigned long
>> *elem, uint64_t start, uint64_t l
>> return blanked;
>> }
>> -/* The recursive workhorse (the depth is limited to
>> HBITMAP_LEVELS)... */
>> -static void hb_reset_between(HBitmap *hb, int level, uint64_t start,
>> uint64_t last)
>> +/* The recursive workhorse (the depth is limited to HBITMAP_LEVELS)...
>> + * Returns true if at least one bit is changed. */
>> +static bool hb_reset_between(HBitmap *hb, int level, uint64_t start,
>> + uint64_t last)
>> {
>> size_t pos = start >> BITS_PER_LEVEL;
>> size_t lastpos = last >> BITS_PER_LEVEL;
>> @@ -339,21 +351,28 @@ static void hb_reset_between(HBitmap *hb, int
>> level, uint64_t start, uint64_t la
>> if (level > 0 && changed) {
>> hb_reset_between(hb, level - 1, pos, lastpos);
>> }
>> +
>> + return changed;
>> +
>> }
>> void hbitmap_reset(HBitmap *hb, uint64_t start, uint64_t count)
>> {
>> /* Compute range in the last layer. */
>> + uint64_t first;
>> uint64_t last = start + count - 1;
>> trace_hbitmap_reset(hb, start, count,
>> start >> hb->granularity, last >>
>> hb->granularity);
>> - start >>= hb->granularity;
>> + first = start >> hb->granularity;
>> last >>= hb->granularity;
>> - hb->count -= hb_count_between(hb, start, last);
>> - hb_reset_between(hb, HBITMAP_LEVELS - 1, start, last);
>> + hb->count -= hb_count_between(hb, first, last);
>> + if (hb_reset_between(hb, HBITMAP_LEVELS - 1, first, last) &&
>> + hb->meta) {
>
> and here
>
>> + hbitmap_set(hb->meta, start, count);
>> + }
>> }
>> void hbitmap_reset_all(HBitmap *hb)
>> @@ -384,6 +403,9 @@ void hbitmap_free(HBitmap *hb)
>> for (i = HBITMAP_LEVELS; i-- > 0; ) {
>> g_free(hb->levels[i]);
>> }
>> + if (hb->meta) {
>> + hbitmap_free(hb->meta);
>> + }
>
> hmm, not obvious for me.. why not "the one who creates must than destroy"?
>
>> g_free(hb);
>> }
>> @@ -493,3 +515,12 @@ bool hbitmap_merge(HBitmap *a, const HBitmap *b)
>> return true;
>> }
>> +
>> +HBitmap *hbitmap_create_meta(HBitmap *hb, int chunk_size)
>> +{
>> + assert(!(chunk_size & (chunk_size - 1)));
>> + assert(!hb->meta);
>> + hb->meta = hbitmap_alloc(hb->size << hb->granularity,
>> + hb->granularity + ctz32(chunk_size));
>> + return hb->meta;
>> +}
>
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 06/13] HBitmap: Introduce "meta" bitmap to track bit changes
2016-01-11 18:56 ` John Snow
@ 2016-01-12 8:25 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 36+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2016-01-12 8:25 UTC (permalink / raw)
To: John Snow, Fam Zheng, qemu-devel; +Cc: Kevin Wolf, Jeff Cody, qemu-block
On 11.01.2016 21:56, John Snow wrote:
>
> On 01/11/2016 10:40 AM, Vladimir Sementsov-Ogievskiy wrote:
>> On 04.01.2016 13:27, Fam Zheng wrote:
>>> Upon each bit toggle, the corresponding bit in the meta bitmap will be
>>> set.
>>>
>>> Signed-off-by: Fam Zheng <famz@redhat.com>
>>> ---
>>> include/qemu/hbitmap.h | 8 +++++++
>>> util/hbitmap.c | 61
>>> +++++++++++++++++++++++++++++++++++++-------------
>>> 2 files changed, 54 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
>>> index bb94a00..ed672e7 100644
>>> --- a/include/qemu/hbitmap.h
>>> +++ b/include/qemu/hbitmap.h
>>> @@ -181,6 +181,14 @@ void hbitmap_iter_init(HBitmapIter *hbi, const
>>> HBitmap *hb, uint64_t first);
>>> */
>>> unsigned long hbitmap_iter_skip_words(HBitmapIter *hbi);
>>> +/* hbitmap_create_meta
>>> + * Create a "meta" hbitmap to track dirtiness of the bits in this
>>> HBitmap.
>>> + *
>>> + * @hb: The HBitmap to operate on.
>>> + * @chunk_size: How many bits in @hb does one bit in the meta track.
>>> + */
>>> +HBitmap *hbitmap_create_meta(HBitmap *hb, int chunk_size);
>>> +
>>> /**
>>> * hbitmap_iter_next:
>>> * @hbi: HBitmapIter to operate on.
>>> diff --git a/util/hbitmap.c b/util/hbitmap.c
>>> index 50b888f..55d3182 100644
>>> --- a/util/hbitmap.c
>>> +++ b/util/hbitmap.c
>>> @@ -81,6 +81,9 @@ struct HBitmap {
>>> */
>>> int granularity;
>>> + /* A meta dirty bitmap to track the dirtiness of bits in this
>>> HBitmap. */
>>> + HBitmap *meta;
>>> +
>>> /* A number of progressively less coarse bitmaps (i.e. level 0
>>> is the
>>> * coarsest). Each bit in level N represents a word in level
>>> N+1 that
>>> * has a set bit, except the last level where each bit
>>> represents the
>>> @@ -212,25 +215,27 @@ static uint64_t hb_count_between(HBitmap *hb,
>>> uint64_t start, uint64_t last)
>>> }
>>> /* Setting starts at the last layer and propagates up if an element
>>> - * changes from zero to non-zero.
>>> + * changes.
>>> */
>>> static inline bool hb_set_elem(unsigned long *elem, uint64_t start,
>>> uint64_t last)
>>> {
>>> unsigned long mask;
>>> - bool changed;
>>> + unsigned long old;
>>> assert((last >> BITS_PER_LEVEL) == (start >> BITS_PER_LEVEL));
>>> assert(start <= last);
>>> mask = 2UL << (last & (BITS_PER_LONG - 1));
>>> mask -= 1UL << (start & (BITS_PER_LONG - 1));
>>> - changed = (*elem == 0);
>>> + old = *elem;
>>> *elem |= mask;
>>> - return changed;
>>> + return old != *elem;
>>> }
>>> -/* The recursive workhorse (the depth is limited to
>>> HBITMAP_LEVELS)... */
>>> -static void hb_set_between(HBitmap *hb, int level, uint64_t start,
>>> uint64_t last)
>>> +/* The recursive workhorse (the depth is limited to HBITMAP_LEVELS)...
>>> + * Returns true if at least one bit is changed. */
>>> +static bool hb_set_between(HBitmap *hb, int level, uint64_t start,
>>> + uint64_t last)
>>> {
>>> size_t pos = start >> BITS_PER_LEVEL;
>>> size_t lastpos = last >> BITS_PER_LEVEL;
>>> @@ -259,22 +264,27 @@ static void hb_set_between(HBitmap *hb, int
>>> level, uint64_t start, uint64_t last
>>> if (level > 0 && changed) {
>>> hb_set_between(hb, level - 1, pos, lastpos);
>>> }
>>> + return changed;
>>> }
>>> void hbitmap_set(HBitmap *hb, uint64_t start, uint64_t count)
>>> {
>>> /* Compute range in the last layer. */
>>> + uint64_t first, n;
>>> uint64_t last = start + count - 1;
>>> trace_hbitmap_set(hb, start, count,
>>> start >> hb->granularity, last >>
>>> hb->granularity);
>>> - start >>= hb->granularity;
>>> + first = start >> hb->granularity;
>>> last >>= hb->granularity;
>>> - count = last - start + 1;
>>> + n = last - first + 1;
>>> - hb->count += count - hb_count_between(hb, start, last);
>>> - hb_set_between(hb, HBITMAP_LEVELS - 1, start, last);
>>> + hb->count += n - hb_count_between(hb, first, last);
>>> + if (hb_set_between(hb, HBITMAP_LEVELS - 1, first, last) &&
>>> + hb->meta) {
>> I don't know, what optimizer things about it, but definetly
>>
>> + if (hb->meta &&
>> + hb_set_between(hb, HBITMAP_LEVELS - 1, first, last))
>>
>> should work faster for most cases, when hb->meta == NULL.
>>
>>
> The hb_set_between is first to ensure it always happens.
oh, right. imho, it would be better then to add bool changed =
hb_set_between(), and then if (changed && hb->meta), but it's up to you
>
>>> + hbitmap_set(hb->meta, start, count);
>>> + }
>>> }
>>> /* Resetting works the other way round: propagate up if the new
>>> @@ -295,8 +305,10 @@ static inline bool hb_reset_elem(unsigned long
>>> *elem, uint64_t start, uint64_t l
>>> return blanked;
>>> }
>>> -/* The recursive workhorse (the depth is limited to
>>> HBITMAP_LEVELS)... */
>>> -static void hb_reset_between(HBitmap *hb, int level, uint64_t start,
>>> uint64_t last)
>>> +/* The recursive workhorse (the depth is limited to HBITMAP_LEVELS)...
>>> + * Returns true if at least one bit is changed. */
>>> +static bool hb_reset_between(HBitmap *hb, int level, uint64_t start,
>>> + uint64_t last)
>>> {
>>> size_t pos = start >> BITS_PER_LEVEL;
>>> size_t lastpos = last >> BITS_PER_LEVEL;
>>> @@ -339,21 +351,28 @@ static void hb_reset_between(HBitmap *hb, int
>>> level, uint64_t start, uint64_t la
>>> if (level > 0 && changed) {
>>> hb_reset_between(hb, level - 1, pos, lastpos);
>>> }
>>> +
>>> + return changed;
>>> +
>>> }
>>> void hbitmap_reset(HBitmap *hb, uint64_t start, uint64_t count)
>>> {
>>> /* Compute range in the last layer. */
>>> + uint64_t first;
>>> uint64_t last = start + count - 1;
>>> trace_hbitmap_reset(hb, start, count,
>>> start >> hb->granularity, last >>
>>> hb->granularity);
>>> - start >>= hb->granularity;
>>> + first = start >> hb->granularity;
>>> last >>= hb->granularity;
>>> - hb->count -= hb_count_between(hb, start, last);
>>> - hb_reset_between(hb, HBITMAP_LEVELS - 1, start, last);
>>> + hb->count -= hb_count_between(hb, first, last);
>>> + if (hb_reset_between(hb, HBITMAP_LEVELS - 1, first, last) &&
>>> + hb->meta) {
>> and here
>>
>>> + hbitmap_set(hb->meta, start, count);
>>> + }
>>> }
>>> void hbitmap_reset_all(HBitmap *hb)
>>> @@ -384,6 +403,9 @@ void hbitmap_free(HBitmap *hb)
>>> for (i = HBITMAP_LEVELS; i-- > 0; ) {
>>> g_free(hb->levels[i]);
>>> }
>>> + if (hb->meta) {
>>> + hbitmap_free(hb->meta);
>>> + }
>> hmm, not obvious for me.. why not "the one who creates must than destroy"?
>>
>>> g_free(hb);
>>> }
>>> @@ -493,3 +515,12 @@ bool hbitmap_merge(HBitmap *a, const HBitmap *b)
>>> return true;
>>> }
>>> +
>>> +HBitmap *hbitmap_create_meta(HBitmap *hb, int chunk_size)
>>> +{
>>> + assert(!(chunk_size & (chunk_size - 1)));
>>> + assert(!hb->meta);
>>> + hb->meta = hbitmap_alloc(hb->size << hb->granularity,
>>> + hb->granularity + ctz32(chunk_size));
>>> + return hb->meta;
>>> +}
>>
--
Best regards,
Vladimir
* now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 05/13] block: Hide HBitmap in block dirty bitmap interface
2016-01-05 23:01 ` John Snow
@ 2016-01-20 5:09 ` Fam Zheng
0 siblings, 0 replies; 36+ messages in thread
From: Fam Zheng @ 2016-01-20 5:09 UTC (permalink / raw)
To: John Snow
Cc: Kevin Wolf, Jeff Cody, Vladimir Sementsov-Ogievskiy, qemu-devel,
qemu-block
On Tue, 01/05 18:01, John Snow wrote:
> Should we skip adding the typedef for HBitmapIter if we're just going to
> use this instead?
Yes, we can clean this up.
>
> On 01/04/2016 05:27 AM, Fam Zheng wrote:
> > HBitmap is an implementation detail of block dirty bitmap that should be hidden
> > from users. Introduce a BdrvDirtyBitmapIter to encapsulate the underlying
> > HBitmapIter.
> >
> > A small difference in the interface is, before, an HBitmapIter is initialized
> > in place, now the new BdrvDirtyBitmapIter must be dynamically allocated because
> > the structure definition is in block.c.
> >
> > Two current users are converted too.
> >
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> > block/backup.c | 14 ++++++++------
> > block/dirty-bitmap.c | 36 +++++++++++++++++++++++++++++++-----
> > block/mirror.c | 14 ++++++++------
> > include/block/dirty-bitmap.h | 7 +++++--
> > include/qemu/typedefs.h | 1 +
> > 5 files changed, 53 insertions(+), 19 deletions(-)
> >
> > diff --git a/block/backup.c b/block/backup.c
> > index 56ddec0..2388039 100644
> > --- a/block/backup.c
> > +++ b/block/backup.c
> > @@ -326,14 +326,14 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
> > int64_t end;
> > int64_t last_cluster = -1;
> > BlockDriverState *bs = job->common.bs;
> > - HBitmapIter hbi;
> > + BdrvDirtyBitmapIter *dbi;
> >
> > granularity = bdrv_dirty_bitmap_granularity(job->sync_bitmap);
> > clusters_per_iter = MAX((granularity / BACKUP_CLUSTER_SIZE), 1);
> > - bdrv_dirty_iter_init(job->sync_bitmap, &hbi);
> > + dbi = bdrv_dirty_iter_new(job->sync_bitmap, 0);
> >
> > /* Find the next dirty sector(s) */
> > - while ((sector = hbitmap_iter_next(&hbi)) != -1) {
> > + while ((sector = bdrv_dirty_iter_next(dbi)) != -1) {
> > cluster = sector / BACKUP_SECTORS_PER_CLUSTER;
> >
> > /* Fake progress updates for any clusters we skipped */
> > @@ -345,7 +345,7 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
> > for (end = cluster + clusters_per_iter; cluster < end; cluster++) {
> > do {
> > if (yield_and_check(job)) {
> > - return ret;
> > + goto out;
> > }
> > ret = backup_do_cow(bs, cluster * BACKUP_SECTORS_PER_CLUSTER,
> > BACKUP_SECTORS_PER_CLUSTER, &error_is_read,
> > @@ -353,7 +353,7 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
> > if ((ret < 0) &&
> > backup_error_action(job, error_is_read, -ret) ==
> > BLOCK_ERROR_ACTION_REPORT) {
> > - return ret;
> > + goto out;
> > }
> > } while (ret < 0);
> > }
> > @@ -361,7 +361,7 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
> > /* If the bitmap granularity is smaller than the backup granularity,
> > * we need to advance the iterator pointer to the next cluster. */
> > if (granularity < BACKUP_CLUSTER_SIZE) {
> > - bdrv_set_dirty_iter(&hbi, cluster * BACKUP_SECTORS_PER_CLUSTER);
> > + bdrv_set_dirty_iter(dbi, cluster * BACKUP_SECTORS_PER_CLUSTER);
> > }
> >
> > last_cluster = cluster - 1;
> > @@ -373,6 +373,8 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
> > job->common.offset += ((end - last_cluster - 1) * BACKUP_CLUSTER_SIZE);
> > }
> >
> > +out:
> > + bdrv_dirty_iter_free(dbi);
> > return ret;
> > }
> >
> > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> > index 7924c38..53cf88d 100644
> > --- a/block/dirty-bitmap.c
> > +++ b/block/dirty-bitmap.c
> > @@ -41,9 +41,15 @@ struct BdrvDirtyBitmap {
> > char *name; /* Optional non-empty unique ID */
> > int64_t size; /* Size of the bitmap (Number of sectors) */
> > bool disabled; /* Bitmap is read-only */
> > + int active_iterators; /* How many iterators are active */
> > QLIST_ENTRY(BdrvDirtyBitmap) list;
> > };
> >
> > +struct BdrvDirtyBitmapIter {
> > + HBitmapIter hbi;
> > + BdrvDirtyBitmap *bitmap;
> > +};
> > +
> > BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, const char *name)
> > {
> > BdrvDirtyBitmap *bm;
> > @@ -221,6 +227,7 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
> > BdrvDirtyBitmap *bm, *next;
> > QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
> > if (bm == bitmap) {
> > + assert(!bitmap->active_iterators);
>
> Should we add any assertions into the truncate function, too?
Good idea.
Thanks.
Fam
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 08/13] block: Support meta dirty bitmap
2016-01-07 19:30 ` John Snow
@ 2016-01-20 6:07 ` Fam Zheng
2016-01-20 21:46 ` John Snow
0 siblings, 1 reply; 36+ messages in thread
From: Fam Zheng @ 2016-01-20 6:07 UTC (permalink / raw)
To: John Snow
Cc: Kevin Wolf, Jeff Cody, Vladimir Sementsov-Ogievskiy, qemu-devel,
qemu-block
On Thu, 01/07 14:30, John Snow wrote:
> > +void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap)
> > +{
> > + assert(bitmap->meta);
> > + hbitmap_free(bitmap->meta);
>
> This leaves a dangling pointer inside the Hbitmap, no?
Yes, will fix.
>
> > + bitmap->meta = NULL;
> > +}
> > +
> > +int bdrv_dirty_bitmap_get_meta(BlockDriverState *bs,
> > + BdrvDirtyBitmap *bitmap, int64_t sector,
> > + int nb_sectors)
> > +{
> > + uint64_t i;
> > + int gran = bdrv_dirty_bitmap_granularity(bitmap) >> BDRV_SECTOR_BITS;
> > +
> > + /* To optimize: we can make hbitmap to internally check the range in a
> > + * coarse level, or at least do it word by word. */
> > + for (i = sector; i < sector + nb_sectors; i += gran) {
> > + if (hbitmap_get(bitmap->meta, i)) {
> > + return true;
> > + }
> > + }
> > + return false;
> > +}
> > +
>
> In essence get_meta() is a greedy algorithm that simply returns true if
> anything is set between [sector, sector + nb_sectors], yes?
>
> Is this more useful than just using an iterator directly on the
> meta-bitmap?
>
> I haven't finished reading but, I imagine that:
>
> - If we need to check to see what is dirty specifically, we can just use
> the iterator. If the iterator doesn't return anything, we know it's
> empty. If it does return, we know exactly what's dirty.
> - If we need to explicitly check for emptiness in general, we can use
> the internal popcount.
>
>
> I'm not sure when a 'dirty range bool' will be explicitly useful all by
> itself, but maybe that becomes obvious later.
It's for the meta bitmap user to decide. In the case of persistent dirty bitmap
driver, I simply check whether the range of write request is meta-dirty, and
write the corresponding dirty bitmap range accordingly, rather than splitting
one write req into potentially multiple bit ranges that are meta-dirty. I think
this is reasonable, hence the interface.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 08/13] block: Support meta dirty bitmap
2016-01-20 6:07 ` Fam Zheng
@ 2016-01-20 21:46 ` John Snow
0 siblings, 0 replies; 36+ messages in thread
From: John Snow @ 2016-01-20 21:46 UTC (permalink / raw)
To: Fam Zheng
Cc: Kevin Wolf, Jeff Cody, Vladimir Sementsov-Ogievskiy, qemu-devel,
qemu-block
On 01/20/2016 01:07 AM, Fam Zheng wrote:
> On Thu, 01/07 14:30, John Snow wrote:
>>> +void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>>> +{
>>> + assert(bitmap->meta);
>>> + hbitmap_free(bitmap->meta);
>>
>> This leaves a dangling pointer inside the Hbitmap, no?
>
> Yes, will fix.
>
>>
>>> + bitmap->meta = NULL;
>>> +}
>>> +
>>> +int bdrv_dirty_bitmap_get_meta(BlockDriverState *bs,
>>> + BdrvDirtyBitmap *bitmap, int64_t sector,
>>> + int nb_sectors)
>>> +{
>>> + uint64_t i;
>>> + int gran = bdrv_dirty_bitmap_granularity(bitmap) >> BDRV_SECTOR_BITS;
>>> +
>>> + /* To optimize: we can make hbitmap to internally check the range in a
>>> + * coarse level, or at least do it word by word. */
>>> + for (i = sector; i < sector + nb_sectors; i += gran) {
>>> + if (hbitmap_get(bitmap->meta, i)) {
>>> + return true;
>>> + }
>>> + }
>>> + return false;
>>> +}
>>> +
>>
>> In essence get_meta() is a greedy algorithm that simply returns true if
>> anything is set between [sector, sector + nb_sectors], yes?
>>
>> Is this more useful than just using an iterator directly on the
>> meta-bitmap?
>>
>> I haven't finished reading but, I imagine that:
>>
>> - If we need to check to see what is dirty specifically, we can just use
>> the iterator. If the iterator doesn't return anything, we know it's
>> empty. If it does return, we know exactly what's dirty.
>> - If we need to explicitly check for emptiness in general, we can use
>> the internal popcount.
>>
>>
>> I'm not sure when a 'dirty range bool' will be explicitly useful all by
>> itself, but maybe that becomes obvious later.
>
> It's for the meta bitmap user to decide. In the case of persistent dirty bitmap
> driver, I simply check whether the range of write request is meta-dirty, and
> write the corresponding dirty bitmap range accordingly, rather than splitting
> one write req into potentially multiple bit ranges that are meta-dirty. I think
> this is reasonable, hence the interface.
>
OK, I think I see what the use case is, thanks.
^ permalink raw reply [flat|nested] 36+ messages in thread
end of thread, other threads:[~2016-01-20 21:46 UTC | newest]
Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-04 10:27 [Qemu-devel] [PATCH 00/13] Dirty bitmap changes for migration/persistence work Fam Zheng
2016-01-04 10:27 ` [Qemu-devel] [PATCH 01/13] backup: Use Bitmap to replace "s->bitmap" Fam Zheng
2016-01-04 10:27 ` [Qemu-devel] [PATCH 02/13] typedefs: Add BdrvDirtyBitmap and HBitmapIter Fam Zheng
2016-01-05 22:14 ` John Snow
2016-01-08 2:13 ` Fam Zheng
2016-01-04 10:27 ` [Qemu-devel] [PATCH 03/13] block: Move block dirty bitmap code to separate files Fam Zheng
2016-01-05 22:32 ` John Snow
2016-01-04 10:27 ` [Qemu-devel] [PATCH 04/13] block: Remove unused typedef of BlockDriverDirtyHandler Fam Zheng
2016-01-05 22:35 ` John Snow
2016-01-04 10:27 ` [Qemu-devel] [PATCH 05/13] block: Hide HBitmap in block dirty bitmap interface Fam Zheng
2016-01-05 23:01 ` John Snow
2016-01-20 5:09 ` Fam Zheng
2016-01-04 10:27 ` [Qemu-devel] [PATCH 06/13] HBitmap: Introduce "meta" bitmap to track bit changes Fam Zheng
2016-01-06 0:09 ` John Snow
2016-01-11 15:40 ` Vladimir Sementsov-Ogievskiy
2016-01-11 18:56 ` John Snow
2016-01-12 8:25 ` Vladimir Sementsov-Ogievskiy
2016-01-04 10:27 ` [Qemu-devel] [PATCH 07/13] tests: Add test code for meta bitmap Fam Zheng
2016-01-06 20:46 ` John Snow
2016-01-04 10:27 ` [Qemu-devel] [PATCH 08/13] block: Support meta dirty bitmap Fam Zheng
2016-01-07 19:30 ` John Snow
2016-01-20 6:07 ` Fam Zheng
2016-01-20 21:46 ` John Snow
2016-01-04 10:27 ` [Qemu-devel] [PATCH 09/13] block: Add two dirty bitmap getters Fam Zheng
2016-01-07 19:35 ` John Snow
2016-01-04 10:27 ` [Qemu-devel] [PATCH 10/13] block: Assert that bdrv_release_dirty_bitmap succeeded Fam Zheng
2016-01-07 19:38 ` John Snow
2016-01-04 10:27 ` [Qemu-devel] [PATCH 11/13] hbitmap: serialization Fam Zheng
2016-01-07 21:11 ` John Snow
2016-01-11 15:12 ` Vladimir Sementsov-Ogievskiy
2016-01-11 14:48 ` Vladimir Sementsov-Ogievskiy
2016-01-11 15:19 ` Vladimir Sementsov-Ogievskiy
2016-01-04 10:27 ` [Qemu-devel] [PATCH 12/13] block: BdrvDirtyBitmap serialization interface Fam Zheng
2016-01-04 10:27 ` [Qemu-devel] [PATCH 13/13] tests: Add test code for hbitmap serialization Fam Zheng
2016-01-07 21:32 ` [Qemu-devel] [PATCH 00/13] Dirty bitmap changes for migration/persistence work John Snow
2016-01-08 0:29 ` Fam Zheng
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.