Re: [Qemu-devel] [RFC PATCH] block/migration: Disable cache invalidate for incoming migration

[Alexey Kardashevskiy]

-----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-----