0.30.6 #133
Merged
0.30.6 #133
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
donald
force-pushed
the
0.30.6
branch
5 times, most recently
from
c4b0945
to
5c096d1
Compare
Not all fields of struct argp_option and struct argp are initialized:
mxqset.c:192:5: warning: missing initializer for field ‘group’ of ‘const struct argp_option’ [-Wmissing-field-initializers]
{"whitelist", 15, "", 0, NULL},
mxqset.c:196:21: warning: missing initializer for field ‘children’ of ‘const struct argp’ [-Wmissing-field-initializers]
static const struct argp argp = { options, parser, NULL, NULL }
Use structure initializers with designators, which initialize all
remaining fields with their zero values.
Gcc can warn on implicit fallthoughs. Mark them with a comment which is recognized by gcc.
Make slots_to_start unsigned to match slots_per_job with which we compare.
donald
force-pushed
the
0.30.6
branch
3 times, most recently
from
c8ed211
to
ae9e30d
Compare
mx_vasprintf_forever and mx_asprintf_forever can't fail, so don't return an (always zero) status but the allocated string instead.
Remove two functions which have degenerated into a single source line and have one caller only.
In child, call _exit() instead of exit() after failed exec() to avoid library destructors being called. Nitpicks: Change pid to pid_t, don't let the child check for fork error.
Avoid the variadic api, because its slower. Also, the variadic funtions is more prone to errors, because its easy to forget the last argument which must be NULL.
Return 0 on success, -1 on failure. This change will make the following commit easier to follow.
exec_reaper only returns on failure, so don't return a status value.
Use close_range to set all file descriptors to close-on-exec to make sure that we don't leak the mysql socket (or any other) file descriptor to the user process. Now that we are sure, that the mysql socket is not leaked to the user and that the child doesn't call into mysql library code, because it will exec() or _exit(), there no longer is a need to disconnect from and reconnect to the mysql server every time we start a job, which is an expensive operation in mxqd and on the mysql server. Remove mx_mysql_disconnect before and mx_mysql_reconnect after the fork.
Use the external helper to unmount the tmpdir. We are going to experiment with more complex tmpdir settings in the future which might be difficult to code in C.
The number shown in the messages doesn't make any sense when we are running `mxqd --recover-only`, so remove the message. 2022-04-25 19:35:43 +0200 mxqd[122093]: recover: reload 9 running jobs from database 2022-04-25 19:35:43 +0200 mxqd[122093]: job finished (via fspool) : job 38218625 pid 79937 status 0 2022-04-25 19:35:43 +0200 mxqd[122093]: job finished (via fspool) : job 38315022 pid 41063 status 0 2022-04-25 19:35:49 +0200 mxqd[122093]: job finished (via fspool) : job 38357611 pid 21477 status 0 2022-04-25 19:35:49 +0200 mxqd[122093]: job finished (via fspool) : job 38359832 pid 52872 status 0 2022-04-25 19:35:49 +0200 mxqd[122093]: recover: processed 12008 finished jobs from fspool
This reverts commit 83531e8. There will be another solution in the following commit.
The function
int mx_strtoul(char *str, unsigned long int *to)
and three similar functions either returns with the success value 0 or
with a negative error code (-errno). The output argument `to` is only
written, when the return value signals success. If the function returns
with a negative value, the caller can not assument that `to` has been
set.
When a caller does something like this
int value;
res = int mx_strtoul(str, &value);
if (res < 0)
return;
do_something_with(value)
everything is correct according to the description aboce.
The code in mx_strtoul has this pattern:
errno = 0;
ul = strtoul(str, &end, 0);
if (errno) {
return -errno;
}
*to = ul;
When the caller is in the same compilation unit, the compiler (gcc with
-O2 or higher and with -Wall) might raise a "may be used uninitialized"
warning for the output variable (`value` in the above example).
The problem here is, that the caller assumes a negative value for an
error status but the callee returns -errno for any errno value other
than zero which might include negative errno values. We know, that the
errno value will not be negative because we've set it to zero and the
library function will conditionally set it to a valid error number only,
which is alway positiv[1]. But the compiler doesn't share this
assumption and takes a negative errno value into account.
When the caller and the callee are in the same compilation unit, the
interprocedural optimizer might detect the code path with a negative
errno value which would in fact trigger a usage of an unitilialized
value.
We can fix that by just baking the assumption into the comparison:
if (errno > 0)
return -errno;
or by asserting that errno is not negative:
if (errno) {
assert(errno > 0);
return -errno;
}
or by defining and using an __assume macro:
#if defined(__clang__)
#define assume(cond) __builtin_assume(cond)
#elif defined(__GNUC__)
#define assume(cond) do { if (!(cond)) __builtin_unreachable(); } while (0)
#endif
...
if (errno) {
assume(errno > 0);
return -errno;
}
Use the first option for all four affected functions.
Note, that clangs static analyzer is still not happy with this, but
this is probably a false positive of the analyzer [2].
[1]: errno(3): "Valid error numbers are all positive numbers.
[2]: https://github.com/llvm/llvm-project/issues/55241
When we run into a (very unlikely) early error condition, the error
macros might output s->func. While glibc printf functions output
"(null)" if NULL is passed to "%s", this is not guaranteed.
For example, gcc transforms `printf("%s\n", ptr)` into `puts(ptr)`.
Enable -Wextra. Exclude -Wno-override-init for now, because mx_getopt.h relies on it [1]. [1]: #131
The umount of the job tmpdir often takes a lot of time (minutes!) when the user write a lot of data to it, because there are many dirty pages which are written to the disk before the umount completes. This writing is obviously a waste of resources, because as soon as the unmount is finished, the backup image for the filesystem is destroyed anyway. Unfortunatly, we currenty don't have a solid way to avoid the unnecessary writeback. What we can do is to purge the directory. Experiments show, that this is indeed faster with both: few very big files or many small files. So do that. Hard-code the directory prefix into the rm command to decrease the risk of rm removing wrong files in case future code changes are buggy and, for example, missspell a shell variable. Because the cleanup still needs time, do that in the background so that mxqd can continue.
Sign in
to join this conversation on GitHub.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
tmpdir is now on by default (100 GiB)
Performance: mxqd reaper is now implemented as an external helper which gets exec()ed. This way it has an new and small address space, not one cloned from mxqd. The address space is charged for the users, so small mxq jobs can now run with smaller memory limits.
Performance: mxqd no longer disconnects from and reconnects to the mysql server for every job start (and in another place)
umount/cleanup of tmpdir is now done by an external helper for easier maintainability.
Performance: umount/cleanup now tries to remove files before doing the umount to decrease writeback
Performance: umount/cleanup of tmpdir is done in the background, mxqd doesn't wait for it
General code cleanup
Fixes #132
Fixes #121
Fixes #113