Thanks Ben,
It looks good to me,
On Fri, Aug 2, 2013 at 5:33 PM, Ben Pfaff <b...@nicira.com> wrote:
> 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 <shi...@nicira.com>
> Signed-off-by: Ben Pfaff <b...@nicira.com>
> ---
> lib/async-append-aio.c | 15 -----------
> lib/{async-append-sync.c => async-append-null.c} | 29
> +++++++-------------
> lib/async-append.h | 20 +++-----------
> lib/automake.mk | 2 +-
> lib/vlog.c | 31
> +++++++++++++++++++---
> lib/vlog.h | 1 +
> vswitchd/bridge.c | 2 +-
> 7 files changed, 42 insertions(+), 58 deletions(-)
> rename lib/{async-append-sync.c => async-append-null.c} (63%)
>
> diff --git a/lib/async-append-aio.c b/lib/async-append-aio.c
> index 48edc38..23430a4 100644
> --- 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
> --- 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
> --- 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 f18df91..fdda006 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -263,7 +263,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
> --- 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
> --- 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
> --- 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);
> }
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
