Re: [Qemu-devel] [PULL 11/13] migration: add postcopy migration of dirty bitmaps
16.10.2018 15:25, Peter Maydell wrote:
> On 20 June 2018 at 17:58, John Snow wrote:
>>
>> On 06/20/2018 12:43 PM, Peter Maydell wrote:
>>> On 27 April 2018 at 14:22, Peter Maydell wrote:
On 13 March 2018 at 21:14, John Snow wrote:
> From: Vladimir Sementsov-Ogievskiy
>
> Postcopy migration of dirty bitmaps. Only named dirty bitmaps 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
> Reviewed-by: Dr. David Alan Gilbert
> Message-id: 20180313180320.339796-12-vsement...@virtuozzo.com
> [Changed '+' to '*' as per list discussion. --js]
> Signed-off-by: John Snow
> +static int init_dirty_bitmap_migration(void)
> +{
Hi; Coverity (CID1390625) complains about a possible dereference
after NULL check in this function:
> +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;
> +dirty_bitmap_mig_state.no_bitmaps = false;
> +
> +for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
> +const char *drive_name = bdrv_get_device_or_node_name(bs);
> +
> +/* skip automatically inserted nodes */
> +while (bs && bs->drv && bs->implicit) {
> +bs = backing_bs(bs);
> +}
The 'bs' test in this while() loop implies that we might
leave the loop because bs == NULL...
> +
> +for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap;
...but this call to bdrv_dirty_bitmap_next() will always
dereference bs, so if it's NULL we'll crash.
> + bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
>>> Hi -- just a nudge that Coverity thinks this one is still unfixed.
>> Thank you for the reminder, I've been a bit scatter-brained recently.
> Ping? This is still in Coverity's list of unfixed issues.
>
> thanks
> -- PMM
Will send in few seconds, sorry for such a terrible delay :(
--
Best regards,
Vladimir
Re: [Qemu-devel] [PULL 11/13] migration: add postcopy migration of dirty bitmaps
On 20 June 2018 at 17:58, John Snow wrote:
>
>
> On 06/20/2018 12:43 PM, Peter Maydell wrote:
>> On 27 April 2018 at 14:22, Peter Maydell wrote:
>>> On 13 March 2018 at 21:14, John Snow wrote:
From: Vladimir Sementsov-Ogievskiy
Postcopy migration of dirty bitmaps. Only named dirty bitmaps 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
Reviewed-by: Dr. David Alan Gilbert
Message-id: 20180313180320.339796-12-vsement...@virtuozzo.com
[Changed '+' to '*' as per list discussion. --js]
Signed-off-by: John Snow
>>>
+static int init_dirty_bitmap_migration(void)
+{
>>>
>>> Hi; Coverity (CID1390625) complains about a possible dereference
>>> after NULL check in this function:
>>>
+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;
+dirty_bitmap_mig_state.no_bitmaps = false;
+
+for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
+const char *drive_name = bdrv_get_device_or_node_name(bs);
+
+/* skip automatically inserted nodes */
+while (bs && bs->drv && bs->implicit) {
+bs = backing_bs(bs);
+}
>>>
>>> The 'bs' test in this while() loop implies that we might
>>> leave the loop because bs == NULL...
>>>
+
+for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap;
>>>
>>> ...but this call to bdrv_dirty_bitmap_next() will always
>>> dereference bs, so if it's NULL we'll crash.
>>>
+ bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
>>
>> Hi -- just a nudge that Coverity thinks this one is still unfixed.
> Thank you for the reminder, I've been a bit scatter-brained recently.
Ping? This is still in Coverity's list of unfixed issues.
thanks
-- PMM
Re: [Qemu-devel] [PULL 11/13] migration: add postcopy migration of dirty bitmaps
On 06/20/2018 12:43 PM, Peter Maydell wrote:
> On 27 April 2018 at 14:22, Peter Maydell wrote:
>> On 13 March 2018 at 21:14, John Snow wrote:
>>> From: Vladimir Sementsov-Ogievskiy
>>>
>>> Postcopy migration of dirty bitmaps. Only named dirty bitmaps 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
>>> Reviewed-by: Dr. David Alan Gilbert
>>> Message-id: 20180313180320.339796-12-vsement...@virtuozzo.com
>>> [Changed '+' to '*' as per list discussion. --js]
>>> Signed-off-by: John Snow
>>
>>> +static int init_dirty_bitmap_migration(void)
>>> +{
>>
>> Hi; Coverity (CID1390625) complains about a possible dereference
>> after NULL check in this function:
>>
>>> +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;
>>> +dirty_bitmap_mig_state.no_bitmaps = false;
>>> +
>>> +for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
>>> +const char *drive_name = bdrv_get_device_or_node_name(bs);
>>> +
>>> +/* skip automatically inserted nodes */
>>> +while (bs && bs->drv && bs->implicit) {
>>> +bs = backing_bs(bs);
>>> +}
>>
>> The 'bs' test in this while() loop implies that we might
>> leave the loop because bs == NULL...
>>
>>> +
>>> +for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap;
>>
>> ...but this call to bdrv_dirty_bitmap_next() will always
>> dereference bs, so if it's NULL we'll crash.
>>
>>> + bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
>
> Hi -- just a nudge that Coverity thinks this one is still unfixed.
>
> thanks
> -- PMM
>
Thank you for the reminder, I've been a bit scatter-brained recently.
Re: [Qemu-devel] [PULL 11/13] migration: add postcopy migration of dirty bitmaps
On 27 April 2018 at 14:22, Peter Maydell wrote:
> On 13 March 2018 at 21:14, John Snow wrote:
>> From: Vladimir Sementsov-Ogievskiy
>>
>> Postcopy migration of dirty bitmaps. Only named dirty bitmaps 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
>> Reviewed-by: Dr. David Alan Gilbert
>> Message-id: 20180313180320.339796-12-vsement...@virtuozzo.com
>> [Changed '+' to '*' as per list discussion. --js]
>> Signed-off-by: John Snow
>
>> +static int init_dirty_bitmap_migration(void)
>> +{
>
> Hi; Coverity (CID1390625) complains about a possible dereference
> after NULL check in this function:
>
>> +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;
>> +dirty_bitmap_mig_state.no_bitmaps = false;
>> +
>> +for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
>> +const char *drive_name = bdrv_get_device_or_node_name(bs);
>> +
>> +/* skip automatically inserted nodes */
>> +while (bs && bs->drv && bs->implicit) {
>> +bs = backing_bs(bs);
>> +}
>
> The 'bs' test in this while() loop implies that we might
> leave the loop because bs == NULL...
>
>> +
>> +for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap;
>
> ...but this call to bdrv_dirty_bitmap_next() will always
> dereference bs, so if it's NULL we'll crash.
>
>> + bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
Hi -- just a nudge that Coverity thinks this one is still unfixed.
thanks
-- PMM
Re: [Qemu-devel] [PULL 11/13] migration: add postcopy migration of dirty bitmaps
On 04/27/2018 09:22 AM, Peter Maydell wrote:
> On 13 March 2018 at 21:14, John Snow wrote:
>> From: Vladimir Sementsov-Ogievskiy
>>
>> Postcopy migration of dirty bitmaps. Only named dirty bitmaps 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
>> Reviewed-by: Dr. David Alan Gilbert
>> Message-id: 20180313180320.339796-12-vsement...@virtuozzo.com
>> [Changed '+' to '*' as per list discussion. --js]
>> Signed-off-by: John Snow
>
>> +static int init_dirty_bitmap_migration(void)
>> +{
>
> Hi; Coverity (CID1390625) complains about a possible dereference
> after NULL check in this function:
>
>> +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;
>> +dirty_bitmap_mig_state.no_bitmaps = false;
>> +
>> +for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
>> +const char *drive_name = bdrv_get_device_or_node_name(bs);
>> +
>> +/* skip automatically inserted nodes */
>> +while (bs && bs->drv && bs->implicit) {
>> +bs = backing_bs(bs);
>> +}
>
> The 'bs' test in this while() loop implies that we might
> leave the loop because bs == NULL...
>
>> +
>> +for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap;
>
> ...but this call to bdrv_dirty_bitmap_next() will always
> dereference bs, so if it's NULL we'll crash.
>
>> + bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
>
> thanks
> -- PMM
>
I have some patches on the way to clean up this file. Sorry for the delay.
Re: [Qemu-devel] [PULL 11/13] migration: add postcopy migration of dirty bitmaps
On 13 March 2018 at 21:14, John Snow wrote:
> From: Vladimir Sementsov-Ogievskiy
>
> Postcopy migration of dirty bitmaps. Only named dirty bitmaps 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.
Hi. Coverity complains (CID1390627) about a resource leak in this function;
> +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 {
> +size_t ret;
> +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 ||
> +buf_size > QEMU_ALIGN_UP(needed_size, 4 * sizeof(long))
> + /* Here used same alignment as in send_bitmap_bits */
> +) {
> +error_report("Migrated bitmap granularity doesn't "
> + "match the destination bitmap '%s' granularity",
> + bdrv_dirty_bitmap_name(s->bitmap));
> +return -EINVAL;
> +}
> +
> +buf = g_malloc(buf_size);
Here we allocate memory into buf...
> +ret = qemu_get_buffer(f, buf, buf_size);
> +if (ret != buf_size) {
> +error_report("Failed to read bitmap bits");
> +return -EIO;
...but in this error-exit path we do not free it.
> +}
> +
> +bdrv_dirty_bitmap_deserialize_part(s->bitmap, buf, first_byte,
> nr_bytes,
> + false);
> +g_free(buf);
> +}
> +
> +return 0;
> +}
thanks
-- PMM
Re: [Qemu-devel] [PULL 11/13] migration: add postcopy migration of dirty bitmaps
On 13 March 2018 at 21:14, John Snow wrote:
> From: Vladimir Sementsov-Ogievskiy
>
> Postcopy migration of dirty bitmaps. Only named dirty bitmaps 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
> Reviewed-by: Dr. David Alan Gilbert
> Message-id: 20180313180320.339796-12-vsement...@virtuozzo.com
> [Changed '+' to '*' as per list discussion. --js]
> Signed-off-by: John Snow
> +static int init_dirty_bitmap_migration(void)
> +{
Hi; Coverity (CID1390625) complains about a possible dereference
after NULL check in this function:
> +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;
> +dirty_bitmap_mig_state.no_bitmaps = false;
> +
> +for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
> +const char *drive_name = bdrv_get_device_or_node_name(bs);
> +
> +/* skip automatically inserted nodes */
> +while (bs && bs->drv && bs->implicit) {
> +bs = backing_bs(bs);
> +}
The 'bs' test in this while() loop implies that we might
leave the loop because bs == NULL...
> +
> +for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap;
...but this call to bdrv_dirty_bitmap_next() will always
dereference bs, so if it's NULL we'll crash.
> + bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
thanks
-- PMM
[Qemu-devel] [PULL 11/13] migration: add postcopy migration of dirty bitmaps
From: Vladimir Sementsov-Ogievskiy
Postcopy migration of dirty bitmaps. Only named dirty bitmaps 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
Reviewed-by: Dr. David Alan Gilbert
Message-id: 20180313180320.339796-12-vsement...@virtuozzo.com
[Changed '+' to '*' as per list discussion. --js]
Signed-off-by: John Snow
---
include/migration/misc.h | 3 +
migration/Makefile.objs| 1 +
migration/block-dirty-bitmap.c | 746 +
migration/migration.c | 5 +
migration/migration.h | 3 +
migration/savevm.c | 2 +
migration/trace-events | 14 +
vl.c | 1 +
8 files changed, 775 insertions(+)
create mode 100644 migration/block-dirty-bitmap.c
diff --git a/include/migration/misc.h b/include/migration/misc.h
index 77fd4f587c..4ebf24c6c2 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -56,4 +56,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/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/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
new file mode 100644
index 00..dd04f102d8
--- /dev/null
+++ b/migration/block-dirty-bitmap.c
@@ -0,0 +1,746 @@
+/*
+ * Block dirty bitmap postcopy migration
+ *
+ * Copyright IBM, Corp. 2009
+ * Copyright (c) 2016-2017 Virtuozzo International GmbH. All rights reserved.
+ *
+ * Authors:
+ * Liran Schour
+ * Vladimir Sementsov-Ogievskiy
+ *
+ * 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 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 by
