[ovs-dev] [PATCH] async-append: Refactor to avoid requiring enabling while single threaded.

Alex Wang alexw at nicira.com
Sat Aug 3 01:02:30 UTC 2013


Thanks Ben,

It looks good to me,


On Fri, Aug 2, 2013 at 5:33 PM, Ben Pfaff <blp at 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 <shihli at nicira.com>
> Signed-off-by: Ben Pfaff <blp at 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 at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130802/e39efdf6/attachment-0003.html>


More information about the dev mailing list