[PATCH 1/3] migration/colo: Minor fix for colo error message

[PATCH 1/3] migration/colo: Minor fix for colo error message

[Li Zhijian via]

- Explicitly show the missing module name: replication
- Fix capability name to x-colo

Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
 migration/migration.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 6502e169a3..b4a09c561c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -517,13 +517,13 @@ void migration_incoming_disable_colo(void)
 int migration_incoming_enable_colo(void)
 {
 #ifndef CONFIG_REPLICATION
-    error_report("ENABLE_COLO command come in migration stream, but COLO "
-                 "module is not built in");
+    error_report("ENABLE_COLO command come in migration stream, but the "
+                 "replication module is not built in");
     return -ENOTSUP;
 #endif
 
     if (!migrate_colo()) {
-        error_report("ENABLE_COLO command come in migration stream, but c-colo "
+        error_report("ENABLE_COLO command come in migration stream, but x-colo "
                      "capability is not set");
         return -EINVAL;
     }
-- 
2.31.1

[PATCH 2/3] migration/colo: make colo_incoming_co() return void

[Li Zhijian via]

Currently, it always returns 0, no need to check the return value at all.
In addition, enter colo coroutine only if migration_incoming_colo_enabled()
is true.
Once the destination side enters the COLO* state, the COLO process will
take over the remaining processes until COLO exits.

Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
 migration/colo.c      | 9 ++-------
 migration/migration.c | 6 +++---
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/migration/colo.c b/migration/colo.c
index 5600a43d78..991806c06a 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -929,16 +929,13 @@ out:
     return NULL;
 }
 
-int coroutine_fn colo_incoming_co(void)
+void coroutine_fn colo_incoming_co(void)
 {
     MigrationIncomingState *mis = migration_incoming_get_current();
     QemuThread th;
 
     assert(bql_locked());
-
-    if (!migration_incoming_colo_enabled()) {
-        return 0;
-    }
+    assert(migration_incoming_colo_enabled());
 
     qemu_thread_create(&th, "COLO incoming", colo_process_incoming_thread,
                        mis, QEMU_THREAD_JOINABLE);
@@ -954,6 +951,4 @@ int coroutine_fn colo_incoming_co(void)
 
     /* We hold the global BQL, so it is safe here */
     colo_release_ram_cache();
-
-    return 0;
 }
diff --git a/migration/migration.c b/migration/migration.c
index b4a09c561c..6dc1f3bab4 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -789,9 +789,9 @@ process_incoming_migration_co(void *opaque)
         goto fail;
     }
 
-    if (colo_incoming_co() < 0) {
-        error_setg(&local_err, "colo incoming failed");
-        goto fail;
+    if (migration_incoming_colo_enabled()) {
+        /* yield until COLO exit */
+        colo_incoming_co();
     }
 
     migration_bh_schedule(process_incoming_migration_bh, mis);
-- 
2.31.1

[PATCH 3/3] migration/colo: Tidy up bql_unlock() around bdrv_activate_all()

[Li Zhijian via]

Make the code more tight.

Cc: Michael Tokarev <mjt@tls.msk.ru>
Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
This change/comment suggested by "Michael Tokarev <mjt@tls.msk.ru>" came
a bit late at that time, let's update it together in these minor set
this time.
---
 migration/colo.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/migration/colo.c b/migration/colo.c
index 991806c06a..1b6d9da1c8 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -838,12 +838,11 @@ static void *colo_process_incoming_thread(void *opaque)
     /* Make sure all file formats throw away their mutable metadata */
     bql_lock();
     bdrv_activate_all(&local_err);
+    bql_unlock();
     if (local_err) {
-        bql_unlock();
         error_report_err(local_err);
         return NULL;
     }
-    bql_unlock();
 
     failover_init_state();
 
-- 
2.31.1

Re: [PATCH 1/3] migration/colo: Minor fix for colo error message

[Peter Xu]

On Thu, May 09, 2024 at 11:31:04AM +0800, Li Zhijian wrote:
> - Explicitly show the missing module name: replication
> - Fix capability name to x-colo
> 
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH 2/3] migration/colo: make colo_incoming_co() return void

[Peter Xu]

On Thu, May 09, 2024 at 11:31:05AM +0800, Li Zhijian via wrote:
> Currently, it always returns 0, no need to check the return value at all.
> In addition, enter colo coroutine only if migration_incoming_colo_enabled()
> is true.
> Once the destination side enters the COLO* state, the COLO process will
> take over the remaining processes until COLO exits.
> 
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH 3/3] migration/colo: Tidy up bql_unlock() around bdrv_activate_all()

[Peter Xu]

On Thu, May 09, 2024 at 11:31:06AM +0800, Li Zhijian via wrote:
> Make the code more tight.
> 
> Cc: Michael Tokarev <mjt@tls.msk.ru>
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

> ---
> This change/comment suggested by "Michael Tokarev <mjt@tls.msk.ru>" came
> a bit late at that time, let's update it together in these minor set
> this time.

You can use a tag next time:

Suggested-by: Michael Tokarev <mjt@tls.msk.ru>

> ---
>  migration/colo.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/migration/colo.c b/migration/colo.c
> index 991806c06a..1b6d9da1c8 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -838,12 +838,11 @@ static void *colo_process_incoming_thread(void *opaque)
>      /* Make sure all file formats throw away their mutable metadata */
>      bql_lock();
>      bdrv_activate_all(&local_err);
> +    bql_unlock();
>      if (local_err) {
> -        bql_unlock();
>          error_report_err(local_err);
>          return NULL;
>      }
> -    bql_unlock();
>  
>      failover_init_state();
>  
> -- 
> 2.31.1
> 
> 

-- 
Peter Xu

RE: [PATCH 2/3] migration/colo: make colo_incoming_co() return void

[Zhang, Chen]

> -----Original Message-----
> From: Li Zhijian <lizhijian@fujitsu.com>
> Sent: Thursday, May 9, 2024 11:31 AM
> To: Peter Xu <peterx@redhat.com>; Fabiano Rosas <farosas@suse.de>
> Cc: Zhang, Hailiang <zhanghailiang@xfusion.com>; qemu-
> devel@nongnu.org; Zhang, Chen <chen.zhang@intel.com>; Li Zhijian
> <lizhijian@fujitsu.com>
> Subject: [PATCH 2/3] migration/colo: make colo_incoming_co() return void
> 
> Currently, it always returns 0, no need to check the return value at all.
> In addition, enter colo coroutine only if migration_incoming_colo_enabled() is
> true.
> Once the destination side enters the COLO* state, the COLO process will take
> over the remaining processes until COLO exits.
> 
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>

Reviewed-by: Zhang Chen <chen.zhang@intel.com>

> ---
>  migration/colo.c      | 9 ++-------
>  migration/migration.c | 6 +++---
>  2 files changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/migration/colo.c b/migration/colo.c index 5600a43d78..991806c06a
> 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -929,16 +929,13 @@ out:
>      return NULL;
>  }
> 
> -int coroutine_fn colo_incoming_co(void)
> +void coroutine_fn colo_incoming_co(void)
>  {
>      MigrationIncomingState *mis = migration_incoming_get_current();
>      QemuThread th;
> 
>      assert(bql_locked());
> -
> -    if (!migration_incoming_colo_enabled()) {
> -        return 0;
> -    }
> +    assert(migration_incoming_colo_enabled());
> 
>      qemu_thread_create(&th, "COLO incoming",
> colo_process_incoming_thread,
>                         mis, QEMU_THREAD_JOINABLE); @@ -954,6 +951,4 @@ int
> coroutine_fn colo_incoming_co(void)
> 
>      /* We hold the global BQL, so it is safe here */
>      colo_release_ram_cache();
> -
> -    return 0;
>  }
> diff --git a/migration/migration.c b/migration/migration.c index
> b4a09c561c..6dc1f3bab4 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -789,9 +789,9 @@ process_incoming_migration_co(void *opaque)
>          goto fail;
>      }
> 
> -    if (colo_incoming_co() < 0) {
> -        error_setg(&local_err, "colo incoming failed");
> -        goto fail;
> +    if (migration_incoming_colo_enabled()) {
> +        /* yield until COLO exit */
> +        colo_incoming_co();
>      }
> 
>      migration_bh_schedule(process_incoming_migration_bh, mis);
> --
> 2.31.1

RE: [PATCH 1/3] migration/colo: Minor fix for colo error message

[Zhang, Chen]

> -----Original Message-----
> From: Li Zhijian <lizhijian@fujitsu.com>
> Sent: Thursday, May 9, 2024 11:31 AM
> To: Peter Xu <peterx@redhat.com>; Fabiano Rosas <farosas@suse.de>
> Cc: Zhang, Hailiang <zhanghailiang@xfusion.com>; qemu-
> devel@nongnu.org; Zhang, Chen <chen.zhang@intel.com>; Li Zhijian
> <lizhijian@fujitsu.com>
> Subject: [PATCH 1/3] migration/colo: Minor fix for colo error message
> 
> - Explicitly show the missing module name: replication
> - Fix capability name to x-colo
> 
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>

Reviewed-by: Zhang Chen <chen.zhang@intel.com>

> ---
>  migration/migration.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c index
> 6502e169a3..b4a09c561c 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -517,13 +517,13 @@ void migration_incoming_disable_colo(void)
>  int migration_incoming_enable_colo(void)
>  {
>  #ifndef CONFIG_REPLICATION
> -    error_report("ENABLE_COLO command come in migration stream, but
> COLO "
> -                 "module is not built in");
> +    error_report("ENABLE_COLO command come in migration stream, but the
> "
> +                 "replication module is not built in");
>      return -ENOTSUP;
>  #endif
> 
>      if (!migrate_colo()) {
> -        error_report("ENABLE_COLO command come in migration stream, but c-
> colo "
> +        error_report("ENABLE_COLO command come in migration stream, but
> x-colo "
>                       "capability is not set");
>          return -EINVAL;
>      }
> --
> 2.31.1

RE: [PATCH 3/3] migration/colo: Tidy up bql_unlock() around bdrv_activate_all()

[Zhang, Chen]

> -----Original Message-----
> From: Li Zhijian <lizhijian@fujitsu.com>
> Sent: Thursday, May 9, 2024 11:31 AM
> To: Peter Xu <peterx@redhat.com>; Fabiano Rosas <farosas@suse.de>
> Cc: Zhang, Hailiang <zhanghailiang@xfusion.com>; qemu-
> devel@nongnu.org; Zhang, Chen <chen.zhang@intel.com>; Li Zhijian
> <lizhijian@fujitsu.com>; Michael Tokarev <mjt@tls.msk.ru>
> Subject: [PATCH 3/3] migration/colo: Tidy up bql_unlock() around
> bdrv_activate_all()
> 
> Make the code more tight.
> 
> Cc: Michael Tokarev <mjt@tls.msk.ru>
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>

Reviewed-by: Zhang Chen <chen.zhang@intel.com>

Thanks
Chen

> ---
> This change/comment suggested by "Michael Tokarev <mjt@tls.msk.ru>"
> came a bit late at that time, let's update it together in these minor set this
> time.
> ---
>  migration/colo.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/migration/colo.c b/migration/colo.c index 991806c06a..1b6d9da1c8
> 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -838,12 +838,11 @@ static void *colo_process_incoming_thread(void
> *opaque)
>      /* Make sure all file formats throw away their mutable metadata */
>      bql_lock();
>      bdrv_activate_all(&local_err);
> +    bql_unlock();
>      if (local_err) {
> -        bql_unlock();
>          error_report_err(local_err);
>          return NULL;
>      }
> -    bql_unlock();
> 
>      failover_init_state();
> 
> --
> 2.31.1

Re: [PATCH 2/3] migration/colo: make colo_incoming_co() return void

[Fabiano Rosas]

Li Zhijian via <qemu-devel@nongnu.org> writes:

> Currently, it always returns 0, no need to check the return value at all.
> In addition, enter colo coroutine only if migration_incoming_colo_enabled()
> is true.
> Once the destination side enters the COLO* state, the COLO process will
> take over the remaining processes until COLO exits.
>
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
>  migration/colo.c      | 9 ++-------
>  migration/migration.c | 6 +++---
>  2 files changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/migration/colo.c b/migration/colo.c
> index 5600a43d78..991806c06a 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -929,16 +929,13 @@ out:
>      return NULL;
>  }
>  
> -int coroutine_fn colo_incoming_co(void)
> +void coroutine_fn colo_incoming_co(void)
>  {
>      MigrationIncomingState *mis = migration_incoming_get_current();
>      QemuThread th;
>  
>      assert(bql_locked());
> -
> -    if (!migration_incoming_colo_enabled()) {
> -        return 0;
> -    }
> +    assert(migration_incoming_colo_enabled());

FAILED: libcommon.fa.p/migration_colo.c.o
/usr/bin/gcc-13 ... ../migration/colo.c
../migration/colo.c:930:19: error: conflicting types for ‘colo_incoming_co’; have ‘void(void)’
  930 | void coroutine_fn colo_incoming_co(void)
      |                   ^~~~~~~~~~~~~~~~
In file included from ../migration/colo.c:20:
... qemu/include/migration/colo.h:52:18: note: previous declaration of ‘colo_incoming_co’ with type ‘int(void)’
   52 | int coroutine_fn colo_incoming_co(void);
      |                  ^~~~~~~~~~~~~~~~

Re: [PATCH 2/3] migration/colo: make colo_incoming_co() return void

[Zhijian Li (Fujitsu) via]

On 16/05/2024 03:04, Fabiano Rosas wrote:
> Li Zhijian via <qemu-devel@nongnu.org> writes:
> 
>> Currently, it always returns 0, no need to check the return value at all.
>> In addition, enter colo coroutine only if migration_incoming_colo_enabled()
>> is true.
>> Once the destination side enters the COLO* state, the COLO process will
>> take over the remaining processes until COLO exits.
>>
>> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
>> ---
>>   migration/colo.c      | 9 ++-------
>>   migration/migration.c | 6 +++---
>>   2 files changed, 5 insertions(+), 10 deletions(-)
>>
>> diff --git a/migration/colo.c b/migration/colo.c
>> index 5600a43d78..991806c06a 100644
>> --- a/migration/colo.c
>> +++ b/migration/colo.c
>> @@ -929,16 +929,13 @@ out:
>>       return NULL;
>>   }
>>   
>> -int coroutine_fn colo_incoming_co(void)
>> +void coroutine_fn colo_incoming_co(void)
>>   {
>>       MigrationIncomingState *mis = migration_incoming_get_current();
>>       QemuThread th;
>>   
>>       assert(bql_locked());
>> -
>> -    if (!migration_incoming_colo_enabled()) {
>> -        return 0;
>> -    }
>> +    assert(migration_incoming_colo_enabled());
> 
> FAILED: libcommon.fa.p/migration_colo.c.o
> /usr/bin/gcc-13 ... ../migration/colo.c
> ../migration/colo.c:930:19: error: conflicting types for ‘colo_incoming_co’; have ‘void(void)’
>    930 | void coroutine_fn colo_incoming_co(void)
>        |                   ^~~~~~~~~~~~~~~~
> In file included from ../migration/colo.c:20:
> ... qemu/include/migration/colo.h:52:18: note: previous declaration of ‘colo_incoming_co’ with type ‘int(void)’
>     52 | int coroutine_fn colo_incoming_co(void);

My fault, will fix it soon

Thanks
Zhijian
>        |                  ^~~~~~~~~~~~~~~~