On 1/11/24 16:02, Dumitru Ceara wrote:
> 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 <m...@redhat.com>
>>>> Signed-off-by: Ilya Maximets <i.maxim...@ovn.org>
>>>> ---
>>>> 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 <dce...@redhat.com>
Thanks, Mike and Dumitru! I added this, addessed nits from the patch
#2 and applied the set.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
