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

[Alexey Kardashevskiy]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 10/06/2014 09:03 PM, Stefan Hajnoczi wrote:
> On Fri, Oct 03, 2014 at 02:12:06PM +1000, Alexey Kardashevskiy wrote:
>> BDRV_O_INCOMING is only set when QEMU is about to receive migration
>> and we do not want QEMU to check the file at opening time as there
>> is likely garbage. Is there any other use of BDRV_O_INCOMING? There
>> must be some as bdrv_clear_incoming_migration_all() is called at the
>> end of live migration and I believe there must be a reason for it
>> (cache does not migrate, does it?).
> 
> BDRV_O_INCOMING is just for live migration.  The cached data is not 
> migrated, this is why it must be refreshed upon migration handover.
> 
>> bdrv_invalidate_cache() flushes cache as it could be already
>> initialized even if QEMU is receiving migration - QEMU could have
>> cached some of real disk data. Is that correct? I do not really
>> understand why it would happen if there is BDRV_O_INCOMING set but
>> ok.
> 
> .bdrv_open() can load metadata from image files (such as the qcow2 L1 
> tables) and it does this even when BDRV_O_INCOMING is set.  That data 
> needs to be re-read at migration handover to prevent the destination 
> QEMU from running with stale image metadata.


Would not it be easier/more correct if bdrv_open would not load this
metadata if BDRV_O_INCOMING is set?


> 
>> diff --git a/nbd.c b/nbd.c index e9b539b..953c378 100644 - ---
>> a/nbd.c +++ b/nbd.c @@ -972,6 +972,7 @@ 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); +    bdrv_invalidate_cache(bs, NULL); return exp; }
> 
> Please add a comment to explain why this call is necessary:
> 
> /* NBD exports are used for non-shared storage migration.  Make sure *
> that BDRV_O_INCOMING is cleared and the image is ready for write *
> access since the export could be available before migration handover. 
> */
> 
> If someone can come up with a 2- or 3-line comment that explains it 
> better, great.
> 
> The rest of the patch looks like it will work.  I'm not thrilled
> about putting BDRV_O_INCOMING logical inside bdrv_invalidate_cache()
> because there was no coupling there before, but the code seems correct
> now.


Ok, will repost today.



- -- 
Alexey
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBAgAGBQJUMxvkAAoJEIYTPdgrwSC5+GMP/1sDhIJf3BlGb7FFqi7CeRDk
X65p61AKc0BKjchFSGsl62DctcIUFPUN+gO8bGYlfihEdBu7n0pinG0iovg+Dhk3
auExumJGQony7D5K0/7BnR5Ciyvakk8lYs3qUaSE7PD4FrVDtnl8x7RTwP3qet3Q
P4TO9yTVCoqnMBSj3ZZzcirwty8+MpFNWD9vhzBsyC3uVkXG/3pPr2LJWWogzosz
ewmZQPQ5ydFncpBvT8gZhD+eseRVo6y0iTEac7TGmhDGCSWtyNcZng695lmzbUpB
+lIQ5kqXOgGbcn8zgJ2VUwuBy7/sI0TsSIxYO69qwdgOqahSCrd7KgN0t2BoRVtH
SOdxKxZhI3ULaNKAvRax93yq+gL8WZSvVmhpfEVh1EA7mZ2TeGMwZ3OsQzvGwi5/
RjsDTruiWg7FWoU6cI2VuPskNkehr0CXTMsheWoR8xvj+WVGz35quropSp8dw1qy
NnJ8RmL5sQbtmh3Y8njdP+kwRUilqJAPcrpH6p4a+2cnXH4b3By4gyt0UROl7afs
h8Pf/86HFa92kbMZAFs5ajhBj3Dg+SLHdAElzrRuz25/MVREAslaM8Us1q53xx/g
P8neTnQIZfbcaH0b4JLxWPN2JPOyYXDV4IaOLEWyoAG0m2ks085+AB1L0o56hn0/
6RVXGOU8iJKtQ6KGiDy/
=pB8l
-----END PGP SIGNATURE-----