Thanks, I applied it to master.
On Fri, Aug 02, 2013 at 06:02:30PM -0700, Alex Wang wrote:
> 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
