[ovs-dev] [idl 1/2] ovsdb-idl: Fix atomicity of writes that don't change a column's value.

Justin Petbot jpetbot at gmail.com
Fri Apr 1 18:37:52 UTC 2011


On Fri,  1 Apr 2011, at 10:51:39 AM, Ben Pfaff wrote:
> The existing ovsdb_idl_txn_commit() drops any writes that don't change a
> column's value from what was last reported by the database.  But this isn't
> a valid optimization, because it breaks the atomicity of transactions.
> Suppose columns A and B initially have values 1 and 2.  Client 1 writes
> value 1 to both columns in one transaction.  Client 2 writes value 2 to
> both columns in another transaction.  The only possible valid results for
> any serial ordering of transactions are 1,1 or 2,2.  But if both clients
> drop writes to columns that they have not modified, then 2,1 also becomes
> possible (because client 1 just writes to B and client 2 just writes to A).
> 
> However, for write-only columns we can optimize this out because the IDL
> can assume it is the only client writing to a column.
> 
> Found by inspection.
> ---
>  lib/ovsdb-idl.c |   18 +++++++++++++++---
>  1 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> index b264591..7c17414 100644
> --- a/lib/ovsdb-idl.c
> +++ b/lib/ovsdb-idl.c
> @@ -1436,9 +1436,7 @@ ovsdb_idl_txn_commit(struct ovsdb_idl_txn *txn)
>                                                          &class->columns[idx];
>  
>                      if (row->old
> -                        ? !ovsdb_datum_equals(&row->old[idx], &row->new[idx],
> -                                              &column->type)
> -                        : !ovsdb_datum_is_default(&row->new[idx],
> +                        || !ovsdb_datum_is_default(&row->new[idx],
>                                                    &column->type)) {
>                          json_object_put(row_json, column->name,
>                                          substitute_uuids(
> @@ -1633,6 +1631,20 @@ ovsdb_idl_txn_write(const struct ovsdb_idl_row *row_,
>      assert(row->old == NULL ||
>             row->table->modes[column_idx] & OVSDB_IDL_MONITOR);
>  
> +    /* If this is a write-only column and the datum being written is the same
> +     * as the one already there, just skip the update entirely.  This is worth
> +     * optimizing because we have a lot of columns that get periodically
> +     * refreshed into the database but don't actually change that often.
> +     *
> +     * We don't do this for read/write columns because that would break
> +     * atomicity of transactions--some other client might have written a

We should probably include have, since it may reduce readability.

> +     * different value in that column since we read it. */
> +    if (row->table->modes[column_idx] == OVSDB_IDL_MONITOR

This is clearly something you whipped together to try and take away
some of the glory surrounding my Makefile commit from earlier this
afternoon.  It's really kind of pathetic, and I'm not sure I want to
dignify it with a review.

> +        && ovsdb_datum_equals(&row->new[column_idx], datum, &column->type)) {
> +        ovsdb_datum_destroy(datum, &column->type);
> +        return;

You might want to fix the grammar there.

> +    }
> +
>      if (hmap_node_is_null(&row->txn_node)) {
>          hmap_insert(&row->table->idl->txn->txn_rows, &row->txn_node,
>                      uuid_hash(&row->uuid));
> -- 
> 1.7.1
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev



More information about the dev mailing list