diff mbox series

[ovs-dev,04/15] ovsdb-server: Distinguish logs from other replicas.

Message ID 20180101051640.13043-4-blp@ovn.org
State Accepted
Delegated to: Justin Pettit
Headers show
Series [ovs-dev,01/15] log: Add async commit support. | expand

Commit Message

Ben Pfaff Jan. 1, 2018, 5:16 a.m. UTC 
Until now, ovsdb-server has internally chained a list of replicas from each
database.  Whenever ovsdb_txn_commit() commits a transaction, it passes the
transaction to each replica.  The first replica, which is always the disk
file that stores the database, is special because it is the only replica
that can report an error and thereby abort the transaction.  This is a very
special property that genuinely distinguishes this first replica from the
others on the chain.  This commit breaks that first replica out as a
separate kind of entity that is not on the list of replicas.  When later
commits add support for clustering, there will only be more and more
special cases for the "first replica", so it makes sense to distinguish it
this way.

Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 ovsdb/file.c        | 33 ++++++---------------------------
 ovsdb/file.h        |  7 ++++++-
 ovsdb/monitor.c     |  4 +---
 ovsdb/ovsdb.c       |  7 +++++++
 ovsdb/ovsdb.h       |  5 +++--
 ovsdb/transaction.c | 14 +++++++-------
 6 files changed, 30 insertions(+), 40 deletions(-)

Comments

Justin Pettit Jan. 13, 2018, 12:56 a.m. UTC | #1 
> On Dec 31, 2017, at 9:16 PM, Ben Pfaff <blp@ovn.org> wrote:
> 
> +void
> +ovsdb_file_destroy(struct ovsdb_file *file)
> {
>     ovsdb_log_close(file->log);
>     free(file->file_name);
>     free(file);
> }

Generally, we handle the case where a null value is passed in.  It might be nice to provide the same protection.

> @@ -865,17 +866,16 @@ ovsdb_txn_commit_(struct ovsdb_txn *txn, bool durable)
>     }
> 
>     /* Send the commit to each replica. */
> -    LIST_FOR_EACH (replica, node, &txn->db->replicas) {
> -        error = (replica->class->commit)(replica, txn, durable);
> +    if (txn->db->file) {
> +        error = ovsdb_file_commit(txn->db->file, txn, durable);
>         if (error) {
>             ovsdb_txn_abort(txn);
>             return error;
>         }
>     }
> +    LIST_FOR_EACH (replica, node, &txn->db->replicas) {
> +        replica->class->commit(replica, txn, durable);
> +    }

I think the comment about committing to each replica should be moved to this block, and a new one should be written about writing to the log.

> diff --git a/ovsdb/ovsdb.h b/ovsdb/ovsdb.h
> index 89bbfa2512fa..06cd3a72e49e 100644
> --- a/ovsdb/ovsdb.h
> +++ b/ovsdb/ovsdb.h
> @@ -56,6 +56,7 @@ bool ovsdb_schema_equal(const struct ovsdb_schema *,
> /* Database. */
> struct ovsdb {
>     struct ovsdb_schema *schema;
> +    struct ovsdb_file *file;    /* If nonnull, log for transactions. */
>     struct ovs_list replicas;   /* Contains "struct ovsdb_replica"s. */
>     struct shash tables;        /* Contains "struct ovsdb_table *"s. */


The word "file" is kind of generic.  What about something like "log_file" to make it clearer when it's referenced elsewhere?

Acked-by: Justin Pettit <jpettit@ovn.org>

--Justin
Ben Pfaff Jan. 22, 2018, 7:19 p.m. UTC | #2 
On Fri, Jan 12, 2018 at 04:56:00PM -0800, Justin Pettit wrote:
> 
> > On Dec 31, 2017, at 9:16 PM, Ben Pfaff <blp@ovn.org> wrote:
> > 
> > +void
> > +ovsdb_file_destroy(struct ovsdb_file *file)
> > {
> >     ovsdb_log_close(file->log);
> >     free(file->file_name);
> >     free(file);
> > }
> 
> Generally, we handle the case where a null value is passed in.  It
> might be nice to provide the same protection.

Thanks, fixed.

> > @@ -865,17 +866,16 @@ ovsdb_txn_commit_(struct ovsdb_txn *txn, bool durable)
> >     }
> > 
> >     /* Send the commit to each replica. */
> > -    LIST_FOR_EACH (replica, node, &txn->db->replicas) {
> > -        error = (replica->class->commit)(replica, txn, durable);
> > +    if (txn->db->file) {
> > +        error = ovsdb_file_commit(txn->db->file, txn, durable);
> >         if (error) {
> >             ovsdb_txn_abort(txn);
> >             return error;
> >         }
> >     }
> > +    LIST_FOR_EACH (replica, node, &txn->db->replicas) {
> > +        replica->class->commit(replica, txn, durable);
> > +    }
> 
> I think the comment about committing to each replica should be moved
> to this block, and a new one should be written about writing to the
> log.

I updated the comment, thanks.

> > diff --git a/ovsdb/ovsdb.h b/ovsdb/ovsdb.h
> > index 89bbfa2512fa..06cd3a72e49e 100644
> > --- a/ovsdb/ovsdb.h
> > +++ b/ovsdb/ovsdb.h
> > @@ -56,6 +56,7 @@ bool ovsdb_schema_equal(const struct ovsdb_schema *,
> > /* Database. */
> > struct ovsdb {
> >     struct ovsdb_schema *schema;
> > +    struct ovsdb_file *file;    /* If nonnull, log for transactions. */
> >     struct ovs_list replicas;   /* Contains "struct ovsdb_replica"s. */
> >     struct shash tables;        /* Contains "struct ovsdb_table *"s. */
> 
> 
> The word "file" is kind of generic.  What about something like
> "log_file" to make it clearer when it's referenced elsewhere?

Unfortunately we have a different lower-level entity called a "log", so
I doubt that this would clarify much.

I think that there is probably opportunity for better naming here, but I
think it needs to be carefully thought out.

> Acked-by: Justin Pettit <jpettit@ovn.org>

Thanks!
diff mbox series

Patch

diff --git a/ovsdb/file.c b/ovsdb/file.c
index fdd5f8e35a44..4aafb3be8ab4 100644
--- a/ovsdb/file.c
+++ b/ovsdb/file.c
@@ -1,4 +1,4 @@ 
-/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2016 Nicira, Inc.
+/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2016, 2017 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -250,6 +250,7 @@  ovsdb_file_open__(const char *file_name,
         if (filep) {
             *filep = file;
         }
+        db->file = file;
     } else {
         ovsdb_log_close(log);
     }
@@ -500,10 +501,7 @@  ovsdb_file_read_schema(const char *file_name, struct ovsdb_schema **schemap)
     return ovsdb_file_open_log(file_name, OVSDB_LOG_READ_ONLY, NULL, schemap);
 }
 
-/* Replica implementation. */
-
 struct ovsdb_file {
-    struct ovsdb_replica replica;
     struct ovsdb *db;
     struct ovsdb_log *log;
     char *file_name;
@@ -512,8 +510,6 @@  struct ovsdb_file {
     unsigned int n_transactions;
 };
 
-static const struct ovsdb_replica_class ovsdb_file_class;
-
 static struct ovsdb_error *
 ovsdb_file_create(struct ovsdb *db, struct ovsdb_log *log,
                   const char *file_name,
@@ -535,26 +531,17 @@  ovsdb_file_create(struct ovsdb *db, struct ovsdb_log *log,
     }
 
     file = xmalloc(sizeof *file);
-    ovsdb_replica_init(&file->replica, &ovsdb_file_class);
     file->db = db;
     file->log = log;
     file->file_name = abs_name;
     file->last_compact = time_msec();
     file->next_compact = file->last_compact + COMPACT_MIN_MSEC;
     file->n_transactions = n_transactions;
-    ovsdb_add_replica(db, &file->replica);
 
     *filep = file;
     return NULL;
 }
 
-static struct ovsdb_file *
-ovsdb_file_cast(struct ovsdb_replica *replica)
-{
-    ovs_assert(replica->class == &ovsdb_file_class);
-    return CONTAINER_OF(replica, struct ovsdb_file, replica);
-}
-
 static bool
 ovsdb_file_change_cb(const struct ovsdb_row *old,
                      const struct ovsdb_row *new,
@@ -579,11 +566,10 @@  ovsdb_file_txn_annotate(struct json *json, const char *comment)
     return json;
 }
 
-static struct ovsdb_error *
-ovsdb_file_commit(struct ovsdb_replica *replica,
+struct ovsdb_error *
+ovsdb_file_commit(struct ovsdb_file *file,
                   const struct ovsdb_txn *txn, bool durable)
 {
-    struct ovsdb_file *file = ovsdb_file_cast(replica);
     struct ovsdb_file_txn ftxn;
     struct ovsdb_error *error;
 
@@ -764,20 +750,13 @@  exit:
     return error;
 }
 
-static void
-ovsdb_file_destroy(struct ovsdb_replica *replica)
+void
+ovsdb_file_destroy(struct ovsdb_file *file)
 {
-    struct ovsdb_file *file = ovsdb_file_cast(replica);
-
     ovsdb_log_close(file->log);
     free(file->file_name);
     free(file);
 }
-
-static const struct ovsdb_replica_class ovsdb_file_class = {
-    ovsdb_file_commit,
-    ovsdb_file_destroy
-};
 
 static void
 ovsdb_file_txn_init(struct ovsdb_file_txn *ftxn)
diff --git a/ovsdb/file.h b/ovsdb/file.h
index c6ca92621044..a9ef0585b261 100644
--- a/ovsdb/file.h
+++ b/ovsdb/file.h
@@ -1,4 +1,4 @@ 
-/* Copyright (c) 2009, 2010, 2011 Nicira, Inc.
+/* Copyright (c) 2009, 2010, 2011, 2016, 2017 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -23,6 +23,7 @@ 
 struct ovsdb;
 struct ovsdb_file;
 struct ovsdb_schema;
+struct ovsdb_txn;
 
 struct ovsdb_error *ovsdb_file_open(const char *file_name, bool read_only,
                                     struct ovsdb **, struct ovsdb_file **)
@@ -44,6 +45,10 @@  struct ovsdb_error *ovsdb_file_read_schema(const char *file_name,
                                            struct ovsdb_schema **)
     OVS_WARN_UNUSED_RESULT;
 
+struct ovsdb_error *ovsdb_file_commit(struct ovsdb_file *,
+                                      const struct ovsdb_txn *, bool durable);
+void ovsdb_file_destroy(struct ovsdb_file *);
+
 struct json *ovsdb_file_txn_annotate(struct json *, const char *comment);
 
 #endif /* ovsdb/file.h */
diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c
index c0f9c557ab67..b2ecd109ed60 100644
--- a/ovsdb/monitor.c
+++ b/ovsdb/monitor.c
@@ -1573,7 +1573,7 @@  ovsdb_monitor_destroy(struct ovsdb_monitor *dbmon)
     free(dbmon);
 }
 
-static struct ovsdb_error *
+static void
 ovsdb_monitor_commit(struct ovsdb_replica *replica,
                      const struct ovsdb_txn *txn,
                      bool durable OVS_UNUSED)
@@ -1601,8 +1601,6 @@  ovsdb_monitor_commit(struct ovsdb_replica *replica,
         ovsdb_monitor_json_cache_flush(m);
         break;
     }
-
-    return NULL;
 }
 
 static void
diff --git a/ovsdb/ovsdb.c b/ovsdb/ovsdb.c
index d8f441ad0728..213d0d4823e1 100644
--- a/ovsdb/ovsdb.c
+++ b/ovsdb/ovsdb.c
@@ -18,6 +18,7 @@ 
 #include "ovsdb.h"
 
 #include "column.h"
+#include "file.h"
 #include "openvswitch/json.h"
 #include "ovsdb-error.h"
 #include "ovsdb-parser.h"
@@ -328,6 +329,7 @@  ovsdb_create(struct ovsdb_schema *schema)
 
     db = xmalloc(sizeof *db);
     db->schema = schema;
+    db->file = NULL;
     ovs_list_init(&db->replicas);
     ovs_list_init(&db->triggers);
     db->run_triggers = false;
@@ -363,6 +365,11 @@  ovsdb_destroy(struct ovsdb *db)
     if (db) {
         struct shash_node *node;
 
+        /* Close the log. */
+        if (db->file) {
+            ovsdb_file_destroy(db->file);
+        }
+
         /* Remove all the replicas. */
         while (!ovs_list_is_empty(&db->replicas)) {
             struct ovsdb_replica *r
diff --git a/ovsdb/ovsdb.h b/ovsdb/ovsdb.h
index 89bbfa2512fa..06cd3a72e49e 100644
--- a/ovsdb/ovsdb.h
+++ b/ovsdb/ovsdb.h
@@ -56,6 +56,7 @@  bool ovsdb_schema_equal(const struct ovsdb_schema *,
 /* Database. */
 struct ovsdb {
     struct ovsdb_schema *schema;
+    struct ovsdb_file *file;    /* If nonnull, log for transactions. */
     struct ovs_list replicas;   /* Contains "struct ovsdb_replica"s. */
     struct shash tables;        /* Contains "struct ovsdb_table *"s. */
 
@@ -87,8 +88,8 @@  struct ovsdb_replica {
 };
 
 struct ovsdb_replica_class {
-    struct ovsdb_error *(*commit)(struct ovsdb_replica *,
-                                  const struct ovsdb_txn *, bool durable);
+    void (*commit)(struct ovsdb_replica *,
+                   const struct ovsdb_txn *, bool durable);
     void (*destroy)(struct ovsdb_replica *);
 };
 
diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c
index f47d45fca397..ba17834aa633 100644
--- a/ovsdb/transaction.c
+++ b/ovsdb/transaction.c
@@ -1,4 +1,4 @@ 
-/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc.
+/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2017 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -19,6 +19,7 @@ 
 
 #include "bitmap.h"
 #include "openvswitch/dynamic-string.h"
+#include "file.h"
 #include "hash.h"
 #include "openvswitch/hmap.h"
 #include "openvswitch/json.h"
@@ -865,17 +866,16 @@  ovsdb_txn_commit_(struct ovsdb_txn *txn, bool durable)
     }
 
     /* Send the commit to each replica. */
-    LIST_FOR_EACH (replica, node, &txn->db->replicas) {
-        error = (replica->class->commit)(replica, txn, durable);
+    if (txn->db->file) {
+        error = ovsdb_file_commit(txn->db->file, txn, durable);
         if (error) {
-            /* We don't support two-phase commit so only the first replica is
-             * allowed to report an error. */
-            ovs_assert(&replica->node == txn->db->replicas.next);
-
             ovsdb_txn_abort(txn);
             return error;
         }
     }
+    LIST_FOR_EACH (replica, node, &txn->db->replicas) {
+        replica->class->commit(replica, txn, durable);
+    }
 
     /* Finalize commit. */
     txn->db->run_triggers = true;