[Qemu-devel] [PATCH v2 0/2] move qcow2_invalidate_cache() out of coroutine context

[Qemu-devel] [PATCH v2 0/2] move qcow2_invalidate_cache() out of coroutine context

[Denis V. Lunev]

There is a possibility to hit an assert in qcow2_get_specific_info that
s->qcow_version is undefined. This happens when VM in starting from
suspended state, i.e. it processes incoming migration, and in the same
time 'info block' is called.

The problem is that qcow2_invalidate_cache() closes the image and
memset()s BDRVQcowState in the middle.

This operation should not be performed in coroutine context.

Changes from v1:
- fixed spelling. Eric, thank you for spell checking

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Amit Shah <amit.shah@redhat.com>

[Qemu-devel] [PATCH 1/2] migration: move bdrv_invalidate_cache_all of of coroutine context

[Denis V. Lunev]

There is a possibility to hit an assert in qcow2_get_specific_info that
s->qcow_version is undefined. This happens when VM in starting from
suspended state, i.e. it processes incoming migration, and in the same
time 'info block' is called.

The problem is that qcow2_invalidate_cache() closes the image and
memset()s BDRVQcowState in the middle.

The patch moves processing of bdrv_invalidate_cache_all out of
coroutine context for standard migration to avoid that.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Amit Shah <amit.shah@redhat.com>
---
 migration/migration.c | 89 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 49 insertions(+), 40 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index a64cfcd..1f8535e 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -323,13 +323,59 @@ void qemu_start_incoming_migration(const char *uri, Error **errp)
     }
 }
 
+static void process_incoming_migration_bh(void *opaque)
+{
+    Error *local_err = NULL;
+    MigrationIncomingState *mis = opaque;
+
+    /* Make sure all file formats flush their mutable metadata */
+    bdrv_invalidate_cache_all(&local_err);
+    if (local_err) {
+        migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
+                          MIGRATION_STATUS_FAILED);
+        error_report_err(local_err);
+        migrate_decompress_threads_join();
+        exit(EXIT_FAILURE);
+    }
+
+    /*
+     * This must happen after all error conditions are dealt with and
+     * we're sure the VM is going to be running on this host.
+     */
+    qemu_announce_self();
+
+    /* If global state section was not received or we are in running
+       state, we need to obey autostart. Any other state is set with
+       runstate_set. */
+
+    if (!global_state_received() ||
+        global_state_get_runstate() == RUN_STATE_RUNNING) {
+        if (autostart) {
+            vm_start();
+        } else {
+            runstate_set(RUN_STATE_PAUSED);
+        }
+    } else {
+        runstate_set(global_state_get_runstate());
+    }
+    migrate_decompress_threads_join();
+    /*
+     * This must happen after any state changes since as soon as an external
+     * observer sees this event they might start to prod at the VM assuming
+     * it's ready to use.
+     */
+    migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
+                      MIGRATION_STATUS_COMPLETED);
+    migration_incoming_state_destroy();
+}
+
 static void process_incoming_migration_co(void *opaque)
 {
     QEMUFile *f = opaque;
-    Error *local_err = NULL;
     MigrationIncomingState *mis;
     PostcopyState ps;
     int ret;
+    QEMUBH *bh;
 
     mis = migration_incoming_state_new(f);
     postcopy_state_set(POSTCOPY_INCOMING_NONE);
@@ -369,45 +415,8 @@ static void process_incoming_migration_co(void *opaque)
         exit(EXIT_FAILURE);
     }
 
-    /* Make sure all file formats flush their mutable metadata */
-    bdrv_invalidate_cache_all(&local_err);
-    if (local_err) {
-        migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
-                          MIGRATION_STATUS_FAILED);
-        error_report_err(local_err);
-        migrate_decompress_threads_join();
-        exit(EXIT_FAILURE);
-    }
-
-    /*
-     * This must happen after all error conditions are dealt with and
-     * we're sure the VM is going to be running on this host.
-     */
-    qemu_announce_self();
-
-    /* If global state section was not received or we are in running
-       state, we need to obey autostart. Any other state is set with
-       runstate_set. */
-
-    if (!global_state_received() ||
-        global_state_get_runstate() == RUN_STATE_RUNNING) {
-        if (autostart) {
-            vm_start();
-        } else {
-            runstate_set(RUN_STATE_PAUSED);
-        }
-    } else {
-        runstate_set(global_state_get_runstate());
-    }
-    migrate_decompress_threads_join();
-    /*
-     * This must happen after any state changes since as soon as an external
-     * observer sees this event they might start to prod at the VM assuming
-     * it's ready to use.
-     */
-    migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
-                      MIGRATION_STATUS_COMPLETED);
-    migration_incoming_state_destroy();
+    bh = qemu_bh_new(process_incoming_migration_bh, mis);
+    qemu_bh_schedule(bh);
 }
 
 void process_incoming_migration(QEMUFile *f)
-- 
2.1.4

Re: [Qemu-devel] [PATCH 1/2] migration: move bdrv_invalidate_cache_all of of coroutine context

[Fam Zheng]

On Fri, 02/12 09:39, Denis V. Lunev wrote:
> There is a possibility to hit an assert in qcow2_get_specific_info that
> s->qcow_version is undefined. This happens when VM in starting from
> suspended state, i.e. it processes incoming migration, and in the same
> time 'info block' is called.
> 
> The problem is that qcow2_invalidate_cache() closes the image and
> memset()s BDRVQcowState in the middle.
> 
> The patch moves processing of bdrv_invalidate_cache_all out of
> coroutine context for standard migration to avoid that.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Amit Shah <amit.shah@redhat.com>
> ---
>  migration/migration.c | 89 ++++++++++++++++++++++++++++-----------------------
>  1 file changed, 49 insertions(+), 40 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index a64cfcd..1f8535e 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -323,13 +323,59 @@ void qemu_start_incoming_migration(const char *uri, Error **errp)
>      }
>  }
>  
> +static void process_incoming_migration_bh(void *opaque)
> +{
> +    Error *local_err = NULL;
> +    MigrationIncomingState *mis = opaque;
> +
> +    /* Make sure all file formats flush their mutable metadata */
> +    bdrv_invalidate_cache_all(&local_err);
> +    if (local_err) {
> +        migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
> +                          MIGRATION_STATUS_FAILED);
> +        error_report_err(local_err);
> +        migrate_decompress_threads_join();
> +        exit(EXIT_FAILURE);
> +    }
> +
> +    /*
> +     * This must happen after all error conditions are dealt with and
> +     * we're sure the VM is going to be running on this host.
> +     */
> +    qemu_announce_self();
> +
> +    /* If global state section was not received or we are in running
> +       state, we need to obey autostart. Any other state is set with
> +       runstate_set. */
> +
> +    if (!global_state_received() ||
> +        global_state_get_runstate() == RUN_STATE_RUNNING) {
> +        if (autostart) {
> +            vm_start();
> +        } else {
> +            runstate_set(RUN_STATE_PAUSED);
> +        }
> +    } else {
> +        runstate_set(global_state_get_runstate());
> +    }
> +    migrate_decompress_threads_join();
> +    /*
> +     * This must happen after any state changes since as soon as an external
> +     * observer sees this event they might start to prod at the VM assuming
> +     * it's ready to use.
> +     */
> +    migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
> +                      MIGRATION_STATUS_COMPLETED);
> +    migration_incoming_state_destroy();
> +}
> +
>  static void process_incoming_migration_co(void *opaque)
>  {
>      QEMUFile *f = opaque;
> -    Error *local_err = NULL;
>      MigrationIncomingState *mis;
>      PostcopyState ps;
>      int ret;
> +    QEMUBH *bh;
>  
>      mis = migration_incoming_state_new(f);
>      postcopy_state_set(POSTCOPY_INCOMING_NONE);
> @@ -369,45 +415,8 @@ static void process_incoming_migration_co(void *opaque)
>          exit(EXIT_FAILURE);
>      }
>  
> -    /* Make sure all file formats flush their mutable metadata */
> -    bdrv_invalidate_cache_all(&local_err);
> -    if (local_err) {
> -        migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
> -                          MIGRATION_STATUS_FAILED);
> -        error_report_err(local_err);
> -        migrate_decompress_threads_join();
> -        exit(EXIT_FAILURE);
> -    }
> -
> -    /*
> -     * This must happen after all error conditions are dealt with and
> -     * we're sure the VM is going to be running on this host.
> -     */
> -    qemu_announce_self();
> -
> -    /* If global state section was not received or we are in running
> -       state, we need to obey autostart. Any other state is set with
> -       runstate_set. */
> -
> -    if (!global_state_received() ||
> -        global_state_get_runstate() == RUN_STATE_RUNNING) {
> -        if (autostart) {
> -            vm_start();
> -        } else {
> -            runstate_set(RUN_STATE_PAUSED);
> -        }
> -    } else {
> -        runstate_set(global_state_get_runstate());
> -    }
> -    migrate_decompress_threads_join();
> -    /*
> -     * This must happen after any state changes since as soon as an external
> -     * observer sees this event they might start to prod at the VM assuming
> -     * it's ready to use.
> -     */
> -    migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
> -                      MIGRATION_STATUS_COMPLETED);
> -    migration_incoming_state_destroy();
> +    bh = qemu_bh_new(process_incoming_migration_bh, mis);

Don't you need to call qemu_bh_delete somewhere? (Also applies to the other
patch)

Fam

> +    qemu_bh_schedule(bh);
>  }
>  
>  void process_incoming_migration(QEMUFile *f)
> -- 
> 2.1.4
> 
> 

[Qemu-devel] [PATCH 2/2] migration: move bdrv_invalidate_cache_all of of coroutine context

[Denis V. Lunev]

There is a possibility to hit an assert in qcow2_get_specific_info that
s->qcow_version is undefined. This happens when VM in starting from
suspended state, i.e. it processes incoming migration, and in the same
time 'info block' is called.

The problem is that qcow2_invalidate_cache() closes the image and
memset()s BDRVQcowState in the middle.

The patch moves processing of bdrv_invalidate_cache_all out of
coroutine context for postcopy migration to avoid that. This function
is called with the following stack:
  process_incoming_migration_co
  qemu_loadvm_state
  qemu_loadvm_state_main
  loadvm_process_command
  loadvm_postcopy_handle_run

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Amit Shah <amit.shah@redhat.com>
---
 migration/savevm.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index 94f2894..8415fd9 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1496,18 +1496,10 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
     return 0;
 }
 
-/* After all discards we can start running and asking for pages */
-static int loadvm_postcopy_handle_run(MigrationIncomingState *mis)
+static void loadvm_postcopy_handle_run_bh(void *opaque)
 {
-    PostcopyState ps = postcopy_state_set(POSTCOPY_INCOMING_RUNNING);
     Error *local_err = NULL;
 
-    trace_loadvm_postcopy_handle_run();
-    if (ps != POSTCOPY_INCOMING_LISTENING) {
-        error_report("CMD_POSTCOPY_RUN in wrong postcopy state (%d)", ps);
-        return -1;
-    }
-
     /* TODO we should move all of this lot into postcopy_ram.c or a shared code
      * in migration.c
      */
@@ -1519,7 +1511,6 @@ static int loadvm_postcopy_handle_run(MigrationIncomingState *mis)
     bdrv_invalidate_cache_all(&local_err);
     if (local_err) {
         error_report_err(local_err);
-        return -1;
     }
 
     trace_loadvm_postcopy_handle_run_cpu_sync();
@@ -1534,6 +1525,22 @@ static int loadvm_postcopy_handle_run(MigrationIncomingState *mis)
         /* leave it paused and let management decide when to start the CPU */
         runstate_set(RUN_STATE_PAUSED);
     }
+}
+
+/* After all discards we can start running and asking for pages */
+static int loadvm_postcopy_handle_run(MigrationIncomingState *mis)
+{
+    PostcopyState ps = postcopy_state_set(POSTCOPY_INCOMING_RUNNING);
+    QEMUBH *bh;
+
+    trace_loadvm_postcopy_handle_run();
+    if (ps != POSTCOPY_INCOMING_LISTENING) {
+        error_report("CMD_POSTCOPY_RUN in wrong postcopy state (%d)", ps);
+        return -1;
+    }
+
+    bh = qemu_bh_new(loadvm_postcopy_handle_run_bh, NULL);
+    qemu_bh_schedule(bh);
 
     /* We need to finish reading the stream from the package
      * and also stop reading anything more from the stream that loaded the
-- 
2.1.4

Re: [Qemu-devel] [PATCH 2/2] migration: move bdrv_invalidate_cache_all of of coroutine context

[Dr. David Alan Gilbert]

* Denis V. Lunev (den@openvz.org) wrote:
> There is a possibility to hit an assert in qcow2_get_specific_info that
> s->qcow_version is undefined. This happens when VM in starting from
> suspended state, i.e. it processes incoming migration, and in the same
> time 'info block' is called.
> 
> The problem is that qcow2_invalidate_cache() closes the image and
> memset()s BDRVQcowState in the middle.
> 
> The patch moves processing of bdrv_invalidate_cache_all out of
> coroutine context for postcopy migration to avoid that. This function
> is called with the following stack:
>   process_incoming_migration_co
>   qemu_loadvm_state
>   qemu_loadvm_state_main
>   loadvm_process_command
>   loadvm_postcopy_handle_run
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Amit Shah <amit.shah@redhat.com>

I've tested my normal postcopy smoke test and it seems to work,
so
Tested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

however, can you add a comment before loadvm_postcopy_handle_run_bh
to explain why it's a bh, otherwise at some point someone might
put it back.

I'll admit to not really understanding what the difference is
between bh and coroutine context; I'd thought if it was all
in the main thread stuff was safe.

Dave

> ---
>  migration/savevm.c | 27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 94f2894..8415fd9 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1496,18 +1496,10 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
>      return 0;
>  }
>  
> -/* After all discards we can start running and asking for pages */
> -static int loadvm_postcopy_handle_run(MigrationIncomingState *mis)
> +static void loadvm_postcopy_handle_run_bh(void *opaque)
>  {
> -    PostcopyState ps = postcopy_state_set(POSTCOPY_INCOMING_RUNNING);
>      Error *local_err = NULL;
>  
> -    trace_loadvm_postcopy_handle_run();
> -    if (ps != POSTCOPY_INCOMING_LISTENING) {
> -        error_report("CMD_POSTCOPY_RUN in wrong postcopy state (%d)", ps);
> -        return -1;
> -    }
> -
>      /* TODO we should move all of this lot into postcopy_ram.c or a shared code
>       * in migration.c
>       */
> @@ -1519,7 +1511,6 @@ static int loadvm_postcopy_handle_run(MigrationIncomingState *mis)
>      bdrv_invalidate_cache_all(&local_err);
>      if (local_err) {
>          error_report_err(local_err);
> -        return -1;
>      }
>  
>      trace_loadvm_postcopy_handle_run_cpu_sync();
> @@ -1534,6 +1525,22 @@ static int loadvm_postcopy_handle_run(MigrationIncomingState *mis)
>          /* leave it paused and let management decide when to start the CPU */
>          runstate_set(RUN_STATE_PAUSED);
>      }
> +}
> +
> +/* After all discards we can start running and asking for pages */
> +static int loadvm_postcopy_handle_run(MigrationIncomingState *mis)
> +{
> +    PostcopyState ps = postcopy_state_set(POSTCOPY_INCOMING_RUNNING);
> +    QEMUBH *bh;
> +
> +    trace_loadvm_postcopy_handle_run();
> +    if (ps != POSTCOPY_INCOMING_LISTENING) {
> +        error_report("CMD_POSTCOPY_RUN in wrong postcopy state (%d)", ps);
> +        return -1;
> +    }
> +
> +    bh = qemu_bh_new(loadvm_postcopy_handle_run_bh, NULL);
> +    qemu_bh_schedule(bh);
>  
>      /* We need to finish reading the stream from the package
>       * and also stop reading anything more from the stream that loaded the
> -- 
> 2.1.4
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/2] migration: move bdrv_invalidate_cache_all of of coroutine context

[Paolo Bonzini]

On 12/02/2016 13:50, Dr. David Alan Gilbert wrote:
> I'll admit to not really understanding what the difference is
> between bh and coroutine context; I'd thought if it was all
> in the main thread stuff was safe.

It's arguably a bug in the block layer code.  It assumes that all code
called from a coroutine wants not to block.  Moving stuff to a bottom
half tells the block layer that you want bdrv_invalidate_cache_all to block.

Paolo

Re: [Qemu-devel] [PATCH 2/2] migration: move bdrv_invalidate_cache_all of of coroutine context

[Denis V. Lunev]

On 02/12/2016 03:55 PM, Paolo Bonzini wrote:
>
> On 12/02/2016 13:50, Dr. David Alan Gilbert wrote:
>> I'll admit to not really understanding what the difference is
>> between bh and coroutine context; I'd thought if it was all
>> in the main thread stuff was safe.
> It's arguably a bug in the block layer code.  It assumes that all code
> called from a coroutine wants not to block.  Moving stuff to a bottom
> half tells the block layer that you want bdrv_invalidate_cache_all to block.
>
> Paolo
cool description! Really short and exhaustive!

Re: [Qemu-devel] [PATCH v2 0/2] move qcow2_invalidate_cache() out of coroutine context

[Denis V. Lunev]

On 02/12/2016 09:39 AM, Denis V. Lunev wrote:
> There is a possibility to hit an assert in qcow2_get_specific_info that
> s->qcow_version is undefined. This happens when VM in starting from
> suspended state, i.e. it processes incoming migration, and in the same
> time 'info block' is called.
>
> The problem is that qcow2_invalidate_cache() closes the image and
> memset()s BDRVQcowState in the middle.
>
> This operation should not be performed in coroutine context.
>
> Changes from v1:
> - fixed spelling. Eric, thank you for spell checking
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Amit Shah <amit.shah@redhat.com>
>
ping

Re: [Qemu-devel] [PATCH v2 0/2] move qcow2_invalidate_cache() out of coroutine context

[Amit Shah]

Can someone from the block team please give this a review?

Thanks,

On (Fri) 12 Feb 2016 [09:39:32], Denis V. Lunev wrote:
> There is a possibility to hit an assert in qcow2_get_specific_info that
> s->qcow_version is undefined. This happens when VM in starting from
> suspended state, i.e. it processes incoming migration, and in the same
> time 'info block' is called.
> 
> The problem is that qcow2_invalidate_cache() closes the image and
> memset()s BDRVQcowState in the middle.
> 
> This operation should not be performed in coroutine context.
> 
> Changes from v1:
> - fixed spelling. Eric, thank you for spell checking
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Amit Shah <amit.shah@redhat.com>
> 

		Amit

Re: [Qemu-devel] [PATCH v2 0/2] move qcow2_invalidate_cache() out of coroutine context

[Denis V. Lunev]

On 02/18/2016 08:17 AM, Amit Shah wrote:
> Can someone from the block team please give this a review?
>
> Thanks,
>
> On (Fri) 12 Feb 2016 [09:39:32], Denis V. Lunev wrote:
>> There is a possibility to hit an assert in qcow2_get_specific_info that
>> s->qcow_version is undefined. This happens when VM in starting from
>> suspended state, i.e. it processes incoming migration, and in the same
>> time 'info block' is called.
>>
>> The problem is that qcow2_invalidate_cache() closes the image and
>> memset()s BDRVQcowState in the middle.
>>
>> This operation should not be performed in coroutine context.
>>
>> Changes from v1:
>> - fixed spelling. Eric, thank you for spell checking
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Paolo Bonzini <pbonzini@redhat.com>
>> CC: Juan Quintela <quintela@redhat.com>
>> CC: Amit Shah <amit.shah@redhat.com>
>>
> 		Amit
Paolo, can you pls bless this? I think that the fix comes from the
infrastructure layer and via your proposal.

Den

Re: [Qemu-devel] [PATCH v2 0/2] move qcow2_invalidate_cache() out of coroutine context

[Paolo Bonzini]

On 18/02/2016 09:03, Denis V. Lunev wrote:
> On 02/18/2016 08:17 AM, Amit Shah wrote:
>> Can someone from the block team please give this a review?
>
> Paolo, can you pls bless this? I think that the fix comes from the
> infrastructure layer and via your proposal.

Yes, I think it's fine from the block layer POV.  I proposed Denis this
fix after discussing the bug with Kevin.

Thanks Denis and Amit.

Paolo

Re: [Qemu-devel] [PATCH v2 0/2] move qcow2_invalidate_cache() out of coroutine context

[Amit Shah]

On (Thu) 18 Feb 2016 [15:54:28], Paolo Bonzini wrote:
> 
> 
> On 18/02/2016 09:03, Denis V. Lunev wrote:
> > On 02/18/2016 08:17 AM, Amit Shah wrote:
> >> Can someone from the block team please give this a review?
> >
> > Paolo, can you pls bless this? I think that the fix comes from the
> > infrastructure layer and via your proposal.
> 
> Yes, I think it's fine from the block layer POV.  I proposed Denis this
> fix after discussing the bug with Kevin.

OK - I'll pick it up for my next pull req.

Thanks,

		Amit

Re: [Qemu-devel] [PATCH v2 0/2] move qcow2_invalidate_cache() out of coroutine context

[Denis V. Lunev]

On 02/18/2016 08:17 AM, Amit Shah wrote:
> Can someone from the block team please give this a review?
>
> Thanks,
guys?


> On (Fri) 12 Feb 2016 [09:39:32], Denis V. Lunev wrote:
>> There is a possibility to hit an assert in qcow2_get_specific_info that
>> s->qcow_version is undefined. This happens when VM in starting from
>> suspended state, i.e. it processes incoming migration, and in the same
>> time 'info block' is called.
>>
>> The problem is that qcow2_invalidate_cache() closes the image and
>> memset()s BDRVQcowState in the middle.
>>
>> This operation should not be performed in coroutine context.
>>
>> Changes from v1:
>> - fixed spelling. Eric, thank you for spell checking
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Paolo Bonzini <pbonzini@redhat.com>
>> CC: Juan Quintela <quintela@redhat.com>
>> CC: Amit Shah <amit.shah@redhat.com>
>>
> 		Amit

Re: [Qemu-devel] [PATCH v2 0/2] move qcow2_invalidate_cache() out of coroutine context

[Amit Shah]

On (Fri) 12 Feb 2016 [09:39:32], Denis V. Lunev wrote:
> There is a possibility to hit an assert in qcow2_get_specific_info that
> s->qcow_version is undefined. This happens when VM in starting from
> suspended state, i.e. it processes incoming migration, and in the same
> time 'info block' is called.
> 
> The problem is that qcow2_invalidate_cache() closes the image and
> memset()s BDRVQcowState in the middle.
> 
> This operation should not be performed in coroutine context.
> 
> Changes from v1:
> - fixed spelling. Eric, thank you for spell checking

BTW I noticed this just now: the commit subject line for both the
patches is the same.  Please avoid that in the future.

		Amit

Re: [Qemu-devel] [PATCH v2 0/2] move qcow2_invalidate_cache() out of coroutine context

[Amit Shah]

Hi Denis,

On (Tue) 23 Feb 2016 [13:09:26], Amit Shah wrote:
> On (Fri) 12 Feb 2016 [09:39:32], Denis V. Lunev wrote:
> > There is a possibility to hit an assert in qcow2_get_specific_info that
> > s->qcow_version is undefined. This happens when VM in starting from
> > suspended state, i.e. it processes incoming migration, and in the same
> > time 'info block' is called.
> > 
> > The problem is that qcow2_invalidate_cache() closes the image and
> > memset()s BDRVQcowState in the middle.
> > 
> > This operation should not be performed in coroutine context.
> > 
> > Changes from v1:
> > - fixed spelling. Eric, thank you for spell checking
> 
> BTW I noticed this just now: the commit subject line for both the
> patches is the same.  Please avoid that in the future.

The pull req failed to apply due to another patchset - could you
please do a v3 with better subject lines for the patches?

Thanks,

		Amit

Re: [Qemu-devel] [PATCH v2 0/2] move qcow2_invalidate_cache() out of coroutine context

[Denis V. Lunev]

On 02/23/2016 01:47 PM, Amit Shah wrote:
> Hi Denis,
>
> On (Tue) 23 Feb 2016 [13:09:26], Amit Shah wrote:
>> On (Fri) 12 Feb 2016 [09:39:32], Denis V. Lunev wrote:
>>> There is a possibility to hit an assert in qcow2_get_specific_info that
>>> s->qcow_version is undefined. This happens when VM in starting from
>>> suspended state, i.e. it processes incoming migration, and in the same
>>> time 'info block' is called.
>>>
>>> The problem is that qcow2_invalidate_cache() closes the image and
>>> memset()s BDRVQcowState in the middle.
>>>
>>> This operation should not be performed in coroutine context.
>>>
>>> Changes from v1:
>>> - fixed spelling. Eric, thank you for spell checking
>> BTW I noticed this just now: the commit subject line for both the
>> patches is the same.  Please avoid that in the future.
> The pull req failed to apply due to another patchset - could you
> please do a v3 with better subject lines for the patches?
>
> Thanks,
>
> 		Amit
no prob, I'll send new patch later today.