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