[ovs-dev] [PATCH 1/2] ovsdb windows: Allow online compacting

Ben Pfaff blp at ovn.org
Wed Nov 2 03:26:06 UTC 2016


On Thu, Oct 27, 2016 at 09:45:42PM +0000, Alin Serdean wrote:
> This patch allows online compacting to be done under Windows.
> 
> To achieve the above we need to close all file handles before trying to
> rename the file, switch from rename to MoveFileEx (because rename/MoveFile
> fails if the destination exists), reopen the right type of log after the
> rename.
> 
> If we could not reopen the compacted database or the original database
> after the close simply abort and rely on the service manager. This
> can be changed in the future.
> 
> Signed-off-by: Alin Gabriel Serdean <aserdean at cloudbasesolutions.com>

This had some bugs.

The worst one was that it deleted all the data from the database.  This
wasn't evident from the test because it only deleted it from the on-disk
representation and the test didn't actually restart the server.  I've
fixed both problems.

The lesser one was that it had more duplicate code than needed and extra
#ifdefs made it harder than necessary to test the "Windows" approach on
Unix-like systems.  I've fixed that too.

So, here's my proposed version of the patch.  What do you think?

Thanks,

Ben.

--8<--------------------------cut here-------------------------->8--

From: Alin Serdean <aserdean at cloudbasesolutions.com>
Date: Thu, 27 Oct 2016 21:45:42 +0000
Subject: [PATCH] ovsdb windows: Allow online compacting

This patch allows online compacting to be done under Windows.

To achieve the above we need to close all file handles before trying to
rename the file, switch from rename to MoveFileEx (because rename/MoveFile
fails if the destination exists), reopen the right type of log after the
rename.

If we could not reopen the compacted database or the original database
after the close simply abort and rely on the service manager. This
can be changed in the future.

Signed-off-by: Alin Gabriel Serdean <aserdean at cloudbasesolutions.com>
Co-authored-by: Ben Pfaff <blp at ovn.org>
Signed-off-by: Ben Pfaff <blp at ovn.org>
---
 ovsdb/file.c          | 87 ++++++++++++++++++++++++++++++++++++++++++++-------
 tests/ovsdb-server.at |  6 +++-
 2 files changed, 81 insertions(+), 12 deletions(-)

diff --git a/ovsdb/file.c b/ovsdb/file.c
index 7f8554a..41c660a 100644
--- a/ovsdb/file.c
+++ b/ovsdb/file.c
@@ -622,6 +622,25 @@ ovsdb_file_commit(struct ovsdb_replica *replica,
     return NULL;
 }
 
+/* Rename 'old' to 'new', replacing 'new' if it exists.  Returns NULL if
+ * successful, otherwise an ovsdb_error that the caller must destroy. */
+static struct ovsdb_error * OVS_WARN_UNUSED_RESULT
+ovsdb_rename(const char *old, const char *new)
+{
+#ifdef _WIN32
+    int error = (MoveFileEx(old, new, MOVEFILE_REPLACE_EXISTING
+                            | MOVEFILE_WRITE_THROUGH | MOVEFILE_COPY_ALLOWED)
+                 ? 0 : EACCES);
+#else
+    int error = rename(old, new) ? errno : 0;
+#endif
+
+    return (error
+            ? ovsdb_io_error(error, "failed to rename \"%s\" to \"%s\"",
+                             old, new)
+            : NULL);
+}
+
 struct ovsdb_error *
 ovsdb_file_compact(struct ovsdb_file *file)
 {
@@ -667,22 +686,68 @@ ovsdb_file_compact(struct ovsdb_file *file)
         goto exit;
     }
 
-    /* Replace original by temporary. */
-    if (rename(tmp_name, file->file_name)) {
-        error = ovsdb_io_error(errno, "failed to rename \"%s\" to \"%s\"",
-                               tmp_name, file->file_name);
+    /* Replace original file by the temporary file.
+     *
+     * We support two strategies:
+     *
+     *     - The preferred strategy is to rename the temporary file over the
+     *       original one in-place, then close the original one.  This works on
+     *       Unix-like systems.  It does not work on Windows, which does not
+     *       allow open files to be renamed.  The approach has the advantage
+     *       that, at any point, we can drop back to something that already
+     *       works.
+     *
+     *     - Alternatively, we can close both files, rename, then open the new
+     *       file (which now has the original name).  This works on all
+     *       systems, but if reopening the file fails then we're stuck and have
+     *       to abort (XXX although it would be better to retry).
+     *
+     * We make the strategy a variable instead of an #ifdef to make it easier
+     * to test both strategies on Unix-like systems, and to make the code
+     * easier to read. */
+#ifdef _WIN32
+    bool rename_open_files = false;
+#else
+    bool rename_open_files = false;
+#endif
+    if (!rename_open_files) {
+        ovsdb_log_close(file->log);
+        ovsdb_log_close(new_log);
+        file->log = NULL;
+        new_log = NULL;
+    }
+    error = ovsdb_rename(tmp_name, file->file_name);
+    if (error) {
         goto exit;
     }
-    fsync_parent_dir(file->file_name);
-
-exit:
-    if (!error) {
+    if (rename_open_files) {
+        fsync_parent_dir(file->file_name);
         ovsdb_log_close(file->log);
         file->log = new_log;
-        file->last_compact = time_msec();
-        file->next_compact = file->last_compact + COMPACT_MIN_MSEC;
-        file->n_transactions = 1;
     } else {
+        /* Re-open the log.  This skips past the schema log record. */
+        error = ovsdb_file_open_log(file->file_name, OVSDB_LOG_READ_WRITE,
+                                    &file->log, NULL);
+        if (error) {
+            ovs_fatal(0, "could not reopen database");
+        }
+
+        /* Skip past the data log reecord. */
+        struct json *json;
+        error = ovsdb_log_read(file->log, &json);
+        if (error) {
+            ovs_fatal(0, "error reading database");
+        }
+        json_destroy(json);
+    }
+
+    /* Success! */
+    file->last_compact = time_msec();
+    file->next_compact = file->last_compact + COMPACT_MIN_MSEC;
+    file->n_transactions = 1;
+
+exit:
+    if (error) {
         ovsdb_log_close(new_log);
         if (tmp_lock) {
             unlink(tmp_name);
diff --git a/tests/ovsdb-server.at b/tests/ovsdb-server.at
index ae1aab8..bbdd54b 100644
--- a/tests/ovsdb-server.at
+++ b/tests/ovsdb-server.at
@@ -671,7 +671,11 @@ AT_CHECK(
 dnl There should be 6 lines in the log now.
 AT_CHECK([test `wc -l < db` -eq 6], [0], [], [],
   [test ! -e pid || kill `cat pid`])
-dnl Then check that the dumped data is correct.
+dnl Then check that the dumped data is correct.  This time first kill
+dnl and restart the database server to ensure that the data is correct on
+dnl disk as well as in memory.
+OVS_APP_EXIT_AND_WAIT([ovsdb-server])
+AT_CHECK([ovsdb-server --detach --no-chdir --pidfile --remote=punix:socket --log-file="`pwd`"/ovsdb-server.log db], [0], [ignore], [ignore])
 AT_CHECK([ovsdb-client dump unix:socket ordinals], [0], [stdout], [ignore],
   [test ! -e pid || kill `cat pid`])
 AT_CHECK([${PERL} $srcdir/uuidfilt.pl stdout], [0], [dnl
-- 
2.1.3




More information about the dev mailing list