On Tue, Dec 12, 2023 at 7:45 AM Dumitru Ceara <[email protected]> wrote: > > On 12/12/23 05:04, Mike Pattrick wrote: > > In the case of a weak reference, clearing all change_seqno's can delete > > useful information. Instead of clearing all seqno's when removing > > track_node, only clear those values in cases including row insertion, > > and row deleting if no dst_arcs remain. > > > > Fixes: 95689f166818 ("ovsdb-idl: Preserve references for deleted rows.") > > Reported-at: https://issues.redhat.com/browse/FDP-193 > > Signed-off-by: Mike Pattrick <[email protected]> > > --- > > Thanks for the fix, Mike! Overall I think the idea of the fix is OK, I > only have one comment below. > > > lib/ovsdb-idl.c | 13 +++++++++++-- > > 1 file changed, 11 insertions(+), 2 deletions(-) > > > > diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c > > index 634fbb56d..77f1a6db4 100644 > > --- a/lib/ovsdb-idl.c > > +++ b/lib/ovsdb-idl.c > > @@ -177,6 +177,7 @@ static void > > ovsdb_idl_row_mark_backrefs_for_reparsing(struct ovsdb_idl_row *); > > static void ovsdb_idl_row_track_change(struct ovsdb_idl_row *, > > enum ovsdb_idl_change); > > static void ovsdb_idl_row_untrack_change(struct ovsdb_idl_row *); > > +static void ovsdb_idl_row_clear_changeseqno(struct ovsdb_idl_row *); > > > > static void ovsdb_idl_txn_abort_all(struct ovsdb_idl *); > > static bool ovsdb_idl_txn_extract_mutations(struct ovsdb_idl_row *, > > @@ -1374,6 +1375,7 @@ ovsdb_idl_track_clear__(struct ovsdb_idl *idl, bool > > flush_all) > > row->updated = NULL; > > } > > ovsdb_idl_row_untrack_change(row); > > + ovsdb_idl_row_clear_changeseqno(row); > > > > if (ovsdb_idl_row_is_orphan(row)) { > > ovsdb_idl_row_unparse(row); > > @@ -1554,6 +1556,8 @@ ovsdb_idl_reparse_deleted(struct ovsdb_idl *db) > > if (ovs_list_is_empty(&row->dst_arcs) > > || ovsdb_idl_track_is_set(row->table)) { > > ovsdb_idl_row_track_change(row, OVSDB_IDL_CHANGE_DELETE); > > + } else { > > + ovsdb_idl_row_clear_changeseqno(row); > > I'm not sure we need this call to ovsdb_idl_row_clear_changeseqno() > here. If I comment it out all tests still pass. What case is it taking > care of? > > If a deleted row is "re-inserted" we already cleared the change seqno in > ovsdb_idl_process_update(). > > If a deleted row is not referenced anymore (no dst arcs) then it's fine > to keep its deletion change seqno. > > Or am I missing something?
No, that's a good point. I can resubmit without that line. -M > > FWIW, I did run the OVN test suite with this OVS change applied and > those tests passed too: > > https://github.com/dceara/ovn/actions/runs/7180690942 > > Thanks, > Dumitru > > > } > > } > > } > > @@ -1632,6 +1636,7 @@ ovsdb_idl_process_update(struct ovsdb_idl_table > > *table, > > ru->columns); > > } else if (ovsdb_idl_row_is_orphan(row)) { > > ovsdb_idl_row_untrack_change(row); > > + ovsdb_idl_row_clear_changeseqno(row); > > ovsdb_idl_insert_row(row, ru->columns); > > } else { > > VLOG_ERR_RL(&semantic_rl, "cannot add existing row "UUID_FMT" > > to " > > @@ -2283,11 +2288,15 @@ ovsdb_idl_row_untrack_change(struct ovsdb_idl_row > > *row) > > return; > > } > > > > + ovs_list_remove(&row->track_node); > > + ovs_list_init(&row->track_node); > > +} > > + > > +static void ovsdb_idl_row_clear_changeseqno(struct ovsdb_idl_row *row) > > +{ > > 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); > > } > > > > static struct ovsdb_idl_row * > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
