Re: [Qemu-devel] [RFC PATCH] qcow2: Fix race in cache invalidation

[Alexey Kardashevskiy]

On 09/23/2014 06:47 PM, Alexey Kardashevskiy wrote:
> 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!

To be more precise, I can remove that abort() and it seems working for a
while but when shutting migrated guest down, the disk fails:

Will now unmount local filesystems:sd 0:0:0:0: [sda]
Result: hostbyte=0x00 driverbyte=0x08
sd 0:0:0:0: [sda]
Sense Key : 0xb [current]
sd 0:0:0:0: [sda]
ASC=0x0 ASCQ=0x6
sd 0:0:0:0: [sda] CDB:
cdb[0]=0x2a: 2a 00 00 3c 10 10 00 00 08 00
end_request: I/O error, dev sda, sector 3936272
end_request: I/O error, dev sda, sector 3936272
Buffer I/O error on device sda, logical block 492034
lost page write due to I/O error on sda
JBD2: Error -5 detected when updating journal superblock for sda-8.
[...]

spapr-vscsi or virtio-scsi - does not matter. Or crash:

Program received signal SIGSEGV, Segmentation fault.
0x000000001050a69c in qcow2_cache_find_entry_to_replace (c=0x10038317bb0)
at /home/alexey/p/qemu/block/qcow2-cache.c:256
(gdb) l
251                 min_count = c->entries[i].cache_hits;
(gdb) p i
$2 = 0xfd6
(gdb) p c->size
$3 = 0x3ffe
(gdb) p c->entries[i]
$5 = {
  table = 0x804dd70210,
  offset = 0x40,
  dirty = 0x0,
  cache_hits = 0xee498,
  ref = 0x0
}

Weird things are happening, that's my point :)


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


-- 
Alexey