diff mbox series

[ovs-dev,5/6] ovsdb: Avoid converting database twice on an initiator.

Message ID 20230327194301.3773536-6-i.maximets@ovn.org
State Accepted
Commit 172c935ed9a8332004fcc15fbf4ab43c9f5fe043
Headers show
Series ovsdb: conversion: Bug fixes & Optimizations. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation success test: success

Commit Message

Ilya Maximets March 27, 2023, 7:43 p.m. UTC
Cluster member, that initiates the schema conversion, converts the
database twice.  First time while verifying the possibility of the
conversion, and the second time after reading conversion request
back from the storage.

Keep the converted database from the first time around and use it
after reading the request back from the storage.  This cuts in half
the conversion CPU cost.

Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---
 ovsdb/ovsdb-server.c | 22 +++++++++++++++-------
 ovsdb/relay.c        |  4 ++--
 ovsdb/relay.h        |  2 ++
 ovsdb/transaction.c  |  6 +++---
 ovsdb/transaction.h  |  3 ++-
 ovsdb/trigger.c      | 41 ++++++++++++++++++++++++++++++++++-------
 ovsdb/trigger.h      |  7 +++++++
 7 files changed, 65 insertions(+), 20 deletions(-)

Comments

Simon Horman March 31, 2023, 11:57 a.m. UTC | #1
On Mon, Mar 27, 2023 at 09:43:00PM +0200, Ilya Maximets wrote:
> Cluster member, that initiates the schema conversion, converts the
> database twice.  First time while verifying the possibility of the
> conversion, and the second time after reading conversion request
> back from the storage.
> 
> Keep the converted database from the first time around and use it
> after reading the request back from the storage.  This cuts in half
> the conversion CPU cost.
> 
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

Reviewed-by: Simon Horman <simon.horman@corigine.com>
Dumitru Ceara April 6, 2023, 8:21 a.m. UTC | #2
On 3/27/23 21:43, Ilya Maximets wrote:
> Cluster member, that initiates the schema conversion, converts the
> database twice.  First time while verifying the possibility of the
> conversion, and the second time after reading conversion request
> back from the storage.
> 
> Keep the converted database from the first time around and use it
> after reading the request back from the storage.  This cuts in half
> the conversion CPU cost.
> 
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> ---

Looks good to me, thanks!

Acked-by: Dumitru Ceara <dceara@redhat.com>
diff mbox series

Patch

diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
index b64814076..9bad0c8dd 100644
--- a/ovsdb/ovsdb-server.c
+++ b/ovsdb/ovsdb-server.c
@@ -574,7 +574,9 @@  close_db(struct server_config *config, struct db *db, char *comment)
 }
 
 static struct ovsdb_error * OVS_WARN_UNUSED_RESULT
-update_schema(struct ovsdb *db, const struct ovsdb_schema *schema,
+update_schema(struct ovsdb *db,
+              const struct ovsdb_schema *schema,
+              const struct uuid *txnid,
               bool conversion_with_no_data, void *aux)
 {
     struct server_config *config = aux;
@@ -591,11 +593,17 @@  update_schema(struct ovsdb *db, const struct ovsdb_schema *schema,
         struct ovsdb *new_db = NULL;
         struct ovsdb_error *error;
 
-        error = ovsdb_convert(db, schema, &new_db);
-        if (error) {
-            /* Should never happen, because conversion should have been
-             * checked before writing the schema to the storage. */
-            return error;
+        /* If conversion was triggered by the current process, we might
+         * already have converted version of a database. */
+        new_db = ovsdb_trigger_find_and_steal_converted_db(db, txnid);
+        if (!new_db) {
+            /* No luck.  Converting. */
+            error = ovsdb_convert(db, schema, &new_db);
+            if (error) {
+                /* Should never happen, because conversion should have been
+                 * checked before writing the schema to the storage. */
+                return error;
+            }
         }
         ovsdb_replace(db, new_db);
     } else {
@@ -635,7 +643,7 @@  parse_txn(struct server_config *config, struct db *db,
             return error;
         }
 
-        error = update_schema(db->db, schema, txn_json == NULL, config);
+        error = update_schema(db->db, schema, txnid, txn_json == NULL, config);
         if (error) {
             return error;
         }
diff --git a/ovsdb/relay.c b/ovsdb/relay.c
index 4a00f0a0a..81c2c340b 100644
--- a/ovsdb/relay.c
+++ b/ovsdb/relay.c
@@ -310,8 +310,8 @@  ovsdb_relay_parse_update(struct relay_ctx *ctx,
     if (update->monitor_reply && ctx->new_schema) {
         /* There was a schema change.  Updating a database with a new schema
          * before processing monitor reply with the new data. */
-        error = ctx->schema_change_cb(ctx->db, ctx->new_schema, false,
-                                      ctx->schema_change_aux);
+        error = ctx->schema_change_cb(ctx->db, ctx->new_schema, &UUID_ZERO,
+                                      false, ctx->schema_change_aux);
         if (error) {
             /* Should never happen, but handle this case anyway. */
             char *s = ovsdb_error_to_string_free(error);
diff --git a/ovsdb/relay.h b/ovsdb/relay.h
index 2d66b5e5f..f841554ca 100644
--- a/ovsdb/relay.h
+++ b/ovsdb/relay.h
@@ -22,10 +22,12 @@ 
 struct json;
 struct ovsdb;
 struct ovsdb_schema;
+struct uuid;
 
 typedef struct ovsdb_error *(*schema_change_callback)(
                                        struct ovsdb *,
                                        const struct ovsdb_schema *,
+                                       const struct uuid *,
                                        bool conversion_with_no_data,
                                        void *aux);
 
diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c
index f01de2a34..7cf4a851a 100644
--- a/ovsdb/transaction.c
+++ b/ovsdb/transaction.c
@@ -1252,14 +1252,14 @@  ovsdb_txn_precheck_prereq(const struct ovsdb *db)
 struct ovsdb_txn_progress *
 ovsdb_txn_propose_schema_change(struct ovsdb *db,
                                 const struct ovsdb_schema *schema,
-                                const struct json *data)
+                                const struct json *data,
+                                struct uuid *txnid)
 {
     struct ovsdb_txn_progress *progress = xzalloc(sizeof *progress);
     progress->storage = db->storage;
 
-    struct uuid next;
     struct ovsdb_write *write = ovsdb_storage_write_schema_change(
-        db->storage, schema, data, &db->prereq, &next);
+        db->storage, schema, data, &db->prereq, txnid);
     if (!ovsdb_write_is_complete(write)) {
         progress->write = write;
     } else {
diff --git a/ovsdb/transaction.h b/ovsdb/transaction.h
index 9991f34d2..0e054eef3 100644
--- a/ovsdb/transaction.h
+++ b/ovsdb/transaction.h
@@ -42,7 +42,8 @@  struct ovsdb_error *ovsdb_txn_propose_commit_block(struct ovsdb_txn *,
 void ovsdb_txn_complete(struct ovsdb_txn *);
 
 struct ovsdb_txn_progress *ovsdb_txn_propose_schema_change(
-    struct ovsdb *, const struct ovsdb_schema *, const struct json *data);
+    struct ovsdb *, const struct ovsdb_schema *,
+    const struct json *data, struct uuid *txnid);
 
 bool ovsdb_txn_progress_is_complete(const struct ovsdb_txn_progress *);
 const struct ovsdb_error *ovsdb_txn_progress_get_error(
diff --git a/ovsdb/trigger.c b/ovsdb/trigger.c
index 0706d66cc..0edcdd89c 100644
--- a/ovsdb/trigger.c
+++ b/ovsdb/trigger.c
@@ -31,6 +31,7 @@ 
 #include "transaction-forward.h"
 #include "openvswitch/vlog.h"
 #include "util.h"
+#include "uuid.h"
 
 VLOG_DEFINE_THIS_MODULE(trigger);
 
@@ -52,6 +53,7 @@  ovsdb_trigger_init(struct ovsdb_session *session, struct ovsdb *db,
     trigger->db = db;
     ovs_list_push_back(&trigger->db->triggers, &trigger->node);
     trigger->request = request;
+    trigger->converted_db = NULL;
     trigger->reply = NULL;
     trigger->progress = NULL;
     trigger->txn_forward = NULL;
@@ -69,6 +71,7 @@  ovsdb_trigger_destroy(struct ovsdb_trigger *trigger)
     ovsdb_txn_progress_destroy(trigger->progress);
     ovsdb_txn_forward_destroy(trigger->db, trigger->txn_forward);
     ovs_list_remove(&trigger->node);
+    ovsdb_destroy(trigger->converted_db);
     jsonrpc_msg_destroy(trigger->request);
     jsonrpc_msg_destroy(trigger->reply);
     free(trigger->role);
@@ -143,6 +146,30 @@  ovsdb_trigger_prereplace_db(struct ovsdb_trigger *trigger)
     }
 }
 
+/* Find among incomplete triggers one that caused database conversion
+ * with specified transaction ID. */
+struct ovsdb *
+ovsdb_trigger_find_and_steal_converted_db(const struct ovsdb *db,
+                                          const struct uuid *txnid)
+{
+    struct ovsdb *converted_db = NULL;
+    struct ovsdb_trigger *t;
+
+    if (uuid_is_zero(txnid)) {
+        return NULL;
+    }
+
+    LIST_FOR_EACH_SAFE (t, node, &db->triggers) {
+        if (t->db == db && t->converted_db
+            && uuid_equals(&t->conversion_txnid, txnid)) {
+            converted_db = t->converted_db;
+            t->converted_db = NULL;
+            break;
+        }
+    }
+    return converted_db;
+}
+
 bool
 ovsdb_trigger_run(struct ovsdb *db, long long int now)
 {
@@ -200,7 +227,6 @@  ovsdb_trigger_try(struct ovsdb_trigger *t, long long int now)
         ovs_assert(!t->progress);
 
         struct ovsdb_txn *txn = NULL;
-        struct ovsdb *newdb = NULL;
         if (!strcmp(t->request->method, "transact")) {
             if (!ovsdb_txn_precheck_prereq(t->db)) {
                 return false;
@@ -272,7 +298,8 @@  ovsdb_trigger_try(struct ovsdb_trigger *t, long long int now)
                                     new_schema->name, t->db->schema->name);
             }
             if (!error) {
-                error = ovsdb_convert(t->db, new_schema, &newdb);
+                ovsdb_destroy(t->converted_db);
+                error = ovsdb_convert(t->db, new_schema, &t->converted_db);
             }
             if (error) {
                 ovsdb_schema_destroy(new_schema);
@@ -286,12 +313,12 @@  ovsdb_trigger_try(struct ovsdb_trigger *t, long long int now)
             } else {
                 /* Make the new copy into a transaction log record. */
                 txn_json = ovsdb_to_txn_json(
-                                newdb, "converted by ovsdb-server", true);
+                    t->converted_db, "converted by ovsdb-server", true);
             }
 
             /* Propose the change. */
             t->progress = ovsdb_txn_propose_schema_change(
-                t->db, new_schema, txn_json);
+                t->db, new_schema, txn_json, &t->conversion_txnid);
             ovsdb_schema_destroy(new_schema);
             json_destroy(txn_json);
             t->reply = jsonrpc_create_reply(json_object_create(),
@@ -313,13 +340,13 @@  ovsdb_trigger_try(struct ovsdb_trigger *t, long long int now)
             ovsdb_txn_progress_destroy(t->progress);
             t->progress = NULL;
             ovsdb_trigger_complete(t);
-            if (newdb) {
-                ovsdb_replace(t->db, newdb);
+            if (t->converted_db) {
+                ovsdb_replace(t->db, t->converted_db);
+                t->converted_db = NULL;
                 return true;
             }
             return false;
         }
-        ovsdb_destroy(newdb);
 
         /* Fall through to the general handling for the "committing" state.  We
          * abort the transaction--if and when it eventually commits, we'll read
diff --git a/ovsdb/trigger.h b/ovsdb/trigger.h
index d060c72e5..87ff4d053 100644
--- a/ovsdb/trigger.h
+++ b/ovsdb/trigger.h
@@ -17,6 +17,7 @@ 
 #define OVSDB_TRIGGER_H 1
 
 #include "openvswitch/list.h"
+#include "openvswitch/uuid.h"
 
 struct ovsdb;
 
@@ -54,6 +55,8 @@  struct ovsdb_trigger {
     struct ovs_list node;
     struct ovsdb_session *session; /* Session that owns this trigger. */
     struct ovsdb *db;           /* Database on which trigger acts. */
+    struct ovsdb *converted_db;   /* Result of the 'convert' request. */
+    struct uuid conversion_txnid; /* txnid of the conversion request. */
     struct jsonrpc_msg *request; /* Database request. */
     struct jsonrpc_msg *reply;   /* Result (null if none yet). */
     struct ovsdb_txn_progress *progress;
@@ -77,6 +80,10 @@  void ovsdb_trigger_cancel(struct ovsdb_trigger *, const char *reason);
 
 void ovsdb_trigger_prereplace_db(struct ovsdb_trigger *);
 
+struct ovsdb *ovsdb_trigger_find_and_steal_converted_db(
+        const struct ovsdb *, const struct uuid *)
+    OVS_WARN_UNUSED_RESULT;
+
 bool ovsdb_trigger_run(struct ovsdb *, long long int now);
 void ovsdb_trigger_wait(struct ovsdb *, long long int now);