Check for buggy preadv2

[Darksonn]

Closes: #3803

cc @br0adcast @wmanley

[wmanley]

You beat me to it :)

FWIW in the impl I was brewing I made this comparison with [i8] avoiding the conversion via CStr like this:

        const _5_9: [i8; 4] = [53, 46, 57, 46]; // [i8] repr of b"5.9."
        const _5_10: [i8; 5] = [53, 46, 49, 48, 46]; // [i8] repr of b"5.10."
        uts.release[..4] == _5_9 || uts.release[..5] == _5_10

Not that it matters, this function will only be called ~once per process.

[wmanley]

I'll run my tests with this impl.

[wmanley]

I've run my test on 5.10 and it succeeds with this PR, and fails on master.

[Darksonn]

@wmanley I don't really understand the Linux kernel version numbers here. The Linux git repository only has tags for v5.10 and v5.11, and not for things like v5.10.17 like the version from the original issue.

[wmanley]

@wmanley I don't really understand the Linux kernel version numbers here. The Linux git repository only has tags for v5.10 and v5.11, and not for things like v5.10.17 like the version from the original issue.

Here's the git repo with the stable kernel versions: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git . This is where the point releases are tagged. I've tested both the mainline and the stable kernel tags.

[Darksonn]

I've tested both the mainline and the stable kernel tags.

I got the impression that you just did a bisection? Which tags did you try?

[wmanley]

I've tested both the mainline and the stable kernel tags.

I got the impression that you just did a bisection? Which tags did you try?

I did both.

I did a bisection on mainline to identify the specific commits where it was introduced and fixed. Having discovered that it was fixed on mainline I separately and additionally ran the test on the latest stable releases to see if the fix had been cherry-picked to any stable branch. The results of the stable branch checking is as follows:

5.6.19: OK
5.7.19: OK
5.8.18: OK
5.9.16: BAD
5.10.38: BAD
5.11.22: OK

My conclusion is that the kernel developers aren't/weren't even aware that the bug existed, so the fix for it hasn't been backported.

Bisecting to find first bad commit: #3803 (comment)
Bisecting to find first fixed commit: #3803 (comment)
Testing stable branches: #3803 (comment)

[carllerche]

Before merging this, I think that we need to have a plan to include testing different kernel versions as part of CI. we don't want to accidentally introduce regressions if we change this code.

Granted, testing different kernel versions is not trivial.

[blasrodri]

Before merging this, I think that we need to have a plan to include testing different kernel versions as part of CI. we don't want to accidentally introduce regressions if we change this code.

Granted, testing different kernel versions is not trivial.

I have not had the time to check in detail. But this project seems like an alternative https://github.com/linuxkit/linuxkit

[wmanley]

Before merging this, I think that we need to have a plan to include testing different kernel versions as part of CI. we don't want to accidentally introduce regressions if we change this code.

I'd be happy to have a crack at this, adapting the kernel testing I'd done in #3803 to CI. So we'd have a few pre-built kernels and run qemu in CI to see if the bug is present or not on each. Ideally we'd have a way of running against the latest RC kernel too to catch future regressions before they happen.

But first I'd like to know - is it worthwhile? Does anyone have a real-world benchmark that this helps with, or is there some other reason to want this?

[Darksonn]

A benchmark? It's mostly to make sure we catch regressions if we break something on some kernel versions only.

[wmanley]

A benchmark? It's mostly to make sure we catch regressions if we break something on some kernel versions only.

Not a benchmark for testing, but for #3518. The value in the kernel testing is to see if we can un-revert #3518, so the question is of whether #3518 is of sufficient value to bother. I wrote #3518 for fun rather than to satisfy a particular need that I have - I'm not actually a tokio user. I'm happy to have a go at test-infra, but not for it's own sake - only if it's actually providing someone with value. So it comes down to - is this something that will actually help people - for performance reasons, or otherwise?

[Darksonn]

Generally, file reading involves a whole bunch of cross-thread communication with the current implementation, so it would surprise me if it doesn't help.

I tried running the benchmarks in benches/fs.rs with and without the change.

Without the change
test async_read_buf      ... bench:   6,714,074 ns/iter (+/- 1,063,411)
test async_read_codec    ... bench:   6,727,703 ns/iter (+/- 893,984)

With the change
test async_read_buf      ... bench:   6,363,540 ns/iter (+/- 807,035)
test async_read_codec    ... bench:   6,454,059 ns/iter (+/- 857,237)

Though it's just reading from /dev/zero. I don't know how representative that is.

[the8472]

Instead of probing the kernel version wouldn't it be better to do a normal read when reaching end of file to verify correct behavior? Since the file should be at its end the kernel should be able to serve the request without blocking unless something is appending to the file between the preadv2 and the read call. But if something else is concurrently writing to the file I would expect the some internal locks causing the threadpool fallback anyway.