3:47 a.m.
On 09/19/2014 06:47 PM, Kevin Wolf wrote:> Am 16.09.2014 um 14:59 hat Paolo Bonzini
geschrieben:
> Il 16/09/2014 14:52, Kevin Wolf ha scritto:
>> Yes, that's true. We can't fix this problem in qcow2, though, because
>> it's a more general one. I think we must make sure that
>> bdrv_invalidate_cache() doesn't yield.
>>
>> Either by forbidding to run bdrv_invalidate_cache() in a coroutine and
>> moving the problem to the caller (where and why is it even called from a
>> coroutine?), or possibly by creating a new coroutine for the driver
>> callback and running that in a nested event loop that only handles
>> bdrv_invalidate_cache() callbacks, so that the NBD server doesn't get a
>> chance to process new requests in this thread.
>
> Incoming migration runs in a coroutine (the coroutine entry point is
> process_incoming_migration_co). But everything after qemu_fclose() can
> probably be moved into a separate bottom half, so that it gets out of
> coroutine context.
Alexey, you should probably rather try this (and add a bdrv_drain_all()
in bdrv_invalidate_cache) than messing around with qcow2 locks. This
isn't a problem that can be completely fixed in qcow2.
Ok. Tried :) Not very successful though. The patch is below.
Is that the correct bottom half? When I did it, I started getting crashes
in various sport on accesses to s->l1_cache which is NULL after qcow2_close.
Normally the code would check s->l1_size and then use but they are out of sync.
So I clear it in qcow2_close(). This allowed migrated guest to work and not
to crash until I shut it down when it aborted at "HERE IT FAILS ON SHUTDOWN".
Here I realized I am missing something in this picture again, what is it?
Thanks!
---
block.c | 2 ++
block/qcow2-cache.c | 2 +-
block/qcow2.c | 50 ++++++++++++++++++++++++++++++++++++--------------
block/qcow2.h | 4 ++++
4 files changed, 43 insertions(+), 15 deletions(-)
diff --git a/block.c b/block.c
index d06dd51..1e6dfd1 100644
--- a/block.c
+++ b/block.c
@@ -5044,6 +5044,8 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
error_setg_errno(errp, -ret, "Could not refresh total sector count");
return;
}
+
+ bdrv_drain_all();
}
void bdrv_invalidate_cache_all(Error **errp)
diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
index 904f6b1..59ff48c 100644
--- a/block/qcow2-cache.c
+++ b/block/qcow2-cache.c
@@ -259,7 +259,7 @@ static int qcow2_cache_find_entry_to_replace(Qcow2Cache *c)
if (min_index == -1) {
/* This can't happen in current synchronous code, but leave the check
* here as a reminder for whoever starts using AIO with the cache */
- abort();
+ abort(); // <==== HERE IT FAILS ON SHUTDOWN
}
return min_index;
}
diff --git a/block/qcow2.c b/block/qcow2.c
index f9e045f..2b84562 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1399,6 +1399,7 @@ static void qcow2_close(BlockDriverState *bs)
qemu_vfree(s->l1_table);
/* else pre-write overlap checks in cache_destroy may crash */
s->l1_table = NULL;
+ s->l1_size = 0;
if (!(bs->open_flags & BDRV_O_INCOMING)) {
qcow2_cache_flush(bs, s->l2_table_cache);
@@ -1419,16 +1420,11 @@ static void qcow2_close(BlockDriverState *bs)
qcow2_free_snapshots(bs);
}
+static void qcow2_invalidate_cache_bh_cb(void *opaque);
+
static void qcow2_invalidate_cache(BlockDriverState *bs, Error **errp)
{
BDRVQcowState *s = bs->opaque;
- int flags = s->flags;
- AES_KEY aes_encrypt_key;
- AES_KEY aes_decrypt_key;
- uint32_t crypt_method = 0;
- QDict *options;
- Error *local_err = NULL;
- int ret;
/*
* Backing files are read-only which makes all of their metadata immutable,
@@ -1436,13 +1432,28 @@ static void qcow2_invalidate_cache(BlockDriverState *bs, Error
**errp)
*/
if (s->crypt_method) {
- crypt_method = s->crypt_method;
- memcpy(&aes_encrypt_key, &s->aes_encrypt_key,
sizeof(aes_encrypt_key));
- memcpy(&aes_decrypt_key, &s->aes_decrypt_key,
sizeof(aes_decrypt_key));
+ s->bh_crypt_method = s->crypt_method;
+ memcpy(&s->bh_aes_encrypt_key, &s->aes_encrypt_key,
sizeof(s->bh_aes_encrypt_key));
+ memcpy(&s->bh_aes_decrypt_key, &s->aes_decrypt_key,
sizeof(s->bh_aes_decrypt_key));
+ } else {
+ s->bh_crypt_method = 0;
}
qcow2_close(bs);
+ s->cache_inv_bh = aio_bh_new(bdrv_get_aio_context(bs),
+ qcow2_invalidate_cache_bh_cb,
+ bs);
+}
+
+static void qcow2_invalidate_cache_bh(BlockDriverState *bs, Error **errp)
+{
+ BDRVQcowState *s = bs->opaque;
+ int flags = s->flags;
+ QDict *options;
+ Error *local_err = NULL;
+ int ret;
+
bdrv_invalidate_cache(bs->file, &local_err);
if (local_err) {
error_propagate(errp, local_err);
@@ -1464,11 +1475,22 @@ static void qcow2_invalidate_cache(BlockDriverState *bs, Error
**errp)
return;
}
- if (crypt_method) {
- s->crypt_method = crypt_method;
- memcpy(&s->aes_encrypt_key, &aes_encrypt_key,
sizeof(aes_encrypt_key));
- memcpy(&s->aes_decrypt_key, &aes_decrypt_key,
sizeof(aes_decrypt_key));
+ if (s->bh_crypt_method) {
+ s->crypt_method = s->bh_crypt_method;
+ memcpy(&s->aes_encrypt_key, &s->bh_aes_encrypt_key,
sizeof(s->bh_aes_encrypt_key));
+ memcpy(&s->aes_decrypt_key, &s->bh_aes_decrypt_key,
sizeof(s->bh_aes_decrypt_key));
}
+
+ qemu_bh_delete(s->cache_inv_bh);
+ s->cache_inv_bh = NULL;
+}
+
+static void qcow2_invalidate_cache_bh_cb(void *opaque)
+{
+ BlockDriverState *bs = opaque;
+ Error *local_err = NULL;
+
+ qcow2_invalidate_cache_bh(bs, &local_err);
}
static size_t header_ext_add(char *buf, uint32_t magic, const void *s,
diff --git a/block/qcow2.h b/block/qcow2.h
index 6aeb7ea..58d1859 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -271,6 +271,10 @@ typedef struct BDRVQcowState {
QLIST_HEAD(, Qcow2UnknownHeaderExtension) unknown_header_ext;
QTAILQ_HEAD (, Qcow2DiscardRegion) discards;
bool cache_discards;
+ QEMUBH *cache_inv_bh;
+ AES_KEY bh_aes_encrypt_key;
+ AES_KEY bh_aes_decrypt_key;
+ uint32_t bh_crypt_method;
} BDRVQcowState;
/* XXX: use std qcow open function ? */
--
2.0.0