Hi, Ilya. Thanks for your comment.
On 7/4/23 4:10 AM, Ilya Maximets wrote:
On 7/2/23 11:59, Tao Liu wrote:
Commit 02f31a1262fc has fixed the row references when insertion and
deletion execute in one IDL run. However, we can still hit ovn-controller
crash in pressure test where pb->datapath == NULL. It is triggered by
reference dst deletion in same IDL run:
idl run:
update1:
insert port_binding
insert datapath_binding
update2:
delete datapath_binding
delete port_binding
Fix it by reparse reference src before dst deletion.
Hi. Thanks for the patch!
There seems to be an issue indeed. Removed rows should have valid
references to other removed rows.
However, I'm not sure this solution solves the issue in a general case
as it depends on the order of events. If events in the update2 will
be in opposite order, the issue will still be there.
The opposite order of update2 has been fixed in commit 02f31a1262fc,
which now moved to
first reparse_row().
In a more general case we may also have two rows reference each other
and we need to preserve both references.
We probably could:
1. Postpone all the deletions, execute insertions and updates.
2. Call ovsdb_idl_reparse_refs_to_inserted() to re-parse every row
that needs re-parsing due to newly added rows.
3. Delete the rows that need to be deleted.
4. Call ovsdb_idl_reparse_deleted() to get rid of references to
deleted rows from non-deleted ones.
Sounds like a good idea, that makes things more clear.
What do you think?
Dumitru, maybe you have some thoughts on this?
And we will need need a unit test for this issue.
Will try to add one.
Best regards, Ilya Maximets.
Fixes: 02f31a1262fc ("ovsdb-idl: Preserve references for rows deleted in same IDL
run as their insertion.")
Signed-off-by: Tao Liu <[email protected]>
---
lib/ovsdb-idl.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index 634fbb56d..89393b52b 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -2425,11 +2425,25 @@ ovsdb_idl_insert_row(struct ovsdb_idl_row *row, const
struct shash *data)
}
static void
-ovsdb_idl_delete_row(struct ovsdb_idl_row *row)
+reparse_row(struct ovsdb_idl_row *row)
{
- /* If row has to be reparsed, reparse it before it's deleted. */
if (!ovs_list_is_empty(&row->reparse_node)) {
ovsdb_idl_row_parse(row);
+ ovs_list_remove(&row->reparse_node);
+ ovs_list_init(&row->reparse_node);
+ }
+}
+
+static void
+ovsdb_idl_delete_row(struct ovsdb_idl_row *row)
+{
+ struct ovsdb_idl_arc *arc;
+
+ /* If row has to be reparsed, reparse it before it's deleted. */
+ reparse_row(row);
+ /* Reparse src before dst is deleted */
+ LIST_FOR_EACH (arc, dst_node, &row->dst_arcs) {
+ reparse_row(arc->src);
}
ovsdb_idl_remove_from_indexes(row);
ovsdb_idl_row_clear_arcs(row, true);
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
