[ovs-dev] [PATCH] ovsdb: Update _version more accurately in transaction commit.

[Andy Zhou]

Acked-by: Andy Zhou <azhou at nicira.com>

On Mon, Aug 31, 2015 at 9:52 AM, Ben Pfaff <blp at nicira.com> wrote:
> The _version column in each OVSDB row is supposed to be updated whenever
> any other column in the row changes.  However, the transaction code was
> not careful to do this only when a row actually changed--there were other
> cases where a row was considered at transaction commit time and _version
> updated even though the row did not actually change.  For example,
> ovsdb_txn_adjust_atom_refs() calls find_or_make_txn_row(), which calls
> ovsdb_txn_row_modify(), which updates _version, but
> ovsdb_txn_adjust_atom_refs() doesn't actually update any data.
>
> One way to fix this would be to carefully consider and adjust all the code
> that looks at transaction rows.  However, this seems somewhat error prone
> and thus difficult to test.  This commit takes a different approach: it
> drops the code that adjusts _version on the fly, instead replacing it by
> a final pass over the database at the end of the commit process that checks
> for each row whether any columns changed and updates _version at that point
> if any did.  That seems pretty foolproof to me.
>
> Reported-by: RishiRaj Maulick <rishi.raj2509 at gmail.com>
> Reported-at: http://openvswitch.org/pipermail/dev/2015-August/059439.html
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
>  ovsdb/transaction.c | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c
> index 83ddaff..2c85fee 100644
> --- a/ovsdb/transaction.c
> +++ b/ovsdb/transaction.c
> @@ -1,4 +1,4 @@
> -/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
> +/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc.
>   *
>   * Licensed under the Apache License, Version 2.0 (the "License");
>   * you may not use this file except in compliance with the License.
> @@ -534,7 +534,6 @@ assess_weak_refs(struct ovsdb_txn *txn, struct ovsdb_txn_row *txn_row)
>          }
>
>          if (datum->n != orig_n) {
> -            bitmap_set1(txn_row->changed, OVSDB_COL_VERSION);
>              bitmap_set1(txn_row->changed, column->index);
>              ovsdb_datum_sort_assert(datum, column->type.key.type);
>              if (datum->n < column->type.n_min) {
> @@ -748,6 +747,21 @@ check_index_uniqueness(struct ovsdb_txn *txn OVS_UNUSED,
>      return NULL;
>  }
>
> +static struct ovsdb_error * OVS_WARN_UNUSED_RESULT
> +update_version(struct ovsdb_txn *txn OVS_UNUSED, struct ovsdb_txn_row *txn_row)
> +{
> +    struct ovsdb_table *table = txn_row->table;
> +    size_t n_columns = shash_count(&table->schema->columns);
> +
> +    if (txn_row->old && txn_row->new
> +        && !bitmap_is_all_zeros(txn_row->changed, n_columns)) {
> +        bitmap_set1(txn_row->changed, OVSDB_COL_VERSION);
> +        uuid_generate(ovsdb_row_get_version_rw(txn_row->new));
> +    }
> +
> +    return NULL;
> +}
> +
>  static struct ovsdb_error *
>  ovsdb_txn_commit_(struct ovsdb_txn *txn, bool durable)
>  {
> @@ -801,6 +815,12 @@ ovsdb_txn_commit_(struct ovsdb_txn *txn, bool durable)
>          return error;
>      }
>
> +    /* Update _version for rows that changed.  */
> +    error = for_each_txn_row(txn, update_version);
> +    if (error) {
> +        return OVSDB_WRAP_BUG("can't happen", error);
> +    }
> +
>      /* Send the commit to each replica. */
>      LIST_FOR_EACH (replica, node, &txn->db->replicas) {
>          error = (replica->class->commit)(replica, txn, durable);
> @@ -915,7 +935,6 @@ ovsdb_txn_row_modify(struct ovsdb_txn *txn, const struct ovsdb_row *ro_row_)
>
>          rw_row = ovsdb_row_clone(ro_row);
>          rw_row->n_refs = ro_row->n_refs;
> -        uuid_generate(ovsdb_row_get_version_rw(rw_row));
>          ovsdb_txn_row_create(txn, table, ro_row, rw_row);
>          hmap_replace(&table->rows, &ro_row->hmap_node, &rw_row->hmap_node);
>
> --
> 2.1.3
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev