On 12/12/23 16:03, Mike Pattrick wrote: > 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. >
I guess that might be best. Thanks! > -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
