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

[wmanley]

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]

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]

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]

I guess that does sound like it should work.

[carllerche]

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]

It seems like the libc change has been released.

[wmanley]

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.

[mqudsi]

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

[wmanley]

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]

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

[wmanley]

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.

[Darksonn]

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]

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?

[wmanley]

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]

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

[Darksonn]

Thanks, I think this looks good now.

[wmanley]

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 :).