On Mon, 25.05.15 13:48, jsyna...@redhat.com (jsyna...@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 _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel