[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH] block/migration: Disable cache invalidate f
From: |
Alexey Kardashevskiy |
Subject: |
Re: [Qemu-devel] [RFC PATCH] block/migration: Disable cache invalidate for incoming migration |
Date: |
Thu, 02 Oct 2014 20:19:38 +1000 |
User-agent: |
Mozilla/5.0 (X11; Linux i686 on x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.1.2 |
-----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 <address@hidden> ---
>>
>>
>> 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-----