[systemd-devel] [PATCH] [RFCv5] Add sync timer to journal server

[Lennart Poettering]

On Fri, 22.03.13 22:49, Oleksii Shevchuk (alxchk at gmail.com) wrote:

> Sync journal with fdatasync after 10s of inactivity (by default), or
> or after 10m of last commit (by default). Intervals configured via
> SyncIntervalIdleSec and SyncIntervalMaxSec options at journal.conf.
> Manual sync can be performed via sending SIGUSR1.
> ---
>  src/journal/journal-file.c       |  29 ++++++-
>  src/journal/journal-file.h       |   1 +
>  src/journal/journald-gperf.gperf |   2 +
>  src/journal/journald-server.c    | 161 ++++++++++++++++++++++++++++++++++++++-
>  src/journal/journald-server.h    |   7 ++
>  src/journal/journald.conf        |   2 +
>  6 files changed, 196 insertions(+), 6 deletions(-)
> 
> diff --git a/src/journal/journal-file.c b/src/journal/journal-file.c
> index 13fc8ed..8597869 100644
> --- a/src/journal/journal-file.c
> +++ b/src/journal/journal-file.c
> @@ -68,6 +68,26 @@
>  /* How many entries to keep in the entry array chain cache at max */
>  #define CHAIN_CACHE_MAX 20
>  
> +int journal_file_sync(JournalFile *f) {
> +        assert(f);
> +
> +        if (f->header->state != STATE_ONLINE)
> +                return 0;
> +
> +        if (! (f->writable && f->fd >= 0))
> +                return -EINVAL;
> +
> +        if (fdatasync(f->fd))
> +                return -errno;
> +
> +        f->header->state = STATE_OFFLINE;
> +
> +        if(fdatasync(f->fd))
> +                return -errno;

I wonder if msync() needs to be called too here, btw? After all this is
all memory mapped stuff, and I don't know the precise semantics of
fdatasync() vs. mmap() vs. msync() here? Probably something to
investigate first...

> @@ -457,6 +476,9 @@ int journal_file_append_object(JournalFile *f, int type, uint64_t size, Object *
>          assert(offset);
>          assert(ret);
>  
> +        if (f->header->state == STATE_OFFLINE)
> +                f->header->state = STATE_ONLINE;
> +

Hmm, presumably we'd have to fdatasync() here first, too, to act as a
barrier so that later changes don't hit the disk before we mark the file
online again? We really should make sure the file is marked online
first...

Might make sense to split this bit into a new function
journal_file_online() (even if it is called only once), and then maybe
rename _sync() to _offline() so that they are a nice pair?


>          p = le64toh(f->header->tail_object_offset);
>          if (p == 0)
>                  p = le64toh(f->header->header_size);
> @@ -1270,6 +1292,9 @@ int journal_file_append_entry(JournalFile *f, const dual_timestamp *ts, const st
>          if (!f->writable)
>                  return -EPERM;
>  
> +        if (f->header->state == STATE_OFFLINE)
> +                f->header->state = STATE_ONLINE;
> +

This seems unnecessary, no? AFAICS _append_entry() doesn't change the
file except via _append_object(), so no need to set the state
explicitly...

> +#define DEFAULT_SYNC_INTERVAL_IDLE_USEC (10*USEC_PER_SEC)
> +#define DEFAULT_SYNC_INTERVAL_MAX_USEC (10*USEC_PER_MINUTE)

So if I grok this correctly, then this will sync to disk 10s after the
*last* write but at least every 10min or so?

Maybe we should make this one setting only: "5min after the *first*
write". i.e. whatever you log, you know it will hit the disk the latest
after 5min, but possibly earlier. And if nothing happens at all actually
nothing is every written at all.


> +        if (s->runtime_journal) {
> +                r = journal_file_sync(s->runtime_journal);
> +                if (r < 0)
> +                        log_error("Failed to sync runtime journal: %s", strerror(-r));
> +        }

No need to sync the runtime journal. It's on tmpfs, hence fdatasync() is
a NOP there...

> +        log_debug("Setting sync interval to %" PRIu64, s->sync_interval_idle_usec);
> +

Please print usecs with format_timespan().

Also, needs a man page!

Thanks!

Lennart

-- 
Lennart Poettering - Red Hat, Inc.