| Message ID | 1441039925-6681-1-git-send-email-blp@nicira.com |
|---|---|
| State | Accepted |
| Headers | show |
Acked-by: Andy Zhou <azhou@nicira.com> On Mon, Aug 31, 2015 at 9:52 AM, Ben Pfaff <blp@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@gmail.com> > Reported-at: http://openvswitch.org/pipermail/dev/2015-August/059439.html > Signed-off-by: Ben Pfaff <blp@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@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
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@nicira.com> > > On Mon, Aug 31, 2015 at 9:52 AM, Ben Pfaff <blp@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@gmail.com> > > Reported-at: http://openvswitch.org/pipermail/dev/2015-August/059439.html > > Signed-off-by: Ben Pfaff <blp@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@openvswitch.org > > http://openvswitch.org/mailman/listinfo/dev
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);
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@gmail.com> Reported-at: http://openvswitch.org/pipermail/dev/2015-August/059439.html Signed-off-by: Ben Pfaff <blp@nicira.com> --- ovsdb/transaction.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-)