Re: [libvirt] [RFC PATCH] qcow2: Fix race in cache invalidation

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 ? */