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

2018-03-10 Thread Daniel Alvarez Sanchez 
Done: https://patchwork.ozlabs.org/patch/884143/
Thanks a lot again for taking a look.
I'd appreciate the backport into 2.9 as well if you all agree.

Regards,
Daniel

On Sat, Mar 10, 2018 at 1:35 PM, Daniel Alvarez Sanchez  wrote:

> 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  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 
>> > >Reported-at: https://mail.openvswitch.org/p
>> ipermail/ovs-discuss/2018-March/046309.html
>> > >Signed-off-by: Daniel Alvarez 
>> > >---
>> > >  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();
>> > >  ovsdb_txn_for_each_change(txn, ovsdb_file_change_cb, );
>> > >@@ -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?
>>
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

2018-03-10 Thread 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  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 
> > >Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2018-
> March/046309.html
> > >Signed-off-by: Daniel Alvarez 
> > >---
> > >  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();
> > >  ovsdb_txn_for_each_change(txn, ovsdb_file_change_cb, );
> > >@@ -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?
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

2018-03-09 Thread Ben Pfaff 
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 
> >Reported-at: 
> >https://mail.openvswitch.org/pipermail/ovs-discuss/2018-March/046309.html
> >Signed-off-by: Daniel Alvarez 
> >---
> >  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();
> >  ovsdb_txn_for_each_change(txn, ovsdb_file_change_cb, );
> >@@ -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?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

2018-03-09 Thread Ben Pfaff 
On Thu, Mar 08, 2018 at 11:20:56PM +0100, 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 
> Reported-at: 
> https://mail.openvswitch.org/pipermail/ovs-discuss/2018-March/046309.html
> Signed-off-by: Daniel Alvarez 

I applied this to master, thanks!

If you consider this a bug fix and want it on branch-2.9 also, let me
know.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

2018-03-09 Thread Mark Michelson 

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 
Reported-at: 
https://mail.openvswitch.org/pipermail/ovs-discuss/2018-March/046309.html
Signed-off-by: Daniel Alvarez 
---
  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();

  ovsdb_txn_for_each_change(txn, ovsdb_file_change_cb, );
@@ -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


+|| ovsdb_log_grew_lots(file->log))) {
  error = ovsdb_file_compact(file);
  if (error) {
  char *s = ovsdb_error_to_string_free(error);
diff --git a/ovsdb/log.c b/ovsdb/log.c
index 0f8dafa30..721f53710 100644
--- a/ovsdb/log.c
+++ b/ovsdb/log.c
@@ -657,7 +657,7 @@ ovsdb_log_mark_base(struct ovsdb_log *log)
  bool
  ovsdb_log_grew_lots(const struct ovsdb_log *log)
  {
-return log->offset > 10 * 1024 * 1024 && log->offset / 4 > log->base;
+return log->offset > 10 * 1024 * 1024 && log->offset / 2 > log->base;
  }
  
  /* Attempts to atomically replace the contents of 'log', on disk, by the 'n'
diff --git a/ovsdb/ovsdb-server.1.in b/ovsdb/ovsdb-server.1.in
index 15ff77fd2..4bace04f0 100644
--- a/ovsdb/ovsdb-server.1.in
+++ b/ovsdb/ovsdb-server.1.in
@@ -184,10 +184,12 @@ Causes \fBovsdb\-server\fR to gracefully terminate.
  .IP "\fBovsdb\-server/compact\fR [\fIdb\fR]"
  Compacts database \fIdb\fR in-place.  If \fIdb\fR is not
  specified, compacts every database in-place.  A database is also
-compacted automatically when a transaction is logged if it is over 4
+compacted automatically when a transaction is logged if it is over 2
  times as large as its previous compacted size (and at least 10 MB),
  but not before 100 commits have been added or 10 minutes have elapsed
-since the last compaction.
+since the last compaction. It will also be compacted automatically
+after 24 hours since the last compaction if 100 commits were added
+regardless of its size.
  .
  .IP "\fBovsdb\-server/reconnect\fR"
  Makes \fBovsdb\-server\fR drop all of the JSON\-RPC



___
dev mailing list
d...@openvswitch.org