[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 12/17] migration: add postcopy migration of dirt

From: Vladimir Sementsov-Ogievskiy
Subject: Re: [Qemu-devel] [PATCH 12/17] migration: add postcopy migration of dirty bitmaps
Date: Wed, 26 Apr 2017 16:11:48 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0

25.02.2017 20:56, Vladimir Sementsov-Ogievskiy wrote:
16.02.2017 16:04, Fam Zheng wrote:
On Mon, 02/13 12:54, 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), than, if their granularities are


+#define CHUNK_SIZE     (1 << 10)
+/* Flags occupy from one to four bytes. In all but one the 7-th (EXTRA_FLAGS)
+ * bit should be set. */
+#define DIRTY_BITMAP_MIG_FLAG_EOS           0x01
+#define DIRTY_BITMAP_MIG_FLAG_ZEROES        0x02
+#define DIRTY_BITMAP_MIG_FLAG_START         0x10
+#define DIRTY_BITMAP_MIG_FLAG_BITS          0x40
+#define DIRTY_BITMAP_MIG_EXTRA_FLAGS        0x80
+#define DIRTY_BITMAP_MIG_FLAGS_SIZE_16      0x8000
This flag means two bytes, right? But your above comment says "7-th bit should
be set". This doesn't make sense. Should this be "0x80" too?

Hmm, good caught, you are right. Also, the comment should be fixed so that there are may be 1,2 or 4 bytes, and of course EXTRA_FLAGS bit may be only in first and second bytes (the code do so).

Aha, now I understand that 0x8000 is ok for big endian

+#define DIRTY_BITMAP_MIG_FLAGS_SIZE_32      0x8080

and this is not ok)

I think, I'll just drop this. Anyway it is a dead code, we do not send flags more than 1 byte in the code. On the other hand, receive flags path is absolutely ok, and it is for backward compatibility - we just ignore unknown flags.



+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, nr_sectors);
+    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, nr_sectors);
While these bdrv_dirty_bitmap_* calls here seem fine, BdrvDirtyBitmap API is not in general thread-safe, while this function is called without any lock. This feels dangerous, as noted below, I'm most concerned about use-after-free.

This should be safe as it is a postcopy migration - source should be already inactive.

+    if (buffer_is_zero(buf, buf_size)) {
+        g_free(buf);
+        buf = NULL;
+    }
+    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);


+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();
Why do you need the BQL here but not in bulk_phase()?

bulk_phase is in postcopy, source is inactive

Best regards,

reply via email to

[Prev in Thread] Current Thread [Next in Thread]