[ovs-dev] [PATCH] ovsdb: compact databases more strictly

[Daniel Alvarez Sanchez]

Thanks a lot Ben and Mark.
Yes, I'll be sending it right away. Thanks a lot guys.

On Fri, Mar 9, 2018 at 8:13 PM, Ben Pfaff <blp at ovn.org> wrote:

> On Fri, Mar 09, 2018 at 08:05:22AM -0600, Mark Michelson wrote:
> > Hi Daniel,
> >
> > Mostly this looks correct. I had one small finding and have noted it
> in-line
> > down below.
> >
> > On 03/08/2018 04:20 PM, Daniel Alvarez wrote:
> > >Before this patch, the databases were automatically compacted when a
> > >transaction is logged when:
> > >
> > >* It's been > 10 minutes after last compaction AND
> > >* At least 100 commits have occurred AND
> > >* Database has grown at least 4x since last compaction (and it's > 10M)
> > >
> > >This patch changes the conditions as follows:
> > >
> > >* It's been > 10 minutes after last compaction AND
> > >* At least 100 commits have occurred AND either
> > >    - It's been > 24 hours after the last compaction OR
> > >    - Database has grown at least 2x since last compaction (and it's >
> 10M)
> > >
> > >Reported-by: Daniel Alvarez <dalvarez at redhat.com>
> > >Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2018-
> March/046309.html
> > >Signed-off-by: Daniel Alvarez <dalvarez at redhat.com>
> > >---
> > >  ovsdb/file.c            | 17 +++++++++++++----
> > >  ovsdb/log.c             |  2 +-
> > >  ovsdb/ovsdb-server.1.in |  6 ++++--
> > >  3 files changed, 18 insertions(+), 7 deletions(-)
> > >
> > >diff --git a/ovsdb/file.c b/ovsdb/file.c
> > >index 4b7ad52ab..02e0e8b76 100644
> > >--- a/ovsdb/file.c
> > >+++ b/ovsdb/file.c
> > >@@ -46,6 +46,10 @@ VLOG_DEFINE_THIS_MODULE(ovsdb_file);
> > >   * compacting fails. */
> > >  #define COMPACT_RETRY_MSEC      (60 * 1000)      /* 1 minute. */
> > >+/* Maximum number of milliseconds before compacting the database
> regardless
> > >+ * of its size. */
> > >+#define COMPACT_MAX_MSEC        (24 * 60 * 60 * 1000) /* 24 hours. */
> > >+
> > >  /* A transaction being converted to JSON for writing to a file. */
> > >  struct ovsdb_file_txn {
> > >      struct json *json;          /* JSON for the whole transaction. */
> > >@@ -586,6 +590,7 @@ ovsdb_file_commit(struct ovsdb_replica *replica,
> > >      struct ovsdb_file *file = ovsdb_file_cast(replica);
> > >      struct ovsdb_file_txn ftxn;
> > >      struct ovsdb_error *error;
> > >+    long long int next_compact_elapsed;
> > >      ovsdb_file_txn_init(&ftxn);
> > >      ovsdb_txn_for_each_change(txn, ovsdb_file_change_cb, &ftxn);
> > >@@ -604,11 +609,15 @@ ovsdb_file_commit(struct ovsdb_replica *replica,
> > >      /* If it has been at least COMPACT_MIN_MSEC ms since the last
> time we
> > >       * compacted (or at least COMPACT_RETRY_MSEC ms since the last
> time we
> > >       * tried), and if there are at least 100 transactions in the
> database, and
> > >-     * if the database is at least 10 MB, and the database is at least
> 4x the
> > >-     * size of the previous snapshot, then compact the database. */
> > >-    if (time_msec() >= file->next_compact
> > >+     * if the database is at least 10 MB, and the database is at least
> 2x the
> > >+     * size of the previous snapshot, then compact the database.
> However, if
> > >+     * it has been over COMPACT_MAX_MSEC ms, the database size is not
> taken
> > >+     * into account. */
> > >+    next_compact_elapsed = time_msec() - file->next_compact;
> > >+    if (next_compact_elapsed >= 0
> > >          && file->n_transactions >= 100
> > >-        && ovsdb_log_grew_lots(file->log)) {
> > >+        && (next_compact_elapsed >= COMPACT_MAX_MSEC
> >
> > I think this line is not right. If the idea is to compact if it has been
> > more than 24 hours since the last time we did, then we should be doing
> >
> > time_msec() - file->last_compact >= COMPACT_MAX_MSEC
> >
> > instead of
> >
> > time_msec() - file->next_compact >= COMPACT_MAX_MSEC
>
> Oops, I missed this before I committed.  Daniel, would you mind sending
> a fix?
>