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.
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.
What do you think?
Dumitru, maybe you have some thoughts on this?
And we will need need a unit test for this issue.
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
