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

2016-11-01 Thread Ben Pfaff
On Tue, Nov 01, 2016 at 08:26:06PM -0700, Ben Pfaff wrote:
> +/* 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;

This should read "= true".  (I tested both cases here and forgot to
update the commit before posting.)
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev


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

2016-11-01 Thread Ben Pfaff
On Thu, Oct 27, 2016 at 09:45:42PM +, 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 

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 
Date: Thu, 27 Oct 2016 21:45:42 +
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 
Co-authored-by: Ben Pfaff 
Signed-off-by: Ben Pfaff 
---
 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_transa