Re: [ovs-dev] [PATCH 01/15] log: Add async commit support.

2018-01-11 Thread Yifeng Sun 
After further study, I understand the "seq" code and think it is
well-designed.
Thank you for this patch and the previous reply.

This patch looks good to me.

Reviewed-by: Yifeng Sun 

On Wed, Jan 10, 2018 at 4:21 PM, Ben Pfaff  wrote:

> Thanks for the review.
>
> The "seq" code introduces kind of a weird concept.  I might have
> invented it, not sure, but it's probably some kind of distortion of a
> more standard idea.  I'd like it to be easy for people to understand.
> Are there any questions that I could help to answer?  Maybe I could
> answer them directly, or maybe I could update the documentation in seq.h
> (which tries to be comprehensive but maybe it isn't?).
>
> On Wed, Jan 10, 2018 at 04:04:53PM -0800, Yifeng Sun wrote:
> > I read through lib/seq.c to learn how this patch works.
> > Looks good to me, but I feel not very confident.
> >
> > Reviewed-by: Yifeng Sun 
> >
> > On Sun, Dec 31, 2017 at 9:16 PM, Ben Pfaff  wrote:
> >
> > > The OVSDB log code has always had the ability to commit the log to
> disk and
> > > wait for the commit to finish.  This patch introduces a new feature
> that
> > > allows the client to start a commit in the background and then to
> determine
> > > asynchronously that the commit has completed.  This will be especially
> > > useful later for the distributed database feature.
> > >
> > > Signed-off-by: Ben Pfaff 
> > > ---
> > >  ovsdb/file.c   |   4 +-
> > >  ovsdb/log.c| 152 ++
> > > +--
> > >  ovsdb/log.h|   7 ++-
> > >  ovsdb/ovsdb-tool.c |   2 +-
> > >  tests/test-ovsdb.c |   2 +-
> > >  5 files changed, 158 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/ovsdb/file.c b/ovsdb/file.c
> > > index 90c2b9d20a9a..fdd5f8e35a44 100644
> > > --- a/ovsdb/file.c
> > > +++ b/ovsdb/file.c
> > > @@ -661,7 +661,7 @@ ovsdb_file_compact(struct ovsdb_file *file)
> > >
> > >  /* Commit the old version, so that we can be assured that we'll
> > > eventually
> > >   * have either the old or the new version. */
> > > -error = ovsdb_log_commit(file->log);
> > > +error = ovsdb_log_commit_block(file->log);
> > >  if (error) {
> > >  goto exit;
> > >  }
> > > @@ -857,7 +857,7 @@ ovsdb_file_txn_commit(struct json *json, const char
> > > *comment,
> > >  }
> > >
> > >  if (durable) {
> > > -error = ovsdb_log_commit(log);
> > > +error = ovsdb_log_commit_block(log);
> > >  if (error) {
> > >  return ovsdb_wrap_error(error, "committing transaction
> > > failed");
> > >  }
> > > diff --git a/ovsdb/log.c b/ovsdb/log.c
> > > index 0f8dafa30a8f..cc4bc2c6243e 100644
> > > --- a/ovsdb/log.c
> > > +++ b/ovsdb/log.c
> > > @@ -24,12 +24,17 @@
> > >  #include 
> > >  #include 
> > >
> > > +#include "lockfile.h"
> > >  #include "openvswitch/dynamic-string.h"
> > >  #include "openvswitch/json.h"
> > >  #include "openvswitch/vlog.h"
> > > -#include "lockfile.h"
> > > -#include "ovsdb.h"
> > > +#include "ovs-atomic.h"
> > > +#include "ovs-rcu.h"
> > > +#include "ovs-thread.h"
> > >  #include "ovsdb-error.h"
> > > +#include "ovsdb.h"
> > > +#include "openvswitch/poll-loop.h"
> > > +#include "seq.h"
> > >  #include "sha1.h"
> > >  #include "socket-util.h"
> > >  #include "transaction.h"
> > > @@ -78,6 +83,7 @@ struct ovsdb_log {
> > >  struct lockfile *lockfile;
> > >  FILE *stream;
> > >  off_t base;
> > > +struct afsync *afsync;
> > >  };
> > >
> > >  /* Whether the OS supports renaming open files.
> > > @@ -95,6 +101,9 @@ static bool parse_header(char *header, const char
> > > **magicp,
> > >   uint8_t sha1[SHA1_DIGEST_SIZE]);
> > >  static bool is_magic_ok(const char *needle, const char *haystack);
> > >
> > > +static struct afsync *afsync_create(int fd, uint64_t initial_ticket);
> > > +static uint64_t afsync_destroy(struct afsync *);
> > > +
> > >  /* Attempts to open 'name' with the specified 'open_mode'.  On
> success,
> > > stores
> > >   * the new log into '*filep' and returns NULL; otherwise returns NULL
> and
> > >   * stores NULL into '*filep'.
> > > @@ -269,6 +278,7 @@ ovsdb_log_open(const char *name, const char *magic,
> > >  file->prev_offset = 0;
> > >  file->offset = 0;
> > >  file->base = 0;
> > > +file->afsync = NULL;
> > >  *filep = file;
> > >  return NULL;
> > >
> > > @@ -308,6 +318,7 @@ ovsdb_log_close(struct ovsdb_log *file)
> > >  {
> > >  if (file) {
> > >  ovsdb_error_destroy(file->error);
> > > +afsync_destroy(file->afsync);
> > >  free(file->name);
> > >  free(file->display_name);
> > >  free(file->magic);
> > > @@ -634,8 +645,10 @@ ovsdb_log_write(struct ovsdb_log *file, const
> struct
> > > json *json)
> > >  return NULL;
> > >  }
> > >
> > > +/* Attempts to commit 'file' to disk.  Waits for the commit to
> succeed or
> > >

Re: [ovs-dev] [PATCH 01/15] log: Add async commit support.

2018-01-10 Thread Ben Pfaff 
Thanks for the review.

The "seq" code introduces kind of a weird concept.  I might have
invented it, not sure, but it's probably some kind of distortion of a
more standard idea.  I'd like it to be easy for people to understand.
Are there any questions that I could help to answer?  Maybe I could
answer them directly, or maybe I could update the documentation in seq.h
(which tries to be comprehensive but maybe it isn't?).

On Wed, Jan 10, 2018 at 04:04:53PM -0800, Yifeng Sun wrote:
> I read through lib/seq.c to learn how this patch works.
> Looks good to me, but I feel not very confident.
> 
> Reviewed-by: Yifeng Sun 
> 
> On Sun, Dec 31, 2017 at 9:16 PM, Ben Pfaff  wrote:
> 
> > The OVSDB log code has always had the ability to commit the log to disk and
> > wait for the commit to finish.  This patch introduces a new feature that
> > allows the client to start a commit in the background and then to determine
> > asynchronously that the commit has completed.  This will be especially
> > useful later for the distributed database feature.
> >
> > Signed-off-by: Ben Pfaff 
> > ---
> >  ovsdb/file.c   |   4 +-
> >  ovsdb/log.c| 152 ++
> > +--
> >  ovsdb/log.h|   7 ++-
> >  ovsdb/ovsdb-tool.c |   2 +-
> >  tests/test-ovsdb.c |   2 +-
> >  5 files changed, 158 insertions(+), 9 deletions(-)
> >
> > diff --git a/ovsdb/file.c b/ovsdb/file.c
> > index 90c2b9d20a9a..fdd5f8e35a44 100644
> > --- a/ovsdb/file.c
> > +++ b/ovsdb/file.c
> > @@ -661,7 +661,7 @@ ovsdb_file_compact(struct ovsdb_file *file)
> >
> >  /* Commit the old version, so that we can be assured that we'll
> > eventually
> >   * have either the old or the new version. */
> > -error = ovsdb_log_commit(file->log);
> > +error = ovsdb_log_commit_block(file->log);
> >  if (error) {
> >  goto exit;
> >  }
> > @@ -857,7 +857,7 @@ ovsdb_file_txn_commit(struct json *json, const char
> > *comment,
> >  }
> >
> >  if (durable) {
> > -error = ovsdb_log_commit(log);
> > +error = ovsdb_log_commit_block(log);
> >  if (error) {
> >  return ovsdb_wrap_error(error, "committing transaction
> > failed");
> >  }
> > diff --git a/ovsdb/log.c b/ovsdb/log.c
> > index 0f8dafa30a8f..cc4bc2c6243e 100644
> > --- a/ovsdb/log.c
> > +++ b/ovsdb/log.c
> > @@ -24,12 +24,17 @@
> >  #include 
> >  #include 
> >
> > +#include "lockfile.h"
> >  #include "openvswitch/dynamic-string.h"
> >  #include "openvswitch/json.h"
> >  #include "openvswitch/vlog.h"
> > -#include "lockfile.h"
> > -#include "ovsdb.h"
> > +#include "ovs-atomic.h"
> > +#include "ovs-rcu.h"
> > +#include "ovs-thread.h"
> >  #include "ovsdb-error.h"
> > +#include "ovsdb.h"
> > +#include "openvswitch/poll-loop.h"
> > +#include "seq.h"
> >  #include "sha1.h"
> >  #include "socket-util.h"
> >  #include "transaction.h"
> > @@ -78,6 +83,7 @@ struct ovsdb_log {
> >  struct lockfile *lockfile;
> >  FILE *stream;
> >  off_t base;
> > +struct afsync *afsync;
> >  };
> >
> >  /* Whether the OS supports renaming open files.
> > @@ -95,6 +101,9 @@ static bool parse_header(char *header, const char
> > **magicp,
> >   uint8_t sha1[SHA1_DIGEST_SIZE]);
> >  static bool is_magic_ok(const char *needle, const char *haystack);
> >
> > +static struct afsync *afsync_create(int fd, uint64_t initial_ticket);
> > +static uint64_t afsync_destroy(struct afsync *);
> > +
> >  /* Attempts to open 'name' with the specified 'open_mode'.  On success,
> > stores
> >   * the new log into '*filep' and returns NULL; otherwise returns NULL and
> >   * stores NULL into '*filep'.
> > @@ -269,6 +278,7 @@ ovsdb_log_open(const char *name, const char *magic,
> >  file->prev_offset = 0;
> >  file->offset = 0;
> >  file->base = 0;
> > +file->afsync = NULL;
> >  *filep = file;
> >  return NULL;
> >
> > @@ -308,6 +318,7 @@ ovsdb_log_close(struct ovsdb_log *file)
> >  {
> >  if (file) {
> >  ovsdb_error_destroy(file->error);
> > +afsync_destroy(file->afsync);
> >  free(file->name);
> >  free(file->display_name);
> >  free(file->magic);
> > @@ -634,8 +645,10 @@ ovsdb_log_write(struct ovsdb_log *file, const struct
> > json *json)
> >  return NULL;
> >  }
> >
> > +/* Attempts to commit 'file' to disk.  Waits for the commit to succeed or
> > fail.
> > + * Returns NULL if successful, otherwise the error that occurred. */
> >  struct ovsdb_error *
> > -ovsdb_log_commit(struct ovsdb_log *file)
> > +ovsdb_log_commit_block(struct ovsdb_log *file)
> >  {
> >  if (file->stream && fsync(fileno(file->stream))) {
> >  return ovsdb_io_error(errno, "%s: fsync failed",
> > file->display_name);
> > @@ -740,7 +753,7 @@ ovsdb_rename(const char *old, const char *new)
> >  struct ovsdb_error * OVS_WARN_UNUSED_RESULT
> >  ovsdb_log_replace_commit(struct ovsdb_log *old,

Re: [ovs-dev] [PATCH 01/15] log: Add async commit support.

2018-01-10 Thread Yifeng Sun 
I read through lib/seq.c to learn how this patch works.
Looks good to me, but I feel not very confident.

Reviewed-by: Yifeng Sun 

On Sun, Dec 31, 2017 at 9:16 PM, Ben Pfaff  wrote:

> The OVSDB log code has always had the ability to commit the log to disk and
> wait for the commit to finish.  This patch introduces a new feature that
> allows the client to start a commit in the background and then to determine
> asynchronously that the commit has completed.  This will be especially
> useful later for the distributed database feature.
>
> Signed-off-by: Ben Pfaff 
> ---
>  ovsdb/file.c   |   4 +-
>  ovsdb/log.c| 152 ++
> +--
>  ovsdb/log.h|   7 ++-
>  ovsdb/ovsdb-tool.c |   2 +-
>  tests/test-ovsdb.c |   2 +-
>  5 files changed, 158 insertions(+), 9 deletions(-)
>
> diff --git a/ovsdb/file.c b/ovsdb/file.c
> index 90c2b9d20a9a..fdd5f8e35a44 100644
> --- a/ovsdb/file.c
> +++ b/ovsdb/file.c
> @@ -661,7 +661,7 @@ ovsdb_file_compact(struct ovsdb_file *file)
>
>  /* Commit the old version, so that we can be assured that we'll
> eventually
>   * have either the old or the new version. */
> -error = ovsdb_log_commit(file->log);
> +error = ovsdb_log_commit_block(file->log);
>  if (error) {
>  goto exit;
>  }
> @@ -857,7 +857,7 @@ ovsdb_file_txn_commit(struct json *json, const char
> *comment,
>  }
>
>  if (durable) {
> -error = ovsdb_log_commit(log);
> +error = ovsdb_log_commit_block(log);
>  if (error) {
>  return ovsdb_wrap_error(error, "committing transaction
> failed");
>  }
> diff --git a/ovsdb/log.c b/ovsdb/log.c
> index 0f8dafa30a8f..cc4bc2c6243e 100644
> --- a/ovsdb/log.c
> +++ b/ovsdb/log.c
> @@ -24,12 +24,17 @@
>  #include 
>  #include 
>
> +#include "lockfile.h"
>  #include "openvswitch/dynamic-string.h"
>  #include "openvswitch/json.h"
>  #include "openvswitch/vlog.h"
> -#include "lockfile.h"
> -#include "ovsdb.h"
> +#include "ovs-atomic.h"
> +#include "ovs-rcu.h"
> +#include "ovs-thread.h"
>  #include "ovsdb-error.h"
> +#include "ovsdb.h"
> +#include "openvswitch/poll-loop.h"
> +#include "seq.h"
>  #include "sha1.h"
>  #include "socket-util.h"
>  #include "transaction.h"
> @@ -78,6 +83,7 @@ struct ovsdb_log {
>  struct lockfile *lockfile;
>  FILE *stream;
>  off_t base;
> +struct afsync *afsync;
>  };
>
>  /* Whether the OS supports renaming open files.
> @@ -95,6 +101,9 @@ static bool parse_header(char *header, const char
> **magicp,
>   uint8_t sha1[SHA1_DIGEST_SIZE]);
>  static bool is_magic_ok(const char *needle, const char *haystack);
>
> +static struct afsync *afsync_create(int fd, uint64_t initial_ticket);
> +static uint64_t afsync_destroy(struct afsync *);
> +
>  /* Attempts to open 'name' with the specified 'open_mode'.  On success,
> stores
>   * the new log into '*filep' and returns NULL; otherwise returns NULL and
>   * stores NULL into '*filep'.
> @@ -269,6 +278,7 @@ ovsdb_log_open(const char *name, const char *magic,
>  file->prev_offset = 0;
>  file->offset = 0;
>  file->base = 0;
> +file->afsync = NULL;
>  *filep = file;
>  return NULL;
>
> @@ -308,6 +318,7 @@ ovsdb_log_close(struct ovsdb_log *file)
>  {
>  if (file) {
>  ovsdb_error_destroy(file->error);
> +afsync_destroy(file->afsync);
>  free(file->name);
>  free(file->display_name);
>  free(file->magic);
> @@ -634,8 +645,10 @@ ovsdb_log_write(struct ovsdb_log *file, const struct
> json *json)
>  return NULL;
>  }
>
> +/* Attempts to commit 'file' to disk.  Waits for the commit to succeed or
> fail.
> + * Returns NULL if successful, otherwise the error that occurred. */
>  struct ovsdb_error *
> -ovsdb_log_commit(struct ovsdb_log *file)
> +ovsdb_log_commit_block(struct ovsdb_log *file)
>  {
>  if (file->stream && fsync(fileno(file->stream))) {
>  return ovsdb_io_error(errno, "%s: fsync failed",
> file->display_name);
> @@ -740,7 +753,7 @@ ovsdb_rename(const char *old, const char *new)
>  struct ovsdb_error * OVS_WARN_UNUSED_RESULT
>  ovsdb_log_replace_commit(struct ovsdb_log *old, struct ovsdb_log *new)
>  {
> -struct ovsdb_error *error = ovsdb_log_commit(new);
> +struct ovsdb_error *error = ovsdb_log_commit_block(new);
>  if (error) {
>  ovsdb_log_replace_abort(new);
>  return error;
> @@ -812,6 +825,10 @@ ovsdb_log_replace_commit(struct ovsdb_log *old,
> struct ovsdb_log *new)
>  ovsdb_error_destroy(old->error);
>  old->error = NULL;
>  /* prev_offset only matters for OVSDB_LOG_READ. */
> +if (old->afsync) {
> +uint64_t ticket = afsync_destroy(old->afsync);
> +old->afsync = afsync_create(fileno(old->stream), ticket + 1);
> +}
>  old->offset = new->offset;
>  /* Keep old->name. */
>  free(old->magic);
> @@ -844,3 +861,130 @@