[systemd-devel] [PATCH] journal, coredump: allow relative values in some configuration options

[Lennart Poettering]

On Mon, 25.05.15 13:48, jsynacek at redhat.com (jsynacek at redhat.com) wrote:

> -static off_t arg_process_size_max = PROCESS_SIZE_MAX;
> -static off_t arg_external_size_max = EXTERNAL_SIZE_MAX;
> -static size_t arg_journal_size_max = JOURNAL_SIZE_MAX;
> -static off_t arg_keep_free = (off_t) -1;
> -static off_t arg_max_use = (off_t) -1;
> +static SizeParameter arg_process_size_max = {PROCESS_SIZE_MAX, false};
> +static SizeParameter arg_external_size_max = {EXTERNAL_SIZE_MAX, false};
> +static SizeParameter arg_journal_size_max = {JOURNAL_SIZE_MAX, false};
> +static SizeParameter arg_keep_free = {(uint64_t) -1, false};
> +static SizeParameter arg_max_use = {(uint64_t) -1, false};

So far we placed a space between { and the first word, and before }...

That said, I think it would make ton of sense to introduce a macro:

#define SIZE_PARAMETER_ABSOLUTE(x) { .value = (x), .relative = false }

And then use that everywhere.

>  
>  static int parse_config(void) {
>          static const ConfigTableItem items[] = {
> @@ -109,7 +111,7 @@ static int parse_config(void) {
>                  { "Coredump", "Compress",         config_parse_bool,              0, &arg_compress          },
>                  { "Coredump", "ProcessSizeMax",   config_parse_iec_off,           0, &arg_process_size_max  },
>                  { "Coredump", "ExternalSizeMax",  config_parse_iec_off,           0, &arg_external_size_max },
> -                { "Coredump", "JournalSizeMax",   config_parse_iec_size,          0, &arg_journal_size_max  },
> +                { "Coredump", "JournalSizeMax",   config_parse_iec_off,           0, &arg_journal_size_max  },
>                  { "Coredump", "KeepFree",         config_parse_iec_off,           0, &arg_keep_free         },
>                  { "Coredump", "MaxUse",           config_parse_iec_off,           0, &arg_max_use           },
>                  {}

I am pretty sure "config_parse_iec_off" should be renamed to
"config_parse_size_parameter" or so if it's now parsing things into that.

> @@ -122,6 +124,15 @@ static int parse_config(void) {
>                                   false, NULL);
>  }
>  
> +static uint64_t get_available(void) {
> +        struct statvfs ss;
> +
> +        if (statvfs("/var/lib/systemd/coredump", &ss) >= 0)
> +                return ss.f_bsize * ss.f_bavail;
> +
> +        return 0;
> +}

I'd be careful with this. Turning failure to query the disk space into
0, sounds wrong, because validly the disk space might be zero, which
cannot be distinguished then. And more importantly, if we cannot query
the disk space we should probably warn about that, and then fall back
to some constant absolute values, and not 0...

>  
> +        journal_size_max =
> size_parameter_evaluate(&arg_journal_size_max, available);

I don't think that we should really do any such logic for the journal
size limit. This should stay an absolute value I am sure. In
particular since the journal does not store its data in
/var/lib/coredump, which you base your evaluation on.

>  static int journal_file_setup_data_hash_table(JournalFile *f) {
>          uint64_t s, p;
>          Object *o;
> +        struct statvfs ss;
>          int r;
>  
>          assert(f);
>  
> +        if (fstatvfs(f->fd, &ss) < 0)
> +                return -errno;
> +
>          /* We estimate that we need 1 hash table entry per 768 of
>             journal file and we want to make sure we never get beyond
>             75% fill level. Calculate the hash table size for the
>             maximum file size based on these metrics. */
>  
> -        s = (f->metrics.max_size * 4 / 768 / 3) * sizeof(HashItem);
> +        s = (size_parameter_evaluate(&f->metrics.max_size, ss.f_bsize * ss.f_bavail) * 4 / 768 / 3) * sizeof(HashItem);
>          if (s < DEFAULT_DATA_HASH_TABLE_SIZE)
>                  s = DEFAULT_DATA_HASH_TABLE_SIZE;

Hmm, this doesn't look right. here we choose the hash table sizes to
use for a file, and I doubt we should base this on the currently
available disk space, since sizing the hashtable will have an effect
on the entire lifetime of the file, during which the available disk
space might change wildly.

I think it would be best not to do relative sizes for the journal file
max size at all, and continue to only support and absolute value for
that. 

> +
> +uint64_t size_parameter_evaluate(const SizeParameter *sp, uint64_t available) {
> +        if (sp->value == (uint64_t) -1)
> +                return (uint64_t) -1;
> +
> +        if (sp->relative)
> +                return sp->value * 0.01 * available;

Hmm, so this implements this as percentage after all. as mentioned in
my earlier mail, I think this should be normalized to 2^32 instead, so
that 2^32 maps to 100%...

Lennart

-- 
Lennart Poettering, Red Hat