On 1/11/24 15:44, Ilya Maximets wrote: > On 1/10/24 14:12, Dumitru Ceara wrote: >> On 1/9/24 20:54, Ilya Maximets wrote: >>> Database file contains the column diff, but it is discarded once >>> the 'new' state of a row is constructed. Keep it in the transaction >>> row, as it can be used later by other parts of the code. >>> >>> Diffs do not live long, we keep them around only while transaction >>> is alive, so should not affect memory consumption. >>> >>> Users for this data will be added in later commits. >>> >>> Acked-by: Mike Pattrick <[email protected]> >>> Signed-off-by: Ilya Maximets <[email protected]> >>> --- >>> ovsdb/execution.c | 14 ++++++++--- >>> ovsdb/file.c | 22 +++++++++++----- >>> ovsdb/ovsdb-server.c | 7 ++++-- >>> ovsdb/table.c | 6 +++-- >>> ovsdb/transaction.c | 60 +++++++++++++++++++++++++++++++++----------- >>> ovsdb/transaction.h | 6 +++-- >>> tests/test-ovsdb.c | 2 +- >>> 7 files changed, 86 insertions(+), 31 deletions(-) >>> >>> diff --git a/ovsdb/execution.c b/ovsdb/execution.c >>> index 5587ef96f..8c20c3b54 100644 >>> --- a/ovsdb/execution.c >>> +++ b/ovsdb/execution.c >>> @@ -490,9 +490,11 @@ update_row_cb(const struct ovsdb_row *row, void *ur_) >>> >>> ur->n_matches++; >>> if (!ovsdb_row_equal_columns(row, ur->row, ur->columns)) { >>> + struct ovsdb_row *rw_row; >>> + >>> + ovsdb_txn_row_modify(ur->txn, row, &rw_row, NULL); >>> ovsdb_error_assert(ovsdb_row_update_columns( >>> - ovsdb_txn_row_modify(ur->txn, row), >>> - ur->row, ur->columns, false)); >>> + rw_row, ur->row, ur->columns, false)); >>> } >>> >>> return true; >>> @@ -572,10 +574,14 @@ static bool >>> mutate_row_cb(const struct ovsdb_row *row, void *mr_) >>> { >>> struct mutate_row_cbdata *mr = mr_; >>> + struct ovsdb_row *rw_row; >>> + >>> + /* Not trying to track the row diff here, because user transactions >>> + * may attempt to add duplicates or remove elements that do not exist. >>> */ >>> + ovsdb_txn_row_modify(mr->txn, row, &rw_row, NULL); >>> >>> mr->n_matches++; >>> - *mr->error = ovsdb_mutation_set_execute(ovsdb_txn_row_modify(mr->txn, >>> row), >>> - mr->mutations); >>> + *mr->error = ovsdb_mutation_set_execute(rw_row, mr->mutations); >>> return *mr->error == NULL; >>> } >>> >>> diff --git a/ovsdb/file.c b/ovsdb/file.c >>> index 8bd1d4af3..77a89fd1a 100644 >>> --- a/ovsdb/file.c >>> +++ b/ovsdb/file.c >>> @@ -80,8 +80,8 @@ ovsdb_file_column_diff_disable(void) >>> } >>> >>> static struct ovsdb_error * >>> -ovsdb_file_update_row_from_json(struct ovsdb_row *row, bool converting, >>> - bool row_contains_diff, >>> +ovsdb_file_update_row_from_json(struct ovsdb_row *row, struct ovsdb_row >>> *diff, >>> + bool converting, bool row_contains_diff, >>> const struct json *json) >>> { >>> struct ovsdb_table_schema *schema = row->table->schema; >>> @@ -122,6 +122,12 @@ ovsdb_file_update_row_from_json(struct ovsdb_row *row, >>> bool converting, >>> error = ovsdb_datum_apply_diff_in_place( >>> &row->fields[column->index], >>> &datum, &column->type); >>> + if (!error && diff) { >>> + >>> ovs_assert(ovsdb_datum_is_default(&diff->fields[column->index], >>> + &column->type)); >>> + ovsdb_datum_swap(&diff->fields[column->index], &datum); >>> + } >>> + >>> ovsdb_datum_destroy(&datum, &column->type); >>> if (error) { >>> return error; >>> @@ -150,16 +156,20 @@ ovsdb_file_txn_row_from_json(struct ovsdb_txn *txn, >>> struct ovsdb_table *table, >>> ovsdb_txn_row_delete(txn, row); >>> return NULL; >>> } else if (row) { >>> - return ovsdb_file_update_row_from_json(ovsdb_txn_row_modify(txn, >>> row), >>> - converting, >>> row_contains_diff, >>> - json); >>> + struct ovsdb_row *new, *diff = NULL; >>> + >>> + ovsdb_txn_row_modify(txn, row, &new, >>> + row_contains_diff ? &diff : NULL); >>> + return ovsdb_file_update_row_from_json(new, diff, converting, >>> + row_contains_diff, json); >>> } else { >>> struct ovsdb_error *error; >>> struct ovsdb_row *new; >>> >>> new = ovsdb_row_create(table); >>> *ovsdb_row_get_uuid_rw(new) = *row_uuid; >>> - error = ovsdb_file_update_row_from_json(new, converting, false, >>> json); >>> + error = ovsdb_file_update_row_from_json(new, NULL, converting, >>> + false, json); >>> if (error) { >>> ovsdb_row_destroy(new); >>> } else { >>> diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c >>> index 4d29043f4..dbf85fe3b 100644 >>> --- a/ovsdb/ovsdb-server.c >>> +++ b/ovsdb/ovsdb-server.c >>> @@ -1111,7 +1111,7 @@ update_remote_row(const struct ovsdb_row *row, struct >>> ovsdb_txn *txn, >>> /* Bad remote spec or incorrect schema. */ >>> return; >>> } >>> - rw_row = ovsdb_txn_row_modify(txn, row); >>> + ovsdb_txn_row_modify(txn, row, &rw_row, NULL); >>> ovsdb_jsonrpc_server_get_remote_status(jsonrpc, target, &status); >>> >>> /* Update status information columns. */ >>> @@ -1301,7 +1301,10 @@ update_server_status(struct shash *all_dbs) >>> if (!db || !db->db) { >>> ovsdb_txn_row_delete(txn, row); >>> } else { >>> - update_database_status(ovsdb_txn_row_modify(txn, row), db); >>> + struct ovsdb_row *rw_row; >>> + >>> + ovsdb_txn_row_modify(txn, row, &rw_row, NULL); >>> + update_database_status(rw_row, db); >>> } >>> } >>> >>> diff --git a/ovsdb/table.c b/ovsdb/table.c >>> index 0792e1580..3e89ddd44 100644 >>> --- a/ovsdb/table.c >>> +++ b/ovsdb/table.c >>> @@ -415,8 +415,10 @@ ovsdb_table_execute_update(struct ovsdb_txn *txn, >>> const struct uuid *row_uuid, >>> NULL, &columns, xor); >>> >>> if (!error && (xor || !ovsdb_row_equal_columns(row, update, >>> &columns))) { >>> - error = ovsdb_row_update_columns(ovsdb_txn_row_modify(txn, row), >>> - update, &columns, xor); >>> + struct ovsdb_row *rw_row; >>> + >>> + ovsdb_txn_row_modify(txn, row, &rw_row, NULL); >>> + error = ovsdb_row_update_columns(rw_row, update, &columns, xor); >>> } >>> >>> ovsdb_column_set_destroy(&columns); >>> diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c >>> index a482588a0..b69d03b5a 100644 >>> --- a/ovsdb/transaction.c >>> +++ b/ovsdb/transaction.c >>> @@ -72,6 +72,8 @@ struct ovsdb_txn_table { >>> * 'new'. >>> * >>> * - A row modified by a transaction will have non-null 'old' and >>> 'new'. >>> + * It may have non-null 'diff' as well in this case, but it is not >>> + * necessary. >>> * >>> * - 'old' and 'new' both null indicates that a row was added then >>> deleted >>> * within a single transaction. Most of the time we instead delete >>> the >>> @@ -83,6 +85,7 @@ struct ovsdb_txn_row { >>> struct hmap_node hmap_node; /* In ovsdb_txn_table's txn_rows hmap. */ >>> struct ovsdb_row *old; /* The old row. */ >>> struct ovsdb_row *new; /* The new row. */ >>> + struct ovsdb_row *diff; /* The difference between old and new. */ >>> size_t n_refs; /* Number of remaining references. */ >>> >>> /* These members are the same as the corresponding members of 'old' or >>> @@ -155,6 +158,7 @@ ovsdb_txn_row_abort(struct ovsdb_txn *txn OVS_UNUSED, >>> { >>> struct ovsdb_row *old = txn_row->old; >>> struct ovsdb_row *new = txn_row->new; >>> + struct ovsdb_row *diff = txn_row->diff; >>> >>> ovsdb_txn_row_prefree(txn_row); >>> if (!old) { >>> @@ -184,6 +188,7 @@ ovsdb_txn_row_abort(struct ovsdb_txn *txn OVS_UNUSED, >>> } >>> >>> ovsdb_row_destroy(new); >>> + ovsdb_row_destroy(diff); >>> free(txn_row); >>> >>> return NULL; >>> @@ -250,7 +255,10 @@ find_or_make_txn_row(struct ovsdb_txn *txn, const >>> struct ovsdb_table *table, >>> if (!txn_row) { >>> const struct ovsdb_row *row = ovsdb_table_get_row(table, uuid); >>> if (row) { >>> - txn_row = ovsdb_txn_row_modify(txn, row)->txn_row; >>> + struct ovsdb_row *rw_row; >>> + >>> + ovsdb_txn_row_modify(txn, row, &rw_row, NULL); >>> + txn_row = rw_row->txn_row; >>> } >>> } >>> return txn_row; >>> @@ -433,6 +441,7 @@ delete_garbage_row(struct ovsdb_txn *txn, struct >>> ovsdb_txn_row *txn_row) >>> return NULL; >>> } >>> >>> + ovsdb_row_destroy(txn_row->diff); >> >> I think in practice 'txn_row' will always be destroyed by the time we >> exit the delete_garbage_row() function. But given that we do have some >> theoretical error cases (when delete_row_refs() returns error) in which >> we don't destroy 'row' should we set txn_row->diff to NULL here? > > delete_row_refs() can't actually fail, because we just checked the > referential integrity of strong references before colliecting > garbage rows. And you may see that garbage collection failure is > wrapped in OVSDB_WRAP_BUG("can't happen", error). And it actually > can't happen, otherwise we have a serious referential integrity > bug in ovsdb-server. >
Right, I saw that, that's why in practice this will probably never happen. > But I can add txn_row->diff = NULL, just to not crash somewhere > later because of this, even if we could crash due to referential > integrity issues. > > What do you think? > Sounds good. With that: Acked-by: Dumitru Ceara <[email protected]> >> >> OTOH, just below we remove 'row' from the hmap so it's not reachable >> after we exit this function. Is that an (unrelated) theoretical memory >> leak? > > Yeah, the 'new' row will be leaked, but it is an unrelated issue > that can't happen. We can adjust that code separately. > Ok. >> >> The rest looks good to me. >> >> Thanks, >> Dumitru >> >>> row = txn_row->new; >>> txn_row->new = NULL; >>> hmap_remove(&txn_row->table->rows, &row->hmap_node); >>> @@ -544,6 +553,7 @@ ovsdb_txn_row_commit(struct ovsdb_txn *txn OVS_UNUSED, >>> txn_row->new->n_refs = txn_row->n_refs; >>> } >>> ovsdb_row_destroy(txn_row->old); >>> + ovsdb_row_destroy(txn_row->diff); >>> free(txn_row); >>> >>> return NULL; >>> @@ -1178,6 +1188,7 @@ ovsdb_txn_destroy_cloned(struct ovsdb_txn *txn) >>> if (r->new) { >>> ovsdb_row_destroy(r->new); >>> } >>> + ovs_assert(!r->diff); >>> hmap_remove(&t->txn_rows, &r->hmap_node); >>> free(r); >>> } >>> @@ -1439,7 +1450,8 @@ ovsdb_txn_create_txn_table(struct ovsdb_txn *txn, >>> struct ovsdb_table *table) >>> >>> static struct ovsdb_txn_row * >>> ovsdb_txn_row_create(struct ovsdb_txn *txn, struct ovsdb_table *table, >>> - const struct ovsdb_row *old_, struct ovsdb_row *new) >>> + const struct ovsdb_row *old_, struct ovsdb_row *new, >>> + struct ovsdb_row *diff) >>> { >>> const struct ovsdb_row *row = old_ ? old_ : new; >>> struct ovsdb_row *old = CONST_CAST(struct ovsdb_row *, old_); >>> @@ -1453,6 +1465,7 @@ ovsdb_txn_row_create(struct ovsdb_txn *txn, struct >>> ovsdb_table *table, >>> txn_row->table = row->table; >>> txn_row->old = old; >>> txn_row->new = new; >>> + txn_row->diff = diff; >>> txn_row->n_refs = old ? old->n_refs : 0; >>> txn_row->serial = serial - 1; >>> >>> @@ -1465,6 +1478,9 @@ ovsdb_txn_row_create(struct ovsdb_txn *txn, struct >>> ovsdb_table *table, >>> if (new) { >>> new->txn_row = txn_row; >>> } >>> + if (diff) { >>> + diff->txn_row = txn_row; >>> + } >>> >>> txn_table = ovsdb_txn_create_txn_table(txn, table); >>> hmap_insert(&txn_table->txn_rows, &txn_row->hmap_node, >>> @@ -1473,24 +1489,38 @@ ovsdb_txn_row_create(struct ovsdb_txn *txn, struct >>> ovsdb_table *table, >>> return txn_row; >>> } >>> >>> -struct ovsdb_row * >>> -ovsdb_txn_row_modify(struct ovsdb_txn *txn, const struct ovsdb_row >>> *ro_row_) >>> +void >>> +ovsdb_txn_row_modify(struct ovsdb_txn *txn, const struct ovsdb_row >>> *ro_row_, >>> + struct ovsdb_row **rw_row, struct ovsdb_row **diff) >>> { >>> struct ovsdb_row *ro_row = CONST_CAST(struct ovsdb_row *, ro_row_); >>> >>> + ovs_assert(ro_row); >>> + ovs_assert(rw_row); >>> + >>> if (ro_row->txn_row) { >>> ovs_assert(ro_row == ro_row->txn_row->new); >>> - return ro_row; >>> + *rw_row = ro_row; >>> + if (diff) { >>> + *diff = ro_row->txn_row->diff; >>> + } else { >>> + /* Caller will modify the row without updating the diff. >>> + * Destroy the existing diff, if any, so it will not be >>> + * used for this row anymore. Modification will always >>> + * return NULL from this point on. */ >>> + ovsdb_row_destroy(ro_row->txn_row->diff); >>> + ro_row->txn_row->diff = NULL; >>> + } >>> } else { >>> struct ovsdb_table *table = ro_row->table; >>> - struct ovsdb_row *rw_row; >>> >>> - rw_row = ovsdb_row_clone(ro_row); >>> - rw_row->n_refs = ro_row->n_refs; >>> - ovsdb_txn_row_create(txn, table, ro_row, rw_row); >>> - hmap_replace(&table->rows, &ro_row->hmap_node, &rw_row->hmap_node); >>> - >>> - return rw_row; >>> + *rw_row = ovsdb_row_clone(ro_row); >>> + (*rw_row)->n_refs = ro_row->n_refs; >>> + if (diff) { >>> + *diff = ovsdb_row_create(table); >>> + } >>> + ovsdb_txn_row_create(txn, table, ro_row, *rw_row, diff ? *diff : >>> NULL); >>> + hmap_replace(&table->rows, &ro_row->hmap_node, >>> &(*rw_row)->hmap_node); >>> } >>> } >>> >>> @@ -1502,7 +1532,7 @@ ovsdb_txn_row_insert(struct ovsdb_txn *txn, struct >>> ovsdb_row *row) >>> >>> uuid_generate(ovsdb_row_get_version_rw(row)); >>> >>> - ovsdb_txn_row_create(txn, table, NULL, row); >>> + ovsdb_txn_row_create(txn, table, NULL, row, NULL); >>> hmap_insert(&table->rows, &row->hmap_node, hash); >>> } >>> >>> @@ -1518,9 +1548,11 @@ ovsdb_txn_row_delete(struct ovsdb_txn *txn, const >>> struct ovsdb_row *row_) >>> hmap_remove(&table->rows, &row->hmap_node); >>> >>> if (!txn_row) { >>> - ovsdb_txn_row_create(txn, table, row, NULL); >>> + ovsdb_txn_row_create(txn, table, row, NULL, NULL); >>> } else { >>> ovs_assert(txn_row->new == row); >>> + ovsdb_row_destroy(txn_row->diff); >>> + txn_row->diff = NULL; >>> if (txn_row->old) { >>> txn_row->new = NULL; >>> } else { >>> diff --git a/ovsdb/transaction.h b/ovsdb/transaction.h >>> index 0e054eef3..f659838dc 100644 >>> --- a/ovsdb/transaction.h >>> +++ b/ovsdb/transaction.h >>> @@ -21,6 +21,7 @@ >>> >>> struct json; >>> struct ovsdb; >>> +struct ovsdb_row; >>> struct ovsdb_schema; >>> struct ovsdb_table; >>> struct uuid; >>> @@ -50,8 +51,9 @@ const struct ovsdb_error *ovsdb_txn_progress_get_error( >>> const struct ovsdb_txn_progress *); >>> void ovsdb_txn_progress_destroy(struct ovsdb_txn_progress *); >>> >>> -struct ovsdb_row *ovsdb_txn_row_modify(struct ovsdb_txn *, >>> - const struct ovsdb_row *); >>> +void ovsdb_txn_row_modify(struct ovsdb_txn *, const struct ovsdb_row *, >>> + struct ovsdb_row **row_new, >>> + struct ovsdb_row **row_diff); >>> void ovsdb_txn_row_insert(struct ovsdb_txn *, struct ovsdb_row *); >>> void ovsdb_txn_row_delete(struct ovsdb_txn *, const struct ovsdb_row *); >>> >>> diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c >>> index c761822e6..0394eafd2 100644 >>> --- a/tests/test-ovsdb.c >>> +++ b/tests/test-ovsdb.c >>> @@ -1798,7 +1798,7 @@ do_transact_modify(struct ovs_cmdl_context *ctx) >>> struct ovsdb_row *row_rw; >>> >>> row_ro = do_transact_find_row(ctx->argv[1]); >>> - row_rw = ovsdb_txn_row_modify(do_transact_txn, row_ro); >>> + ovsdb_txn_row_modify(do_transact_txn, row_ro, &row_rw, NULL); >>> do_transact_set_i_j(row_rw, ctx->argv[2], ctx->argv[3]); >>> } >>> >> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
