async-append: Refactor to avoid requiring enabling while single threaded.

Until now, the async append interface has required async_append_enable()
to be called while the process was still single-threaded, with the
rationale being that async_append_enable() could race with
async_append_write() on some existing async_append object.  This was a
difficult problem when the async append interface was introduced, because
at the time Open vSwitch did not have any infrastructure for inter-thread
synchronization.

Now it is easy to solve, by introducing synchronization into the
async append module.  However, that's more or less wasted, because the
client is already required to serialize access to async append objects.
Moreover, vlog, the only existing client, needs to serialize access for
other reasons, so it wouldn't even be possible to just drop the client's
synchronization.

This commit therefore takes another approach.  It drops the
async_append_enable() interface entirely.  Now any existing async_append
object is always enabled.  The responsibility for "enabling", then, now
rests in whether the client creates and uses an async_append object, and
so vlog now takes care of that by itself.  Also, since vlog now has to
deal with sometimes having an async_append and sometimes not having one,
we might as well allow creating an async_append to fail, thereby slightly
simplifying the "no async I/O" implementation from "write synchronously"
to "always fail creating an async_append".

Reported-by: Shih-Hao Li <shihli@nicira.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>

diff --git a/lib/async-append-aio.c b/lib/async-append-aio.c
index 48edc38..23430a4 100644 (file)
--- a/lib/async-append-aio.c
+++ b/lib/async-append-aio.c
@@ -50,16 +50,6 @@ struct async_append {
     struct byteq byteq;
 };
 
-static bool async_append_enabled;
-
-void
-async_append_enable(void)
-{
-    assert_single_threaded();
-    forbid_forking("async i/o enabled");
-    async_append_enabled = true;
-}
-
 struct async_append *
 async_append_create(int fd)
 {
@@ -128,11 +118,6 @@ async_append_write(struct async_append *ap, const void *data_, size_t size)
 {
     const uint8_t *data = data_;
 
-    if (!async_append_enabled) {
-        ignore(write(ap->fd, data, size));
-        return;
-    }
-
     while (size > 0) {
         struct aiocb *aiocb;
         size_t chunk_size;

diff --git a/lib/async-append-sync.c b/lib/async-append-null.c
similarity index 63%
rename from lib/async-append-sync.c
rename to lib/async-append-null.c
index d40fdc8..3eef26e 100644 (file)
--- a/lib/async-append-sync.c
+++ b/lib/async-append-null.c
@@ -15,8 +15,8 @@
 
 #include <config.h>
 
-/* This implementation of the async-append.h interface uses ordinary
- * synchronous I/O, so it should be portable everywhere. */
+/* This is a null implementation of the asynchronous I/O interface for systems
+ * that don't have a form of asynchronous I/O. */
 
 #include "async-append.h"
 
@@ -25,38 +25,27 @@
 
 #include "util.h"
 
-struct async_append {
-    int fd;
-};
-
-void
-async_append_enable(void)
-{
-    /* Nothing to do. */
-}
-
 struct async_append *
-async_append_create(int fd)
+async_append_create(int fd OVS_UNUSED)
 {
-    struct async_append *ap = xmalloc(sizeof *ap);
-    ap->fd = fd;
-    return ap;
+    return NULL;
 }
 
 void
 async_append_destroy(struct async_append *ap)
 {
-    free(ap);
+    ovs_assert(ap == NULL);
 }
 
 void
-async_append_write(struct async_append *ap, const void *data, size_t size)
+async_append_write(struct async_append *ap OVS_UNUSED,
+                   const void *data OVS_UNUSED, size_t size OVS_UNUSED)
 {
-    ignore(write(ap->fd, data, size));
+    NOT_REACHED();
 }
 
 void
 async_append_flush(struct async_append *ap OVS_UNUSED)
 {
-    /* Nothing to do. */
+    NOT_REACHED();
 }

diff --git a/lib/async-append.h b/lib/async-append.h
index fb0ce52..0f7a4ae 100644 (file)
--- a/lib/async-append.h
+++ b/lib/async-append.h
@@ -32,24 +32,10 @@
  * Only a single thread may use a given 'struct async_append' at one time.
  */
 
-/* Enables using asynchronous I/O.  Some implementations may treat this as a
- * no-op.
- *
- * Before this function is called, the POSIX aio implementation uses ordinary
- * synchronous I/O because some POSIX aio libraries actually use threads
- * internally, which has enough cost and robustness implications that it's
- * better to use asynchronous I/O only when it has real expected benefits.
- *
- * Must be called while the process is still single-threaded.  May forbid the
- * process from subsequently forking. */
-void async_append_enable(void);
-
 /* Creates and returns a new asynchronous appender for file descriptor 'fd',
- * which the caller must have opened in append mode (O_APPEND).
- *
- * This function must always succeed.  If the system is for some reason unable
- * to support asynchronous I/O on 'fd' then the library must fall back to
- * syncrhonous I/O. */
+ * which the caller must have opened in append mode (O_APPEND).  If the system
+ * is for some reason unable to support asynchronous I/O on 'fd' this function
+ * may return NULL. */
 struct async_append *async_append_create(int fd);
 
 /* Destroys 'ap', without closing its underlying file descriptor. */

diff --git a/lib/automake.mk b/lib/automake.mk
index 4cdfcfd..b46a868 100644 (file)
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -261,7 +261,7 @@ endif
 if HAVE_POSIX_AIO
 lib_libopenvswitch_a_SOURCES += lib/async-append-aio.c
 else
-lib_libopenvswitch_a_SOURCES += lib/async-append-sync.c
+lib_libopenvswitch_a_SOURCES += lib/async-append-null.c
 endif
 
 if ESX

diff --git a/lib/vlog.c b/lib/vlog.c
index 6f0a256..26d0e6c 100644 (file)
--- a/lib/vlog.c
+++ b/lib/vlog.c
@@ -110,6 +110,7 @@ static struct ovs_mutex log_file_mutex = OVS_ADAPTIVE_MUTEX_INITIALIZER;
 static char *log_file_name OVS_GUARDED_BY(log_file_mutex);
 static int log_fd OVS_GUARDED_BY(log_file_mutex) = -1;
 static struct async_append *log_writer OVS_GUARDED_BY(log_file_mutex);
+static bool log_async OVS_GUARDED_BY(log_file_mutex);
 
 static void format_log_message(const struct vlog_module *, enum vlog_level,
                                enum vlog_facility,
@@ -344,7 +345,9 @@ vlog_set_log_file(const char *file_name)
 
     log_file_name = xstrdup(new_log_file_name);
     log_fd = new_log_fd;
-    log_writer = async_append_create(new_log_fd);
+    if (log_async) {
+        log_writer = async_append_create(new_log_fd);
+    }
 
     for (mp = vlog_modules; mp < &vlog_modules[n_vlog_modules]; mp++) {
         update_min_level(*mp);
@@ -617,6 +620,22 @@ vlog_init(void)
     pthread_once(&once, vlog_init__);
 }
 
+/* Enables VLF_FILE log output to be written asynchronously to disk.
+ * Asynchronous file writes avoid blocking the process in the case of a busy
+ * disk, but on the other hand they are less robust: there is a chance that the
+ * write will not make it to the log file if the process crashes soon after the
+ * log call. */
+void
+vlog_enable_async(void)
+{
+    ovs_mutex_lock(&log_file_mutex);
+    log_async = true;
+    if (log_fd >= 0 && !log_writer) {
+        log_writer = async_append_create(log_fd);
+    }
+    ovs_mutex_unlock(&log_file_mutex);
+}
+
 /* Print the current logging level for each module. */
 char *
 vlog_get_levels(void)
@@ -836,9 +855,13 @@ vlog_valist(const struct vlog_module *module, enum vlog_level level,
 
             ovs_mutex_lock(&log_file_mutex);
             if (log_fd >= 0) {
-                async_append_write(log_writer, s.string, s.length);
-                if (level == VLL_EMER) {
-                    async_append_flush(log_writer);
+                if (log_writer) {
+                    async_append_write(log_writer, s.string, s.length);
+                    if (level == VLL_EMER) {
+                        async_append_flush(log_writer);
+                    }
+                } else {
+                    ignore(write(log_fd, s.string, s.length));
                 }
             }
             ovs_mutex_unlock(&log_file_mutex);

diff --git a/lib/vlog.h b/lib/vlog.h
index 901b3d3..87a9654 100644 (file)
--- a/lib/vlog.h
+++ b/lib/vlog.h
@@ -141,6 +141,7 @@ int vlog_reopen_log_file(void);
 
 /* Initialization. */
 void vlog_init(void);
+void vlog_enable_async(void);
 
 /* Functions for actual logging. */
 void vlog(const struct vlog_module *, enum vlog_level, const char *format, ...)

diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 9bbd559..abbda56 100644 (file)
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -2447,7 +2447,7 @@ bridge_run(void)
              * process that forked us to exit successfully. */
             daemonize_complete();
 
-            async_append_enable();
+            vlog_enable_async();
 
             VLOG_INFO_ONCE("%s (Open vSwitch) %s", program_name, VERSION);
         }