From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35804) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c2eFa-0004wR-Ko for qemu-devel@nongnu.org; Fri, 04 Nov 2016 09:09:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c2eFW-0005h6-55 for qemu-devel@nongnu.org; Fri, 04 Nov 2016 09:09:42 -0400 From: Juan Quintela In-Reply-To: <1471343175-14945-13-git-send-email-vsementsov@virtuozzo.com> (Vladimir Sementsov-Ogievskiy's message of "Tue, 16 Aug 2016 13:26:09 +0300") References: <1471343175-14945-1-git-send-email-vsementsov@virtuozzo.com> <1471343175-14945-13-git-send-email-vsementsov@virtuozzo.com> Reply-To: quintela@redhat.com Date: Fri, 04 Nov 2016 14:09:21 +0100 Message-ID: <87h97no0hq.fsf@emacs.mitica> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH 12/18] migration: add postcopy migration of dirty bitmaps List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladimir Sementsov-Ogievskiy Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org, pbonzini@redhat.com, armbru@redhat.com, eblake@redhat.com, famz@redhat.com, stefanha@redhat.com, amit.shah@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 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 > 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 > diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c > new file mode 100644 > index 0000000..c668d02 > --- /dev/null > +++ b/migration/block-dirty-bitmap.c > @@ -0,0 +1,699 @@ > +/* > + * QEMU dirty bitmap migration > + * > + * 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 > + * 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. > + * > + * 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 enabled flag > + * > + * # 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. > + * > + * > + * This file is derived from migration/block.c > + * > + * Author: > + * Vladimir Sementsov-Ogievskiy > + * > + * original copyright message: > + * ===================================================================== > + * Copyright IBM, Corp. 2009 > + * > + * Authors: > + * Liran Schour > + * > + * This work is licensed under the terms of the GNU GPL, version 2. See > + * the COPYING file in the top-level directory. > + * > + * Contributions after 2012-01-13 are licensed under the terms of the > + * GNU GPL, version 2 or (at your option) any later version. > + * ===================================================================== > + */ I think that the normal practice is putting first the copyright and then the comment of the file. > +static void qemu_put_bitmap_flags(QEMUFile *f, uint32_t flags) > +{ > + if (!(flags & 0xffffff00)) { > + qemu_put_byte(f, flags); > + return; > + } > + > + if (!(flags & 0xffff0000)) { > + qemu_put_be16(f, flags | DIRTY_BITMAP_MIG_FLAGS_SIZE_16); > + return; > + } > + > + qemu_put_be32(f, flags | DIRTY_BITMAP_MIG_FLAGS_SIZE_32); > +} Do need flags so many times to be a good idea to spend two flags and make the code more complex? Couldn't just sent always the 32bit word and call it a day? I have only looked at the stuff quickly from the migration point of view, not about the bitmap stuff. > +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, 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); > + > + if (buffer_is_zero(buf, buf_size)) { > + g_free(buf); > + buf = NULL; > + flags |= DIRTY_BITMAP_MIG_FLAG_ZEROES; > + } > + > + DPRINTF("parameters:" > + "\n flags: %x" > + "\n start_sector: %" PRIu64 > + "\n nr_sectors: %" PRIu32 > + "\n data_size: %" PRIu64 "\n", > + flags, start_sector, nr_sectors, buf_size); Now we are adding traces, not DPRINF's to new code in general. Same for all DPRINTFs > + > + 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. > + * also, skip writing block when migrate only dirty bitmaps. */ > + if (flags & DIRTY_BITMAP_MIG_FLAG_ZEROES) { > + qemu_fflush(f); I thought that we were missing the g_free(buf) here, not sure if it is better to put the return here, or do an else for the rest of the code.