Re: [Qemu-block] [PATCH 12/17] migration: add postcopy migration of dirty bitmaps

[Vladimir Sementsov-Ogievskiy]

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_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_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).

> > +#define DIRTY_BITMAP_MIG_FLAGS_SIZE_32      0x8080
> > +
> > +#define DEBUG_DIRTY_BITMAP_MIGRATION 0

[...]

> +
> +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;
> > +        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);
> > +}
> > +
> > +

[...]

> > +
> > +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,
Vladimir