On Mon, Nov 30, 2020 at 8:41 AM Dumitru Ceara <dce...@redhat.com> wrote:
>
> Considering the following updates processed by an IDL client:
> 1. Delete row R1 from table A while R1 is also referenced by row R2 from
> table B:
> - because row R2 still refers to row R1, this will create an orphan
> R1 but also sets row->tracked_old_datum to report to the IDL client
> that the row has been deleted.
> 2. Insert row R1 to table A.
> - because orphan R1 already existed in the IDL, it will be reused.
> - R1 still has row->tracked_old_datum set (and may also be on the
> table->track_list).
> 3. Delete row R2 from table B and row R1 from table A.
> - row->tracked_old_datum is set again but the previous
> tracked_old_datum was never freed.
>
> IDL clients use the deleted old_datum values so when multiple delete
> operations are received for a row, always track the first one as that
> will match the contents of the row the IDL client knew about.
>
> Running the newly added test case with valgrind, without the fix,
> produces the following report:
>
> ==23113== 327 (240 direct, 87 indirect) bytes in 1 blocks are definitely
lost in loss record 43 of 43
> ==23113== at 0x4C29F73: malloc (vg_replace_malloc.c:309)
> ==23113== by 0x476761: xmalloc (util.c:138)
> ==23113== by 0x45D8B3: ovsdb_idl_insert_row (ovsdb-idl.c:3431)
> ==23113== by 0x45B7F9: ovsdb_idl_process_update2 (ovsdb-idl.c:2670)
> ==23113== by 0x45AFCF: ovsdb_idl_db_parse_update__ (ovsdb-idl.c:2479)
> ==23113== by 0x45B262: ovsdb_idl_db_parse_update (ovsdb-idl.c:2542)
> ==23113== by 0x45ABBE: ovsdb_idl_db_parse_update_rpc (ovsdb-idl.c:2358)
> ==23113== by 0x4576DD: ovsdb_idl_process_msg (ovsdb-idl.c:865)
> ==23113== by 0x457973: ovsdb_idl_run (ovsdb-idl.c:944)
> ==23113== by 0x40B7B9: do_idl (test-ovsdb.c:2523)
> ==23113== by 0x44425D: ovs_cmdl_run_command__ (command-line.c:247)
> ==23113== by 0x44430E: ovs_cmdl_run_command (command-line.c:278)
> ==23113== by 0x404BA6: main (test-ovsdb.c:76)
>
> Fixes: 72aeb243a52a ("ovsdb-idl: Tracking - preserve data for deleted
rows.")
> Signed-off-by: Dumitru Ceara <dce...@redhat.com>
> ---
> lib/ovsdb-idl.c | 2 +-
> tests/ovsdb-idl.at | 52
++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 53 insertions(+), 1 deletion(-)
>
> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> index 23648ff..6afae2d 100644
> --- a/lib/ovsdb-idl.c
> +++ b/lib/ovsdb-idl.c
> @@ -3227,7 +3227,7 @@ ovsdb_idl_row_clear_old(struct ovsdb_idl_row *row)
> {
> ovs_assert(row->old_datum == row->new_datum);
> if (!ovsdb_idl_row_is_orphan(row)) {
> - if (ovsdb_idl_track_is_set(row->table)) {
> + if (ovsdb_idl_track_is_set(row->table) &&
!row->tracked_old_datum) {
> row->tracked_old_datum = row->old_datum;
> } else {
> const struct ovsdb_idl_table_class *class =
row->table->class_;
> diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
> index 406a576..4b4791a 100644
> --- a/tests/ovsdb-idl.at
> +++ b/tests/ovsdb-idl.at
> @@ -1225,6 +1225,58 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl,
initially populated, orphan weak refer
> 008: done
> ]])
>
> +dnl This test creates database with weak references and checks that the
> +dnl content of orphaned rows created for weak references after monitor
> +dnl condition change are not leaked when the row is reinserted and
deleted.
> +OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated, orphan
rows, conditional],
> + [['["idltest",
> + {"op": "insert",
> + "table": "simple",
> + "row": {"s": "row0_s"},
> + "uuid-name": "weak_row0"},
> + {"op": "insert",
> + "table": "simple",
> + "row": {"s": "row1_s"},
> + "uuid-name": "weak_row1"},
> + {"op": "insert",
> + "table": "simple6",
> + "row": {"name": "first_row",
> + "weak_ref": ["set",
> + [["named-uuid", "weak_row0"]]
> + ]}}]']],
> + [['condition simple []' \
> + 'condition simple [["s","==","row0_s"]]' \
> + 'condition simple [["s","==","row1_s"]]' \
> + 'condition simple [["s","==","row0_s"]]' \
> + '["idltest",
> + {"op": "delete",
> + "table": "simple6",
> + "where": []}]']],
> + [[000: change conditions
> +001: inserted row: uuid=<0>
> +001: name=first_row weak_ref=[] uuid=<0>
> +001: updated columns: name weak_ref
> +002: change conditions
> +003: i=0 r=0 b=false s=row0_s u=<1> ia=[] ra=[] ba=[] sa=[] ua=[]
uuid=<2>
> +003: inserted row: uuid=<2>
> +003: name=first_row weak_ref=[<2>] uuid=<0>
> +003: updated columns: s
> +004: change conditions
> +005: i=0 r=0 b=false s=row1_s u=<1> ia=[] ra=[] ba=[] sa=[] ua=[]
uuid=<3>
> +005: inserted row: uuid=<3>
> +005: updated columns: s
> +006: change conditions
> +007: deleted row: uuid=<3>
> +007: i=0 r=0 b=false s=row0_s u=<1> ia=[] ra=[] ba=[] sa=[] ua=[]
uuid=<2>
> +007: i=0 r=0 b=false s=row1_s u=<1> ia=[] ra=[] ba=[] sa=[] ua=[]
uuid=<3>
> +007: inserted row: uuid=<2>
> +007: name=first_row weak_ref=[<2>] uuid=<0>
> +007: updated columns: s
> +008: {"error":null,"result":[{"count":1}]}
> +009: i=0 r=0 b=false s=row0_s u=<1> ia=[] ra=[] ba=[] sa=[] ua=[]
uuid=<2>
> +010: done
> +]])
> +
> OVSDB_CHECK_IDL_TRACK([track, simple idl, initially empty, various ops],
> [],
> [['["idltest",
>
Acked-by: Han Zhou <hz...@ovn.org>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
