From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37509) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XZdUJ-0008Dt-QB for qemu-devel@nongnu.org; Thu, 02 Oct 2014 06:20:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XZdUD-0003nn-Ee for qemu-devel@nongnu.org; Thu, 02 Oct 2014 06:19:55 -0400 Received: from mail-pa0-f43.google.com ([209.85.220.43]:36407) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XZdUD-0003nd-6n for qemu-devel@nongnu.org; Thu, 02 Oct 2014 06:19:49 -0400 Received: by mail-pa0-f43.google.com with SMTP id lf10so1973189pab.16 for ; Thu, 02 Oct 2014 03:19:48 -0700 (PDT) Message-ID: <542D26BA.9030005@ozlabs.ru> Date: Thu, 02 Oct 2014 20:19:38 +1000 From: Alexey Kardashevskiy MIME-Version: 1.0 References: <1412239972-23493-1-git-send-email-aik@ozlabs.ru> <542D1EA2.6060607@redhat.com> In-Reply-To: <542D1EA2.6060607@redhat.com> Content-Type: text/plain; charset=koi8-r Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [RFC PATCH] block/migration: Disable cache invalidate for incoming migration List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , qemu-devel@nongnu.org Cc: Kevin Wolf , Stefan Hajnoczi -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 10/02/2014 07:45 PM, Paolo Bonzini wrote: > Il 02/10/2014 10:52, Alexey Kardashevskiy ha scritto: >> When migrated using libvirt with "--copy-storage-all", at the end >> of migration there is race between NBD mirroring task trying to do >> flush and migration completion, both end up invalidating cache. >> Since qcow2 driver does not handle this situation very well, random >> crashes happen. >> >> This disables the BDRV_O_INCOMING flag for the block device being >> migrated and restores it when NBD task is done. >> >> Signed-off-by: Alexey Kardashevskiy --- >> >> >> The commit log is not full and most likely incorrect as well as the >> patch :) Please, help. Thanks! >> >> The patch seems to fix the initial problem though. >> >> >> btw is there any easy way to migrate one QEMU to another using NBD >> (i.e. not using "migrate -b") and not using libvirt? What would the >> command line be? Debugging with libvirt is real pain :( >> >> >> --- block.c | 17 ++++------------- migration.c | 1 - nbd.c >> | 11 +++++++++++ 3 files changed, 15 insertions(+), 14 deletions(-) >> >> diff --git a/block.c b/block.c index c5a251c..ed72e0a 100644 --- >> a/block.c +++ b/block.c @@ -5073,6 +5073,10 @@ void >> bdrv_invalidate_cache_all(Error **errp) QTAILQ_FOREACH(bs, >> &bdrv_states, device_list) { AioContext *aio_context = >> bdrv_get_aio_context(bs); >> >> + if (!(bs->open_flags & BDRV_O_INCOMING)) { + >> continue; + } + aio_context_acquire(aio_context); >> bdrv_invalidate_cache(bs, &local_err); >> aio_context_release(aio_context); > > This part is okay, though perhaps we should add it to > bdrv_invalidate_cache instead? Yes, makes perfect sense. > >> @@ -5083,19 +5087,6 @@ void bdrv_invalidate_cache_all(Error **errp) >> } } >> >> -void bdrv_clear_incoming_migration_all(void) -{ - >> BlockDriverState *bs; - - QTAILQ_FOREACH(bs, &bdrv_states, >> device_list) { - AioContext *aio_context = >> bdrv_get_aio_context(bs); - - >> aio_context_acquire(aio_context); - bs->open_flags = >> bs->open_flags & ~(BDRV_O_INCOMING); - >> aio_context_release(aio_context); - } -} - int >> bdrv_flush(BlockDriverState *bs) { Coroutine *co; diff --git >> a/migration.c b/migration.c index 8d675b3..c49a05a 100644 --- >> a/migration.c +++ b/migration.c @@ -103,7 +103,6 @@ static void >> process_incoming_migration_co(void *opaque) } qemu_announce_self(); >> >> - bdrv_clear_incoming_migration_all(); /* Make sure all file >> formats flush their mutable metadata */ >> bdrv_invalidate_cache_all(&local_err); if (local_err) { > > This part I don't understand. > > Shouldn't you at least be adding > > bs->open_flags = bs->open_flags & ~(BDRV_O_INCOMING); > > to bdrv_invalidate_cache? Reset the flag after caches has been invalidated? What is the exact semantic of this BDRV_O_INCOMING? blockdev_init() sets it, we reset it on the first bdrv_invalidate_cache() and then we never set it again? I am still missing the bigger picture... >> diff --git a/nbd.c b/nbd.c index e9b539b..7b479c0 100644 --- >> a/nbd.c +++ b/nbd.c @@ -106,6 +106,7 @@ struct NBDExport { off_t >> dev_offset; off_t size; uint32_t nbdflags; + bool >> restore_incoming; QTAILQ_HEAD(, NBDClient) clients; >> QTAILQ_ENTRY(NBDExport) next; >> >> @@ -972,6 +973,13 @@ NBDExport *nbd_export_new(BlockDriverState *bs, >> off_t dev_offset, exp->ctx = bdrv_get_aio_context(bs); >> bdrv_ref(bs); bdrv_add_aio_context_notifier(bs, bs_aio_attached, >> bs_aio_detach, exp); + + if (bs->open_flags & BDRV_O_INCOMING) { >> + bdrv_invalidate_cache(bs, NULL); + >> exp->restore_incoming = !!(bs->open_flags & BDRV_O_INCOMING); + >> bs->open_flags &= ~(BDRV_O_INCOMING); + } + return exp; } >> >> @@ -1021,6 +1029,9 @@ void nbd_export_close(NBDExport *exp) if >> (exp->bs) { bdrv_remove_aio_context_notifier(exp->bs, >> bs_aio_attached, bs_aio_detach, exp); + if >> (exp->restore_incoming) { + exp->bs->open_flags |= >> BDRV_O_INCOMING; + } bdrv_unref(exp->bs); exp->bs = NULL; } >> > > For this, I don't think you even need exp->restore_incoming, and then > it can simply be a one-liner > > + bdrv_invalidate_cache(bs, NULL); > > if you modify bdrv_invalidate_cache instead of > bdrv_invalidate_cache_all. I did not understand that modification but if I do not restore BDRV_O_INCOMING, then changes to the disk I made on the source side before migration - they are lost after rebooting the destination guest. - -- Alexey -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJULSalAAoJEIYTPdgrwSC5tVcP/RAKlDwP2E3hRpfqK4oR+BnR /OF89+kVvlvXtKSImY1/8oHlPwoKIqA974ZuxYnJNZw3xx2xDmMnT3V3UOVs77Te rRhs87ps/xjk+FXrqRQnuITyoJzOCjIuhkx5cVO66caLyfJaesbPmKgPbThH3EoI FHbwe/XsKjttMGAwd721tDrx/1fwAp5BnpFOMP2ZgqMGkRC3+9+xnxIWqOUvpMTl AVsjWvWO5rRSyj/QE+8RQi+XNPtqfiCYaUHLNy+g23GQjIAjol+zY88sS5f9axJD e4BthhumaALrCfJXf/3p0kszV+oUZ6SSnFcbZnMNe90o5+erDjNEt2i2HGW82sPY 42NP6Tpdg3q01L9zzw7Q+kR8dSy8SQKxeC8Brdi2sfX3KS0JI8mYtYdvWsRjeQ1L OpAYh2eWcqbb9JI1mIE5KWLF/hZPj0epWYNz1VUTB5zmT2VqtmPd+7Xf1mAbh2xN EUWhNQOSrnIxwVcm62SiSy8jYVXfzKIfgmz2Ax/W12Q0zqSxo4896zvaep3PlC+l Ms33JpDPa2qIyWBhZ9ofufV+smqnOgPxC9+Spg4QSlTAL4MHBUGH+fVhml/p4/rn jQo8+0ifbvl9ARv+B0oEERk2Lr1LL7fIcmZDyddQUTswmSK7vTUeKZqpCMN00Ryx 9ms4MHSEolQQJUhrVnX2 =qkLF -----END PGP SIGNATURE-----