[ovs-dev] [gc 11/13] ovsdb: Provide a way for for_each_txn_row() callback to delete any row.
Ethan Jackson
ethan at nicira.com
Fri Mar 4 22:38:01 UTC 2011
Looks Good.
On Tue, Mar 1, 2011 at 2:19 PM, Ben Pfaff <blp at nicira.com> wrote:
> for_each_txn_row() restricts the txn_rows that its callback may delete.
> Until now, this has meant that its callback could not delete any rows
> that were created within the transaction being processed. These rows have
> txn_rows with null 'old' and nonnull 'new', so to delete them requires
> either removing the txn_row entirely (forbidden by for_each_txn_row()) or
> clearing its 'new' to null. The latter is forbidden because a txn_row
> is not allowed to have both 'old' and 'new' null.
>
> Until now, this has not been a significant restriction, because none of
> the processing at transaction commit time required deleting arbitrary rows.
> Implementing garbage collection, however, does require this ability, so
> this commit makes it possible by eliminating the requirement that at least
> 'old' or 'new' be nonnull.
> ---
> ovsdb/transaction.c | 46 ++++++++++++++++++++++++++++------------------
> 1 files changed, 28 insertions(+), 18 deletions(-)
>
> diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c
> index b7c57e5..f67018b 100644
> --- a/ovsdb/transaction.c
> +++ b/ovsdb/transaction.c
> @@ -57,9 +57,11 @@ struct ovsdb_txn_table {
> *
> * - A row modified by a transaction will have non-null 'old' and 'new'.
> *
> - * - 'old' and 'new' both null is invalid. It would indicate that a row
> - * was added then deleted within a single transaction, but we instead
> - * handle that case by deleting the txn_row entirely.
> + * - '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
> + * ovsdb_txn_row entirely, but inside a for_each_txn_row() callback
> + * there are restrictions that sometimes mean we have to leave the
> + * ovsdb_txn_row in place.
> */
> struct ovsdb_txn_row {
> struct hmap_node hmap_node; /* In ovsdb_txn_table's txn_rows hmap. */
> @@ -67,6 +69,12 @@ struct ovsdb_txn_row {
> struct ovsdb_row *new; /* The new row. */
> size_t n_refs; /* Number of remaining references. */
>
> + /* These members are the same as the corresponding members of 'old' or
> + * 'new'. They are present here for convenience and because occasionally
> + * there can be an ovsdb_txn_row where both 'old' and 'new' are NULL. */1
> + struct uuid uuid;
> + struct ovsdb_table *table;
> +
> /* Used by for_each_txn_row(). */
> unsigned int serial; /* Serial number of in-progress commit. */
>
> @@ -110,7 +118,9 @@ ovsdb_txn_row_abort(struct ovsdb_txn *txn OVS_UNUSED,
>
> ovsdb_txn_row_prefree(txn_row);
> if (!old) {
> - hmap_remove(&new->table->rows, &new->hmap_node);
> + if (new) {
> + hmap_remove(&new->table->rows, &new->hmap_node);
> + }
> } else if (!new) {
> hmap_insert(&old->table->rows, &old->hmap_node, ovsdb_row_hash(old));
> } else {
> @@ -140,10 +150,7 @@ find_txn_row(const struct ovsdb_table *table, const struct uuid *uuid)
>
> HMAP_FOR_EACH_WITH_HASH (txn_row, hmap_node,
> uuid_hash(uuid), &table->txn_table->txn_rows) {
> - const struct ovsdb_row *row;
> -
> - row = txn_row->old ? txn_row->old : txn_row->new;
> - if (uuid_equals(uuid, ovsdb_row_get_uuid(row))) {
> + if (uuid_equals(uuid, &txn_row->uuid)) {
> return txn_row;
> }
> }
> @@ -208,7 +215,7 @@ ovsdb_txn_adjust_row_refs(struct ovsdb_txn *txn, const struct ovsdb_row *r,
> static struct ovsdb_error * WARN_UNUSED_RESULT
> update_row_ref_count(struct ovsdb_txn *txn, struct ovsdb_txn_row *r)
> {
> - struct ovsdb_table *table = r->old ? r->old->table : r->new->table;
> + struct ovsdb_table *table = r->table;
> struct shash_node *node;
>
> SHASH_FOR_EACH (node, &table->schema->columns) {
> @@ -241,8 +248,7 @@ check_ref_count(struct ovsdb_txn *txn OVS_UNUSED, struct ovsdb_txn_row *r)
> return ovsdb_error("referential integrity violation",
> "cannot delete %s row "UUID_FMT" because "
> "of %zu remaining reference(s)",
> - r->old->table->schema->name,
> - UUID_ARGS(ovsdb_row_get_uuid(r->old)),
> + r->table->schema->name, UUID_ARGS(&r->uuid),
> r->n_refs);
> }
> }
> @@ -328,7 +334,7 @@ assess_weak_refs(struct ovsdb_txn *txn, struct ovsdb_txn_row *txn_row)
> return NULL;
> }
>
> - table = txn_row->new->table;
> + table = txn_row->table;
> SHASH_FOR_EACH (node, &table->schema->columns) {
> const struct ovsdb_column *column = node->data;
> struct ovsdb_datum *datum = &txn_row->new->fields[column->index];
> @@ -412,9 +418,8 @@ assess_weak_refs(struct ovsdb_txn *txn, struct ovsdb_txn_row *txn_row)
> static struct ovsdb_error * WARN_UNUSED_RESULT
> determine_changes(struct ovsdb_txn *txn, struct ovsdb_txn_row *txn_row)
> {
> - struct ovsdb_table *table;
> + struct ovsdb_table *table = txn_row->table;
>
> - table = (txn_row->old ? txn_row->old : txn_row->new)->table;
> if (txn_row->old && txn_row->new) {
> struct shash_node *node;
> bool changed = false;
> @@ -534,7 +539,7 @@ ovsdb_txn_for_each_change(const struct ovsdb_txn *txn,
>
> LIST_FOR_EACH (t, node, &txn->txn_tables) {
> HMAP_FOR_EACH (r, hmap_node, &t->txn_rows) {
> - if (!cb(r->old, r->new, r->changed, aux)) {
> + if ((r->old || r->new) && !cb(r->old, r->new, r->changed, aux)) {
> break;
> }
> }
> @@ -560,6 +565,7 @@ 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 *row = old_ ? old_ : new;
> struct ovsdb_row *old = (struct ovsdb_row *) old_;
> size_t n_columns = shash_count(&table->schema->columns);
> struct ovsdb_txn_table *txn_table;
> @@ -567,7 +573,9 @@ ovsdb_txn_row_create(struct ovsdb_txn *txn, struct ovsdb_table *table,
>
> txn_row = xzalloc(offsetof(struct ovsdb_txn_row, changed)
> + bitmap_n_bytes(n_columns));
> - txn_row->old = (struct ovsdb_row *) old;
> + txn_row->uuid = *ovsdb_row_get_uuid(row);
> + txn_row->table = row->table;
> + txn_row->old = old;
> txn_row->new = new;
> txn_row->n_refs = old ? old->n_refs : 0;
> txn_row->serial = serial - 1;
> @@ -663,8 +671,7 @@ ovsdb_txn_get_comment(const struct ovsdb_txn *txn)
> static void
> ovsdb_txn_row_prefree(struct ovsdb_txn_row *txn_row)
> {
> - struct ovsdb_row *row = txn_row->old ? txn_row->old : txn_row->new;
> - struct ovsdb_txn_table *txn_table = row->table->txn_table;
> + struct ovsdb_txn_table *txn_table = txn_row->table->txn_table;
>
> txn_table->n_processed--;
> hmap_remove(&txn_table->txn_rows, &txn_row->hmap_node);
> @@ -697,6 +704,9 @@ ovsdb_txn_table_destroy(struct ovsdb_txn_table *txn_table)
> * in within the same txn_table. It may *not* delete any txn_tables. As long
> * as these rules are followed, 'cb' will be called exactly once for each
> * txn_row in 'txn', even those added by 'cb'.
> + *
> + * (Even though 'cb' is not allowed to delete some txn_rows, it can still
> + * delete any actual row by clearing a txn_row's 'new' member.)
> */
> static struct ovsdb_error * WARN_UNUSED_RESULT
> for_each_txn_row(struct ovsdb_txn *txn,
> --
> 1.7.1
>
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev_openvswitch.org
>
More information about the dev
mailing list