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?
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
