migration: Unify and trace vmstate field_exists() checks

[PATCH] migration: Unify and trace vmstate field_exists() checks

Posted by Peter Xu 8 months, 1 week ago

For both save/load we actually share the logic on deciding whether a field
should exist.  Merge the checks into a helper and use it for both save and
load.  When doing so, add documentations and reformat the code to make it
much easier to read.

The real benefit here (besides code cleanups) is we add a trace-point for
this; this is a known spot where we can easily break migration
compatibilities between binaries, and this trace point will be critical for
us to identify such issues.

For example, this will be handy when debugging things like:

https://gitlab.com/qemu-project/qemu/-/issues/932

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/vmstate.c    | 34 ++++++++++++++++++++++++++--------
 migration/trace-events |  1 +
 2 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/migration/vmstate.c b/migration/vmstate.c
index 31842c3afb..73e74ddea0 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -25,6 +25,30 @@ static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
 static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
                                    void *opaque);
 
+/* Whether this field should exist for either save or load the VM? */
+static bool
+vmstate_field_exists(const VMStateDescription *vmsd, const VMStateField *field,
+                     void *opaque, int version_id)
+{
+    bool result;
+
+    if (field->field_exists) {
+        /* If there's the function checker, that's the solo truth */
+        result = field->field_exists(opaque, version_id);
+        trace_vmstate_field_exists(vmsd->name, field->name, field->version_id,
+                                   version_id, result);
+    } else {
+        /*
+         * Otherwise, we only save/load if field version is same or older.
+         * For example, when loading from an old binary with old version,
+         * we ignore new fields with newer version_ids.
+         */
+        result = field->version_id <= version_id;
+    }
+
+    return result;
+}
+
 static int vmstate_n_elems(void *opaque, const VMStateField *field)
 {
     int n_elems = 1;
@@ -104,10 +128,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
     }
     while (field->name) {
         trace_vmstate_load_state_field(vmsd->name, field->name);
-        if ((field->field_exists &&
-             field->field_exists(opaque, version_id)) ||
-            (!field->field_exists &&
-             field->version_id <= version_id)) {
+        if (vmstate_field_exists(vmsd, field, opaque, version_id)) {
             void *first_elem = opaque + field->offset;
             int i, n_elems = vmstate_n_elems(opaque, field);
             int size = vmstate_size(opaque, field);
@@ -342,10 +363,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
     }
 
     while (field->name) {
-        if ((field->field_exists &&
-             field->field_exists(opaque, version_id)) ||
-            (!field->field_exists &&
-             field->version_id <= version_id)) {
+        if (vmstate_field_exists(vmsd, field, opaque, version_id)) {
             void *first_elem = opaque + field->offset;
             int i, n_elems = vmstate_n_elems(opaque, field);
             int size = vmstate_size(opaque, field);
diff --git a/migration/trace-events b/migration/trace-events
index 4666f19325..446db0b7ce 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -66,6 +66,7 @@ vmstate_save_state_loop(const char *name, const char *field, int n_elems) "%s/%s
 vmstate_save_state_top(const char *idstr) "%s"
 vmstate_subsection_save_loop(const char *name, const char *sub) "%s/%s"
 vmstate_subsection_save_top(const char *idstr) "%s"
+vmstate_field_exists(const char *vmsd, const char *name, int field_version, int version, int result) "%s:%s field_version %d version %d result %d"
 
 # vmstate-types.c
 get_qtailq(const char *name, int version_id) "%s v%d"
-- 
2.41.0

Re: [PATCH] migration: Unify and trace vmstate field_exists() checks

Posted by Juan Quintela 7 months, 1 week ago

Peter Xu <peterx@redhat.com> wrote:
> For both save/load we actually share the logic on deciding whether a field
> should exist.  Merge the checks into a helper and use it for both save and
> load.  When doing so, add documentations and reformat the code to make it
> much easier to read.
>
> The real benefit here (besides code cleanups) is we add a trace-point for
> this; this is a known spot where we can easily break migration
> compatibilities between binaries, and this trace point will be critical for
> us to identify such issues.
>
> For example, this will be handy when debugging things like:
>
> https://gitlab.com/qemu-project/qemu/-/issues/932
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

queued.

Re: [PATCH] migration: Unify and trace vmstate field_exists() checks

Posted by Fabiano Rosas 8 months, 1 week ago

Peter Xu <peterx@redhat.com> writes:

> For both save/load we actually share the logic on deciding whether a field
> should exist.  Merge the checks into a helper and use it for both save and
> load.  When doing so, add documentations and reformat the code to make it
> much easier to read.
>
> The real benefit here (besides code cleanups) is we add a trace-point for
> this; this is a known spot where we can easily break migration
> compatibilities between binaries, and this trace point will be critical for
> us to identify such issues.
>
> For example, this will be handy when debugging things like:
>
> https://gitlab.com/qemu-project/qemu/-/issues/932
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/vmstate.c    | 34 ++++++++++++++++++++++++++--------
>  migration/trace-events |  1 +
>  2 files changed, 27 insertions(+), 8 deletions(-)
>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 31842c3afb..73e74ddea0 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -25,6 +25,30 @@ static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
>  static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
>                                     void *opaque);
>  
> +/* Whether this field should exist for either save or load the VM? */
> +static bool
> +vmstate_field_exists(const VMStateDescription *vmsd, const VMStateField *field,
> +                     void *opaque, int version_id)
> +{
> +    bool result;
> +
> +    if (field->field_exists) {
> +        /* If there's the function checker, that's the solo truth */
> +        result = field->field_exists(opaque, version_id);
> +        trace_vmstate_field_exists(vmsd->name, field->name, field->version_id,
> +                                   version_id, result);
> +    } else {
> +        /*
> +         * Otherwise, we only save/load if field version is same or older.
> +         * For example, when loading from an old binary with old version,
> +         * we ignore new fields with newer version_ids.
> +         */
> +        result = field->version_id <= version_id;

This one doesn't get a trace?

Aside from that:

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

Re: [PATCH] migration: Unify and trace vmstate field_exists() checks

Posted by Peter Xu 7 months, 1 week ago

On Thu, Sep 07, 2023 at 08:55:52AM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > For both save/load we actually share the logic on deciding whether a field
> > should exist.  Merge the checks into a helper and use it for both save and
> > load.  When doing so, add documentations and reformat the code to make it
> > much easier to read.
> >
> > The real benefit here (besides code cleanups) is we add a trace-point for
> > this; this is a known spot where we can easily break migration
> > compatibilities between binaries, and this trace point will be critical for
> > us to identify such issues.
> >
> > For example, this will be handy when debugging things like:
> >
> > https://gitlab.com/qemu-project/qemu/-/issues/932
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  migration/vmstate.c    | 34 ++++++++++++++++++++++++++--------
> >  migration/trace-events |  1 +
> >  2 files changed, 27 insertions(+), 8 deletions(-)
> >
> > diff --git a/migration/vmstate.c b/migration/vmstate.c
> > index 31842c3afb..73e74ddea0 100644
> > --- a/migration/vmstate.c
> > +++ b/migration/vmstate.c
> > @@ -25,6 +25,30 @@ static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
> >  static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
> >                                     void *opaque);
> >  
> > +/* Whether this field should exist for either save or load the VM? */
> > +static bool
> > +vmstate_field_exists(const VMStateDescription *vmsd, const VMStateField *field,
> > +                     void *opaque, int version_id)
> > +{
> > +    bool result;
> > +
> > +    if (field->field_exists) {
> > +        /* If there's the function checker, that's the solo truth */
> > +        result = field->field_exists(opaque, version_id);
> > +        trace_vmstate_field_exists(vmsd->name, field->name, field->version_id,
> > +                                   version_id, result);
> > +    } else {
> > +        /*
> > +         * Otherwise, we only save/load if field version is same or older.
> > +         * For example, when loading from an old binary with old version,
> > +         * we ignore new fields with newer version_ids.
> > +         */
> > +        result = field->version_id <= version_id;
> 
> This one doesn't get a trace?

Right, I didn't add that since I found mostly the bug comes from
field_exists() returning different values for different qemu binaries.
So I explicitly didn't include that otherwise we'll see tons of entries
under that regard but is probably safe.

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

Thanks!

-- 
Peter Xu