[PATCH v1 0/2] Migration: fix missing iothread locking

[PATCH v1 0/2] Migration: fix missing iothread locking

[Emanuele Giuseppe Esposito]

Some functions (in this case qemu_savevm_state_complete_postcopy() and
init_dirty_bitmap_migration()) assume and document that
qemu_mutex_lock_iothread() is hold.

This seems to have been forgotten in some places, and this series
aims to fix that.

Patch 1 was part of my RFC block layer series "block layer: split
block APIs in graph and I/O" but I decided to do a separate series
for these two bugs, as they are independent from the API split.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

Emanuele Giuseppe Esposito (2):
  migration: block-dirty-bitmap: add missing qemu_mutex_lock_iothread
  migration: add missing qemu_mutex_lock_iothread in
    migration_completion

 migration/block-dirty-bitmap.c | 5 ++++-
 migration/migration.c          | 3 +++
 2 files changed, 7 insertions(+), 1 deletion(-)

-- 
2.27.0

[PATCH v1 1/2] migration: block-dirty-bitmap: add missing qemu_mutex_lock_iothread

[Emanuele Giuseppe Esposito]

init_dirty_bitmap_migration assumes the iothread lock (BQL)
to be held, but instead it isn't.

Instead of adding the lock to qemu_savevm_state_setup(),
follow the same pattern as the other ->save_setup callbacks
and lock+unlock inside dirty_bitmap_save_setup().

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 migration/block-dirty-bitmap.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 35f5ef688d..9aba7d9c22 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -1215,7 +1215,10 @@ static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque)
 {
     DBMSaveState *s = &((DBMState *)opaque)->save;
     SaveBitmapState *dbms = NULL;
+
+    qemu_mutex_lock_iothread();
     if (init_dirty_bitmap_migration(s) < 0) {
+        qemu_mutex_unlock_iothread();
         return -1;
     }
 
@@ -1223,7 +1226,7 @@ static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque)
         send_bitmap_start(f, s, dbms);
     }
     qemu_put_bitmap_flags(f, DIRTY_BITMAP_MIG_FLAG_EOS);
-
+    qemu_mutex_unlock_iothread();
     return 0;
 }
 
-- 
2.27.0

Re: [PATCH v1 1/2] migration: block-dirty-bitmap: add missing qemu_mutex_lock_iothread

[Juan Quintela]

Emanuele Giuseppe Esposito <eesposit@redhat.com> wrote:
> init_dirty_bitmap_migration assumes the iothread lock (BQL)
> to be held, but instead it isn't.
>
> Instead of adding the lock to qemu_savevm_state_setup(),
> follow the same pattern as the other ->save_setup callbacks
> and lock+unlock inside dirty_bitmap_save_setup().
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

queued.

Re: [PATCH v1 1/2] migration: block-dirty-bitmap: add missing qemu_mutex_lock_iothread

[Juan Quintela]

Juan Quintela <quintela@redhat.com> wrote:
> Emanuele Giuseppe Esposito <eesposit@redhat.com> wrote:
>> init_dirty_bitmap_migration assumes the iothread lock (BQL)
>> to be held, but instead it isn't.
>>
>> Instead of adding the lock to qemu_savevm_state_setup(),
>> follow the same pattern as the other ->save_setup callbacks
>> and lock+unlock inside dirty_bitmap_save_setup().
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>
> Reviewed-by: Juan Quintela <quintela@redhat.com>

And I realized that this patch is already on the tree, just not through
the migration tree.

Ignore the noise.

Later, Juan.

[PATCH v1 2/2] migration: add missing qemu_mutex_lock_iothread in migration_completion

[Emanuele Giuseppe Esposito]

qemu_savevm_state_complete_postcopy assumes the iothread lock (BQL)
to be held, but instead it isn't.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 migration/migration.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index 041b8451a6..215d5281f2 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3182,7 +3182,10 @@ static void migration_completion(MigrationState *s)
     } else if (s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE) {
         trace_migration_completion_postcopy_end();
 
+        qemu_mutex_lock_iothread();
         qemu_savevm_state_complete_postcopy(s->to_dst_file);
+        qemu_mutex_unlock_iothread();
+
         trace_migration_completion_postcopy_end_after_complete();
     } else if (s->state == MIGRATION_STATUS_CANCELLING) {
         goto fail;
-- 
2.27.0

Re: [PATCH v1 2/2] migration: add missing qemu_mutex_lock_iothread in migration_completion

[Dr. David Alan Gilbert]

* Emanuele Giuseppe Esposito (eesposit@redhat.com) wrote:
> qemu_savevm_state_complete_postcopy assumes the iothread lock (BQL)
> to be held, but instead it isn't.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

Interesting, I think you're right - and I think it's been missing it
from the start.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/migration.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 041b8451a6..215d5281f2 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -3182,7 +3182,10 @@ static void migration_completion(MigrationState *s)
>      } else if (s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE) {
>          trace_migration_completion_postcopy_end();
>  
> +        qemu_mutex_lock_iothread();
>          qemu_savevm_state_complete_postcopy(s->to_dst_file);
> +        qemu_mutex_unlock_iothread();
> +
>          trace_migration_completion_postcopy_end_after_complete();
>      } else if (s->state == MIGRATION_STATUS_CANCELLING) {
>          goto fail;
> -- 
> 2.27.0
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK