[ovs-dev] [PATCH] ovsdb-idl: Fix *_is_new() IDL functions

Han Zhou hzhou at ovn.org
Wed Sep 30 20:04:27 UTC 2020


On Wed, Sep 30, 2020 at 2:21 AM Dumitru Ceara <dceara at redhat.com> wrote:
>
> On 9/29/20 8:34 PM, Mark Gray wrote:
> > Currently all functions of the type *_is_new() always return
> > 'false'. This patch resolves this issue by using the
> > 'OVSDB_IDL_CHANGE_INSERT' 'change_seqno' instead of the
> > 'OVSDB_IDL_CHANGE_MODIFY' 'change_seqno' to determine if a row
> > is new and by resetting the 'OVSDB_IDL_CHANGE_INSERT'
> > 'change_seqno' on clear.
> >
> > Further to this, the code is also updated to match this
> > behaviour:
> >
> > When a row is inserted, the 'OVSDB_IDL_CHANGE_INSERT'
> > 'change_seqno' is updated to match the new database
> > change_seqno. The 'OVSDB_IDL_CHANGE_MODIFY' 'change_seqno'
> > is not set for inserted rows (only for updated rows).
> >
> > At the end of a run, ovsdb_idl_db_track_clear() should be
> > called to clear all tracking information, this includes
> > resetting all row 'change_seqno' to zero. This will ensure
> > that subsequent runs will not see a previously 'new' row.
> >
> > add_tracked_change_for_references() is updated to only
> > track rows that reference the current row.
> >
> > Signed-off-by: Mark Gray <mark.d.gray at redhat.com>
> > Suggested-by: Dumitru Ceara <dceara at redhat.com>
> > Reported-at: https://bugzilla.redhat.com/1883562
>
> Hi Mark,
>
> Thanks for working on this, it definitely looks like a nasty bug to me!
>
> We were lucky to not hit it in ovn-controller before. That's just
> because all the "update" operations were handled there as "delete" +
> "insert" but that is not mandatory and might change.
>

Agree. Thanks for this critical finding! It was my bad - commit ca545a787ac
introduced this bug, which was fixing another bug related to is_new. I
should have added test cases to avoid the miss. Apologize.

For this fix, I don't understand why we need to check
OVSDB_IDL_CHANGE_INSERT. I think the root cause is that in
add_tracked_change_for_references() it updated the OVSDB_IDL_CHANGE_MODIFY
seqno for the initial inserted *new* row, while it shouldn't. So should the
fix only include the changes in add_tracked_change_for_references() and
ovsdb_idl_row_change__()? Why do we need to change the implementation of
_is_new() itself? Could you explain?

> > ---
> >  lib/ovsdb-idl.c     | 39 +++++++++++++++++++++++++++------------
> >  ovsdb/ovsdb-idlc.in |  2 +-
> >  2 files changed, 28 insertions(+), 13 deletions(-)
> >
> > diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> > index d8f221ca6..3cfd73871 100644
> > --- a/lib/ovsdb-idl.c
> > +++ b/lib/ovsdb-idl.c
> > @@ -1959,6 +1959,11 @@ ovsdb_idl_db_track_clear(struct ovsdb_idl_db *db)
> >                      free(row->updated);
> >                      row->updated = NULL;
> >                  }
> > +
> > +                row->change_seqno[OVSDB_IDL_CHANGE_INSERT] =
> > +                    row->change_seqno[OVSDB_IDL_CHANGE_MODIFY] =
> > +                    row->change_seqno[OVSDB_IDL_CHANGE_DELETE] = 0;
> > +
> >                  ovs_list_remove(&row->track_node);
> >                  ovs_list_init(&row->track_node);
> >                  if (ovsdb_idl_row_is_orphan(row) &&
row->tracked_old_datum) {
> > @@ -2684,24 +2689,27 @@ ovsdb_idl_process_update2(struct
ovsdb_idl_table *table,
> >      return OVSDB_IDL_UPDATE_DB_CHANGED;
> >  }
> >
> > -/* Recursively add rows to tracked change lists for current row
> > - * and the rows that reference this row. */
> > +/* Recursively add rows to tracked change lists for all rows that
reference
> > +   'row'. */
> >  static void
> >  add_tracked_change_for_references(struct ovsdb_idl_row *row)
> >  {
> > -    if (ovs_list_is_empty(&row->track_node) &&
> > -            ovsdb_idl_track_is_set(row->table)) {
> > -        ovs_list_push_back(&row->table->track_list,
> > -                           &row->track_node);
> > -        row->change_seqno[OVSDB_IDL_CHANGE_MODIFY]
> > -            = row->table->change_seqno[OVSDB_IDL_CHANGE_MODIFY]
> > -            = row->table->db->change_seqno + 1;
> > -
> >          const struct ovsdb_idl_arc *arc;
> >          LIST_FOR_EACH (arc, dst_node, &row->dst_arcs) {
> > -            add_tracked_change_for_references(arc->src);
> > +            struct ovsdb_idl_row *ref = arc->src;
> > +
> > +            if (ovs_list_is_empty(&ref->track_node) &&
> > +                ovsdb_idl_track_is_set(ref->table)) {
> > +                    ovs_list_push_back(&ref->table->track_list,
> > +                                       &ref->track_node);
> > +
> > +                ref->change_seqno[OVSDB_IDL_CHANGE_MODIFY]
> > +                    = ref->table->change_seqno[OVSDB_IDL_CHANGE_MODIFY]
> > +                    = ref->table->db->change_seqno + 1;
> > +
> > +                add_tracked_change_for_references(ref);
> > +            }
> >          }
> > -    }
> >  }
> >
> >
> > @@ -2767,7 +2775,14 @@ ovsdb_idl_row_change__(struct ovsdb_idl_row
*row, const struct json *row_json,
> >                      row->change_seqno[change]
> >                          = row->table->change_seqno[change]
> >                          = row->table->db->change_seqno + 1;
> > +
> >                      if (table->modes[column_idx] & OVSDB_IDL_TRACK) {
> > +                        if (ovs_list_is_empty(&row->track_node) &&
> > +                            ovsdb_idl_track_is_set(row->table)) {
> > +                            ovs_list_push_back(&row->table->track_list,
> > +                                               &row->track_node);
> > +                        }
> > +
> >                          add_tracked_change_for_references(row);
> >                          if (!row->updated) {
> >                              row->updated =
bitmap_allocate(class->n_columns);
> > diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in
> > index 698fe25f3..89c3eb682 100755
> > --- a/ovsdb/ovsdb-idlc.in
> > +++ b/ovsdb/ovsdb-idlc.in
> > @@ -282,7 +282,7 @@ const struct %(s)s
*%(s)s_table_track_get_first(const struct %(s)s_table *);
> >  /* Returns true if 'row' was inserted since the last change tracking
reset. */
> >  static inline bool %(s)s_is_new(const struct %(s)s *row)
> >  {
> > -    return %(s)s_row_get_seqno(row, OVSDB_IDL_CHANGE_MODIFY) == 0;
> > +    return %(s)s_row_get_seqno(row, OVSDB_IDL_CHANGE_INSERT) > 0;
>
> This causes some issues with records created by the IDL client (e.g.,
> ovn-controller). Those are created with ovsdb_idl_txn_insert() and we
> don't set the OVSDB_IDL_CHANGE_INSERT change_seqno in that case.
>
> Essentially *_is_new() will never return true for rows inserted locally.
>
Hi Dumitru, I have a different understanding here. I think this is not a
problem. Whatever changes made locally by the IDL will be discarded when
committing to server and then rely on the notifications from the server to
finally update the IDL data, which updates the change_seqno accordingly.

> Regards,
> Dumitru
>
> >  }
> >
> >  /* Returns true if 'row' was deleted since the last change tracking
reset. */
> >
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list