1. Patchew
  2. QEMU
  3. [PATCH 00/14] migration: Improve error reporting
  4. View patch

[PATCH 04/14] migration: Modify ram_init_bitmaps() to report dirty tracking errors

Cédric Le Goater posted 14 patches 9 months, 3 weeks ago
Maintainers: Stefano Stabellini <sstabellini@kernel.org>, Anthony Perard <anthony.perard@citrix.com>, Paul Durrant <paul@xen.org>, "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Eduardo Habkost <eduardo@habkost.net>, Nicholas Piggin <npiggin@gmail.com>, Daniel Henrique Barboza <danielhb413@gmail.com>, "Cédric Le Goater" <clg@kaod.org>, David Gibson <david@gibson.dropbear.id.au>, Harsh Prateek Bora <harshpb@linux.ibm.com>, Halil Pasic <pasic@linux.ibm.com>, Christian Borntraeger <borntraeger@linux.ibm.com>, Eric Farman <farman@linux.ibm.com>, David Hildenbrand <david@redhat.com>, Ilya Leoshkevich <iii@linux.ibm.com>, Thomas Huth <thuth@redhat.com>, Alex Williamson <alex.williamson@redhat.com>, Peter Xu <peterx@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Fabiano Rosas <farosas@suse.de>, Stefan Hajnoczi <stefanha@redhat.com>, Fam Zheng <fam@euphon.net>, Eric Blake <eblake@redhat.com>, Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>, John Snow <jsnow@redhat.com>, Hyman Huang <yong.huang@smartx.com>
There is a newer version of this series
[PATCH 04/14] migration: Modify ram_init_bitmaps() to report dirty tracking errors
Posted by Cédric Le Goater 9 months, 3 weeks ago 
The .save_setup() handler has now an Error** argument that we can use
to propagate errors reported by the .log_global_start() handler. Do
that for the RAM. qemu_savevm_state_setup() will store the error under
the migration stream for later detection in the migration sequence.

Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 migration/ram.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index d86626bb1c704b2d3497b323a702ca6ca8939a79..b87245466bb46937fd0358d0c66432bcc6280018 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2802,19 +2802,17 @@ static void migration_bitmap_clear_discarded_pages(RAMState *rs)
     }
 }
 
-static void ram_init_bitmaps(RAMState *rs)
+static void ram_init_bitmaps(RAMState *rs, Error **errp)
 {
-    Error *local_err = NULL;
-
     qemu_mutex_lock_ramlist();
 
     WITH_RCU_READ_LOCK_GUARD() {
         ram_list_init_bitmaps();
         /* We don't use dirty log with background snapshots */
         if (!migrate_background_snapshot()) {
-            memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, &local_err);
-            if (local_err) {
-                error_report_err(local_err);
+            memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, errp);
+            if (*errp) {
+                break;
             }
             migration_bitmap_sync_precopy(rs, false);
         }
@@ -2828,7 +2826,7 @@ static void ram_init_bitmaps(RAMState *rs)
     migration_bitmap_clear_discarded_pages(rs);
 }
 
-static int ram_init_all(RAMState **rsp)
+static int ram_init_all(RAMState **rsp, Error **errp)
 {
     if (ram_state_init(rsp)) {
         return -1;
@@ -2839,7 +2837,10 @@ static int ram_init_all(RAMState **rsp)
         return -1;
     }
 
-    ram_init_bitmaps(*rsp);
+    ram_init_bitmaps(*rsp, errp);
+    if (*errp) {
+        return -1;
+    }
 
     return 0;
 }
@@ -2952,7 +2953,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque, Error **errp)
 
     /* migration has already setup the bitmap, reuse it. */
     if (!migration_in_colo_state()) {
-        if (ram_init_all(rsp) != 0) {
+        if (ram_init_all(rsp, errp) != 0) {
             compress_threads_save_cleanup();
             return -1;
         }
-- 
2.43.0
Re: [PATCH 04/14] migration: Modify ram_init_bitmaps() to report dirty tracking errors
Posted by Avihai Horon 9 months, 2 weeks ago 
Hi Cedric,

On 07/02/2024 15:33, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> The .save_setup() handler has now an Error** argument that we can use
> to propagate errors reported by the .log_global_start() handler. Do
> that for the RAM. qemu_savevm_state_setup() will store the error under
> the migration stream for later detection in the migration sequence.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   migration/ram.c | 19 ++++++++++---------
>   1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index d86626bb1c704b2d3497b323a702ca6ca8939a79..b87245466bb46937fd0358d0c66432bcc6280018 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2802,19 +2802,17 @@ static void migration_bitmap_clear_discarded_pages(RAMState *rs)
>       }
>   }
>
> -static void ram_init_bitmaps(RAMState *rs)
> +static void ram_init_bitmaps(RAMState *rs, Error **errp)
>   {
> -    Error *local_err = NULL;
> -
>       qemu_mutex_lock_ramlist();
>
>       WITH_RCU_READ_LOCK_GUARD() {
>           ram_list_init_bitmaps();
>           /* We don't use dirty log with background snapshots */
>           if (!migrate_background_snapshot()) {
> -            memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, &local_err);
> -            if (local_err) {
> -                error_report_err(local_err);
> +            memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, errp);
> +            if (*errp) {

I think we should use ERRP_GUARD() or a local error here and also below 
at ram_init_bitmaps() (or return bool like Philippe suggested).

Thanks.

> +                break;
>               }
>               migration_bitmap_sync_precopy(rs, false);
>           }
> @@ -2828,7 +2826,7 @@ static void ram_init_bitmaps(RAMState *rs)
>       migration_bitmap_clear_discarded_pages(rs);
>   }
>
> -static int ram_init_all(RAMState **rsp)
> +static int ram_init_all(RAMState **rsp, Error **errp)
>   {
>       if (ram_state_init(rsp)) {
>           return -1;
> @@ -2839,7 +2837,10 @@ static int ram_init_all(RAMState **rsp)
>           return -1;
>       }
>
> -    ram_init_bitmaps(*rsp);
> +    ram_init_bitmaps(*rsp, errp);
> +    if (*errp) {
> +        return -1;
> +    }
>
>       return 0;
>   }
> @@ -2952,7 +2953,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque, Error **errp)
>
>       /* migration has already setup the bitmap, reuse it. */
>       if (!migration_in_colo_state()) {
> -        if (ram_init_all(rsp) != 0) {
> +        if (ram_init_all(rsp, errp) != 0) {
>               compress_threads_save_cleanup();
>               return -1;
>           }
> --
> 2.43.0
>
>
Re: [PATCH 04/14] migration: Modify ram_init_bitmaps() to report dirty tracking errors
Posted by Cédric Le Goater 9 months, 2 weeks ago 
On 2/12/24 09:51, Avihai Horon wrote:
> Hi Cedric,
> 
> On 07/02/2024 15:33, Cédric Le Goater wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> The .save_setup() handler has now an Error** argument that we can use
>> to propagate errors reported by the .log_global_start() handler. Do
>> that for the RAM. qemu_savevm_state_setup() will store the error under
>> the migration stream for later detection in the migration sequence.
>>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>   migration/ram.c | 19 ++++++++++---------
>>   1 file changed, 10 insertions(+), 9 deletions(-)
>>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index d86626bb1c704b2d3497b323a702ca6ca8939a79..b87245466bb46937fd0358d0c66432bcc6280018 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -2802,19 +2802,17 @@ static void migration_bitmap_clear_discarded_pages(RAMState *rs)
>>       }
>>   }
>>
>> -static void ram_init_bitmaps(RAMState *rs)
>> +static void ram_init_bitmaps(RAMState *rs, Error **errp)
>>   {
>> -    Error *local_err = NULL;
>> -
>>       qemu_mutex_lock_ramlist();
>>
>>       WITH_RCU_READ_LOCK_GUARD() {
>>           ram_list_init_bitmaps();
>>           /* We don't use dirty log with background snapshots */
>>           if (!migrate_background_snapshot()) {
>> -            memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, &local_err);
>> -            if (local_err) {
>> -                error_report_err(local_err);
>> +            memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, errp);
>> +            if (*errp) {
> 
> I think we should use ERRP_GUARD() or a local error here and also below at ram_init_bitmaps() (or return bool like Philippe suggested).

yes. I will rework that part.

Thanks,

C.


> 
> Thanks.
> 
>> +                break;
>>               }
>>               migration_bitmap_sync_precopy(rs, false);
>>           }
>> @@ -2828,7 +2826,7 @@ static void ram_init_bitmaps(RAMState *rs)
>>       migration_bitmap_clear_discarded_pages(rs);
>>   }
>>
>> -static int ram_init_all(RAMState **rsp)
>> +static int ram_init_all(RAMState **rsp, Error **errp)
>>   {
>>       if (ram_state_init(rsp)) {
>>           return -1;
>> @@ -2839,7 +2837,10 @@ static int ram_init_all(RAMState **rsp)
>>           return -1;
>>       }
>>
>> -    ram_init_bitmaps(*rsp);
>> +    ram_init_bitmaps(*rsp, errp);
>> +    if (*errp) {
>> +        return -1;
>> +    }
>>
>>       return 0;
>>   }
>> @@ -2952,7 +2953,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque, Error **errp)
>>
>>       /* migration has already setup the bitmap, reuse it. */
>>       if (!migration_in_colo_state()) {
>> -        if (ram_init_all(rsp) != 0) {
>> +        if (ram_init_all(rsp, errp) != 0) {
>>               compress_threads_save_cleanup();
>>               return -1;
>>           }
>> -- 
>> 2.43.0
>>
>>
>
Re: [PATCH 04/14] migration: Modify ram_init_bitmaps() to report dirty tracking errors
Posted by Philippe Mathieu-Daudé 9 months, 3 weeks ago 
On 7/2/24 14:33, Cédric Le Goater wrote:
> The .save_setup() handler has now an Error** argument that we can use
> to propagate errors reported by the .log_global_start() handler. Do
> that for the RAM. qemu_savevm_state_setup() will store the error under
> the migration stream for later detection in the migration sequence.
> 
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   migration/ram.c | 19 ++++++++++---------
>   1 file changed, 10 insertions(+), 9 deletions(-)


> -static void ram_init_bitmaps(RAMState *rs)
> +static void ram_init_bitmaps(RAMState *rs, Error **errp)

Please return a boolean.

>   {
> -    Error *local_err = NULL;
> -
>       qemu_mutex_lock_ramlist();
>   
>       WITH_RCU_READ_LOCK_GUARD() {
>           ram_list_init_bitmaps();
>           /* We don't use dirty log with background snapshots */
>           if (!migrate_background_snapshot()) {
> -            memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, &local_err);
> -            if (local_err) {
> -                error_report_err(local_err);
> +            memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, errp);
> +            if (*errp) {
> +                break;
>               }
>               migration_bitmap_sync_precopy(rs, false);
>           }
> @@ -2828,7 +2826,7 @@ static void ram_init_bitmaps(RAMState *rs)
>       migration_bitmap_clear_discarded_pages(rs);
>   }

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.