From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40577) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1d3Mjt-0007h5-B9 for qemu-devel@nongnu.org; Wed, 26 Apr 2017 09:12:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1d3Mjn-0004fW-0W for qemu-devel@nongnu.org; Wed, 26 Apr 2017 09:12:13 -0400 References: <1486979689-230770-1-git-send-email-vsementsov@virtuozzo.com> <1486979689-230770-13-git-send-email-vsementsov@virtuozzo.com> <20170216130409.GA28784@lemon.lan> <665e5244-6f2c-1a34-f7bb-1021ced052c2@virtuozzo.com> From: Vladimir Sementsov-Ogievskiy Message-ID: <81c4cd1f-5125-f26d-bf12-3d24d75c0c03@virtuozzo.com> Date: Wed, 26 Apr 2017 16:11:48 +0300 MIME-Version: 1.0 In-Reply-To: <665e5244-6f2c-1a34-f7bb-1021ced052c2@virtuozzo.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 12/17] migration: add postcopy migration of dirty bitmaps List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: qemu-block@nongnu.org, qemu-devel@nongnu.org, pbonzini@redhat.com, armbru@redhat.com, eblake@redhat.com, stefanha@redhat.com, amit.shah@redhat.com, quintela@redhat.com, mreitz@redhat.com, kwolf@redhat.com, peter.maydell@linaro.org, dgilbert@redhat.com, den@openvz.org, jsnow@redhat.com, lirans@il.ibm.com 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_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). 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. >>> + >>> +#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