[PATCH] migration: Check current_migration in migration_is_running()

[PATCH] migration: Check current_migration in migration_is_running()

[Peter Xu]

Report shows that commit 34a8892dec broke iotest 055:

https://lore.kernel.org/r/b8806360-a2b6-4608-83a3-db67e264c733@linaro.org

When replacing migration_is_idle() with "!migration_is_running()", it was
overlooked that the idle helper also checks for current_migration being
available first.

The check would be there if the whole series was applied, but since the
last patches in the previous series rely on some other patches to land
first, we need to recover the behavior of migration_is_idle() first before
that whole set will be merged.

I left migration_is_active / migration_is_device alone, as I don't think
it's possible for them to hit his case (current_migration not initialized).
Also they're prone to removal soon from VFIO side.

Cc: Fabiano Rosas <farosas@suse.de>
Cc: Peter Maydell <peter.maydell@linaro.org>
Fixes: 34a8892dec ("migration: Drop migration_is_idle()")
Reported-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Tested-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index aedf7f0751..8c5bd0a75c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1117,6 +1117,10 @@ bool migration_is_running(void)
 {
     MigrationState *s = current_migration;
 
+    if (!s) {
+        return false;
+    }
+
     switch (s->state) {
     case MIGRATION_STATUS_ACTIVE:
     case MIGRATION_STATUS_POSTCOPY_ACTIVE:
-- 
2.45.0

Re: [PATCH] migration: Check current_migration in migration_is_running()

[Pierrick Bouvier]

On 11/5/24 10:27, Peter Xu wrote:
> Report shows that commit 34a8892dec broke iotest 055:
> 
> https://lore.kernel.org/r/b8806360-a2b6-4608-83a3-db67e264c733@linaro.org
> 
> When replacing migration_is_idle() with "!migration_is_running()", it was
> overlooked that the idle helper also checks for current_migration being
> available first.
> 
> The check would be there if the whole series was applied, but since the
> last patches in the previous series rely on some other patches to land
> first, we need to recover the behavior of migration_is_idle() first before
> that whole set will be merged.
> 
> I left migration_is_active / migration_is_device alone, as I don't think
> it's possible for them to hit his case (current_migration not initialized).
> Also they're prone to removal soon from VFIO side.
> 
> Cc: Fabiano Rosas <farosas@suse.de>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Fixes: 34a8892dec ("migration: Drop migration_is_idle()")
> Reported-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Tested-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   migration/migration.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index aedf7f0751..8c5bd0a75c 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1117,6 +1117,10 @@ bool migration_is_running(void)
>   {
>       MigrationState *s = current_migration;
>   
> +    if (!s) {
> +        return false;
> +    }
> +
>       switch (s->state) {
>       case MIGRATION_STATUS_ACTIVE:
>       case MIGRATION_STATUS_POSTCOPY_ACTIVE:

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH] migration: Check current_migration in migration_is_running()

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> Report shows that commit 34a8892dec broke iotest 055:
>
> https://lore.kernel.org/r/b8806360-a2b6-4608-83a3-db67e264c733@linaro.org
>
> When replacing migration_is_idle() with "!migration_is_running()", it was
> overlooked that the idle helper also checks for current_migration being
> available first.
>
> The check would be there if the whole series was applied, but since the
> last patches in the previous series rely on some other patches to land
> first, we need to recover the behavior of migration_is_idle() first before
> that whole set will be merged.
>
> I left migration_is_active / migration_is_device alone, as I don't think
> it's possible for them to hit his case (current_migration not initialized).
> Also they're prone to removal soon from VFIO side.
>
> Cc: Fabiano Rosas <farosas@suse.de>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Fixes: 34a8892dec ("migration: Drop migration_is_idle()")
> Reported-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Tested-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH] migration: Check current_migration in migration_is_running()

[Thomas Huth]

On 05/11/2024 19.27, Peter Xu wrote:
> Report shows that commit 34a8892dec broke iotest 055:
> 
> https://lore.kernel.org/r/b8806360-a2b6-4608-83a3-db67e264c733@linaro.org

FWIW, this patch also fixes a lot of other broken iotests (vmdk and vpc) 
that occur when running "make check SPEED=thorough".

Tested-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH] migration: Check current_migration in migration_is_running()

[Pierrick Bouvier]

On 11/6/24 01:43, Thomas Huth wrote:
> On 05/11/2024 19.27, Peter Xu wrote:
>> Report shows that commit 34a8892dec broke iotest 055:
>>
>> https://lore.kernel.org/r/b8806360-a2b6-4608-83a3-db67e264c733@linaro.org
> 
> FWIW, this patch also fixes a lot of other broken iotests (vmdk and vpc)
> that occur when running "make check SPEED=thorough".
> 
> Tested-by: Thomas Huth <thuth@redhat.com>
> 
> 
> 

Good news!

I'm a bit confused by your message. I thought SPEED=slow was the most 
complete test setup, but is it SPEED=thorough instead?

Re: [PATCH] migration: Check current_migration in migration_is_running()

[Thomas Huth]

On 06/11/2024 18.18, Pierrick Bouvier wrote:
> On 11/6/24 01:43, Thomas Huth wrote:
>> On 05/11/2024 19.27, Peter Xu wrote:
>>> Report shows that commit 34a8892dec broke iotest 055:
>>>
>>> https://lore.kernel.org/r/b8806360-a2b6-4608-83a3-db67e264c733@linaro.org
>>
>> FWIW, this patch also fixes a lot of other broken iotests (vmdk and vpc)
>> that occur when running "make check SPEED=thorough".
>>
>> Tested-by: Thomas Huth <thuth@redhat.com>
>>
>>
>>
> 
> Good news!
> 
> I'm a bit confused by your message. I thought SPEED=slow was the most 
> complete test setup, but is it SPEED=thorough instead?

It depends... for the qtests, "slow" and "thorough" is the same, but for the 
iotests, we enable even more additional formats with "thorough".

  Thomas