| Message ID | 20180308222056.8009-1-dalvarez@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] ovsdb: compact databases more strictly | expand |
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@redhat.com> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2018-March/046309.html > Signed-off-by: Daniel Alvarez <dalvarez@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 > + || 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 >
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 <dalvarez@redhat.com> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2018-March/046309.html > Signed-off-by: Daniel Alvarez <dalvarez@redhat.com> I applied this to master, thanks! If you consider this a bug fix and want it on branch-2.9 also, let me know.
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@redhat.com> > >Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2018-March/046309.html > >Signed-off-by: Daniel Alvarez <dalvarez@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?
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@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@redhat.com> > > >Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2018- > March/046309.html > > >Signed-off-by: Daniel Alvarez <dalvarez@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? >
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 <dalvarez@redhat.com > 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 <blp@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@redhat.com> >> > >Reported-at: https://mail.openvswitch.org/p >> ipermail/ovs-discuss/2018-March/046309.html >> > >Signed-off-by: Daniel Alvarez <dalvarez@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? >> > >
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 + || 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
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@redhat.com> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2018-March/046309.html Signed-off-by: Daniel Alvarez <dalvarez@redhat.com> --- ovsdb/file.c | 17 +++++++++++++---- ovsdb/log.c | 2 +- ovsdb/ovsdb-server.1.in | 6 ++++-- 3 files changed, 18 insertions(+), 7 deletions(-)