* [Qemu-devel] [PATCH v9 01/13] block/dirty-bitmap: add bdrv_dirty_bitmap_enable_successor()
2017-12-20 15:49 [Qemu-devel] [PATCH v9 00/13] Dirty bitmaps postcopy migration Vladimir Sementsov-Ogievskiy
@ 2017-12-20 15:49 ` Vladimir Sementsov-Ogievskiy
2017-12-28 5:16 ` Fam Zheng
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 02/13] block/dirty-bitmap: fix locking in bdrv_reclaim_dirty_bitmap Vladimir Sementsov-Ogievskiy
` (11 subsequent siblings)
12 siblings, 1 reply; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-12-20 15:49 UTC (permalink / raw)
To: qemu-block, qemu-devel
Cc: pbonzini, armbru, eblake, famz, stefanha, amit.shah, quintela,
mreitz, kwolf, peter.maydell, dgilbert, den, jsnow, vsementsov,
lirans
Enabling bitmap successor is necessary to enable successors of bitmaps
being migrated before target vm start.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
include/block/dirty-bitmap.h | 1 +
block/dirty-bitmap.c | 8 ++++++++
2 files changed, 9 insertions(+)
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 3579a7597c..93d4336505 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -20,6 +20,7 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap,
Error **errp);
+void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap *bitmap);
BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
const char *name);
void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index bd04e991b1..81adbeb6d4 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -237,6 +237,14 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
return 0;
}
+/* Called with BQL taken. */
+void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap *bitmap)
+{
+ qemu_mutex_lock(bitmap->mutex);
+ bdrv_enable_dirty_bitmap(bitmap->successor);
+ qemu_mutex_unlock(bitmap->mutex);
+}
+
/**
* For a bitmap with a successor, yield our name to the successor,
* delete the old bitmap, and return a handle to the new bitmap.
--
2.11.1
^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v9 01/13] block/dirty-bitmap: add bdrv_dirty_bitmap_enable_successor()
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 01/13] block/dirty-bitmap: add bdrv_dirty_bitmap_enable_successor() Vladimir Sementsov-Ogievskiy
@ 2017-12-28 5:16 ` Fam Zheng
0 siblings, 0 replies; 50+ messages in thread
From: Fam Zheng @ 2017-12-28 5:16 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: qemu-block, qemu-devel, kwolf, peter.maydell, lirans, quintela,
jsnow, armbru, mreitz, stefanha, den, amit.shah, pbonzini,
dgilbert
On Wed, 12/20 18:49, Vladimir Sementsov-Ogievskiy wrote:
> Enabling bitmap successor is necessary to enable successors of bitmaps
> being migrated before target vm start.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: John Snow <jsnow@redhat.com>
> ---
> include/block/dirty-bitmap.h | 1 +
> block/dirty-bitmap.c | 8 ++++++++
> 2 files changed, 9 insertions(+)
>
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 3579a7597c..93d4336505 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -20,6 +20,7 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
> BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
> BdrvDirtyBitmap *bitmap,
> Error **errp);
> +void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap *bitmap);
> BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
> const char *name);
> void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap);
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index bd04e991b1..81adbeb6d4 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -237,6 +237,14 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
> return 0;
> }
>
> +/* Called with BQL taken. */
> +void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap *bitmap)
> +{
> + qemu_mutex_lock(bitmap->mutex);
> + bdrv_enable_dirty_bitmap(bitmap->successor);
> + qemu_mutex_unlock(bitmap->mutex);
> +}
> +
> /**
> * For a bitmap with a successor, yield our name to the successor,
> * delete the old bitmap, and return a handle to the new bitmap.
> --
> 2.11.1
>
>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 50+ messages in thread
* [Qemu-devel] [PATCH v9 02/13] block/dirty-bitmap: fix locking in bdrv_reclaim_dirty_bitmap
2017-12-20 15:49 [Qemu-devel] [PATCH v9 00/13] Dirty bitmaps postcopy migration Vladimir Sementsov-Ogievskiy
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 01/13] block/dirty-bitmap: add bdrv_dirty_bitmap_enable_successor() Vladimir Sementsov-Ogievskiy
@ 2017-12-20 15:49 ` Vladimir Sementsov-Ogievskiy
2017-12-28 5:20 ` Fam Zheng
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap Vladimir Sementsov-Ogievskiy
` (10 subsequent siblings)
12 siblings, 1 reply; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-12-20 15:49 UTC (permalink / raw)
To: qemu-block, qemu-devel
Cc: pbonzini, armbru, eblake, famz, stefanha, amit.shah, quintela,
mreitz, kwolf, peter.maydell, dgilbert, den, jsnow, vsementsov,
lirans
Like other setters here these functions should take a lock.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/dirty-bitmap.c | 85 ++++++++++++++++++++++++++++++++--------------------
1 file changed, 53 insertions(+), 32 deletions(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 81adbeb6d4..d83da077d5 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -245,6 +245,51 @@ void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap *bitmap)
qemu_mutex_unlock(bitmap->mutex);
}
+/* Called within bdrv_dirty_bitmap_lock..unlock */
+static void bdrv_do_release_matching_dirty_bitmap_locked(
+ BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+ bool (*cond)(BdrvDirtyBitmap *bitmap))
+{
+ BdrvDirtyBitmap *bm, *next;
+
+ QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
+ if ((!bitmap || bm == bitmap) && (!cond || cond(bm))) {
+ assert(!bm->active_iterators);
+ assert(!bdrv_dirty_bitmap_frozen(bm));
+ assert(!bm->meta);
+ QLIST_REMOVE(bm, list);
+ hbitmap_free(bm->bitmap);
+ g_free(bm->name);
+ g_free(bm);
+
+ if (bitmap) {
+ return;
+ }
+ }
+ }
+
+ if (bitmap) {
+ abort();
+ }
+}
+
+/* Called with BQL taken. */
+static void bdrv_do_release_matching_dirty_bitmap(
+ BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+ bool (*cond)(BdrvDirtyBitmap *bitmap))
+{
+ bdrv_dirty_bitmaps_lock(bs);
+ bdrv_do_release_matching_dirty_bitmap_locked(bs, bitmap, cond);
+ bdrv_dirty_bitmaps_unlock(bs);
+}
+
+/* Called within bdrv_dirty_bitmap_lock..unlock */
+static void bdrv_release_dirty_bitmap_locked(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap)
+{
+ bdrv_do_release_matching_dirty_bitmap_locked(bs, bitmap, NULL);
+}
+
/**
* For a bitmap with a successor, yield our name to the successor,
* delete the old bitmap, and return a handle to the new bitmap.
@@ -286,7 +331,11 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
BdrvDirtyBitmap *parent,
Error **errp)
{
- BdrvDirtyBitmap *successor = parent->successor;
+ BdrvDirtyBitmap *successor;
+
+ qemu_mutex_lock(parent->mutex);
+
+ successor = parent->successor;
if (!successor) {
error_setg(errp, "Cannot reclaim a successor when none is present");
@@ -297,9 +346,11 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
error_setg(errp, "Merging of parent and successor bitmap failed");
return NULL;
}
- bdrv_release_dirty_bitmap(bs, successor);
+ bdrv_release_dirty_bitmap_locked(bs, successor);
parent->successor = NULL;
+ qemu_mutex_unlock(parent->mutex);
+
return parent;
}
@@ -327,36 +378,6 @@ static bool bdrv_dirty_bitmap_has_name(BdrvDirtyBitmap *bitmap)
}
/* Called with BQL taken. */
-static void bdrv_do_release_matching_dirty_bitmap(
- BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
- bool (*cond)(BdrvDirtyBitmap *bitmap))
-{
- BdrvDirtyBitmap *bm, *next;
- bdrv_dirty_bitmaps_lock(bs);
- QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
- if ((!bitmap || bm == bitmap) && (!cond || cond(bm))) {
- assert(!bm->active_iterators);
- assert(!bdrv_dirty_bitmap_frozen(bm));
- assert(!bm->meta);
- QLIST_REMOVE(bm, list);
- hbitmap_free(bm->bitmap);
- g_free(bm->name);
- g_free(bm);
-
- if (bitmap) {
- goto out;
- }
- }
- }
- if (bitmap) {
- abort();
- }
-
-out:
- bdrv_dirty_bitmaps_unlock(bs);
-}
-
-/* Called with BQL taken. */
void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
{
bdrv_do_release_matching_dirty_bitmap(bs, bitmap, NULL);
--
2.11.1
^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v9 02/13] block/dirty-bitmap: fix locking in bdrv_reclaim_dirty_bitmap
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 02/13] block/dirty-bitmap: fix locking in bdrv_reclaim_dirty_bitmap Vladimir Sementsov-Ogievskiy
@ 2017-12-28 5:20 ` Fam Zheng
0 siblings, 0 replies; 50+ messages in thread
From: Fam Zheng @ 2017-12-28 5:20 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: qemu-block, qemu-devel, kwolf, peter.maydell, lirans, quintela,
jsnow, armbru, mreitz, stefanha, den, amit.shah, pbonzini,
dgilbert
On Wed, 12/20 18:49, Vladimir Sementsov-Ogievskiy wrote:
> Like other setters here these functions should take a lock.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 50+ messages in thread
* [Qemu-devel] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap
2017-12-20 15:49 [Qemu-devel] [PATCH v9 00/13] Dirty bitmaps postcopy migration Vladimir Sementsov-Ogievskiy
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 01/13] block/dirty-bitmap: add bdrv_dirty_bitmap_enable_successor() Vladimir Sementsov-Ogievskiy
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 02/13] block/dirty-bitmap: fix locking in bdrv_reclaim_dirty_bitmap Vladimir Sementsov-Ogievskiy
@ 2017-12-20 15:49 ` Vladimir Sementsov-Ogievskiy
2017-12-28 5:24 ` Fam Zheng
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 04/13] dirty-bitmap: add locked state Vladimir Sementsov-Ogievskiy
` (9 subsequent siblings)
12 siblings, 1 reply; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-12-20 15:49 UTC (permalink / raw)
To: qemu-block, qemu-devel
Cc: pbonzini, armbru, eblake, famz, stefanha, amit.shah, quintela,
mreitz, kwolf, peter.maydell, dgilbert, den, jsnow, vsementsov,
lirans
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
include/block/dirty-bitmap.h | 3 +++
block/dirty-bitmap.c | 25 ++++++++++++++++---------
2 files changed, 19 insertions(+), 9 deletions(-)
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 93d4336505..caf1f3d861 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -92,5 +92,8 @@ bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap);
char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp);
+BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap,
+ Error **errp);
#endif
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index d83da077d5..fe27ddfb83 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -327,15 +327,11 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
* The merged parent will be un-frozen, but not explicitly re-enabled.
* Called with BQL taken.
*/
-BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
- BdrvDirtyBitmap *parent,
- Error **errp)
+BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
+ BdrvDirtyBitmap *parent,
+ Error **errp)
{
- BdrvDirtyBitmap *successor;
-
- qemu_mutex_lock(parent->mutex);
-
- successor = parent->successor;
+ BdrvDirtyBitmap *successor = parent->successor;
if (!successor) {
error_setg(errp, "Cannot reclaim a successor when none is present");
@@ -349,9 +345,20 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
bdrv_release_dirty_bitmap_locked(bs, successor);
parent->successor = NULL;
+ return parent;
+}
+
+BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
+ BdrvDirtyBitmap *parent,
+ Error **errp)
+{
+ BdrvDirtyBitmap *ret;
+
+ qemu_mutex_lock(parent->mutex);
+ ret = bdrv_reclaim_dirty_bitmap_locked(bs, parent, errp);
qemu_mutex_unlock(parent->mutex);
- return parent;
+ return ret;
}
/**
--
2.11.1
^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap Vladimir Sementsov-Ogievskiy
@ 2017-12-28 5:24 ` Fam Zheng
2017-12-28 11:16 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 50+ messages in thread
From: Fam Zheng @ 2017-12-28 5:24 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: qemu-block, qemu-devel, pbonzini, armbru, eblake, stefanha,
amit.shah, quintela, mreitz, kwolf, peter.maydell, dgilbert, den,
jsnow, lirans
On Wed, 12/20 18:49, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> include/block/dirty-bitmap.h | 3 +++
> block/dirty-bitmap.c | 25 ++++++++++++++++---------
> 2 files changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 93d4336505..caf1f3d861 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -92,5 +92,8 @@ bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
> BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
> BdrvDirtyBitmap *bitmap);
> char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp);
> +BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
> + BdrvDirtyBitmap *bitmap,
> + Error **errp);
>
> #endif
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index d83da077d5..fe27ddfb83 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -327,15 +327,11 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
> * The merged parent will be un-frozen, but not explicitly re-enabled.
> * Called with BQL taken.
Maybe update the comment as
s/Called with BQL taken/Called within bdrv_dirty_bitmap_lock..unlock/
and ...
> */
> -BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
> - BdrvDirtyBitmap *parent,
> - Error **errp)
> +BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
> + BdrvDirtyBitmap *parent,
> + Error **errp)
> {
> - BdrvDirtyBitmap *successor;
> -
> - qemu_mutex_lock(parent->mutex);
> -
> - successor = parent->successor;
> + BdrvDirtyBitmap *successor = parent->successor;
>
> if (!successor) {
> error_setg(errp, "Cannot reclaim a successor when none is present");
> @@ -349,9 +345,20 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
> bdrv_release_dirty_bitmap_locked(bs, successor);
> parent->successor = NULL;
>
> + return parent;
> +}
> +
... move the "Called with BQL taken" comment here?
> +BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
> + BdrvDirtyBitmap *parent,
> + Error **errp)
> +{
> + BdrvDirtyBitmap *ret;
> +
> + qemu_mutex_lock(parent->mutex);
> + ret = bdrv_reclaim_dirty_bitmap_locked(bs, parent, errp);
> qemu_mutex_unlock(parent->mutex);
>
> - return parent;
> + return ret;
> }
>
> /**
> --
> 2.11.1
>
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap
2017-12-28 5:24 ` Fam Zheng
@ 2017-12-28 11:16 ` Vladimir Sementsov-Ogievskiy
2017-12-29 1:31 ` Fam Zheng
0 siblings, 1 reply; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-12-28 11:16 UTC (permalink / raw)
To: Fam Zheng
Cc: qemu-block, qemu-devel, pbonzini, armbru, eblake, stefanha,
amit.shah, quintela, mreitz, kwolf, peter.maydell, dgilbert, den,
jsnow, lirans
28.12.2017 08:24, Fam Zheng wrote:
> On Wed, 12/20 18:49, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>> include/block/dirty-bitmap.h | 3 +++
>> block/dirty-bitmap.c | 25 ++++++++++++++++---------
>> 2 files changed, 19 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
>> index 93d4336505..caf1f3d861 100644
>> --- a/include/block/dirty-bitmap.h
>> +++ b/include/block/dirty-bitmap.h
>> @@ -92,5 +92,8 @@ bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
>> BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
>> BdrvDirtyBitmap *bitmap);
>> char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp);
>> +BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
>> + BdrvDirtyBitmap *bitmap,
>> + Error **errp);
>>
>> #endif
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index d83da077d5..fe27ddfb83 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -327,15 +327,11 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
>> * The merged parent will be un-frozen, but not explicitly re-enabled.
>> * Called with BQL taken.
> Maybe update the comment as
>
> s/Called with BQL taken/Called within bdrv_dirty_bitmap_lock..unlock/
>
> and ...
we have the following comment:
/* Writing to the list requires the BQL _and_ the dirty_bitmap_mutex.
* Reading from the list can be done with either the BQL or the
* dirty_bitmap_mutex. Modifying a bitmap only requires
* dirty_bitmap_mutex. */
QemuMutex dirty_bitmap_mutex;
(I don't understand well the logic, why is it so. Paolo introduced the
lock, but didn't update some functions..)
so, actually, here we need both BQL and mutex.
>
>> */
>> -BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
>> - BdrvDirtyBitmap *parent,
>> - Error **errp)
>> +BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
>> + BdrvDirtyBitmap *parent,
>> + Error **errp)
>> {
>> - BdrvDirtyBitmap *successor;
>> -
>> - qemu_mutex_lock(parent->mutex);
>> -
>> - successor = parent->successor;
>> + BdrvDirtyBitmap *successor = parent->successor;
>>
>> if (!successor) {
>> error_setg(errp, "Cannot reclaim a successor when none is present");
>> @@ -349,9 +345,20 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
>> bdrv_release_dirty_bitmap_locked(bs, successor);
>> parent->successor = NULL;
>>
>> + return parent;
>> +}
>> +
> ... move the "Called with BQL taken" comment here?
and here BQL.
Ok, I'll add/fix comments.
>
>> +BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
>> + BdrvDirtyBitmap *parent,
>> + Error **errp)
>> +{
>> + BdrvDirtyBitmap *ret;
>> +
>> + qemu_mutex_lock(parent->mutex);
>> + ret = bdrv_reclaim_dirty_bitmap_locked(bs, parent, errp);
>> qemu_mutex_unlock(parent->mutex);
>>
>> - return parent;
>> + return ret;
>> }
>>
>> /**
>> --
>> 2.11.1
>>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap
2017-12-28 11:16 ` Vladimir Sementsov-Ogievskiy
@ 2017-12-29 1:31 ` Fam Zheng
2018-01-18 8:43 ` Paolo Bonzini
0 siblings, 1 reply; 50+ messages in thread
From: Fam Zheng @ 2017-12-29 1:31 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: kwolf, peter.maydell, lirans, qemu-block, quintela, jsnow,
qemu-devel, armbru, stefanha, den, amit.shah, pbonzini, mreitz,
dgilbert
On Thu, 12/28 14:16, Vladimir Sementsov-Ogievskiy wrote:
> 28.12.2017 08:24, Fam Zheng wrote:
> > On Wed, 12/20 18:49, Vladimir Sementsov-Ogievskiy wrote:
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > > ---
> > > include/block/dirty-bitmap.h | 3 +++
> > > block/dirty-bitmap.c | 25 ++++++++++++++++---------
> > > 2 files changed, 19 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> > > index 93d4336505..caf1f3d861 100644
> > > --- a/include/block/dirty-bitmap.h
> > > +++ b/include/block/dirty-bitmap.h
> > > @@ -92,5 +92,8 @@ bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
> > > BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
> > > BdrvDirtyBitmap *bitmap);
> > > char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp);
> > > +BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
> > > + BdrvDirtyBitmap *bitmap,
> > > + Error **errp);
> > > #endif
> > > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> > > index d83da077d5..fe27ddfb83 100644
> > > --- a/block/dirty-bitmap.c
> > > +++ b/block/dirty-bitmap.c
> > > @@ -327,15 +327,11 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
> > > * The merged parent will be un-frozen, but not explicitly re-enabled.
> > > * Called with BQL taken.
> > Maybe update the comment as
> >
> > s/Called with BQL taken/Called within bdrv_dirty_bitmap_lock..unlock/
> >
> > and ...
>
> we have the following comment:
>
> /* Writing to the list requires the BQL _and_ the dirty_bitmap_mutex.
> * Reading from the list can be done with either the BQL or the
> * dirty_bitmap_mutex. Modifying a bitmap only requires
> * dirty_bitmap_mutex. */
> QemuMutex dirty_bitmap_mutex;
>
> (I don't understand well the logic, why is it so. Paolo introduced the lock,
> but didn't update some functions..)
>
> so, actually, here we need both BQL and mutex.
Yes, because of that comment my interpretion has been both "BQL and the mutex"
whereever we say "within bdrv_dirty_bitmap_lock..unlock", as to some other
places in this file.
Fam
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap
2017-12-29 1:31 ` Fam Zheng
@ 2018-01-18 8:43 ` Paolo Bonzini
2018-01-18 9:55 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 50+ messages in thread
From: Paolo Bonzini @ 2018-01-18 8:43 UTC (permalink / raw)
To: Fam Zheng, Vladimir Sementsov-Ogievskiy
Cc: kwolf, peter.maydell, lirans, qemu-block, quintela, jsnow,
qemu-devel, armbru, stefanha, den, amit.shah, mreitz, dgilbert
On 29/12/2017 02:31, Fam Zheng wrote:
>> we have the following comment:
>>
>> /* Writing to the list requires the BQL _and_ the dirty_bitmap_mutex.
>> * Reading from the list can be done with either the BQL or the
>> * dirty_bitmap_mutex. Modifying a bitmap only requires
>> * dirty_bitmap_mutex. */
>> QemuMutex dirty_bitmap_mutex;
>>
>> (I don't understand well the logic, why is it so. Paolo introduced the lock,
>> but didn't update some functions..)
>>
>> so, actually, here we need both BQL and mutex.
>
> Yes, because of that comment my interpretion has been both "BQL and the mutex"
> whereever we say "within bdrv_dirty_bitmap_lock..unlock", as to some other
> places in this file.
A bit late, but---no, "within bdrv_dirty_bitmap_lock..unlock" means it's
the "modifying the bitmap" case.
Most functions that looks at the list are "called with BQL taken".
Functions that write to the list are "called with BQL taken" and call
bdrv_dirty_bitmaps_lock/bdrv_dirty_bitmaps_unlock themselves.
Paolo
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap
2018-01-18 8:43 ` Paolo Bonzini
@ 2018-01-18 9:55 ` Vladimir Sementsov-Ogievskiy
2018-01-18 10:09 ` Paolo Bonzini
0 siblings, 1 reply; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-01-18 9:55 UTC (permalink / raw)
To: Paolo Bonzini, Fam Zheng
Cc: kwolf, peter.maydell, lirans, qemu-block, quintela, jsnow,
qemu-devel, armbru, stefanha, den, amit.shah, mreitz, dgilbert
18.01.2018 11:43, Paolo Bonzini wrote:
> On 29/12/2017 02:31, Fam Zheng wrote:
>>> we have the following comment:
>>>
>>> /* Writing to the list requires the BQL _and_ the dirty_bitmap_mutex.
>>> * Reading from the list can be done with either the BQL or the
>>> * dirty_bitmap_mutex. Modifying a bitmap only requires
>>> * dirty_bitmap_mutex. */
>>> QemuMutex dirty_bitmap_mutex;
>>>
>>> (I don't understand well the logic, why is it so. Paolo introduced the lock,
>>> but didn't update some functions..)
>>>
>>> so, actually, here we need both BQL and mutex.
>> Yes, because of that comment my interpretion has been both "BQL and the mutex"
>> whereever we say "within bdrv_dirty_bitmap_lock..unlock", as to some other
>> places in this file.
> A bit late, but---no, "within bdrv_dirty_bitmap_lock..unlock" means it's
> the "modifying the bitmap" case.
>
> Most functions that looks at the list are "called with BQL taken".
> Functions that write to the list are "called with BQL taken" and call
> bdrv_dirty_bitmaps_lock/bdrv_dirty_bitmaps_unlock themselves.
>
> Paolo
Paolo, could you please explain about bitmap locking in more details?
Why do we need mutexes? Why do we do not need them
on read from the bitmap, only on write?
I imaging the following cases for locks:
- mutex + BQL(do aio_context_acquire take it?)
- only mutex
- only BQL
- no locks
and following operation on bitmaps:
- r/w bitmaps list (i.e. change .next fields of bitmaps and head of the
list)
- r/w BdrvDirtyBitmap fields
- r/w HBitmap fields
- r/w HBitmap levels (set/unset/read bits)
what is the relations between locking cases and operations and why?
Sorry if I'm asking stupid questions :(
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap
2018-01-18 9:55 ` Vladimir Sementsov-Ogievskiy
@ 2018-01-18 10:09 ` Paolo Bonzini
2018-01-19 14:12 ` Vladimir Sementsov-Ogievskiy
2018-02-12 17:30 ` Vladimir Sementsov-Ogievskiy
0 siblings, 2 replies; 50+ messages in thread
From: Paolo Bonzini @ 2018-01-18 10:09 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, Fam Zheng
Cc: kwolf, peter.maydell, lirans, qemu-block, quintela, jsnow,
qemu-devel, armbru, stefanha, den, amit.shah, mreitz, dgilbert
On 18/01/2018 10:55, Vladimir Sementsov-Ogievskiy wrote:
>>
>> Most functions that looks at the list are "called with BQL taken".
>> Functions that write to the list are "called with BQL taken" and call
>> bdrv_dirty_bitmaps_lock/bdrv_dirty_bitmaps_unlock themselves.
>
> Paolo, could you please explain about bitmap locking in more details?
> Why do we need mutexes?
We have three cases:
1) monitor creates and destroy bitmaps.
2) monitor also has to read the list. We know it operates with BQL.
3) users such as mirror.c create a dirty bitmap in the monitor command
(under BQL), but they can operate without BQL in a separate iothread so
we create a separate lock (bitmap->mutex).
While in the second and third case, bitmaps cannot disappear. So in the
first case you operate with BQL+dirty bitmap mutex. The result is that
you lock out both the second and the third case while creating and
destroying bitmaps.
> Why do we do not need them
> on read from the bitmap, only on write?
Indeed, reading the bitmap also requires taking the lock. So
s/Modifying/Accessing/ in that comment.
Paolo
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap
2018-01-18 10:09 ` Paolo Bonzini
@ 2018-01-19 14:12 ` Vladimir Sementsov-Ogievskiy
2018-01-22 12:14 ` Vladimir Sementsov-Ogievskiy
2018-02-12 17:30 ` Vladimir Sementsov-Ogievskiy
1 sibling, 1 reply; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-01-19 14:12 UTC (permalink / raw)
To: Paolo Bonzini, Fam Zheng
Cc: kwolf, peter.maydell, lirans, qemu-block, quintela, jsnow,
qemu-devel, armbru, stefanha, den, amit.shah, mreitz, dgilbert
18.01.2018 13:09, Paolo Bonzini wrote:
> On 18/01/2018 10:55, Vladimir Sementsov-Ogievskiy wrote:
>>> Most functions that looks at the list are "called with BQL taken".
>>> Functions that write to the list are "called with BQL taken" and call
>>> bdrv_dirty_bitmaps_lock/bdrv_dirty_bitmaps_unlock themselves.
>> Paolo, could you please explain about bitmap locking in more details?
>> Why do we need mutexes?
> We have three cases:
>
> 1) monitor creates and destroy bitmaps.
>
> 2) monitor also has to read the list. We know it operates with BQL.
>
> 3) users such as mirror.c create a dirty bitmap in the monitor command
> (under BQL), but they can operate without BQL in a separate iothread so
> we create a separate lock (bitmap->mutex).
>
> While in the second and third case, bitmaps cannot disappear. So in the
> first case you operate with BQL+dirty bitmap mutex. The result is that
> you lock out both the second and the third case while creating and
> destroying bitmaps.
>
>> Why do we do not need them
>> on read from the bitmap, only on write?
> Indeed, reading the bitmap also requires taking the lock. So
> s/Modifying/Accessing/ in that comment.
>
> Paolo
so,
/* Writing to the list requires the BQL_and_ the dirty_bitmap_mutex.
* Reading from the list can be done with either the BQL or the
* dirty_bitmap_mutex. Accessing a bitmap only requires
* dirty_bitmap_mutex. */
QemuMutex dirty_bitmap_mutex;
so, accessing the bitmap needs mutex lock?
Then what do you mean under accessing the bitmap? Any touch of
BdrvDirtyBitmap fields? Then "reading the list" will require bitmap
mutex too.
Or accessing the bitmap is accessing any field except
BdrvDirtyBitmap.list? Then in (2), what do you mean? For example
query-block will go through
the list, but it touches other fields too, so it should lock mutex.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap
2018-01-19 14:12 ` Vladimir Sementsov-Ogievskiy
@ 2018-01-22 12:14 ` Vladimir Sementsov-Ogievskiy
2018-01-22 17:50 ` John Snow
2018-01-24 10:16 ` Paolo Bonzini
0 siblings, 2 replies; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-01-22 12:14 UTC (permalink / raw)
To: Paolo Bonzini, Fam Zheng
Cc: kwolf, peter.maydell, lirans, qemu-block, quintela, jsnow,
qemu-devel, armbru, stefanha, den, amit.shah, mreitz, dgilbert
19.01.2018 17:12, Vladimir Sementsov-Ogievskiy wrote:
> 18.01.2018 13:09, Paolo Bonzini wrote:
>> On 18/01/2018 10:55, Vladimir Sementsov-Ogievskiy wrote:
>>>> Most functions that looks at the list are "called with BQL taken".
>>>> Functions that write to the list are "called with BQL taken" and call
>>>> bdrv_dirty_bitmaps_lock/bdrv_dirty_bitmaps_unlock themselves.
>>> Paolo, could you please explain about bitmap locking in more details?
>>> Why do we need mutexes?
>> We have three cases:
>>
>> 1) monitor creates and destroy bitmaps.
>>
>> 2) monitor also has to read the list. We know it operates with BQL.
>>
>> 3) users such as mirror.c create a dirty bitmap in the monitor command
>> (under BQL), but they can operate without BQL in a separate iothread so
>> we create a separate lock (bitmap->mutex).
>>
>> While in the second and third case, bitmaps cannot disappear. So in the
>> first case you operate with BQL+dirty bitmap mutex. The result is that
>> you lock out both the second and the third case while creating and
>> destroying bitmaps.
>>
>>> Why do we do not need them
>>> on read from the bitmap, only on write?
>> Indeed, reading the bitmap also requires taking the lock. So
>> s/Modifying/Accessing/ in that comment.
>>
>> Paolo
>
> so,
>
> /* Writing to the list requires the BQL_and_ the dirty_bitmap_mutex.
> * Reading from the list can be done with either the BQL or the
> * dirty_bitmap_mutex. Accessing a bitmap only requires
> * dirty_bitmap_mutex. */
> QemuMutex dirty_bitmap_mutex;
>
>
>
> so, accessing the bitmap needs mutex lock?
>
> Then what do you mean under accessing the bitmap? Any touch of
> BdrvDirtyBitmap fields? Then "reading the list" will require bitmap
> mutex too.
> Or accessing the bitmap is accessing any field except
> BdrvDirtyBitmap.list? Then in (2), what do you mean? For example
> query-block will go through
> the list, but it touches other fields too, so it should lock mutex.
>
and one more question:
What about qmp transactions? Should we lock mutex during the whole
transaction?
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap
2018-01-22 12:14 ` Vladimir Sementsov-Ogievskiy
@ 2018-01-22 17:50 ` John Snow
2018-01-24 10:16 ` Paolo Bonzini
1 sibling, 0 replies; 50+ messages in thread
From: John Snow @ 2018-01-22 17:50 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, Paolo Bonzini, Fam Zheng
Cc: Kevin Wolf, peter.maydell, lirans, qemu-block, quintela,
qemu-devel, armbru, stefanha, den, amit.shah, mreitz, dgilbert
On 01/22/2018 07:14 AM, Vladimir Sementsov-Ogievskiy wrote:
> 19.01.2018 17:12, Vladimir Sementsov-Ogievskiy wrote:
>> 18.01.2018 13:09, Paolo Bonzini wrote:
>>> On 18/01/2018 10:55, Vladimir Sementsov-Ogievskiy wrote:
>>>>> Most functions that looks at the list are "called with BQL taken".
>>>>> Functions that write to the list are "called with BQL taken" and call
>>>>> bdrv_dirty_bitmaps_lock/bdrv_dirty_bitmaps_unlock themselves.
>>>> Paolo, could you please explain about bitmap locking in more details?
>>>> Why do we need mutexes?
>>> We have three cases:
>>>
>>> 1) monitor creates and destroy bitmaps.
>>>
>>> 2) monitor also has to read the list. We know it operates with BQL.
>>>
>>> 3) users such as mirror.c create a dirty bitmap in the monitor command
>>> (under BQL), but they can operate without BQL in a separate iothread so
>>> we create a separate lock (bitmap->mutex).
>>>
>>> While in the second and third case, bitmaps cannot disappear. So in the
>>> first case you operate with BQL+dirty bitmap mutex. The result is that
>>> you lock out both the second and the third case while creating and
>>> destroying bitmaps.
>>>
>>>> Why do we do not need them
>>>> on read from the bitmap, only on write?
>>> Indeed, reading the bitmap also requires taking the lock. So
>>> s/Modifying/Accessing/ in that comment.
>>>
>>> Paolo
>>
>> so,
>>
>> /* Writing to the list requires the BQL_and_ the dirty_bitmap_mutex.
>> * Reading from the list can be done with either the BQL or the
>> * dirty_bitmap_mutex. Accessing a bitmap only requires
>> * dirty_bitmap_mutex. */
>> QemuMutex dirty_bitmap_mutex;
>>
>>
>>
>> so, accessing the bitmap needs mutex lock?
>>
>> Then what do you mean under accessing the bitmap? Any touch of
>> BdrvDirtyBitmap fields? Then "reading the list" will require bitmap
>> mutex too.
>> Or accessing the bitmap is accessing any field except
>> BdrvDirtyBitmap.list? Then in (2), what do you mean? For example
>> query-block will go through
>> the list, but it touches other fields too, so it should lock mutex.
>>
>
> and one more question:
>
> What about qmp transactions? Should we lock mutex during the whole
> transaction?
>
For bitmaps?
hmm...
at the moment, Transactions still do the tepid bdrv_drain_all() prior to
the transaction, and then I suspect they rely on the QMP context holding
the big lock to prevent any new IO occurring.
It should be a quiescent point, I think, but I've lost track of how
exactly it behaves presently. Didn't we need a
bdrv_drained_all_begin/_end pair here? Did we not do that? I forget why...
(...is it related to how we don't know how to implement this in a
context where graph changes might occur, which can happen during a
transaction?)
it might not be possible to grab the bitmap locks during .prepare and
release them in .cleanup, because we might want to add two actions to
the same transaction that operate on the same bitmap (full backup +
clear, for instance?) and they'll deadlock against each other.
It might be sufficient to just lock and release per each action, until
the deeper issues with transactions are resolved. If the transaction is
properly quiescent, you shouldn't run into any bitmap inconsistency
problems anyway.
Hoping Kevin and Paolo can chime in to remind me of the details, here.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap
2018-01-22 12:14 ` Vladimir Sementsov-Ogievskiy
2018-01-22 17:50 ` John Snow
@ 2018-01-24 10:16 ` Paolo Bonzini
2018-01-24 22:29 ` John Snow
2018-02-06 14:53 ` Vladimir Sementsov-Ogievskiy
1 sibling, 2 replies; 50+ messages in thread
From: Paolo Bonzini @ 2018-01-24 10:16 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, Fam Zheng
Cc: kwolf, peter.maydell, lirans, qemu-block, quintela, jsnow,
qemu-devel, armbru, stefanha, den, amit.shah, mreitz, dgilbert
On 22/01/2018 13:14, Vladimir Sementsov-Ogievskiy wrote:
>> so, accessing the bitmap needs mutex lock?
>>
>> Then what do you mean under accessing the bitmap? Any touch of
>> BdrvDirtyBitmap fields? Then "reading the list" will require bitmap
>> mutex too.
>> Or accessing the bitmap is accessing any field except
>> BdrvDirtyBitmap.list? Then in (2), what do you mean? For example
>> query-block will go through
>> the list, but it touches other fields too, so it should lock mutex.
The bitmap mutex is internal to block/dirty-bitmap.c.
> and one more question:
>
> What about qmp transactions? Should we lock mutex during the whole
> transaction?
Transactions hold the BQL, but you don't need to lock the bitmap mutex.
Paolo
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap
2018-01-24 10:16 ` Paolo Bonzini
@ 2018-01-24 22:29 ` John Snow
2018-02-06 14:53 ` Vladimir Sementsov-Ogievskiy
1 sibling, 0 replies; 50+ messages in thread
From: John Snow @ 2018-01-24 22:29 UTC (permalink / raw)
To: Paolo Bonzini, Vladimir Sementsov-Ogievskiy, Fam Zheng
Cc: kwolf, peter.maydell, lirans, qemu-block, quintela, qemu-devel,
armbru, stefanha, den, amit.shah, mreitz, dgilbert
On 01/24/2018 05:16 AM, Paolo Bonzini wrote:
> On 22/01/2018 13:14, Vladimir Sementsov-Ogievskiy wrote:
>>> so, accessing the bitmap needs mutex lock?
>>>
>>> Then what do you mean under accessing the bitmap? Any touch of
>>> BdrvDirtyBitmap fields? Then "reading the list" will require bitmap
>>> mutex too.
>>> Or accessing the bitmap is accessing any field except
>>> BdrvDirtyBitmap.list? Then in (2), what do you mean? For example
>>> query-block will go through
>>> the list, but it touches other fields too, so it should lock mutex.
>
> The bitmap mutex is internal to block/dirty-bitmap.c.
>
>> and one more question:
>>
>> What about qmp transactions? Should we lock mutex during the whole
>> transaction?
>
> Transactions hold the BQL, but you don't need to lock the bitmap mutex.
>
> Paolo
>
I would further add that attempting to lock all of the bitmap mutexes
during a transaction might cause complications if you try to perform two
actions on the same bitmap, and it deadlocks.
With the BQL held we're probably okay, but worst-case scenario locking
and unlocking per-each action is probably sufficient, I'd think.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap
2018-01-24 10:16 ` Paolo Bonzini
2018-01-24 22:29 ` John Snow
@ 2018-02-06 14:53 ` Vladimir Sementsov-Ogievskiy
1 sibling, 0 replies; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-02-06 14:53 UTC (permalink / raw)
To: Paolo Bonzini, Fam Zheng
Cc: kwolf, peter.maydell, lirans, qemu-block, quintela, jsnow,
qemu-devel, armbru, stefanha, den, amit.shah, mreitz, dgilbert
24.01.2018 13:16, Paolo Bonzini wrote:
> On 22/01/2018 13:14, Vladimir Sementsov-Ogievskiy wrote:
>>> so, accessing the bitmap needs mutex lock?
>>>
>>> Then what do you mean under accessing the bitmap? Any touch of
>>> BdrvDirtyBitmap fields? Then "reading the list" will require bitmap
>>> mutex too.
>>> Or accessing the bitmap is accessing any field except
>>> BdrvDirtyBitmap.list? Then in (2), what do you mean? For example
>>> query-block will go through
>>> the list, but it touches other fields too, so it should lock mutex.
> The bitmap mutex is internal to block/dirty-bitmap.c.
yes and query-block actually calls bdrv_query_dirty_bitmaps, which locks
mutex..
>
>> and one more question:
>>
>> What about qmp transactions? Should we lock mutex during the whole
>> transaction?
> Transactions hold the BQL, but you don't need to lock the bitmap mutex.
I don't understand. But "Accessing a bitmap only requires
dirty_bitmap_mutex".. So, if we have several operations on one bitmap,
each of them will lock/unlock mutex by itself? Then we'll have unlocked
"holes" in our transaction. Or this doesn't matter?
>
> Paolo
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap
2018-01-18 10:09 ` Paolo Bonzini
2018-01-19 14:12 ` Vladimir Sementsov-Ogievskiy
@ 2018-02-12 17:30 ` Vladimir Sementsov-Ogievskiy
2018-02-13 7:45 ` Paolo Bonzini
1 sibling, 1 reply; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-02-12 17:30 UTC (permalink / raw)
To: Paolo Bonzini, Fam Zheng
Cc: kwolf, peter.maydell, lirans, qemu-block, quintela, jsnow,
qemu-devel, armbru, stefanha, den, amit.shah, mreitz, dgilbert
18.01.2018 13:09, Paolo Bonzini wrote:
> On 18/01/2018 10:55, Vladimir Sementsov-Ogievskiy wrote:
>>> Most functions that looks at the list are "called with BQL taken".
>>> Functions that write to the list are "called with BQL taken" and call
>>> bdrv_dirty_bitmaps_lock/bdrv_dirty_bitmaps_unlock themselves.
>> Paolo, could you please explain about bitmap locking in more details?
>> Why do we need mutexes?
> We have three cases:
>
> 1) monitor creates and destroy bitmaps.
>
> 2) monitor also has to read the list. We know it operates with BQL.
>
> 3) users such as mirror.c create a dirty bitmap in the monitor command
> (under BQL), but they can operate without BQL in a separate iothread so
> we create a separate lock (bitmap->mutex).
>
> While in the second and third case, bitmaps cannot disappear. So in the
> first case you operate with BQL+dirty bitmap mutex. The result is that
> you lock out both the second and the third case while creating and
> destroying bitmaps.
>
>> Why do we do not need them
>> on read from the bitmap, only on write?
> Indeed, reading the bitmap also requires taking the lock. So
> s/Modifying/Accessing/ in that comment.
>
> Paolo
So, finally, the whole thing is:
1. any access to dirty_bitmaps list needs BQL or dirty_bitmap_mutex
2. bitmap creation or removing needs both BQL and dirty_bitmap_mutex
yes?
and one more question:
Do we really have users, which accesses dirty bitmaps with only BQL?
query-block uses dirty_bitmap_mutex..
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap
2018-02-12 17:30 ` Vladimir Sementsov-Ogievskiy
@ 2018-02-13 7:45 ` Paolo Bonzini
0 siblings, 0 replies; 50+ messages in thread
From: Paolo Bonzini @ 2018-02-13 7:45 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, Fam Zheng
Cc: kwolf, peter.maydell, lirans, qemu-block, quintela, jsnow,
qemu-devel, armbru, stefanha, den, amit.shah, mreitz, dgilbert
On 12/02/2018 18:30, Vladimir Sementsov-Ogievskiy wrote:
> 18.01.2018 13:09, Paolo Bonzini wrote:>> We have three cases:
>>
>> 1) monitor creates and destroy bitmaps.
>>
>> 2) monitor also has to read the list. We know it operates with BQL.
>>
>> 3) users such as mirror.c create a dirty bitmap in the monitor command
>> (under BQL), but they can operate without BQL in a separate iothread so
>> we create a separate lock (bitmap->mutex).
>>
>> While in the second and third case, bitmaps cannot disappear. So in the
>> first case you operate with BQL+dirty bitmap mutex. The result is that
>> you lock out both the second and the third case while creating and
>> destroying bitmaps.
>>
>>> Why do we do not need them
>>> on read from the bitmap, only on write?
>>
>> Indeed, reading the bitmap also requires taking the lock. So
>> s/Modifying/Accessing/ in that comment.
>
> So, finally, the whole thing is:
>
> 1. any access to dirty_bitmaps list needs BQL or dirty_bitmap_mutex
> 2. bitmap creation or removing needs both BQL and dirty_bitmap_mutex
3. any access to a dirty bitmap needs dirty_bitmap_mutex
Paolo
> yes?
>
> and one more question:
> Do we really have users, which accesses dirty bitmaps with only BQL?
> query-block uses dirty_bitmap_mutex..
>
>
^ permalink raw reply [flat|nested] 50+ messages in thread
* [Qemu-devel] [PATCH v9 04/13] dirty-bitmap: add locked state
2017-12-20 15:49 [Qemu-devel] [PATCH v9 00/13] Dirty bitmaps postcopy migration Vladimir Sementsov-Ogievskiy
` (2 preceding siblings ...)
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap Vladimir Sementsov-Ogievskiy
@ 2017-12-20 15:49 ` Vladimir Sementsov-Ogievskiy
2017-12-22 2:03 ` John Snow
` (2 more replies)
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 05/13] migration: introduce postcopy-only pending Vladimir Sementsov-Ogievskiy
` (8 subsequent siblings)
12 siblings, 3 replies; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-12-20 15:49 UTC (permalink / raw)
To: qemu-block, qemu-devel
Cc: pbonzini, armbru, eblake, famz, stefanha, amit.shah, quintela,
mreitz, kwolf, peter.maydell, dgilbert, den, jsnow, vsementsov,
lirans
Add special state, when qmp operations on the bitmap are disabled.
It is needed during bitmap migration. "Frozen" state is not
appropriate here, because it looks like bitmap is unchanged.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
qapi/block-core.json | 5 ++++-
include/block/dirty-bitmap.h | 3 +++
block/dirty-bitmap.c | 16 ++++++++++++++++
blockdev.c | 19 +++++++++++++++++++
4 files changed, 42 insertions(+), 1 deletion(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index dd763dcf87..48c30da6cb 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -426,10 +426,13 @@
# @active: The bitmap is actively monitoring for new writes, and can be cleared,
# deleted, or used for backup operations.
#
+# @locked: The bitmap is currently in-use by some operation and can not be
+# cleared, deleted, or used for backup operations. (Since 2.12)
+#
# Since: 2.4
##
{ 'enum': 'DirtyBitmapStatus',
- 'data': ['active', 'disabled', 'frozen'] }
+ 'data': ['active', 'disabled', 'frozen', 'locked'] }
##
# @BlockDirtyInfo:
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index caf1f3d861..517b04176f 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -69,6 +69,8 @@ void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap, bool value);
void bdrv_dirty_bitmap_set_autoload(BdrvDirtyBitmap *bitmap, bool autoload);
void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
bool persistent);
+void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool qmp_locked);
+
/* Functions that require manual locking. */
void bdrv_dirty_bitmap_lock(BdrvDirtyBitmap *bitmap);
@@ -88,6 +90,7 @@ bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap);
+bool bdrv_dirty_bitmap_qmp_locked(BdrvDirtyBitmap *bitmap);
bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index fe27ddfb83..6218740c95 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -40,6 +40,8 @@ struct BdrvDirtyBitmap {
QemuMutex *mutex;
HBitmap *bitmap; /* Dirty bitmap implementation */
HBitmap *meta; /* Meta dirty bitmap */
+ bool qmp_locked; /* Bitmap is frozen, it can't be modified
+ through QMP */
BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */
char *name; /* Optional non-empty unique ID */
int64_t size; /* Size of the bitmap, in bytes */
@@ -186,6 +188,18 @@ bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
return bitmap->successor;
}
+void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool qmp_locked)
+{
+ qemu_mutex_lock(bitmap->mutex);
+ bitmap->qmp_locked = qmp_locked;
+ qemu_mutex_unlock(bitmap->mutex);
+}
+
+bool bdrv_dirty_bitmap_qmp_locked(BdrvDirtyBitmap *bitmap)
+{
+ return bitmap->qmp_locked;
+}
+
/* Called with BQL taken. */
bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
{
@@ -197,6 +211,8 @@ DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap)
{
if (bdrv_dirty_bitmap_frozen(bitmap)) {
return DIRTY_BITMAP_STATUS_FROZEN;
+ } else if (!bdrv_dirty_bitmap_qmp_locked(bitmap)) {
+ return DIRTY_BITMAP_STATUS_LOCKED;
} else if (!bdrv_dirty_bitmap_enabled(bitmap)) {
return DIRTY_BITMAP_STATUS_DISABLED;
} else {
diff --git a/blockdev.c b/blockdev.c
index 56a6b24a0b..82b2956c5c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2029,6 +2029,9 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common,
if (bdrv_dirty_bitmap_frozen(state->bitmap)) {
error_setg(errp, "Cannot modify a frozen bitmap");
return;
+ } else if (bdrv_dirty_bitmap_qmp_locked(state->bitmap)) {
+ error_setg(errp, "Cannot modify a locked bitmap");
+ return;
} else if (!bdrv_dirty_bitmap_enabled(state->bitmap)) {
error_setg(errp, "Cannot clear a disabled bitmap");
return;
@@ -2780,6 +2783,11 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
"Bitmap '%s' is currently frozen and cannot be removed",
name);
return;
+ } else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) {
+ error_setg(errp,
+ "Bitmap '%s' is currently locked and cannot be removed",
+ name);
+ return;
}
if (bdrv_dirty_bitmap_get_persistance(bitmap)) {
@@ -2814,6 +2822,11 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name,
"Bitmap '%s' is currently frozen and cannot be modified",
name);
return;
+ } else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) {
+ error_setg(errp,
+ "Bitmap '%s' is currently locked and cannot be modified",
+ name);
+ return;
} else if (!bdrv_dirty_bitmap_enabled(bitmap)) {
error_setg(errp,
"Bitmap '%s' is currently disabled and cannot be cleared",
@@ -3288,6 +3301,12 @@ static BlockJob *do_drive_backup(DriveBackup *backup, BlockJobTxn *txn,
bdrv_unref(target_bs);
goto out;
}
+ if (bdrv_dirty_bitmap_qmp_locked(bmap)) {
+ error_setg(errp,
+ "Bitmap '%s' is currently locked and cannot be used for "
+ "backup", backup->bitmap);
+ goto out;
+ }
}
job = backup_job_create(backup->job_id, bs, target_bs, backup->speed,
--
2.11.1
^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v9 04/13] dirty-bitmap: add locked state
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 04/13] dirty-bitmap: add locked state Vladimir Sementsov-Ogievskiy
@ 2017-12-22 2:03 ` John Snow
2017-12-22 9:37 ` Vladimir Sementsov-Ogievskiy
2017-12-22 8:46 ` Vladimir Sementsov-Ogievskiy
2017-12-28 5:32 ` Fam Zheng
2 siblings, 1 reply; 50+ messages in thread
From: John Snow @ 2017-12-22 2:03 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block, qemu-devel
Cc: kwolf, peter.maydell, famz, lirans, quintela, armbru, mreitz,
stefanha, den, amit.shah, pbonzini, dgilbert
On 12/20/2017 10:49 AM, Vladimir Sementsov-Ogievskiy wrote:
> Add special state, when qmp operations on the bitmap are disabled.
> It is needed during bitmap migration. "Frozen" state is not
> appropriate here, because it looks like bitmap is unchanged.
>
As of right now this breaks test 124, do you want me to review the rest
anyway?
--js
(As a warning, most of the US goes on holiday today, but I will try to
check my mail for just this one series as I'd really like it get it
through. I will be back to normal duties 9th January.)
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v9 04/13] dirty-bitmap: add locked state
2017-12-22 2:03 ` John Snow
@ 2017-12-22 9:37 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-12-22 9:37 UTC (permalink / raw)
To: John Snow, qemu-block, qemu-devel
Cc: kwolf, peter.maydell, famz, lirans, quintela, armbru, mreitz,
stefanha, den, amit.shah, pbonzini, dgilbert
22.12.2017 05:03, John Snow wrote:
>
> On 12/20/2017 10:49 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Add special state, when qmp operations on the bitmap are disabled.
>> It is needed during bitmap migration. "Frozen" state is not
>> appropriate here, because it looks like bitmap is unchanged.
>>
> As of right now this breaks test 124, do you want me to review the rest
> anyway?
>
> --js
>
> (As a warning, most of the US goes on holiday today, but I will try to
> check my mail for just this one series as I'd really like it get it
> through. I will be back to normal duties 9th January.)
Yes, there is a tiny mistake (which shows that I didn't run iotests, but
now it's done), the rest should be reviewed.
(But I think, it is better have a rest on holidays :)
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v9 04/13] dirty-bitmap: add locked state
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 04/13] dirty-bitmap: add locked state Vladimir Sementsov-Ogievskiy
2017-12-22 2:03 ` John Snow
@ 2017-12-22 8:46 ` Vladimir Sementsov-Ogievskiy
2017-12-28 5:32 ` Fam Zheng
2 siblings, 0 replies; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-12-22 8:46 UTC (permalink / raw)
To: qemu-block, qemu-devel
Cc: pbonzini, armbru, eblake, famz, stefanha, amit.shah, quintela,
mreitz, kwolf, peter.maydell, dgilbert, den, jsnow, lirans
20.12.2017 18:49, Vladimir Sementsov-Ogievskiy wrote:
> Add special state, when qmp operations on the bitmap are disabled.
> It is needed during bitmap migration. "Frozen" state is not
> appropriate here, because it looks like bitmap is unchanged.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
[...]
> +void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool qmp_locked)
> +{
> + qemu_mutex_lock(bitmap->mutex);
> + bitmap->qmp_locked = qmp_locked;
> + qemu_mutex_unlock(bitmap->mutex);
> +}
> +
> +bool bdrv_dirty_bitmap_qmp_locked(BdrvDirtyBitmap *bitmap)
> +{
> + return bitmap->qmp_locked;
> +}
> +
> /* Called with BQL taken. */
> bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
> {
> @@ -197,6 +211,8 @@ DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap)
> {
> if (bdrv_dirty_bitmap_frozen(bitmap)) {
> return DIRTY_BITMAP_STATUS_FROZEN;
> + } else if (!bdrv_dirty_bitmap_qmp_locked(bitmap)) {
should be without "!", it breaks 124 iotest.
> + return DIRTY_BITMAP_STATUS_LOCKED;
> } else if (!bdrv_dirty_bitmap_enabled(bitmap)) {
> return DIRTY_BITMAP_STATUS_DISABLED;
> } else {
> diff --git a/blockdev.c b/blockdev.c
> index 56a6b24a0b..82b2956c5c 100644
> --- a/blockdev.c
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v9 04/13] dirty-bitmap: add locked state
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 04/13] dirty-bitmap: add locked state Vladimir Sementsov-Ogievskiy
2017-12-22 2:03 ` John Snow
2017-12-22 8:46 ` Vladimir Sementsov-Ogievskiy
@ 2017-12-28 5:32 ` Fam Zheng
2 siblings, 0 replies; 50+ messages in thread
From: Fam Zheng @ 2017-12-28 5:32 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: qemu-block, qemu-devel, kwolf, peter.maydell, lirans, quintela,
jsnow, armbru, mreitz, stefanha, den, amit.shah, pbonzini,
dgilbert
On Wed, 12/20 18:49, Vladimir Sementsov-Ogievskiy wrote:
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index fe27ddfb83..6218740c95 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -40,6 +40,8 @@ struct BdrvDirtyBitmap {
> QemuMutex *mutex;
> HBitmap *bitmap; /* Dirty bitmap implementation */
> HBitmap *meta; /* Meta dirty bitmap */
> + bool qmp_locked; /* Bitmap is frozen, it can't be modified
> + through QMP */
Please fix the alignment when you respin. Thanks.
Fam
^ permalink raw reply [flat|nested] 50+ messages in thread
* [Qemu-devel] [PATCH v9 05/13] migration: introduce postcopy-only pending
2017-12-20 15:49 [Qemu-devel] [PATCH v9 00/13] Dirty bitmaps postcopy migration Vladimir Sementsov-Ogievskiy
` (3 preceding siblings ...)
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 04/13] dirty-bitmap: add locked state Vladimir Sementsov-Ogievskiy
@ 2017-12-20 15:49 ` Vladimir Sementsov-Ogievskiy
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 06/13] qapi: add dirty-bitmaps migration capability Vladimir Sementsov-Ogievskiy
` (7 subsequent siblings)
12 siblings, 0 replies; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-12-20 15:49 UTC (permalink / raw)
To: qemu-block, qemu-devel
Cc: pbonzini, armbru, eblake, famz, stefanha, amit.shah, quintela,
mreitz, kwolf, peter.maydell, dgilbert, den, jsnow, vsementsov,
lirans
There would be savevm states (dirty-bitmap) which can migrate only in
postcopy stage. The corresponding pending is introduced here.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
include/migration/register.h | 17 +++++++++++++++--
migration/savevm.h | 5 +++--
hw/s390x/s390-stattrib.c | 7 ++++---
migration/block.c | 7 ++++---
migration/migration.c | 15 ++++++++-------
migration/ram.c | 9 +++++----
migration/savevm.c | 13 ++++++++-----
migration/trace-events | 2 +-
8 files changed, 48 insertions(+), 27 deletions(-)
diff --git a/include/migration/register.h b/include/migration/register.h
index f4f7bdc177..9436a87678 100644
--- a/include/migration/register.h
+++ b/include/migration/register.h
@@ -37,8 +37,21 @@ typedef struct SaveVMHandlers {
int (*save_setup)(QEMUFile *f, void *opaque);
void (*save_live_pending)(QEMUFile *f, void *opaque,
uint64_t threshold_size,
- uint64_t *non_postcopiable_pending,
- uint64_t *postcopiable_pending);
+ uint64_t *res_precopy_only,
+ uint64_t *res_compatible,
+ uint64_t *res_postcopy_only);
+ /* Note for save_live_pending:
+ * - res_precopy_only is for data which must be migrated in precopy phase
+ * or in stopped state, in other words - before target vm start
+ * - res_compatible is for data which may be migrated in any phase
+ * - res_postcopy_only is for data which must be migrated in postcopy phase
+ * or in stopped state, in other words - after source vm stop
+ *
+ * Sum of res_postcopy_only, res_compatible and res_postcopy_only is the
+ * whole amount of pending data.
+ */
+
+
LoadStateHandler *load_state;
int (*load_setup)(QEMUFile *f, void *opaque);
int (*load_cleanup)(void *opaque);
diff --git a/migration/savevm.h b/migration/savevm.h
index 295c4a1f2c..cf4f0d37ca 100644
--- a/migration/savevm.h
+++ b/migration/savevm.h
@@ -38,8 +38,9 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f);
int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only,
bool inactivate_disks);
void qemu_savevm_state_pending(QEMUFile *f, uint64_t max_size,
- uint64_t *res_non_postcopiable,
- uint64_t *res_postcopiable);
+ uint64_t *res_precopy_only,
+ uint64_t *res_compatible,
+ uint64_t *res_postcopy_only);
void qemu_savevm_send_ping(QEMUFile *f, uint32_t value);
void qemu_savevm_send_open_return_path(QEMUFile *f);
int qemu_savevm_send_packaged(QEMUFile *f, const uint8_t *buf, size_t len);
diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
index 2902f54f11..dd3fbfd1eb 100644
--- a/hw/s390x/s390-stattrib.c
+++ b/hw/s390x/s390-stattrib.c
@@ -183,15 +183,16 @@ static int cmma_save_setup(QEMUFile *f, void *opaque)
}
static void cmma_save_pending(QEMUFile *f, void *opaque, uint64_t max_size,
- uint64_t *non_postcopiable_pending,
- uint64_t *postcopiable_pending)
+ uint64_t *res_precopy_only,
+ uint64_t *res_compatible,
+ uint64_t *res_postcopy_only)
{
S390StAttribState *sas = S390_STATTRIB(opaque);
S390StAttribClass *sac = S390_STATTRIB_GET_CLASS(sas);
long long res = sac->get_dirtycount(sas);
if (res >= 0) {
- *non_postcopiable_pending += res;
+ *res_precopy_only += res;
}
}
diff --git a/migration/block.c b/migration/block.c
index 7147171bb7..e5bf30f5de 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -866,8 +866,9 @@ static int block_save_complete(QEMUFile *f, void *opaque)
}
static void block_save_pending(QEMUFile *f, void *opaque, uint64_t max_size,
- uint64_t *non_postcopiable_pending,
- uint64_t *postcopiable_pending)
+ uint64_t *res_precopy_only,
+ uint64_t *res_compatible,
+ uint64_t *res_postcopy_only)
{
/* Estimate pending number of bytes to send */
uint64_t pending;
@@ -888,7 +889,7 @@ static void block_save_pending(QEMUFile *f, void *opaque, uint64_t max_size,
DPRINTF("Enter save live pending %" PRIu64 "\n", pending);
/* We don't do postcopy */
- *non_postcopiable_pending += pending;
+ *res_precopy_only += pending;
}
static int block_load(QEMUFile *f, void *opaque, int version_id)
diff --git a/migration/migration.c b/migration/migration.c
index 4de3b551fe..e6c9be3cca 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2216,20 +2216,21 @@ static void *migration_thread(void *opaque)
uint64_t pending_size;
if (!qemu_file_rate_limit(s->to_dst_file)) {
- uint64_t pend_post, pend_nonpost;
+ uint64_t pend_pre, pend_compat, pend_post;
- qemu_savevm_state_pending(s->to_dst_file, threshold_size,
- &pend_nonpost, &pend_post);
- pending_size = pend_nonpost + pend_post;
+ qemu_savevm_state_pending(s->to_dst_file, threshold_size, &pend_pre,
+ &pend_compat, &pend_post);
+ pending_size = pend_pre + pend_compat + pend_post;
trace_migrate_pending(pending_size, threshold_size,
- pend_post, pend_nonpost);
+ pend_pre, pend_compat, pend_post);
if (pending_size && pending_size >= threshold_size) {
/* Still a significant amount to transfer */
if (migrate_postcopy() &&
s->state != MIGRATION_STATUS_POSTCOPY_ACTIVE &&
- pend_nonpost <= threshold_size &&
- atomic_read(&s->start_postcopy)) {
+ pend_pre <= threshold_size &&
+ (atomic_read(&s->start_postcopy) ||
+ (pend_pre + pend_compat <= threshold_size))) {
if (!postcopy_start(s, &old_vm_running)) {
current_active_state = MIGRATION_STATUS_POSTCOPY_ACTIVE;
diff --git a/migration/ram.c b/migration/ram.c
index 021d583b9b..f316e2b4dc 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2358,8 +2358,9 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
}
static void ram_save_pending(QEMUFile *f, void *opaque, uint64_t max_size,
- uint64_t *non_postcopiable_pending,
- uint64_t *postcopiable_pending)
+ uint64_t *res_precopy_only,
+ uint64_t *res_compatible,
+ uint64_t *res_postcopy_only)
{
RAMState **temp = opaque;
RAMState *rs = *temp;
@@ -2379,9 +2380,9 @@ static void ram_save_pending(QEMUFile *f, void *opaque, uint64_t max_size,
if (migrate_postcopy_ram()) {
/* We can do postcopy, and all the data is postcopiable */
- *postcopiable_pending += remaining_size;
+ *res_compatible += remaining_size;
} else {
- *non_postcopiable_pending += remaining_size;
+ *res_precopy_only += remaining_size;
}
}
diff --git a/migration/savevm.c b/migration/savevm.c
index b7908f62be..c3c60a15e3 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1218,13 +1218,15 @@ int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only,
* for units that can't do postcopy.
*/
void qemu_savevm_state_pending(QEMUFile *f, uint64_t threshold_size,
- uint64_t *res_non_postcopiable,
- uint64_t *res_postcopiable)
+ uint64_t *res_precopy_only,
+ uint64_t *res_compatible,
+ uint64_t *res_postcopy_only)
{
SaveStateEntry *se;
- *res_non_postcopiable = 0;
- *res_postcopiable = 0;
+ *res_precopy_only = 0;
+ *res_compatible = 0;
+ *res_postcopy_only = 0;
QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
@@ -1237,7 +1239,8 @@ void qemu_savevm_state_pending(QEMUFile *f, uint64_t threshold_size,
}
}
se->ops->save_live_pending(f, se->opaque, threshold_size,
- res_non_postcopiable, res_postcopiable);
+ res_precopy_only, res_compatible,
+ res_postcopy_only);
}
}
diff --git a/migration/trace-events b/migration/trace-events
index 6f29fcc686..a04fffb877 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -86,7 +86,7 @@ migrate_fd_cleanup(void) ""
migrate_fd_error(const char *error_desc) "error=%s"
migrate_fd_cancel(void) ""
migrate_handle_rp_req_pages(const char *rbname, size_t start, size_t len) "in %s at 0x%zx len 0x%zx"
-migrate_pending(uint64_t size, uint64_t max, uint64_t post, uint64_t nonpost) "pending size %" PRIu64 " max %" PRIu64 " (post=%" PRIu64 " nonpost=%" PRIu64 ")"
+migrate_pending(uint64_t size, uint64_t max, uint64_t pre, uint64_t compat, uint64_t post) "pending size %" PRIu64 " max %" PRIu64 " (pre = %" PRIu64 " compat=%" PRIu64 " post=%" PRIu64 ")"
migrate_send_rp_message(int msg_type, uint16_t len) "%d: len %d"
migration_completion_file_err(void) ""
migration_completion_postcopy_end(void) ""
--
2.11.1
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [Qemu-devel] [PATCH v9 06/13] qapi: add dirty-bitmaps migration capability
2017-12-20 15:49 [Qemu-devel] [PATCH v9 00/13] Dirty bitmaps postcopy migration Vladimir Sementsov-Ogievskiy
` (4 preceding siblings ...)
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 05/13] migration: introduce postcopy-only pending Vladimir Sementsov-Ogievskiy
@ 2017-12-20 15:49 ` Vladimir Sementsov-Ogievskiy
2017-12-28 5:41 ` Fam Zheng
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 07/13] migration: include migrate_dirty_bitmaps in migrate_postcopy Vladimir Sementsov-Ogievskiy
` (6 subsequent siblings)
12 siblings, 1 reply; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-12-20 15:49 UTC (permalink / raw)
To: qemu-block, qemu-devel
Cc: pbonzini, armbru, eblake, famz, stefanha, amit.shah, quintela,
mreitz, kwolf, peter.maydell, dgilbert, den, jsnow, vsementsov,
lirans
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
---
qapi/migration.json | 6 +++++-
migration/migration.h | 1 +
migration/migration.c | 9 +++++++++
3 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/qapi/migration.json b/qapi/migration.json
index 03f57c9616..9f20b645a7 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -352,12 +352,16 @@
#
# @x-multifd: Use more than one fd for migration (since 2.11)
#
+# @dirty-bitmaps: If enabled, QEMU will migrate named dirty bitmaps.
+# (since 2.11)
+#
# Since: 1.2
##
{ 'enum': 'MigrationCapability',
'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram',
- 'block', 'return-path', 'pause-before-switchover', 'x-multifd' ] }
+ 'block', 'return-path', 'pause-before-switchover', 'x-multifd',
+ 'dirty-bitmaps' ] }
##
# @MigrationCapabilityStatus:
diff --git a/migration/migration.h b/migration/migration.h
index 663415fe48..50d1f01346 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -181,6 +181,7 @@ bool migrate_postcopy(void);
bool migrate_release_ram(void);
bool migrate_postcopy_ram(void);
bool migrate_zero_blocks(void);
+bool migrate_dirty_bitmaps(void);
bool migrate_auto_converge(void);
bool migrate_use_multifd(void);
diff --git a/migration/migration.c b/migration/migration.c
index e6c9be3cca..1526cd4bff 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1544,6 +1544,15 @@ int migrate_decompress_threads(void)
return s->parameters.decompress_threads;
}
+bool migrate_dirty_bitmaps(void)
+{
+ MigrationState *s;
+
+ s = migrate_get_current();
+
+ return s->enabled_capabilities[MIGRATION_CAPABILITY_DIRTY_BITMAPS];
+}
+
bool migrate_use_events(void)
{
MigrationState *s;
--
2.11.1
^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v9 06/13] qapi: add dirty-bitmaps migration capability
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 06/13] qapi: add dirty-bitmaps migration capability Vladimir Sementsov-Ogievskiy
@ 2017-12-28 5:41 ` Fam Zheng
0 siblings, 0 replies; 50+ messages in thread
From: Fam Zheng @ 2017-12-28 5:41 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: qemu-block, qemu-devel, pbonzini, armbru, eblake, stefanha,
amit.shah, quintela, mreitz, kwolf, peter.maydell, dgilbert, den,
jsnow, lirans
On Wed, 12/20 18:49, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> ---
> qapi/migration.json | 6 +++++-
> migration/migration.h | 1 +
> migration/migration.c | 9 +++++++++
> 3 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 03f57c9616..9f20b645a7 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -352,12 +352,16 @@
> #
> # @x-multifd: Use more than one fd for migration (since 2.11)
> #
> +# @dirty-bitmaps: If enabled, QEMU will migrate named dirty bitmaps.
> +# (since 2.11)
s/2.11/2.12/
But it should be fine to keep all r-b lines when you update that, and add one
more:
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 50+ messages in thread
* [Qemu-devel] [PATCH v9 07/13] migration: include migrate_dirty_bitmaps in migrate_postcopy
2017-12-20 15:49 [Qemu-devel] [PATCH v9 00/13] Dirty bitmaps postcopy migration Vladimir Sementsov-Ogievskiy
` (5 preceding siblings ...)
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 06/13] qapi: add dirty-bitmaps migration capability Vladimir Sementsov-Ogievskiy
@ 2017-12-20 15:49 ` Vladimir Sementsov-Ogievskiy
2017-12-28 5:42 ` Fam Zheng
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 08/13] migration/qemu-file: add qemu_put_counted_string() Vladimir Sementsov-Ogievskiy
` (5 subsequent siblings)
12 siblings, 1 reply; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-12-20 15:49 UTC (permalink / raw)
To: qemu-block, qemu-devel
Cc: pbonzini, armbru, eblake, famz, stefanha, amit.shah, quintela,
mreitz, kwolf, peter.maydell, dgilbert, den, jsnow, vsementsov,
lirans
Enable postcopy if dirty bitmap migration is enabled.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
migration/migration.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/migration/migration.c b/migration/migration.c
index 1526cd4bff..e973837bfd 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1487,7 +1487,7 @@ bool migrate_postcopy_ram(void)
bool migrate_postcopy(void)
{
- return migrate_postcopy_ram();
+ return migrate_postcopy_ram() || migrate_dirty_bitmaps();
}
bool migrate_auto_converge(void)
--
2.11.1
^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v9 07/13] migration: include migrate_dirty_bitmaps in migrate_postcopy
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 07/13] migration: include migrate_dirty_bitmaps in migrate_postcopy Vladimir Sementsov-Ogievskiy
@ 2017-12-28 5:42 ` Fam Zheng
0 siblings, 0 replies; 50+ messages in thread
From: Fam Zheng @ 2017-12-28 5:42 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: qemu-block, qemu-devel, pbonzini, armbru, eblake, stefanha,
amit.shah, quintela, mreitz, kwolf, peter.maydell, dgilbert, den,
jsnow, lirans
On Wed, 12/20 18:49, Vladimir Sementsov-Ogievskiy wrote:
> Enable postcopy if dirty bitmap migration is enabled.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> Reviewed-by: John Snow <jsnow@redhat.com>
> ---
> migration/migration.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 1526cd4bff..e973837bfd 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1487,7 +1487,7 @@ bool migrate_postcopy_ram(void)
>
> bool migrate_postcopy(void)
> {
> - return migrate_postcopy_ram();
> + return migrate_postcopy_ram() || migrate_dirty_bitmaps();
> }
>
> bool migrate_auto_converge(void)
> --
> 2.11.1
>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 50+ messages in thread
* [Qemu-devel] [PATCH v9 08/13] migration/qemu-file: add qemu_put_counted_string()
2017-12-20 15:49 [Qemu-devel] [PATCH v9 00/13] Dirty bitmaps postcopy migration Vladimir Sementsov-Ogievskiy
` (6 preceding siblings ...)
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 07/13] migration: include migrate_dirty_bitmaps in migrate_postcopy Vladimir Sementsov-Ogievskiy
@ 2017-12-20 15:49 ` Vladimir Sementsov-Ogievskiy
2017-12-28 5:48 ` Fam Zheng
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 09/13] migration: add is_active_iterate handler Vladimir Sementsov-Ogievskiy
` (4 subsequent siblings)
12 siblings, 1 reply; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-12-20 15:49 UTC (permalink / raw)
To: qemu-block, qemu-devel
Cc: pbonzini, armbru, eblake, famz, stefanha, amit.shah, quintela,
mreitz, kwolf, peter.maydell, dgilbert, den, jsnow, vsementsov,
lirans
Add function opposite to qemu_get_counted_string.
qemu_put_counted_string puts one-byte length of the string (string
should not be longer than 255 characters), and then it puts the string,
without last zero byte.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
---
migration/qemu-file.h | 2 ++
migration/qemu-file.c | 13 +++++++++++++
2 files changed, 15 insertions(+)
diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index aae4e5ed36..f4f356ab12 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -174,4 +174,6 @@ size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
ram_addr_t offset, size_t size,
uint64_t *bytes_sent);
+void qemu_put_counted_string(QEMUFile *f, const char *name);
+
#endif
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 2ab2bf362d..e85f501f86 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -734,6 +734,19 @@ size_t qemu_get_counted_string(QEMUFile *f, char buf[256])
}
/*
+ * Put a string with one preceding byte containing its length. The length of
+ * the string should be less than 256.
+ */
+void qemu_put_counted_string(QEMUFile *f, const char *str)
+{
+ size_t len = strlen(str);
+
+ assert(len < 256);
+ qemu_put_byte(f, len);
+ qemu_put_buffer(f, (const uint8_t *)str, len);
+}
+
+/*
* Set the blocking state of the QEMUFile.
* Note: On some transports the OS only keeps a single blocking state for
* both directions, and thus changing the blocking on the main
--
2.11.1
^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v9 08/13] migration/qemu-file: add qemu_put_counted_string()
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 08/13] migration/qemu-file: add qemu_put_counted_string() Vladimir Sementsov-Ogievskiy
@ 2017-12-28 5:48 ` Fam Zheng
0 siblings, 0 replies; 50+ messages in thread
From: Fam Zheng @ 2017-12-28 5:48 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: qemu-block, qemu-devel, kwolf, peter.maydell, lirans, quintela,
jsnow, armbru, mreitz, stefanha, den, amit.shah, pbonzini,
dgilbert
On Wed, 12/20 18:49, Vladimir Sementsov-Ogievskiy wrote:
> Add function opposite to qemu_get_counted_string.
> qemu_put_counted_string puts one-byte length of the string (string
> should not be longer than 255 characters), and then it puts the string,
> without last zero byte.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 50+ messages in thread
* [Qemu-devel] [PATCH v9 09/13] migration: add is_active_iterate handler
2017-12-20 15:49 [Qemu-devel] [PATCH v9 00/13] Dirty bitmaps postcopy migration Vladimir Sementsov-Ogievskiy
` (7 preceding siblings ...)
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 08/13] migration/qemu-file: add qemu_put_counted_string() Vladimir Sementsov-Ogievskiy
@ 2017-12-20 15:49 ` Vladimir Sementsov-Ogievskiy
2017-12-28 5:50 ` Fam Zheng
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 10/13] migration: add postcopy migration of dirty bitmaps Vladimir Sementsov-Ogievskiy
` (3 subsequent siblings)
12 siblings, 1 reply; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-12-20 15:49 UTC (permalink / raw)
To: qemu-block, qemu-devel
Cc: pbonzini, armbru, eblake, famz, stefanha, amit.shah, quintela,
mreitz, kwolf, peter.maydell, dgilbert, den, jsnow, vsementsov,
lirans
Only-postcopy savevm states (dirty-bitmap) don't need live iteration, so
to disable them and stop transporting empty sections there is a new
savevm handler.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
include/migration/register.h | 9 +++++++++
migration/savevm.c | 5 +++++
2 files changed, 14 insertions(+)
diff --git a/include/migration/register.h b/include/migration/register.h
index 9436a87678..f6f12f9b1a 100644
--- a/include/migration/register.h
+++ b/include/migration/register.h
@@ -26,6 +26,15 @@ typedef struct SaveVMHandlers {
bool (*is_active)(void *opaque);
bool (*has_postcopy)(void *opaque);
+ /* is_active_iterate
+ * If it is not NULL then qemu_savevm_state_iterate will skip iteration if
+ * it returns false. For example, it is needed for only-postcopy-states,
+ * which needs to be handled by qemu_savevm_state_setup and
+ * qemu_savevm_state_pending, but do not need iterations until not in
+ * postcopy stage.
+ */
+ bool (*is_active_iterate)(void *opaque);
+
/* This runs outside the iothread lock in the migration case, and
* within the lock in the savevm case. The callback had better only
* use data that is local to the migration thread or protected
diff --git a/migration/savevm.c b/migration/savevm.c
index c3c60a15e3..e5d557458e 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1026,6 +1026,11 @@ int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy)
continue;
}
}
+ if (se->ops && se->ops->is_active_iterate) {
+ if (!se->ops->is_active_iterate(se->opaque)) {
+ continue;
+ }
+ }
/*
* In the postcopy phase, any device that doesn't know how to
* do postcopy should have saved it's state in the _complete
--
2.11.1
^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v9 09/13] migration: add is_active_iterate handler
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 09/13] migration: add is_active_iterate handler Vladimir Sementsov-Ogievskiy
@ 2017-12-28 5:50 ` Fam Zheng
0 siblings, 0 replies; 50+ messages in thread
From: Fam Zheng @ 2017-12-28 5:50 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: qemu-block, qemu-devel, kwolf, peter.maydell, lirans, quintela,
jsnow, armbru, mreitz, stefanha, den, amit.shah, pbonzini,
dgilbert
On Wed, 12/20 18:49, Vladimir Sementsov-Ogievskiy wrote:
> Only-postcopy savevm states (dirty-bitmap) don't need live iteration, so
> to disable them and stop transporting empty sections there is a new
> savevm handler.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 50+ messages in thread
* [Qemu-devel] [PATCH v9 10/13] migration: add postcopy migration of dirty bitmaps
2017-12-20 15:49 [Qemu-devel] [PATCH v9 00/13] Dirty bitmaps postcopy migration Vladimir Sementsov-Ogievskiy
` (8 preceding siblings ...)
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 09/13] migration: add is_active_iterate handler Vladimir Sementsov-Ogievskiy
@ 2017-12-20 15:49 ` Vladimir Sementsov-Ogievskiy
2017-12-28 6:14 ` Fam Zheng
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 11/13] iotests: add default node-name Vladimir Sementsov-Ogievskiy
` (2 subsequent siblings)
12 siblings, 1 reply; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-12-20 15:49 UTC (permalink / raw)
To: qemu-block, qemu-devel
Cc: pbonzini, armbru, eblake, famz, stefanha, amit.shah, quintela,
mreitz, kwolf, peter.maydell, dgilbert, den, jsnow, vsementsov,
lirans
Postcopy migration of dirty bitmaps. Only named dirty bitmaps,
associated with root nodes and non-root named nodes are migrated.
If destination qemu is already containing a dirty bitmap with the same name
as a migrated bitmap (for the same node), then, if their granularities are
the same the migration will be done, otherwise the error will be generated.
If destination qemu doesn't contain such bitmap it will be created.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
include/migration/misc.h | 3 +
migration/migration.h | 3 +
migration/block-dirty-bitmap.c | 732 +++++++++++++++++++++++++++++++++++++++++
migration/migration.c | 3 +
migration/savevm.c | 2 +
vl.c | 1 +
migration/Makefile.objs | 1 +
migration/trace-events | 14 +
8 files changed, 759 insertions(+)
create mode 100644 migration/block-dirty-bitmap.c
diff --git a/include/migration/misc.h b/include/migration/misc.h
index c079b7771b..9cc539e232 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -55,4 +55,7 @@ bool migration_has_failed(MigrationState *);
bool migration_in_postcopy_after_devices(MigrationState *);
void migration_global_dump(Monitor *mon);
+/* migration/block-dirty-bitmap.c */
+void dirty_bitmap_mig_init(void);
+
#endif
diff --git a/migration/migration.h b/migration/migration.h
index 50d1f01346..4e3ad04664 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -211,4 +211,7 @@ void migrate_send_rp_pong(MigrationIncomingState *mis,
void migrate_send_rp_req_pages(MigrationIncomingState *mis, const char* rbname,
ram_addr_t start, size_t len);
+void dirty_bitmap_mig_before_vm_start(void);
+void init_dirty_bitmap_incoming_migration(void);
+
#endif
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
new file mode 100644
index 0000000000..782e0fc1d4
--- /dev/null
+++ b/migration/block-dirty-bitmap.c
@@ -0,0 +1,732 @@
+/*
+ * Block dirty bitmap postcopy migration
+ *
+ * Copyright IBM, Corp. 2009
+ * Copyright (c) 2016-2017 Virtuozzo International GmbH. All rights reserved.
+ *
+ * Authors:
+ * Liran Schour <lirans@il.ibm.com>
+ * Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the COPYING file in the top-level directory.
+ * This file is derived from migration/block.c, so it's author and IBM copyright
+ * are here, although content is quite different.
+ *
+ * Contributions after 2012-01-13 are licensed under the terms of the
+ * GNU GPL, version 2 or (at your option) any later version.
+ *
+ * ***
+ *
+ * Here postcopy migration of dirty bitmaps is realized. Only QMP-addressable
+ * bitmaps, associated with root nodes and non-root named nodes are migrated.
+ *
+ * Bitmap migration implies creating bitmap with the same name and granularity
+ * in destination QEMU. If the bitmap with the same name (for the same node)
+ * already exists on destination an error will be generated.
+ *
+ * format of migration:
+ *
+ * # Header (shared for different chunk types)
+ * 1, 2 or 4 bytes: flags (see qemu_{put,put}_flags)
+ * [ 1 byte: node name size ] \ flags & DEVICE_NAME
+ * [ n bytes: node name ] /
+ * [ 1 byte: bitmap name size ] \ flags & BITMAP_NAME
+ * [ n bytes: bitmap name ] /
+ *
+ * # Start of bitmap migration (flags & START)
+ * header
+ * be64: granularity
+ * 1 byte: bitmap flags (corresponds to BdrvDirtyBitmap)
+ * bit 0 - bitmap is enabled
+ * bit 1 - bitmap is persistent
+ * bit 2 - bitmap is autoloading
+ * bits 3-7 - reserved, must be zero
+ *
+ * # Complete of bitmap migration (flags & COMPLETE)
+ * header
+ *
+ * # Data chunk of bitmap migration
+ * header
+ * be64: start sector
+ * be32: number of sectors
+ * [ be64: buffer size ] \ ! (flags & ZEROES)
+ * [ n bytes: buffer ] /
+ *
+ * The last chunk in stream should contain flags & EOS. The chunk may skip
+ * device and/or bitmap names, assuming them to be the same with the previous
+ * chunk.
+ */
+
+#include "qemu/osdep.h"
+#include "block/block.h"
+#include "block/block_int.h"
+#include "sysemu/block-backend.h"
+#include "qemu/main-loop.h"
+#include "qemu/error-report.h"
+#include "migration/misc.h"
+#include "migration/migration.h"
+#include "migration/qemu-file.h"
+#include "migration/vmstate.h"
+#include "migration/register.h"
+#include "qemu/hbitmap.h"
+#include "sysemu/sysemu.h"
+#include "qemu/cutils.h"
+#include "qapi/error.h"
+#include "trace.h"
+
+#define CHUNK_SIZE (1 << 10)
+
+/* Flags occupy one, two or four bytes (Big Endian). The size is determined as
+ * follows:
+ * in first (most significant) byte bit 8 is clear --> one byte
+ * in first byte bit 8 is set --> two or four bytes, depending on second
+ * byte:
+ * | in second byte bit 8 is clear --> two bytes
+ * | in second byte bit 8 is set --> four bytes
+ */
+#define DIRTY_BITMAP_MIG_FLAG_EOS 0x01
+#define DIRTY_BITMAP_MIG_FLAG_ZEROES 0x02
+#define DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME 0x04
+#define DIRTY_BITMAP_MIG_FLAG_DEVICE_NAME 0x08
+#define DIRTY_BITMAP_MIG_FLAG_START 0x10
+#define DIRTY_BITMAP_MIG_FLAG_COMPLETE 0x20
+#define DIRTY_BITMAP_MIG_FLAG_BITS 0x40
+
+#define DIRTY_BITMAP_MIG_EXTRA_FLAGS 0x80
+
+#define DIRTY_BITMAP_MIG_START_FLAG_ENABLED 0x01
+#define DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT 0x02
+#define DIRTY_BITMAP_MIG_START_FLAG_AUTOLOAD 0x04
+#define DIRTY_BITMAP_MIG_START_FLAG_RESERVED_MASK 0xf8
+
+typedef struct DirtyBitmapMigBitmapState {
+ /* Written during setup phase. */
+ BlockDriverState *bs;
+ const char *node_name;
+ BdrvDirtyBitmap *bitmap;
+ uint64_t total_sectors;
+ uint64_t sectors_per_chunk;
+ QSIMPLEQ_ENTRY(DirtyBitmapMigBitmapState) entry;
+ uint8_t flags;
+
+ /* For bulk phase. */
+ bool bulk_completed;
+ uint64_t cur_sector;
+} DirtyBitmapMigBitmapState;
+
+typedef struct DirtyBitmapMigState {
+ QSIMPLEQ_HEAD(dbms_list, DirtyBitmapMigBitmapState) dbms_list;
+
+ bool bulk_completed;
+
+ /* for send_bitmap_bits() */
+ BlockDriverState *prev_bs;
+ BdrvDirtyBitmap *prev_bitmap;
+} DirtyBitmapMigState;
+
+typedef struct DirtyBitmapLoadState {
+ uint32_t flags;
+ char node_name[256];
+ char bitmap_name[256];
+ BlockDriverState *bs;
+ BdrvDirtyBitmap *bitmap;
+} DirtyBitmapLoadState;
+
+static DirtyBitmapMigState dirty_bitmap_mig_state;
+
+typedef struct DirtyBitmapLoadBitmapState {
+ BlockDriverState *bs;
+ BdrvDirtyBitmap *bitmap;
+ bool migrated;
+} DirtyBitmapLoadBitmapState;
+static GSList *enabled_bitmaps;
+QemuMutex finish_lock;
+
+void init_dirty_bitmap_incoming_migration(void)
+{
+ qemu_mutex_init(&finish_lock);
+}
+
+static uint32_t qemu_get_bitmap_flags(QEMUFile *f)
+{
+ uint8_t flags = qemu_get_byte(f);
+ if (flags & DIRTY_BITMAP_MIG_EXTRA_FLAGS) {
+ flags = flags << 8 | qemu_get_byte(f);
+ if (flags & DIRTY_BITMAP_MIG_EXTRA_FLAGS) {
+ flags = flags << 16 | qemu_get_be16(f);
+ }
+ }
+
+ return flags;
+}
+
+static void qemu_put_bitmap_flags(QEMUFile *f, uint32_t flags)
+{
+ /* The code currently do not send flags more than one byte */
+ assert(!(flags & (0xffffff00 | DIRTY_BITMAP_MIG_EXTRA_FLAGS)));
+
+ qemu_put_byte(f, flags);
+}
+
+static void send_bitmap_header(QEMUFile *f, DirtyBitmapMigBitmapState *dbms,
+ uint32_t additional_flags)
+{
+ BlockDriverState *bs = dbms->bs;
+ BdrvDirtyBitmap *bitmap = dbms->bitmap;
+ uint32_t flags = additional_flags;
+ trace_send_bitmap_header_enter();
+
+ if (bs != dirty_bitmap_mig_state.prev_bs) {
+ dirty_bitmap_mig_state.prev_bs = bs;
+ flags |= DIRTY_BITMAP_MIG_FLAG_DEVICE_NAME;
+ }
+
+ if (bitmap != dirty_bitmap_mig_state.prev_bitmap) {
+ dirty_bitmap_mig_state.prev_bitmap = bitmap;
+ flags |= DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME;
+ }
+
+ qemu_put_bitmap_flags(f, flags);
+
+ if (flags & DIRTY_BITMAP_MIG_FLAG_DEVICE_NAME) {
+ qemu_put_counted_string(f, dbms->node_name);
+ }
+
+ if (flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) {
+ qemu_put_counted_string(f, bdrv_dirty_bitmap_name(bitmap));
+ }
+}
+
+static void send_bitmap_start(QEMUFile *f, DirtyBitmapMigBitmapState *dbms)
+{
+ send_bitmap_header(f, dbms, DIRTY_BITMAP_MIG_FLAG_START);
+ qemu_put_be32(f, bdrv_dirty_bitmap_granularity(dbms->bitmap));
+ qemu_put_byte(f, dbms->flags);
+}
+
+static void send_bitmap_complete(QEMUFile *f, DirtyBitmapMigBitmapState *dbms)
+{
+ send_bitmap_header(f, dbms, DIRTY_BITMAP_MIG_FLAG_COMPLETE);
+}
+
+static void send_bitmap_bits(QEMUFile *f, DirtyBitmapMigBitmapState *dbms,
+ uint64_t start_sector, uint32_t nr_sectors)
+{
+ /* align for buffer_is_zero() */
+ uint64_t align = 4 * sizeof(long);
+ uint64_t unaligned_size =
+ bdrv_dirty_bitmap_serialization_size(
+ dbms->bitmap, start_sector << BDRV_SECTOR_BITS,
+ (uint64_t)nr_sectors << BDRV_SECTOR_BITS);
+ uint64_t buf_size = (unaligned_size + align - 1) & ~(align - 1);
+ uint8_t *buf = g_malloc0(buf_size);
+ uint32_t flags = DIRTY_BITMAP_MIG_FLAG_BITS;
+
+ bdrv_dirty_bitmap_serialize_part(
+ dbms->bitmap, buf, start_sector << BDRV_SECTOR_BITS,
+ (uint64_t)nr_sectors << BDRV_SECTOR_BITS);
+
+ if (buffer_is_zero(buf, buf_size)) {
+ g_free(buf);
+ buf = NULL;
+ flags |= DIRTY_BITMAP_MIG_FLAG_ZEROES;
+ }
+
+ trace_send_bitmap_bits(flags, start_sector, nr_sectors, buf_size);
+
+ send_bitmap_header(f, dbms, flags);
+
+ qemu_put_be64(f, start_sector);
+ qemu_put_be32(f, nr_sectors);
+
+ /* if a block is zero we need to flush here since the network
+ * bandwidth is now a lot higher than the storage device bandwidth.
+ * thus if we queue zero blocks we slow down the migration. */
+ if (flags & DIRTY_BITMAP_MIG_FLAG_ZEROES) {
+ qemu_fflush(f);
+ } else {
+ qemu_put_be64(f, buf_size);
+ qemu_put_buffer(f, buf, buf_size);
+ }
+
+ g_free(buf);
+}
+
+/* Called with iothread lock taken. */
+static void dirty_bitmap_mig_cleanup(void)
+{
+ DirtyBitmapMigBitmapState *dbms;
+
+ while ((dbms = QSIMPLEQ_FIRST(&dirty_bitmap_mig_state.dbms_list)) != NULL) {
+ QSIMPLEQ_REMOVE_HEAD(&dirty_bitmap_mig_state.dbms_list, entry);
+ bdrv_dirty_bitmap_set_qmp_locked(dbms->bitmap, false);
+ bdrv_unref(dbms->bs);
+ g_free(dbms);
+ }
+}
+
+/* Called with iothread lock taken. */
+static int init_dirty_bitmap_migration(void)
+{
+ BlockDriverState *bs;
+ BdrvDirtyBitmap *bitmap;
+ DirtyBitmapMigBitmapState *dbms;
+ BdrvNextIterator it;
+
+ dirty_bitmap_mig_state.bulk_completed = false;
+ dirty_bitmap_mig_state.prev_bs = NULL;
+ dirty_bitmap_mig_state.prev_bitmap = NULL;
+
+ for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
+ bool not_named_node = !bdrv_get_device_or_node_name(bs);
+
+ for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap;
+ bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
+ {
+ if (!bdrv_dirty_bitmap_name(bitmap)) {
+ continue;
+ }
+
+ if (not_named_node) {
+ error_report("Found bitmap '%s' in unnamed node %p. It can't "
+ "be migrated", bdrv_dirty_bitmap_name(bitmap), bs);
+ goto fail;
+ }
+
+ if (bdrv_dirty_bitmap_frozen(bitmap)) {
+ error_report("Can't migrate frozen dirty bitmap: '%s",
+ bdrv_dirty_bitmap_name(bitmap));
+ goto fail;
+ }
+
+ if (bdrv_dirty_bitmap_qmp_locked(bitmap)) {
+ error_report("Can't migrate locked dirty bitmap: '%s",
+ bdrv_dirty_bitmap_name(bitmap));
+ goto fail;
+ }
+
+ bdrv_ref(bs);
+ bdrv_dirty_bitmap_set_qmp_locked(bitmap, true);
+
+ dbms = g_new0(DirtyBitmapMigBitmapState, 1);
+ dbms->bs = bs;
+ dbms->node_name = bdrv_get_node_name(bs);
+ if (!dbms->node_name || dbms->node_name[0] == '\0') {
+ dbms->node_name = bdrv_get_device_name(bs);
+ }
+ dbms->bitmap = bitmap;
+ dbms->total_sectors = bdrv_nb_sectors(bs);
+ dbms->sectors_per_chunk = CHUNK_SIZE * 8 *
+ bdrv_dirty_bitmap_granularity(bitmap) >> BDRV_SECTOR_BITS;
+ if (bdrv_dirty_bitmap_enabled(bitmap)) {
+ dbms->flags |= DIRTY_BITMAP_MIG_START_FLAG_ENABLED;
+ }
+ if (bdrv_dirty_bitmap_get_persistance(bitmap)) {
+ dbms->flags |= DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT;
+ }
+ if (bdrv_dirty_bitmap_get_autoload(bitmap)) {
+ dbms->flags |= DIRTY_BITMAP_MIG_START_FLAG_AUTOLOAD;
+ }
+
+ QSIMPLEQ_INSERT_TAIL(&dirty_bitmap_mig_state.dbms_list,
+ dbms, entry);
+ }
+ }
+
+ /* unset persistance here, to not roll back it */
+ QSIMPLEQ_FOREACH(dbms, &dirty_bitmap_mig_state.dbms_list, entry) {
+ bdrv_dirty_bitmap_set_persistance(dbms->bitmap, false);
+ }
+
+ return 0;
+
+fail:
+ dirty_bitmap_mig_cleanup();
+
+ return -1;
+}
+
+/* Called with no lock taken. */
+static void bulk_phase_send_chunk(QEMUFile *f, DirtyBitmapMigBitmapState *dbms)
+{
+ uint32_t nr_sectors = MIN(dbms->total_sectors - dbms->cur_sector,
+ dbms->sectors_per_chunk);
+
+ send_bitmap_bits(f, dbms, dbms->cur_sector, nr_sectors);
+
+ dbms->cur_sector += nr_sectors;
+ if (dbms->cur_sector >= dbms->total_sectors) {
+ dbms->bulk_completed = true;
+ }
+}
+
+/* Called with no lock taken. */
+static void bulk_phase(QEMUFile *f, bool limit)
+{
+ DirtyBitmapMigBitmapState *dbms;
+
+ QSIMPLEQ_FOREACH(dbms, &dirty_bitmap_mig_state.dbms_list, entry) {
+ while (!dbms->bulk_completed) {
+ bulk_phase_send_chunk(f, dbms);
+ if (limit && qemu_file_rate_limit(f)) {
+ return;
+ }
+ }
+ }
+
+ dirty_bitmap_mig_state.bulk_completed = true;
+}
+
+/* for SaveVMHandlers */
+static void dirty_bitmap_save_cleanup(void *opaque)
+{
+ dirty_bitmap_mig_cleanup();
+}
+
+static int dirty_bitmap_save_iterate(QEMUFile *f, void *opaque)
+{
+ trace_dirty_bitmap_save_iterate(migration_in_postcopy());
+
+ if (migration_in_postcopy() && !dirty_bitmap_mig_state.bulk_completed) {
+ bulk_phase(f, true);
+ }
+
+ qemu_put_bitmap_flags(f, DIRTY_BITMAP_MIG_FLAG_EOS);
+
+ return dirty_bitmap_mig_state.bulk_completed;
+}
+
+/* Called with iothread lock taken. */
+
+static int dirty_bitmap_save_complete(QEMUFile *f, void *opaque)
+{
+ DirtyBitmapMigBitmapState *dbms;
+ trace_dirty_bitmap_save_complete_enter();
+
+ if (!dirty_bitmap_mig_state.bulk_completed) {
+ bulk_phase(f, false);
+ }
+
+ QSIMPLEQ_FOREACH(dbms, &dirty_bitmap_mig_state.dbms_list, entry) {
+ send_bitmap_complete(f, dbms);
+ }
+
+ qemu_put_bitmap_flags(f, DIRTY_BITMAP_MIG_FLAG_EOS);
+
+ trace_dirty_bitmap_save_complete_finish();
+
+ dirty_bitmap_mig_cleanup();
+ return 0;
+}
+
+static void dirty_bitmap_save_pending(QEMUFile *f, void *opaque,
+ uint64_t max_size,
+ uint64_t *res_precopy_only,
+ uint64_t *res_compatible,
+ uint64_t *res_postcopy_only)
+{
+ DirtyBitmapMigBitmapState *dbms;
+ uint64_t pending = 0;
+
+ qemu_mutex_lock_iothread();
+
+ QSIMPLEQ_FOREACH(dbms, &dirty_bitmap_mig_state.dbms_list, entry) {
+ uint64_t gran = bdrv_dirty_bitmap_granularity(dbms->bitmap);
+ uint64_t sectors = dbms->bulk_completed ? 0 :
+ dbms->total_sectors - dbms->cur_sector;
+
+ pending += (sectors * BDRV_SECTOR_SIZE + gran - 1) / gran;
+ }
+
+ qemu_mutex_unlock_iothread();
+
+ trace_dirty_bitmap_save_pending(pending, max_size);
+
+ *res_postcopy_only += pending;
+}
+
+/* First occurrence of this bitmap. It should be created if doesn't exist */
+static int dirty_bitmap_load_start(QEMUFile *f, DirtyBitmapLoadState *s)
+{
+ Error *local_err = NULL;
+ uint32_t granularity = qemu_get_be32(f);
+ uint8_t flags = qemu_get_byte(f);
+
+ if (s->bitmap) {
+ error_report("Bitmap with the same name ('%s') already exists on "
+ "destination", bdrv_dirty_bitmap_name(s->bitmap));
+ return -EINVAL;
+ } else {
+ s->bitmap = bdrv_create_dirty_bitmap(s->bs, granularity,
+ s->bitmap_name, &local_err);
+ if (!s->bitmap) {
+ error_report_err(local_err);
+ return -EINVAL;
+ }
+ }
+
+ if (flags & DIRTY_BITMAP_MIG_START_FLAG_RESERVED_MASK) {
+ error_report("Unknown flags in migrated dirty bitmap header: %x",
+ flags);
+ return -EINVAL;
+ }
+
+ if (flags & DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT) {
+ bdrv_dirty_bitmap_set_persistance(s->bitmap, true);
+ }
+ if (flags & DIRTY_BITMAP_MIG_START_FLAG_AUTOLOAD) {
+ bdrv_dirty_bitmap_set_autoload(s->bitmap, true);
+ }
+
+ bdrv_disable_dirty_bitmap(s->bitmap);
+ if (flags & DIRTY_BITMAP_MIG_START_FLAG_ENABLED) {
+ DirtyBitmapLoadBitmapState *b;
+
+ bdrv_dirty_bitmap_create_successor(s->bs, s->bitmap, &local_err);
+ if (local_err) {
+ error_report_err(local_err);
+ return -EINVAL;
+ }
+
+ b = g_new(DirtyBitmapLoadBitmapState, 1);
+ b->bs = s->bs;
+ b->bitmap = s->bitmap;
+ b->migrated = false;
+ enabled_bitmaps = g_slist_prepend(enabled_bitmaps, b);
+ }
+
+ return 0;
+}
+
+void dirty_bitmap_mig_before_vm_start(void)
+{
+ GSList *item;
+
+ qemu_mutex_lock(&finish_lock);
+
+ for (item = enabled_bitmaps; item; item = g_slist_next(item)) {
+ DirtyBitmapLoadBitmapState *b = item->data;
+
+ if (b->migrated) {
+ bdrv_enable_dirty_bitmap(b->bitmap);
+ } else {
+ bdrv_dirty_bitmap_enable_successor(b->bitmap);
+ }
+
+ g_free(b);
+ }
+
+ g_slist_free(enabled_bitmaps);
+ enabled_bitmaps = NULL;
+
+ qemu_mutex_unlock(&finish_lock);
+}
+
+static void dirty_bitmap_load_complete(QEMUFile *f, DirtyBitmapLoadState *s)
+{
+ GSList *item;
+ trace_dirty_bitmap_load_complete();
+ bdrv_dirty_bitmap_deserialize_finish(s->bitmap);
+
+ qemu_mutex_lock(&finish_lock);
+
+ for (item = enabled_bitmaps; item; item = g_slist_next(item)) {
+ DirtyBitmapLoadBitmapState *b = item->data;
+
+ if (b->bitmap == s->bitmap) {
+ b->migrated = true;
+ break;
+ }
+ }
+
+ if (bdrv_dirty_bitmap_frozen(s->bitmap)) {
+ bdrv_dirty_bitmap_lock(s->bitmap);
+ if (enabled_bitmaps == NULL) {
+ /* in postcopy */
+ bdrv_reclaim_dirty_bitmap_locked(s->bs, s->bitmap, &error_abort);
+ bdrv_enable_dirty_bitmap(s->bitmap);
+ } else {
+ /* target not started, successor must be empty */
+ int64_t count = bdrv_get_dirty_count(s->bitmap);
+ BdrvDirtyBitmap *ret = bdrv_reclaim_dirty_bitmap_locked(s->bs,
+ s->bitmap,
+ NULL);
+ /* bdrv_reclaim_dirty_bitmap can fail only on no successor (it
+ * must be) or on merge fail, but merge can't fail when second
+ * bitmap is empty
+ */
+ assert(ret == s->bitmap &&
+ count == bdrv_get_dirty_count(s->bitmap));
+ }
+ bdrv_dirty_bitmap_unlock(s->bitmap);
+ }
+
+ qemu_mutex_unlock(&finish_lock);
+}
+
+static int dirty_bitmap_load_bits(QEMUFile *f, DirtyBitmapLoadState *s)
+{
+ uint64_t first_byte = qemu_get_be64(f) << BDRV_SECTOR_BITS;
+ uint64_t nr_bytes = (uint64_t)qemu_get_be32(f) << BDRV_SECTOR_BITS;
+ trace_dirty_bitmap_load_bits_enter(first_byte >> BDRV_SECTOR_BITS,
+ nr_bytes >> BDRV_SECTOR_BITS);
+
+ if (s->flags & DIRTY_BITMAP_MIG_FLAG_ZEROES) {
+ trace_dirty_bitmap_load_bits_zeroes();
+ bdrv_dirty_bitmap_deserialize_zeroes(s->bitmap, first_byte, nr_bytes,
+ false);
+ } else {
+ uint8_t *buf;
+ uint64_t buf_size = qemu_get_be64(f);
+ uint64_t needed_size =
+ bdrv_dirty_bitmap_serialization_size(s->bitmap,
+ first_byte, nr_bytes);
+
+ if (needed_size > buf_size) {
+ error_report("Error: Migrated bitmap granularity doesn't "
+ "match the destination bitmap '%s' granularity",
+ bdrv_dirty_bitmap_name(s->bitmap));
+ return -EINVAL;
+ }
+
+ buf = g_malloc(buf_size);
+ qemu_get_buffer(f, buf, buf_size);
+ bdrv_dirty_bitmap_deserialize_part(s->bitmap, buf, first_byte, nr_bytes,
+ false);
+ g_free(buf);
+ }
+
+ return 0;
+}
+
+static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState *s)
+{
+ Error *local_err = NULL;
+ s->flags = qemu_get_bitmap_flags(f);
+ trace_dirty_bitmap_load_header(s->flags);
+
+ if (s->flags & DIRTY_BITMAP_MIG_FLAG_DEVICE_NAME) {
+ if (!qemu_get_counted_string(f, s->node_name)) {
+ error_report("Unable to read node name string");
+ return -EINVAL;
+ }
+ s->bs = bdrv_lookup_bs(s->node_name, s->node_name, &local_err);
+ if (!s->bs) {
+ error_report_err(local_err);
+ return -EINVAL;
+ }
+ } else if (!s->bs) {
+ error_report("Error: block device name is not set");
+ return -EINVAL;
+ }
+
+ if (s->flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) {
+ if (!qemu_get_counted_string(f, s->bitmap_name)) {
+ error_report("Unable to read node name string");
+ return -EINVAL;
+ }
+ s->bitmap = bdrv_find_dirty_bitmap(s->bs, s->bitmap_name);
+
+ /* bitmap may be NULL here, it wouldn't be an error if it is the
+ * first occurrence of the bitmap */
+ if (!s->bitmap && !(s->flags & DIRTY_BITMAP_MIG_FLAG_START)) {
+ error_report("Error: unknown dirty bitmap "
+ "'%s' for block device '%s'",
+ s->bitmap_name, s->node_name);
+ return -EINVAL;
+ }
+ } else if (!s->bitmap) {
+ error_report("Error: block device name is not set");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
+{
+ static DirtyBitmapLoadState s;
+ int ret = 0;
+
+ trace_dirty_bitmap_load_enter();
+
+ if (version_id != 1) {
+ return -EINVAL;
+ }
+
+ do {
+ dirty_bitmap_load_header(f, &s);
+
+ if (s.flags & DIRTY_BITMAP_MIG_FLAG_START) {
+ ret = dirty_bitmap_load_start(f, &s);
+ } else if (s.flags & DIRTY_BITMAP_MIG_FLAG_COMPLETE) {
+ dirty_bitmap_load_complete(f, &s);
+ } else if (s.flags & DIRTY_BITMAP_MIG_FLAG_BITS) {
+ ret = dirty_bitmap_load_bits(f, &s);
+ }
+
+ if (!ret) {
+ ret = qemu_file_get_error(f);
+ }
+
+ if (ret) {
+ return ret;
+ }
+ } while (!(s.flags & DIRTY_BITMAP_MIG_FLAG_EOS));
+
+ trace_dirty_bitmap_load_success();
+ return 0;
+}
+
+static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque)
+{
+ DirtyBitmapMigBitmapState *dbms = NULL;
+ if (init_dirty_bitmap_migration() < 0) {
+ return -1;
+ }
+
+ QSIMPLEQ_FOREACH(dbms, &dirty_bitmap_mig_state.dbms_list, entry) {
+ send_bitmap_start(f, dbms);
+ }
+ qemu_put_bitmap_flags(f, DIRTY_BITMAP_MIG_FLAG_EOS);
+
+ return 0;
+}
+
+static bool dirty_bitmap_is_active(void *opaque)
+{
+ return migrate_dirty_bitmaps();
+}
+
+static bool dirty_bitmap_is_active_iterate(void *opaque)
+{
+ return dirty_bitmap_is_active(opaque) && !runstate_is_running();
+}
+
+static bool dirty_bitmap_has_postcopy(void *opaque)
+{
+ return true;
+}
+
+static SaveVMHandlers savevm_dirty_bitmap_handlers = {
+ .save_setup = dirty_bitmap_save_setup,
+ .save_live_complete_postcopy = dirty_bitmap_save_complete,
+ .save_live_complete_precopy = dirty_bitmap_save_complete,
+ .has_postcopy = dirty_bitmap_has_postcopy,
+ .save_live_pending = dirty_bitmap_save_pending,
+ .save_live_iterate = dirty_bitmap_save_iterate,
+ .is_active_iterate = dirty_bitmap_is_active_iterate,
+ .load_state = dirty_bitmap_load,
+ .save_cleanup = dirty_bitmap_save_cleanup,
+ .is_active = dirty_bitmap_is_active,
+};
+
+void dirty_bitmap_mig_init(void)
+{
+ QSIMPLEQ_INIT(&dirty_bitmap_mig_state.dbms_list);
+
+ register_savevm_live(NULL, "dirty-bitmap", 0, 1,
+ &savevm_dirty_bitmap_handlers,
+ &dirty_bitmap_mig_state);
+}
diff --git a/migration/migration.c b/migration/migration.c
index e973837bfd..66e9cf03cd 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -150,6 +150,9 @@ MigrationIncomingState *migration_incoming_get_current(void)
memset(&mis_current, 0, sizeof(MigrationIncomingState));
qemu_mutex_init(&mis_current.rp_mutex);
qemu_event_init(&mis_current.main_thread_load_event, false);
+
+ init_dirty_bitmap_incoming_migration();
+
once = true;
}
return &mis_current;
diff --git a/migration/savevm.c b/migration/savevm.c
index e5d557458e..93b339646b 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1673,6 +1673,8 @@ static void loadvm_postcopy_handle_run_bh(void *opaque)
trace_loadvm_postcopy_handle_run_vmstart();
+ dirty_bitmap_mig_before_vm_start();
+
if (autostart) {
/* Hold onto your hats, starting the CPU */
vm_start();
diff --git a/vl.c b/vl.c
index fc8bd9372f..753a298881 100644
--- a/vl.c
+++ b/vl.c
@@ -4613,6 +4613,7 @@ int main(int argc, char **argv, char **envp)
blk_mig_init();
ram_mig_init();
+ dirty_bitmap_mig_init();
/* If the currently selected machine wishes to override the units-per-bus
* property of its default HBA interface type, do so now. */
diff --git a/migration/Makefile.objs b/migration/Makefile.objs
index 99e038024d..c83ec47ba8 100644
--- a/migration/Makefile.objs
+++ b/migration/Makefile.objs
@@ -6,6 +6,7 @@ common-obj-y += qemu-file.o global_state.o
common-obj-y += qemu-file-channel.o
common-obj-y += xbzrle.o postcopy-ram.o
common-obj-y += qjson.o
+common-obj-y += block-dirty-bitmap.o
common-obj-$(CONFIG_RDMA) += rdma.o
diff --git a/migration/trace-events b/migration/trace-events
index a04fffb877..e9eb8078d4 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -227,3 +227,17 @@ colo_vm_state_change(const char *old, const char *new) "Change '%s' => '%s'"
colo_send_message(const char *msg) "Send '%s' message"
colo_receive_message(const char *msg) "Receive '%s' message"
colo_failover_set_state(const char *new_state) "new state %s"
+
+# migration/block-dirty-bitmap.c
+send_bitmap_header_enter(void) ""
+send_bitmap_bits(uint32_t flags, uint64_t start_sector, uint32_t nr_sectors, uint64_t data_size) "\n flags: 0x%x\n start_sector: %" PRIu64 "\n nr_sectors: %" PRIu32 "\n data_size: %" PRIu64 "\n"
+dirty_bitmap_save_iterate(int in_postcopy) "in postcopy: %d"
+dirty_bitmap_save_complete_enter(void) ""
+dirty_bitmap_save_complete_finish(void) ""
+dirty_bitmap_save_pending(uint64_t pending, uint64_t max_size) "pending %" PRIu64 " max: %" PRIu64
+dirty_bitmap_load_complete(void) ""
+dirty_bitmap_load_bits_enter(uint64_t first_sector, uint32_t nr_sectors) "chunk: %" PRIu64 " %" PRIu32
+dirty_bitmap_load_bits_zeroes(void) ""
+dirty_bitmap_load_header(uint32_t flags) "flags 0x%x"
+dirty_bitmap_load_enter(void) ""
+dirty_bitmap_load_success(void) ""
--
2.11.1
^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v9 10/13] migration: add postcopy migration of dirty bitmaps
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 10/13] migration: add postcopy migration of dirty bitmaps Vladimir Sementsov-Ogievskiy
@ 2017-12-28 6:14 ` Fam Zheng
2017-12-28 11:47 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 50+ messages in thread
From: Fam Zheng @ 2017-12-28 6:14 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: qemu-block, qemu-devel, kwolf, peter.maydell, lirans, quintela,
jsnow, armbru, mreitz, stefanha, den, amit.shah, pbonzini,
dgilbert
On Wed, 12/20 18:49, Vladimir Sementsov-Ogievskiy wrote:
> Postcopy migration of dirty bitmaps. Only named dirty bitmaps,
> associated with root nodes and non-root named nodes are migrated.
>
> If destination qemu is already containing a dirty bitmap with the same name
> as a migrated bitmap (for the same node), then, if their granularities are
> the same the migration will be done, otherwise the error will be generated.
>
> If destination qemu doesn't contain such bitmap it will be created.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Looks good in general, a few nits below.
> +static void send_bitmap_bits(QEMUFile *f, DirtyBitmapMigBitmapState *dbms,
> + uint64_t start_sector, uint32_t nr_sectors)
> +{
> + /* align for buffer_is_zero() */
> + uint64_t align = 4 * sizeof(long);
> + uint64_t unaligned_size =
> + bdrv_dirty_bitmap_serialization_size(
> + dbms->bitmap, start_sector << BDRV_SECTOR_BITS,
> + (uint64_t)nr_sectors << BDRV_SECTOR_BITS);
> + uint64_t buf_size = (unaligned_size + align - 1) & ~(align - 1);
QEMU_ALIGN_UP() ?
> + uint8_t *buf = g_malloc0(buf_size);
> + uint32_t flags = DIRTY_BITMAP_MIG_FLAG_BITS;
> +
> + bdrv_dirty_bitmap_serialize_part(
> + dbms->bitmap, buf, start_sector << BDRV_SECTOR_BITS,
> + (uint64_t)nr_sectors << BDRV_SECTOR_BITS);
> +
> + if (buffer_is_zero(buf, buf_size)) {
> + g_free(buf);
> + buf = NULL;
> + flags |= DIRTY_BITMAP_MIG_FLAG_ZEROES;
> + }
> +
> + trace_send_bitmap_bits(flags, start_sector, nr_sectors, buf_size);
> +
> + send_bitmap_header(f, dbms, flags);
> +
> + qemu_put_be64(f, start_sector);
> + qemu_put_be32(f, nr_sectors);
> +
> + /* if a block is zero we need to flush here since the network
> + * bandwidth is now a lot higher than the storage device bandwidth.
> + * thus if we queue zero blocks we slow down the migration. */
> + if (flags & DIRTY_BITMAP_MIG_FLAG_ZEROES) {
> + qemu_fflush(f);
> + } else {
> + qemu_put_be64(f, buf_size);
> + qemu_put_buffer(f, buf, buf_size);
> + }
> +
> + g_free(buf);
> +}
> +
<snip>
> +static void dirty_bitmap_save_pending(QEMUFile *f, void *opaque,
> + uint64_t max_size,
> + uint64_t *res_precopy_only,
> + uint64_t *res_compatible,
> + uint64_t *res_postcopy_only)
> +{
> + DirtyBitmapMigBitmapState *dbms;
> + uint64_t pending = 0;
> +
> + qemu_mutex_lock_iothread();
> +
> + QSIMPLEQ_FOREACH(dbms, &dirty_bitmap_mig_state.dbms_list, entry) {
> + uint64_t gran = bdrv_dirty_bitmap_granularity(dbms->bitmap);
> + uint64_t sectors = dbms->bulk_completed ? 0 :
> + dbms->total_sectors - dbms->cur_sector;
> +
> + pending += (sectors * BDRV_SECTOR_SIZE + gran - 1) / gran;
DIV_ROUND_UP()?
> + }
> +
> + qemu_mutex_unlock_iothread();
> +
> + trace_dirty_bitmap_save_pending(pending, max_size);
> +
> + *res_postcopy_only += pending;
> +}
<snip>
> +static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState *s)
> +{
> + Error *local_err = NULL;
> + s->flags = qemu_get_bitmap_flags(f);
> + trace_dirty_bitmap_load_header(s->flags);
> +
> + if (s->flags & DIRTY_BITMAP_MIG_FLAG_DEVICE_NAME) {
> + if (!qemu_get_counted_string(f, s->node_name)) {
> + error_report("Unable to read node name string");
> + return -EINVAL;
> + }
> + s->bs = bdrv_lookup_bs(s->node_name, s->node_name, &local_err);
> + if (!s->bs) {
> + error_report_err(local_err);
> + return -EINVAL;
> + }
> + } else if (!s->bs) {
> + error_report("Error: block device name is not set");
> + return -EINVAL;
> + }
> +
> + if (s->flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) {
> + if (!qemu_get_counted_string(f, s->bitmap_name)) {
> + error_report("Unable to read node name string");
s/node name/bitmap name/
> + return -EINVAL;
> + }
> + s->bitmap = bdrv_find_dirty_bitmap(s->bs, s->bitmap_name);
> +
> + /* bitmap may be NULL here, it wouldn't be an error if it is the
> + * first occurrence of the bitmap */
> + if (!s->bitmap && !(s->flags & DIRTY_BITMAP_MIG_FLAG_START)) {
> + error_report("Error: unknown dirty bitmap "
> + "'%s' for block device '%s'",
> + s->bitmap_name, s->node_name);
> + return -EINVAL;
> + }
> + } else if (!s->bitmap) {
> + error_report("Error: block device name is not set");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
Fam
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v9 10/13] migration: add postcopy migration of dirty bitmaps
2017-12-28 6:14 ` Fam Zheng
@ 2017-12-28 11:47 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-12-28 11:47 UTC (permalink / raw)
To: Fam Zheng
Cc: qemu-block, qemu-devel, kwolf, peter.maydell, lirans, quintela,
jsnow, armbru, mreitz, stefanha, den, amit.shah, pbonzini,
dgilbert
28.12.2017 09:14, Fam Zheng wrote:
> On Wed, 12/20 18:49, Vladimir Sementsov-Ogievskiy wrote:
>> Postcopy migration of dirty bitmaps. Only named dirty bitmaps,
>> associated with root nodes and non-root named nodes are migrated.
>>
>> If destination qemu is already containing a dirty bitmap with the same name
>> as a migrated bitmap (for the same node), then, if their granularities are
>> the same the migration will be done, otherwise the error will be generated.
>>
>> If destination qemu doesn't contain such bitmap it will be created.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Looks good in general, a few nits below.
Agree with all, will fix
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 50+ messages in thread
* [Qemu-devel] [PATCH v9 11/13] iotests: add default node-name
2017-12-20 15:49 [Qemu-devel] [PATCH v9 00/13] Dirty bitmaps postcopy migration Vladimir Sementsov-Ogievskiy
` (9 preceding siblings ...)
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 10/13] migration: add postcopy migration of dirty bitmaps Vladimir Sementsov-Ogievskiy
@ 2017-12-20 15:49 ` Vladimir Sementsov-Ogievskiy
2017-12-28 6:15 ` Fam Zheng
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 12/13] iotests: add dirty bitmap migration test Vladimir Sementsov-Ogievskiy
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 13/13] iotests: add dirty bitmap postcopy test Vladimir Sementsov-Ogievskiy
12 siblings, 1 reply; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-12-20 15:49 UTC (permalink / raw)
To: qemu-block, qemu-devel
Cc: pbonzini, armbru, eblake, famz, stefanha, amit.shah, quintela,
mreitz, kwolf, peter.maydell, dgilbert, den, jsnow, vsementsov,
lirans
When testing migration, auto-generated by qemu node-names differs in
source and destination qemu and migration fails. After this patch,
auto-generated by iotest nodenames will be the same.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/iotests.py | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 6f057904a9..95454c1893 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -216,6 +216,8 @@ class VM(qtest.QEMUQtestMachine):
options.append('file=%s' % path)
options.append('format=%s' % format)
options.append('cache=%s' % cachemode)
+ if 'node-name' not in opts:
+ options.append('node-name=drivenode%d' % self._num_drives)
if opts:
options.append(opts)
--
2.11.1
^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v9 11/13] iotests: add default node-name
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 11/13] iotests: add default node-name Vladimir Sementsov-Ogievskiy
@ 2017-12-28 6:15 ` Fam Zheng
0 siblings, 0 replies; 50+ messages in thread
From: Fam Zheng @ 2017-12-28 6:15 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: qemu-block, qemu-devel, pbonzini, armbru, eblake, stefanha,
amit.shah, quintela, mreitz, kwolf, peter.maydell, dgilbert, den,
jsnow, lirans
On Wed, 12/20 18:49, Vladimir Sementsov-Ogievskiy wrote:
> When testing migration, auto-generated by qemu node-names differs in
> source and destination qemu and migration fails. After this patch,
> auto-generated by iotest nodenames will be the same.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
> tests/qemu-iotests/iotests.py | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 6f057904a9..95454c1893 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -216,6 +216,8 @@ class VM(qtest.QEMUQtestMachine):
> options.append('file=%s' % path)
> options.append('format=%s' % format)
> options.append('cache=%s' % cachemode)
> + if 'node-name' not in opts:
> + options.append('node-name=drivenode%d' % self._num_drives)
>
> if opts:
> options.append(opts)
> --
> 2.11.1
>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 50+ messages in thread
* [Qemu-devel] [PATCH v9 12/13] iotests: add dirty bitmap migration test
2017-12-20 15:49 [Qemu-devel] [PATCH v9 00/13] Dirty bitmaps postcopy migration Vladimir Sementsov-Ogievskiy
` (10 preceding siblings ...)
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 11/13] iotests: add default node-name Vladimir Sementsov-Ogievskiy
@ 2017-12-20 15:49 ` Vladimir Sementsov-Ogievskiy
2017-12-28 6:28 ` Fam Zheng
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 13/13] iotests: add dirty bitmap postcopy test Vladimir Sementsov-Ogievskiy
12 siblings, 1 reply; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-12-20 15:49 UTC (permalink / raw)
To: qemu-block, qemu-devel
Cc: pbonzini, armbru, eblake, famz, stefanha, amit.shah, quintela,
mreitz, kwolf, peter.maydell, dgilbert, den, jsnow, vsementsov,
lirans
The test starts two vms (vm_a, vm_b), create dirty bitmap in
the first one, do several writes to corresponding device and
then migrate vm_a to vm_b.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
tests/qemu-iotests/169 | 139 +++++++++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/169.out | 5 ++
tests/qemu-iotests/group | 1 +
3 files changed, 145 insertions(+)
create mode 100755 tests/qemu-iotests/169
create mode 100644 tests/qemu-iotests/169.out
diff --git a/tests/qemu-iotests/169 b/tests/qemu-iotests/169
new file mode 100755
index 0000000000..4b82399591
--- /dev/null
+++ b/tests/qemu-iotests/169
@@ -0,0 +1,139 @@
+#!/usr/bin/env python
+#
+# Tests for dirty bitmaps migration.
+#
+# Copyright (c) 2016-2017 Virtuozzo International GmbH. All rights reserved.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+import os
+import iotests
+import time
+import itertools
+import operator
+import new
+from iotests import qemu_img
+
+
+disk_a = os.path.join(iotests.test_dir, 'disk_a')
+disk_b = os.path.join(iotests.test_dir, 'disk_b')
+size = '1M'
+mig_file = os.path.join(iotests.test_dir, 'mig_file')
+
+
+class TestDirtyBitmapMigration(iotests.QMPTestCase):
+ def tearDown(self):
+ self.vm_a.shutdown()
+ self.vm_b.shutdown()
+ os.remove(disk_a)
+ os.remove(disk_b)
+ os.remove(mig_file)
+
+ def setUp(self):
+ qemu_img('create', '-f', iotests.imgfmt, disk_a, size)
+ qemu_img('create', '-f', iotests.imgfmt, disk_b, size)
+
+ self.vm_a = iotests.VM(path_suffix='a').add_drive(disk_a)
+ self.vm_a.launch()
+
+ self.vm_b = iotests.VM(path_suffix='b')
+ self.vm_b.add_incoming("exec: cat '" + mig_file + "'")
+
+ def add_bitmap(self, vm, granularity, persistent):
+ params = {'node': 'drive0',
+ 'name': 'bitmap0',
+ 'granularity': granularity}
+ if persistent:
+ params['persistent'] = True
+ params['autoload'] = True
+
+ result = vm.qmp('block-dirty-bitmap-add', **params)
+ self.assert_qmp(result, 'return', {});
+
+ def get_bitmap_hash(self, vm):
+ result = vm.qmp('x-debug-block-dirty-bitmap-sha256',
+ node='drive0', name='bitmap0')
+ return result['return']['sha256']
+
+ def check_bitmap(self, vm, sha256):
+ result = vm.qmp('x-debug-block-dirty-bitmap-sha256',
+ node='drive0', name='bitmap0')
+ if sha256:
+ self.assert_qmp(result, 'return/sha256', sha256);
+ else:
+ self.assert_qmp(result, 'error/desc',
+ "Dirty bitmap 'bitmap0' not found");
+
+ def do_test_migration(self, persistent=False, shared_storage=False,
+ migrate_bitmaps=True, online=True):
+ granularity = 512
+
+ # regions = ((start, count), ...)
+ regions = ((0, 0x10000),
+ (0xf0000, 0x10000),
+ (0xa0201, 0x1000))
+
+ should_migrate = migrate_bitmaps or persistent and shared_storage
+
+ self.vm_b.add_drive(disk_a if shared_storage else disk_b)
+
+ if online:
+ os.mkfifo(mig_file)
+ self.vm_b.launch()
+
+ self.add_bitmap(self.vm_a, granularity, persistent)
+ for r in regions:
+ self.vm_a.hmp_qemu_io('drive0', 'write %d %d' % r)
+ sha256 = self.get_bitmap_hash(self.vm_a)
+
+ if migrate_bitmaps:
+ result = self.vm_a.qmp('migrate-set-capabilities',
+ capabilities=[{'capability': 'dirty-bitmaps',
+ 'state': True}])
+ self.assert_qmp(result, 'return', {})
+
+ result = self.vm_a.qmp('migrate', uri='exec:cat>' + mig_file)
+ self.vm_a.event_wait("STOP")
+
+ if not online:
+ self.vm_a.shutdown()
+ self.vm_b.launch()
+
+ self.vm_b.event_wait("RESUME", timeout=10.0)
+
+ self.check_bitmap(self.vm_b, sha256 if should_migrate else False)
+
+ if should_migrate:
+ self.vm_b.shutdown()
+ self.vm_b.launch()
+ self.check_bitmap(self.vm_b, sha256 if persistent else False)
+
+
+def inject_test_case(klass, name, method, *args, **kwargs):
+ mc = operator.methodcaller(method, *args, **kwargs)
+ setattr(klass, 'test_' + name, new.instancemethod(mc, None, klass))
+
+
+for cmb in list(itertools.product((True, False), repeat=4)):
+ name = ('_' if cmb[0] else '_not_') + 'persistent_'
+ name += ('_' if cmb[1] else '_not_') + 'shared_'
+ name += ('_' if cmb[2] else '_not_') + 'migbitmap_'
+ name += '_online' if cmb[3] else '_offline'
+
+ inject_test_case(TestDirtyBitmapMigration, name, 'do_test_migration', *cmb)
+
+
+if __name__ == '__main__':
+ iotests.main(supported_fmts=['qcow2'])
diff --git a/tests/qemu-iotests/169.out b/tests/qemu-iotests/169.out
new file mode 100644
index 0000000000..b6f257674e
--- /dev/null
+++ b/tests/qemu-iotests/169.out
@@ -0,0 +1,5 @@
+................
+----------------------------------------------------------------------
+Ran 16 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 3e688678dd..e879c7ebc7 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -169,6 +169,7 @@
162 auto quick
163 rw auto quick
165 rw auto quick
+169 rw auto quick
170 rw auto quick
171 rw auto quick
172 auto
--
2.11.1
^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v9 12/13] iotests: add dirty bitmap migration test
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 12/13] iotests: add dirty bitmap migration test Vladimir Sementsov-Ogievskiy
@ 2017-12-28 6:28 ` Fam Zheng
0 siblings, 0 replies; 50+ messages in thread
From: Fam Zheng @ 2017-12-28 6:28 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: qemu-block, qemu-devel, kwolf, peter.maydell, lirans, quintela,
jsnow, armbru, mreitz, stefanha, den, amit.shah, pbonzini,
dgilbert
On Wed, 12/20 18:49, Vladimir Sementsov-Ogievskiy wrote:
> +def inject_test_case(klass, name, method, *args, **kwargs):
> + mc = operator.methodcaller(method, *args, **kwargs)
> + setattr(klass, 'test_' + name, new.instancemethod(mc, None, klass))
> +
> +
> +for cmb in list(itertools.product((True, False), repeat=4)):
> + name = ('_' if cmb[0] else '_not_') + 'persistent_'
> + name += ('_' if cmb[1] else '_not_') + 'shared_'
> + name += ('_' if cmb[2] else '_not_') + 'migbitmap_'
> + name += '_online' if cmb[3] else '_offline'
> +
> + inject_test_case(TestDirtyBitmapMigration, name, 'do_test_migration', *cmb)
Personally I'd just spell out the 16 method names and call do_test_migration.
It's much easier to read and modify. Either way,
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 50+ messages in thread
* [Qemu-devel] [PATCH v9 13/13] iotests: add dirty bitmap postcopy test
2017-12-20 15:49 [Qemu-devel] [PATCH v9 00/13] Dirty bitmaps postcopy migration Vladimir Sementsov-Ogievskiy
` (11 preceding siblings ...)
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 12/13] iotests: add dirty bitmap migration test Vladimir Sementsov-Ogievskiy
@ 2017-12-20 15:49 ` Vladimir Sementsov-Ogievskiy
2017-12-28 6:33 ` Fam Zheng
12 siblings, 1 reply; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-12-20 15:49 UTC (permalink / raw)
To: qemu-block, qemu-devel
Cc: pbonzini, armbru, eblake, famz, stefanha, amit.shah, quintela,
mreitz, kwolf, peter.maydell, dgilbert, den, jsnow, vsementsov,
lirans
Test
- start two vms (vm_a, vm_b)
- in a
- do writes from set A
- do writes from set B
- fix bitmap sha256
- clear bitmap
- do writes from set A
- start migration
- than, in b
- wait vm start (postcopy should start)
- do writes from set B
- check bitmap sha256
The test should verify postcopy migration and then merging with delta
(changes in target, during postcopy process).
Reduce supported cache modes to only 'none', because with cache on time
from source.STOP to target.RESUME is unpredictable and we can fail with
timout while waiting for target.RESUME.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
tests/qemu-iotests/199 | 105 ++++++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/199.out | 5 ++
tests/qemu-iotests/group | 1 +
tests/qemu-iotests/iotests.py | 7 ++-
4 files changed, 117 insertions(+), 1 deletion(-)
create mode 100755 tests/qemu-iotests/199
create mode 100644 tests/qemu-iotests/199.out
diff --git a/tests/qemu-iotests/199 b/tests/qemu-iotests/199
new file mode 100755
index 0000000000..f872040a81
--- /dev/null
+++ b/tests/qemu-iotests/199
@@ -0,0 +1,105 @@
+#!/usr/bin/env python
+#
+# Tests for dirty bitmaps postcopy migration.
+#
+# Copyright (c) 2016-2017 Virtuozzo International GmbH. All rights reserved.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+import os
+import iotests
+import time
+from iotests import qemu_img
+
+disk_a = os.path.join(iotests.test_dir, 'disk_a')
+disk_b = os.path.join(iotests.test_dir, 'disk_b')
+size = '256G'
+fifo = os.path.join(iotests.test_dir, 'mig_fifo')
+
+class TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase):
+
+ def tearDown(self):
+ self.vm_a.shutdown()
+ self.vm_b.shutdown()
+ os.remove(disk_a)
+ os.remove(disk_b)
+ os.remove(fifo)
+
+ def setUp(self):
+ os.mkfifo(fifo)
+ qemu_img('create', '-f', iotests.imgfmt, disk_a, size)
+ qemu_img('create', '-f', iotests.imgfmt, disk_b, size)
+ self.vm_a = iotests.VM(path_suffix='a').add_drive(disk_a)
+ self.vm_b = iotests.VM(path_suffix='b').add_drive(disk_b)
+ self.vm_b.add_incoming("exec: cat '" + fifo + "'")
+ self.vm_a.launch()
+ self.vm_b.launch()
+
+ def test_postcopy(self):
+ write_size = 0x40000000
+ granularity = 512
+ chunk = 4096
+
+ result = self.vm_a.qmp('block-dirty-bitmap-add', node='drive0',
+ name='bitmap', granularity=granularity)
+ self.assert_qmp(result, 'return', {});
+
+ s = 0
+ while s < write_size:
+ self.vm_a.hmp_qemu_io('drive0', 'write %d %d' % (s, chunk))
+ s += 0x10000
+ s = 0x8000
+ while s < write_size:
+ self.vm_a.hmp_qemu_io('drive0', 'write %d %d' % (s, chunk))
+ s += 0x10000
+
+ result = self.vm_a.qmp('x-debug-block-dirty-bitmap-sha256',
+ node='drive0', name='bitmap')
+ sha256 = result['return']['sha256']
+
+ result = self.vm_a.qmp('block-dirty-bitmap-clear', node='drive0',
+ name='bitmap')
+ self.assert_qmp(result, 'return', {});
+ s = 0
+ while s < write_size:
+ self.vm_a.hmp_qemu_io('drive0', 'write %d %d' % (s, chunk))
+ s += 0x10000
+
+ result = self.vm_a.qmp('migrate-set-capabilities',
+ capabilities=[{'capability': 'dirty-bitmaps',
+ 'state': True}])
+ self.assert_qmp(result, 'return', {})
+
+ result = self.vm_a.qmp('migrate', uri='exec:cat>' + fifo)
+ self.assertNotEqual(self.vm_a.event_wait("STOP"), None)
+ self.assertNotEqual(self.vm_b.event_wait("RESUME"), None)
+
+ s = 0x8000
+ while s < write_size:
+ self.vm_b.hmp_qemu_io('drive0', 'write %d %d' % (s, chunk))
+ s += 0x10000
+
+ result = self.vm_b.qmp('query-block');
+ while len(result['return'][0]['dirty-bitmaps']) > 1:
+ time.sleep(2)
+ result = self.vm_b.qmp('query-block');
+
+ result = self.vm_b.qmp('x-debug-block-dirty-bitmap-sha256',
+ node='drive0', name='bitmap')
+
+ self.assert_qmp(result, 'return/sha256', sha256);
+
+if __name__ == '__main__':
+ iotests.main(supported_fmts=['qcow2'], supported_cache_modes=['none'])
diff --git a/tests/qemu-iotests/199.out b/tests/qemu-iotests/199.out
new file mode 100644
index 0000000000..ae1213e6f8
--- /dev/null
+++ b/tests/qemu-iotests/199.out
@@ -0,0 +1,5 @@
+.
+----------------------------------------------------------------------
+Ran 1 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index e879c7ebc7..7c1f499802 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -197,4 +197,5 @@
196 rw auto quick
197 rw auto quick
198 rw auto
+199 rw auto
200 rw auto
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 95454c1893..88f73d6441 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -432,6 +432,10 @@ def verify_platform(supported_oses=['linux']):
if True not in [sys.platform.startswith(x) for x in supported_oses]:
notrun('not suitable for this OS: %s' % sys.platform)
+def verify_cache_mode(supported_cache_modes=[]):
+ if supported_cache_modes and (cachemode not in supported_cache_modes):
+ notrun('not suitable for this cache mode: %s' % cachemode)
+
def supports_quorum():
return 'quorum' in qemu_img_pipe('--help')
@@ -440,7 +444,7 @@ def verify_quorum():
if not supports_quorum():
notrun('quorum support missing')
-def main(supported_fmts=[], supported_oses=['linux']):
+def main(supported_fmts=[], supported_oses=['linux'], supported_cache_modes=[]):
'''Run tests'''
global debug
@@ -457,6 +461,7 @@ def main(supported_fmts=[], supported_oses=['linux']):
verbosity = 1
verify_image_format(supported_fmts)
verify_platform(supported_oses)
+ verify_cache_mode(supported_cache_modes)
# We need to filter out the time taken from the output so that qemu-iotest
# can reliably diff the results against master output.
--
2.11.1
^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v9 13/13] iotests: add dirty bitmap postcopy test
2017-12-20 15:49 ` [Qemu-devel] [PATCH v9 13/13] iotests: add dirty bitmap postcopy test Vladimir Sementsov-Ogievskiy
@ 2017-12-28 6:33 ` Fam Zheng
2017-12-28 11:49 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 50+ messages in thread
From: Fam Zheng @ 2017-12-28 6:33 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: qemu-block, qemu-devel, kwolf, peter.maydell, lirans, quintela,
jsnow, armbru, mreitz, stefanha, den, amit.shah, pbonzini,
dgilbert
On Wed, 12/20 18:49, Vladimir Sementsov-Ogievskiy wrote:
> Test
> - start two vms (vm_a, vm_b)
>
> - in a
> - do writes from set A
> - do writes from set B
> - fix bitmap sha256
> - clear bitmap
> - do writes from set A
> - start migration
> - than, in b
> - wait vm start (postcopy should start)
> - do writes from set B
> - check bitmap sha256
>
> The test should verify postcopy migration and then merging with delta
> (changes in target, during postcopy process).
>
> Reduce supported cache modes to only 'none', because with cache on time
> from source.STOP to target.RESUME is unpredictable and we can fail with
> timout while waiting for target.RESUME.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> tests/qemu-iotests/199 | 105 ++++++++++++++++++++++++++++++++++++++++++
> tests/qemu-iotests/199.out | 5 ++
> tests/qemu-iotests/group | 1 +
> tests/qemu-iotests/iotests.py | 7 ++-
> 4 files changed, 117 insertions(+), 1 deletion(-)
> create mode 100755 tests/qemu-iotests/199
> create mode 100644 tests/qemu-iotests/199.out
>
> diff --git a/tests/qemu-iotests/199 b/tests/qemu-iotests/199
> new file mode 100755
> index 0000000000..f872040a81
> --- /dev/null
> +++ b/tests/qemu-iotests/199
> @@ -0,0 +1,105 @@
> +#!/usr/bin/env python
> +#
> +# Tests for dirty bitmaps postcopy migration.
> +#
> +# Copyright (c) 2016-2017 Virtuozzo International GmbH. All rights reserved.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +import os
> +import iotests
> +import time
> +from iotests import qemu_img
> +
> +disk_a = os.path.join(iotests.test_dir, 'disk_a')
> +disk_b = os.path.join(iotests.test_dir, 'disk_b')
> +size = '256G'
> +fifo = os.path.join(iotests.test_dir, 'mig_fifo')
> +
> +class TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase):
> +
> + def tearDown(self):
> + self.vm_a.shutdown()
> + self.vm_b.shutdown()
> + os.remove(disk_a)
> + os.remove(disk_b)
> + os.remove(fifo)
> +
> + def setUp(self):
> + os.mkfifo(fifo)
> + qemu_img('create', '-f', iotests.imgfmt, disk_a, size)
> + qemu_img('create', '-f', iotests.imgfmt, disk_b, size)
> + self.vm_a = iotests.VM(path_suffix='a').add_drive(disk_a)
> + self.vm_b = iotests.VM(path_suffix='b').add_drive(disk_b)
> + self.vm_b.add_incoming("exec: cat '" + fifo + "'")
> + self.vm_a.launch()
> + self.vm_b.launch()
> +
> + def test_postcopy(self):
> + write_size = 0x40000000
> + granularity = 512
> + chunk = 4096
> +
> + result = self.vm_a.qmp('block-dirty-bitmap-add', node='drive0',
> + name='bitmap', granularity=granularity)
> + self.assert_qmp(result, 'return', {});
> +
> + s = 0
> + while s < write_size:
> + self.vm_a.hmp_qemu_io('drive0', 'write %d %d' % (s, chunk))
> + s += 0x10000
> + s = 0x8000
> + while s < write_size:
> + self.vm_a.hmp_qemu_io('drive0', 'write %d %d' % (s, chunk))
> + s += 0x10000
> +
> + result = self.vm_a.qmp('x-debug-block-dirty-bitmap-sha256',
> + node='drive0', name='bitmap')
> + sha256 = result['return']['sha256']
> +
> + result = self.vm_a.qmp('block-dirty-bitmap-clear', node='drive0',
> + name='bitmap')
> + self.assert_qmp(result, 'return', {});
> + s = 0
> + while s < write_size:
> + self.vm_a.hmp_qemu_io('drive0', 'write %d %d' % (s, chunk))
> + s += 0x10000
> +
> + result = self.vm_a.qmp('migrate-set-capabilities',
> + capabilities=[{'capability': 'dirty-bitmaps',
> + 'state': True}])
> + self.assert_qmp(result, 'return', {})
> +
> + result = self.vm_a.qmp('migrate', uri='exec:cat>' + fifo)
> + self.assertNotEqual(self.vm_a.event_wait("STOP"), None)
> + self.assertNotEqual(self.vm_b.event_wait("RESUME"), None)
> +
> + s = 0x8000
> + while s < write_size:
> + self.vm_b.hmp_qemu_io('drive0', 'write %d %d' % (s, chunk))
> + s += 0x10000
> +
> + result = self.vm_b.qmp('query-block');
> + while len(result['return'][0]['dirty-bitmaps']) > 1:
> + time.sleep(2)
> + result = self.vm_b.qmp('query-block');
> +
> + result = self.vm_b.qmp('x-debug-block-dirty-bitmap-sha256',
> + node='drive0', name='bitmap')
> +
> + self.assert_qmp(result, 'return/sha256', sha256);
> +
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v9 13/13] iotests: add dirty bitmap postcopy test
2017-12-28 6:33 ` Fam Zheng
@ 2017-12-28 11:49 ` Vladimir Sementsov-Ogievskiy
2018-01-17 18:30 ` John Snow
0 siblings, 1 reply; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-12-28 11:49 UTC (permalink / raw)
To: Fam Zheng
Cc: qemu-block, qemu-devel, kwolf, peter.maydell, lirans, quintela,
jsnow, armbru, mreitz, stefanha, den, amit.shah, pbonzini,
dgilbert
Thank you for reviewing my code!
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v9 13/13] iotests: add dirty bitmap postcopy test
2017-12-28 11:49 ` Vladimir Sementsov-Ogievskiy
@ 2018-01-17 18:30 ` John Snow
2018-01-18 9:57 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 50+ messages in thread
From: John Snow @ 2018-01-17 18:30 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, Fam Zheng
Cc: qemu-block, qemu-devel, kwolf, peter.maydell, lirans, quintela,
armbru, mreitz, stefanha, den, amit.shah, pbonzini, dgilbert
On 12/28/2017 06:49 AM, Vladimir Sementsov-Ogievskiy wrote:
> Thank you for reviewing my code!
>
Time for the re-spin? There's pretty good pressure to get this into 2.12
and say the non-nbd workflow model is feature complete.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v9 13/13] iotests: add dirty bitmap postcopy test
2018-01-17 18:30 ` John Snow
@ 2018-01-18 9:57 ` Vladimir Sementsov-Ogievskiy
2018-01-19 18:08 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-01-18 9:57 UTC (permalink / raw)
To: John Snow, Fam Zheng
Cc: qemu-block, qemu-devel, kwolf, peter.maydell, lirans, quintela,
armbru, mreitz, stefanha, den, amit.shah, pbonzini, dgilbert
17.01.2018 21:30, John Snow wrote:
>
> On 12/28/2017 06:49 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Thank you for reviewing my code!
>>
> Time for the re-spin? There's pretty good pressure to get this into 2.12
> and say the non-nbd workflow model is feature complete.
Yes, you are right. Hope to do it today or tomorrow.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v9 13/13] iotests: add dirty bitmap postcopy test
2018-01-18 9:57 ` Vladimir Sementsov-Ogievskiy
@ 2018-01-19 18:08 ` Vladimir Sementsov-Ogievskiy
2018-01-19 18:20 ` John Snow
2018-01-20 0:37 ` John Snow
0 siblings, 2 replies; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-01-19 18:08 UTC (permalink / raw)
To: John Snow, Fam Zheng
Cc: qemu-block, qemu-devel, kwolf, peter.maydell, lirans, quintela,
armbru, mreitz, stefanha, den, amit.shah, pbonzini, dgilbert
18.01.2018 12:57, Vladimir Sementsov-Ogievskiy wrote:
> 17.01.2018 21:30, John Snow wrote:
>>
>> On 12/28/2017 06:49 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Thank you for reviewing my code!
>>>
>> Time for the re-spin? There's pretty good pressure to get this into 2.12
>> and say the non-nbd workflow model is feature complete.
>
> Yes, you are right. Hope to do it today or tomorrow.
>
I've rebased, applied comments from review, and even one some new fixes,
but now I understand that it is too early for re-spin.
Now this series depends on
1. [PATCH v2 0/3] fix bitmaps migration through shared storage
- we need to decide, how to fix it. May be, we just do not need
bitmaps in inactive state, and should not load them in do_qcow2_open in
this case
[I can ignore it, just dropping one test case from new iotest, and
fix it later, but..
2. [PATCH v2 0/6] qmp dirty bitmap API
- here autoload is dropped, and migration should be rebased on it.
so, I'll re-spin migration after these two questions are resolved.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v9 13/13] iotests: add dirty bitmap postcopy test
2018-01-19 18:08 ` Vladimir Sementsov-Ogievskiy
@ 2018-01-19 18:20 ` John Snow
2018-01-20 0:37 ` John Snow
1 sibling, 0 replies; 50+ messages in thread
From: John Snow @ 2018-01-19 18:20 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, Fam Zheng
Cc: qemu-block, qemu-devel, kwolf, peter.maydell, lirans, quintela,
armbru, mreitz, stefanha, den, amit.shah, pbonzini, dgilbert
On 01/19/2018 01:08 PM, Vladimir Sementsov-Ogievskiy wrote:
> 18.01.2018 12:57, Vladimir Sementsov-Ogievskiy wrote:
>> 17.01.2018 21:30, John Snow wrote:
>>>
>>> On 12/28/2017 06:49 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> Thank you for reviewing my code!
>>>>
>>> Time for the re-spin? There's pretty good pressure to get this into 2.12
>>> and say the non-nbd workflow model is feature complete.
>>
>> Yes, you are right. Hope to do it today or tomorrow.
>>
>
> I've rebased, applied comments from review, and even one some new fixes,
> but now I understand that it is too early for re-spin.
>
> Now this series depends on
> 1. [PATCH v2 0/3] fix bitmaps migration through shared storage
> - we need to decide, how to fix it. May be, we just do not need
> bitmaps in inactive state, and should not load them in do_qcow2_open in
> this case
> [I can ignore it, just dropping one test case from new iotest, and fix
> it later, but..
> 2. [PATCH v2 0/6] qmp dirty bitmap API
> - here autoload is dropped, and migration should be rebased on it.
>
> so, I'll re-spin migration after these two questions are resolved.
>
You got it, thanks.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v9 13/13] iotests: add dirty bitmap postcopy test
2018-01-19 18:08 ` Vladimir Sementsov-Ogievskiy
2018-01-19 18:20 ` John Snow
@ 2018-01-20 0:37 ` John Snow
2018-01-22 9:06 ` Vladimir Sementsov-Ogievskiy
1 sibling, 1 reply; 50+ messages in thread
From: John Snow @ 2018-01-20 0:37 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, Fam Zheng
Cc: qemu-block, qemu-devel, kwolf, peter.maydell, lirans, quintela,
armbru, mreitz, stefanha, den, amit.shah, pbonzini, dgilbert
On 01/19/2018 01:08 PM, Vladimir Sementsov-Ogievskiy wrote:
> 18.01.2018 12:57, Vladimir Sementsov-Ogievskiy wrote:
>> 17.01.2018 21:30, John Snow wrote:
>>>
>>> On 12/28/2017 06:49 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> Thank you for reviewing my code!
>>>>
>>> Time for the re-spin? There's pretty good pressure to get this into 2.12
>>> and say the non-nbd workflow model is feature complete.
>>
>> Yes, you are right. Hope to do it today or tomorrow.
>>
>
> I've rebased, applied comments from review, and even one some new fixes,
> but now I understand that it is too early for re-spin.
>
> Now this series depends on
> 1. [PATCH v2 0/3] fix bitmaps migration through shared storage
> - we need to decide, how to fix it. May be, we just do not need
> bitmaps in inactive state, and should not load them in do_qcow2_open in
> this case
> [I can ignore it, just dropping one test case from new iotest, and fix
> it later, but..
I'll look at this next.
> 2. [PATCH v2 0/6] qmp dirty bitmap API
> - here autoload is dropped, and migration should be rebased on it.
>
For (2) I really want to see a test case showing the utility for enable,
disable and merge to be added to the API, *or* add an x- prefix to them
for now.
I want to see some use-case tests that demonstrate how they are to be
safely used, basically. You might want to expand test 124 for this.
I'm not sure we need this entire series for migration, but I don't want
to make you re-order absolutely everything. Can we merge patch one by
itself for the purposes of the migration patchset?
> so, I'll re-spin migration after these two questions are resolved.
>
Thanks for your patience, as always. Please send me pings every day on
whatever you have to in order to get migration in to 2.12 -- I'm going
to try to stay focused too, but I've got the attention span of a
goldfish. If I miss something, please speak up.
--js
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [Qemu-devel] [PATCH v9 13/13] iotests: add dirty bitmap postcopy test
2018-01-20 0:37 ` John Snow
@ 2018-01-22 9:06 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 50+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-01-22 9:06 UTC (permalink / raw)
To: John Snow, Fam Zheng
Cc: qemu-block, qemu-devel, kwolf, peter.maydell, lirans, quintela,
armbru, mreitz, stefanha, den, amit.shah, pbonzini, dgilbert
20.01.2018 03:37, John Snow wrote:
>
> On 01/19/2018 01:08 PM, Vladimir Sementsov-Ogievskiy wrote:
>> 18.01.2018 12:57, Vladimir Sementsov-Ogievskiy wrote:
>>> 17.01.2018 21:30, John Snow wrote:
>>>> On 12/28/2017 06:49 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>>> Thank you for reviewing my code!
>>>>>
>>>> Time for the re-spin? There's pretty good pressure to get this into 2.12
>>>> and say the non-nbd workflow model is feature complete.
>>> Yes, you are right. Hope to do it today or tomorrow.
>>>
>> I've rebased, applied comments from review, and even one some new fixes,
>> but now I understand that it is too early for re-spin.
>>
>> Now this series depends on
>> 1. [PATCH v2 0/3] fix bitmaps migration through shared storage
>> - we need to decide, how to fix it. May be, we just do not need
>> bitmaps in inactive state, and should not load them in do_qcow2_open in
>> this case
>> [I can ignore it, just dropping one test case from new iotest, and fix
>> it later, but..
> I'll look at this next.
>
>> 2. [PATCH v2 0/6] qmp dirty bitmap API
>> - here autoload is dropped, and migration should be rebased on it.
>>
> For (2) I really want to see a test case showing the utility for enable,
> disable and merge to be added to the API, *or* add an x- prefix to them
> for now.
tests are inevitable, I'll make them)
>
> I want to see some use-case tests that demonstrate how they are to be
> safely used, basically. You might want to expand test 124 for this.
>
> I'm not sure we need this entire series for migration, but I don't want
> to make you re-order absolutely everything. Can we merge patch one by
> itself for the purposes of the migration patchset?
I think yes, will try.
>> so, I'll re-spin migration after these two questions are resolved.
>>
> Thanks for your patience, as always. Please send me pings every day on
> whatever you have to in order to get migration in to 2.12 -- I'm going
> to try to stay focused too, but I've got the attention span of a
> goldfish. If I miss something, please speak up.
>
> --js
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 50+ messages in thread