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

[Ben Pfaff]

Thanks Andy, I applied this to master, branch-2.4, and branch-2.3.

On Wed, Sep 02, 2015 at 11:45:40AM -0700, Andy Zhou wrote:
> 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