File: Try doing a non-blocking read before punting to the threadpool #3518

wmanley · 2021-02-12T23:26:06Z

Motivation

I saw this on the rust subreddit and I wondered if preadv2(..., RWF_NOWAIT) would help.

Solution

Try doing a non-blocking read before punting to the threadpool on Linux.

If the data is already available in cache this will avoid cross-thread interaction and remove a copy. It should help with latency too as reads that can be satisfied now won't need to wait in queue until other fs operations are complete.

According to my basic testing it helps. The benchmark I ran was https://github.com/wmanley/tokio-fs-bench which is based on the gist from the reddit thread above. On my laptop with the frequency scaling disabled I get 32s with this change and 42s without. For comparison the rust_block_in_place implementation runs in 11s.

So it's faster in this micro-benchmark, but it would be good to see if it actually helps with representative workloads. Perhaps you can suggest something?

~~The implementation is a bit rough-and-ready. It requires an unreleased libc and currently it'll only work on glibc.~~ I wanted to raise it early to get feedback as to whether the work is worthwhile in the first place.

TODO:

Resolve linker failures when compiling against old glibc which predate preadv2
Copy definition of RWF_NOWAIT too
Add tests for preadv2_safe, particularly around uninitialised data handling.
Comment why we're using the syscall directly

Darksonn · 2021-02-13T10:10:44Z

Are we sure this works? I have encountered lots of APIs where files just pretend that they are not blocking and then proceed to block anyway.

wmanley · 2021-02-13T11:59:40Z

Are we sure this works?

The man page says:

   RWF_NOWAIT (since Linux 4.14)
          Do not wait for data which is not immediately available.  If this flag is specified,
          the  preadv2()  system call will return instantly if it would have to read data from
          the backing storage or wait for a lock.  If some data was successfully read, it will
          return  the  number of bytes read.  If no bytes were read, it will return -1 and set
          errno to EAGAIN.  Currently, this flag is meaningful only for preadv2().

And here's a lwn.net article discussing the same: https://lwn.net/Articles/636967/ :

The normal workaround ... is to use thread pools for the I/O, but that pattern "kinda sucks". The latency added due to synchronization between the threads is not insubstantial. It is also often the case that requests that could be satisfied quickly get stuck behind slower requests.

... preadv2(), which is like preadv() except that there is a new flags argument ... There is only one flag available in his patches: RWF_NONBLOCK ... That flag will cause reads to succeed only if the data is already in the page cache, otherwise it will return EAGAIN.

And if RWF_NOWAIT mode isn't supported it returns ENOTSUPP rather than just implicitly blocking. I've seen this when reading from /dev/urandom

Darksonn · 2021-02-13T15:25:04Z

I guess that does sound like it should work.

carllerche · 2021-02-19T18:32:34Z

I skimmed the PR and looks promising. The overall detection strategy and static atomic seem fine as well. I'm not particularly worried about any overhead around checking an atomic. So, I guess apply any changes you still need to and flag the PR for review... a test somehow would be nice, but I'm not sure how that would work off the top of my head.

Darksonn · 2021-03-21T21:20:49Z

It seems like the libc change has been released.

wmanley · 2021-03-22T01:53:40Z

a test somehow would be nice, but I'm not sure how that would work off the top of my head.

I've added a test using fadvise to evict the data from cache to ensure that both the cached and the uncached codepaths are tested. I've also flagged it for review.

Darksonn

In general the PR looks pretty good.

tokio/src/fs/file.rs

mqudsi

This PR is a really good idea. I just corrected a few minor spelling/grammar things in the comments because why not.

tokio/src/fs/file.rs

tokio/tests/fs_file.rs

wmanley · 2021-03-25T17:16:15Z

I don't know why CI is failing now. I only changed some comments, and it seems to affect code that is unrelated to my change.

Darksonn · 2021-03-25T17:32:22Z

It's because the new release of Rust added some new warnings.

...on Linux. If the data is already available in cache this will avoid cross-thread interaction and remove a copy. It should help with latency too as reads that can be satisfied now won't need to wait in queue until other fs operations are complete.

...in an attempt to stimulate both the `preadv2` direct from cache codepath in addition to the punt to a threadpool uncached one.

This way we don't need to worry about compatiblity with old glibc versions and it will work on Android and musl too. The downside is that you can't use `LD_PRELOAD` tricks any more to intercept these calls.

wmanley · 2021-03-26T16:40:13Z

Ok, I've rebased to fix CI squashing in the various fixes. There's nothing left on the TODO list that I can think of now.

tokio/src/fs/file.rs

Darksonn · 2021-03-31T09:26:58Z

I think it would be good to add a short comment that explains why we are calling the syscall directly rather than using the libc function.

Darksonn · 2021-04-07T18:35:08Z

I was just looking through the list of PRs. Are there any things that needs to be done, or do we just need a final review?

…adpool

wmanley · 2021-04-14T00:19:06Z

I think it would be good to add a short comment that explains why we are calling the syscall directly rather than using the libc function.

Done

I was just looking through the list of PRs. Are there any things that needs to be done, or do we just need a final review?

I've resolved all the outstanding issues now. Thanks for the review. I've included my changes since the last review as fixup commits. Let me know if you want me to squash them.

Darksonn · 2021-04-14T07:03:13Z

I squash them on merge, so it is unnecessary in the PR branch.

Darksonn

Thanks, I think this looks good now.

wmanley · 2021-04-14T10:20:40Z

I squash them on merge, so it is unnecessary in the PR branch.

Ok, there will be conflicts, but if you're comfortable with that so am I :).

…dpool (#3518)" This reverts commit 39706b1.

…he threadpool (#3518)"" This reverts commit 3fbcf1b.

Darksonn added A-tokio Area: The main tokio crate M-fs Module: tokio/fs labels Feb 13, 2021

wmanley force-pushed the fs-preadv2-RWF_NOWAIT branch from 227421a to 2b29d82 Compare February 13, 2021 23:03

wmanley force-pushed the fs-preadv2-RWF_NOWAIT branch 3 times, most recently from 23a3c31 to e53eb67 Compare March 21, 2021 23:58

wmanley marked this pull request as ready for review March 22, 2021 01:50

Darksonn reviewed Mar 22, 2021

View reviewed changes

tokio/src/fs/file.rs Outdated Show resolved Hide resolved

mqudsi reviewed Mar 24, 2021

View reviewed changes

tokio/src/fs/file.rs Outdated Show resolved Hide resolved

tokio/tests/fs_file.rs Outdated Show resolved Hide resolved

wmanley force-pushed the fs-preadv2-RWF_NOWAIT branch 2 times, most recently from 5bd8ea4 to 6ff4932 Compare March 26, 2021 01:43

wmanley added 3 commits March 26, 2021 15:50

tests/fs_file.rs: Explicitly drop temp file from cache

48576b5

...in an attempt to stimulate both the `preadv2` direct from cache codepath in addition to the punt to a threadpool uncached one.

File: preadv2: Call the syscall directly rather than via glibc

0b7d90e

This way we don't need to worry about compatiblity with old glibc versions and it will work on Android and musl too. The downside is that you can't use `LD_PRELOAD` tricks any more to intercept these calls.

wmanley force-pushed the fs-preadv2-RWF_NOWAIT branch from 6ff4932 to 0b7d90e Compare March 26, 2021 15:57

Darksonn reviewed Mar 27, 2021

View reviewed changes

tokio/src/fs/file.rs Outdated Show resolved Hide resolved

tokio/src/fs/file.rs Outdated Show resolved Hide resolved

Darksonn mentioned this pull request Mar 31, 2021

tokio::fs + async is 1-2 orders of magnitude slower than a blocking version #3664

Open

Darksonn reviewed Mar 31, 2021

View reviewed changes

tokio/src/fs/file.rs Outdated Show resolved Hide resolved

wmanley force-pushed the fs-preadv2-RWF_NOWAIT branch from b9a8fcd to b0c98df Compare April 13, 2021 23:07

wmanley force-pushed the fs-preadv2-RWF_NOWAIT branch from b0c98df to 007d363 Compare April 13, 2021 23:28

fixup! File: Try doing a non-blocking read before punting to the thre…

223a450

…adpool

wmanley force-pushed the fs-preadv2-RWF_NOWAIT branch from 007d363 to 3a65b19 Compare April 13, 2021 23:43

fixup! File: preadv2: Call the syscall directly rather than via glibc

5874d63

wmanley force-pushed the fs-preadv2-RWF_NOWAIT branch from 3a65b19 to 5874d63 Compare April 13, 2021 23:47

Darksonn approved these changes Apr 14, 2021

View reviewed changes

Darksonn merged commit 39706b1 into tokio-rs:master Apr 14, 2021

wmanley mentioned this pull request Apr 27, 2021

Use new Linux 5.12 openat2 with RESOLVE_CACHED flag #3733

Open

Darksonn mentioned this pull request May 14, 2021

Prepare Tokio v1.6.0 #3782

Merged

wmanley mentioned this pull request May 19, 2021

fs::File reads first 4096 bytes only on Linux v5.9 and v5.10 #3803

Closed

Darksonn added a commit that referenced this pull request May 28, 2021

Revert "fs: try doing a non-blocking read before punting to the threa…

98e1480

…dpool (#3518)" This reverts commit 39706b1.

Darksonn mentioned this pull request May 28, 2021

Prepare tokio 1.6.1 #3822

Merged

carllerche pushed a commit that referenced this pull request May 28, 2021

Revert "fs: try doing a non-blocking read before punting to the threa…

3fbcf1b

…dpool (#3518)" This reverts commit 39706b1.

Darksonn added a commit that referenced this pull request Jun 2, 2021

Revert "Revert "fs: try doing a non-blocking read before punting to t…

72c8816

…he threadpool (#3518)"" This reverts commit 3fbcf1b.

wmanley mentioned this pull request Jul 12, 2021

Check for buggy preadv2 #3821

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

File: Try doing a non-blocking read before punting to the threadpool #3518

File: Try doing a non-blocking read before punting to the threadpool #3518

wmanley commented Feb 12, 2021 •

edited

Loading

Darksonn commented Feb 13, 2021

wmanley commented Feb 13, 2021

Darksonn commented Feb 13, 2021

carllerche commented Feb 19, 2021

Darksonn commented Mar 21, 2021

wmanley commented Mar 22, 2021

Darksonn left a comment

mqudsi left a comment

wmanley commented Mar 25, 2021

Darksonn commented Mar 25, 2021

wmanley commented Mar 26, 2021

Darksonn commented Mar 31, 2021

Darksonn commented Apr 7, 2021

wmanley commented Apr 14, 2021

Darksonn commented Apr 14, 2021

Darksonn left a comment

wmanley commented Apr 14, 2021

File: Try doing a non-blocking read before punting to the threadpool #3518

File: Try doing a non-blocking read before punting to the threadpool #3518

Conversation

wmanley commented Feb 12, 2021 • edited Loading

Motivation

Solution

Darksonn commented Feb 13, 2021

wmanley commented Feb 13, 2021

Darksonn commented Feb 13, 2021

carllerche commented Feb 19, 2021

Darksonn commented Mar 21, 2021

wmanley commented Mar 22, 2021

Darksonn left a comment

Choose a reason for hiding this comment

mqudsi left a comment

Choose a reason for hiding this comment

wmanley commented Mar 25, 2021

Darksonn commented Mar 25, 2021

wmanley commented Mar 26, 2021

Darksonn commented Mar 31, 2021

Darksonn commented Apr 7, 2021

wmanley commented Apr 14, 2021

Darksonn commented Apr 14, 2021

Darksonn left a comment

Choose a reason for hiding this comment

wmanley commented Apr 14, 2021

wmanley commented Feb 12, 2021 •

edited

Loading