From patchwork Mon Aug 31 16:52:05 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Pfaff X-Patchwork-Id: 512549 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from archives.nicira.com (li376-54.members.linode.com [96.126.127.54]) by ozlabs.org (Postfix) with ESMTP id 7E9CE140280 for ; Tue, 1 Sep 2015 02:52:08 +1000 (AEST) Received: from archives.nicira.com (localhost [127.0.0.1]) by archives.nicira.com (Postfix) with ESMTP id 8FA6A108FA; Mon, 31 Aug 2015 09:52:07 -0700 (PDT) X-Original-To: dev@openvswitch.org Delivered-To: dev@openvswitch.org Received: from mx1e3.cudamail.com (mx1.cudamail.com [69.90.118.67]) by archives.nicira.com (Postfix) with ESMTPS id D02B9106FC for ; Mon, 31 Aug 2015 09:52:05 -0700 (PDT) Received: from bar2.cudamail.com (localhost [127.0.0.1]) by mx1e3.cudamail.com (Postfix) with ESMTPS id 188964204B7 for ; Mon, 31 Aug 2015 10:52:05 -0600 (MDT) X-ASG-Debug-ID: 1441039924-03dc535c692505a0001-byXFYA Received: from mx1-pf2.cudamail.com ([192.168.24.2]) by bar2.cudamail.com with ESMTP id IME2EDF23KDOAsAS (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Mon, 31 Aug 2015 10:52:04 -0600 (MDT) X-Barracuda-Envelope-From: blp@nicira.com X-Barracuda-RBL-Trusted-Forwarder: 192.168.24.2 Received: from unknown (HELO mail-pa0-f47.google.com) (209.85.220.47) by mx1-pf2.cudamail.com with ESMTPS (RC4-SHA encrypted); 31 Aug 2015 16:52:04 -0000 Received-SPF: unknown (mx1-pf2.cudamail.com: Multiple SPF records returned) X-Barracuda-RBL-Trusted-Forwarder: 209.85.220.47 Received: by pabpg12 with SMTP id pg12so12239610pab.3 for ; Mon, 31 Aug 2015 09:52:03 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=aHf4SMo7LURzLd4dzdfjaZbzqrqs7YsWoa0nNOdgPqU=; b=iQt7Goufu5vSWtJ3YWSi/msV6NxGGW8vm508noTr9cD94Lw9lnq/RgvTQkpv+AfUvU CgATw1MEHTUvvAy4WgxN2GGVxUH+bokxiLw7wKWclmrkjOdD/A883yC/YF5/yPLf+9Sc SWFG2YEJWf2UDfq1wUchT3v9GTW5kvSbW8aEJaFhDwgcZo+GqRs7OnYpBZs/+uUlpzRh GFtGrWWlnrixxA97TfPzFl2hwSKcEg8cmYD0MXszXc21YVNlDVZ0GvDG0cgV2cxY6cFz 5L8MG8iXun40Rxw4/kiHCLfbYgrai8162OCemaIxACTumxtV5g8J429TKnrzQ2rhoF9g YsaA== X-Gm-Message-State: ALoCoQkaxcqrMk7FtqxAuerKiPX6791fVSamQCALSboQFiaLwKBRIcXs/kgYwCZmlGJaQSwtXrxj X-Received: by 10.67.6.164 with SMTP id cv4mr39103264pad.59.1441039923372; Mon, 31 Aug 2015 09:52:03 -0700 (PDT) Received: from sigabrt.benpfaff.org (173-228-112-192.dsl.dynamic.fusionbroadband.com. [173.228.112.192]) by smtp.gmail.com with ESMTPSA id pf4sm15195257pdb.37.2015.08.31.09.52.02 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Mon, 31 Aug 2015 09:52:02 -0700 (PDT) X-CudaMail-Envelope-Sender: blp@nicira.com X-Barracuda-Apparent-Source-IP: 173.228.112.192 From: Ben Pfaff To: dev@openvswitch.org X-CudaMail-Whitelist-To: dev@openvswitch.org X-CudaMail-MID: CM-E2-830043674 X-CudaMail-DTE: 083115 X-CudaMail-Originating-IP: 209.85.220.47 Date: Mon, 31 Aug 2015 09:52:05 -0700 X-ASG-Orig-Subj: [##CM-E2-830043674##][PATCH] ovsdb: Update _version more accurately in transaction commit. Message-Id: <1441039925-6681-1-git-send-email-blp@nicira.com> X-Mailer: git-send-email 2.1.3 X-Barracuda-Connect: UNKNOWN[192.168.24.2] X-Barracuda-Start-Time: 1441039924 X-Barracuda-Encrypted: DHE-RSA-AES256-SHA X-Barracuda-URL: https://web.cudamail.com:443/cgi-mod/mark.cgi X-ASG-Whitelist: Header =?UTF-8?B?eFwtY3VkYW1haWxcLXdoaXRlbGlzdFwtdG8=?= X-Virus-Scanned: by bsmtpd at cudamail.com X-Barracuda-BRTS-Status: 1 Cc: RishiRaj Maulick , Ben Pfaff Subject: [ovs-dev] [PATCH] ovsdb: Update _version more accurately in transaction commit. X-BeenThere: dev@openvswitch.org X-Mailman-Version: 2.1.16 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: dev-bounces@openvswitch.org Sender: "dev" 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 Reported-at: http://openvswitch.org/pipermail/dev/2015-August/059439.html Signed-off-by: Ben Pfaff Acked-by: Andy Zhou --- 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);