Limit I/O vector count on Unix #75005

adamreichold · 2020-08-01T12:32:54Z

Unix systems enforce limits on the vector count when performing vectored I/O via the readv and writev system calls and return EINVAL when these limits are exceeded. This changes the standard library to handle those limits as short reads and writes to avoid forcing its users to query these limits using platform specific mechanisms.

Fixes #68042

rust-highfive · 2020-08-01T12:32:56Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @hanna-kruppe (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

adamreichold · 2020-08-01T12:38:30Z

This is my first PR and I am sorry if I messed up the procedure.

I also am rather unsure whether the overhead of SyncOnceCell for each call to writev is tenable but I also did not immediately find another instance where a system configuration parameter is queried via sysconf and then used with a frequently invoked system call.

library/std/src/sys/unix/fd.rs

tesuji · 2020-08-01T13:44:26Z

library/std/src/sys/unix/fd.rs

+
+        // 1024 is the default value on modern Linux systems
+        // and hopefully more useful than `c_int::MAX`.
+        lim = if ret > 0 { ret as usize } else { 1024 };


Should we set errno=0 before sysconf and check it again when ret == -1?

Ref https://man7.org/linux/man-pages/man3/sysconf.3p.html#RETURN_VALUE

I thought about this and decided to not try to handle/propagate errors here and silently fallback to a default value instead as

the value will not change for the lifetime of the process and I therefore want to report an error at most once to avoid repeatedly making a failing system call

but I also would not like the first call to FileDesc::write_vectored to behave observably different by reporting the error from libc::sysconf

thereby logging the error seems to be the only reasonable action in addition to the fallback but I did not find any infrastructure for logging within the standard library

While not explicitly check for -1?

I would suggest using 16 as the fallback value since that is the minimum guaranteed by POSIX.

While not explicitly check for -1?

Accepting zero is not reasonable either as calls to write_vectored would not make progress using that limit, hence the check for ret > 0 instead of checking ret == 0 and ret < 0 separately as the reaction is the same: use the fallback value.

I would suggest using 16 as the fallback value since that is the minimum guaranteed by POSIX.

Will do.

hanna-kruppe · 2020-08-01T13:49:33Z

I don't really know anything about vectored I/O. r? @Amanieu maybe?

Amanieu · 2020-08-01T13:51:59Z

On Linux UIO_MAXIOV is a constant defined in the kernel headers:

#define UIO_MAXIOV	1024

adamreichold · 2020-08-01T13:54:29Z

On Linux UIO_MAXIOV is a constant defined in the kernel headers:
#define UIO_MAXIOV	1024

Could you expand on the implication you see here? Do you mean that we should just hard code the constant and thereby avoid the call to sysconf? (Even with the plain relaxed load instead of SyncOnceCell?)

adamreichold · 2020-08-01T13:57:19Z

Could you expand on the implication you see here?

Or put differently, isn't the sysconf mechanism exactly meant to query such configuration parameters in a well-defined manner? Otherwise one would need to change our constant value here if the "official" value in the kernel headers changes or someone builds a custom kernel where the value is modified.

library/std/src/sys/unix/fd.rs

Amanieu · 2020-08-01T14:04:48Z

library/std/src/sys/unix/fd.rs

@@ -26,6 +27,35 @@ const READ_LIMIT: usize = c_int::MAX as usize - 1;
 #[cfg(not(target_os = "macos"))]
 const READ_LIMIT: usize = libc::ssize_t::MAX as usize;

+#[cfg(any(target_os = "linux", target_os = "macos"))]


This shouldn't be restricted to just Linux and macOS, all Unix-like OSes have limits on iovec length. Also it seems that all these targets have a definition of _SC_IOV_MAX so there is no need to special case _SC_UIO_MAXIOV for macOS.

This shouldn't be restricted to just Linux and macOS, all Unix-like OSes have limits on iovec length.

What I was wondering here is whether #[cfg(unix)] implies POSIX and hence allows relying on sysconf and friends? Will change the code under that assumption...

so there is no need to special case _SC_UIO_MAXIOV for macOS.

I am admittedly just not knowledgeable on macOS and took that information from the linked bug report. Will change both to just use _SC_IOV_MAX.

Amanieu · 2020-08-01T14:05:30Z

After thinking about it a bit, I think sticking to sysconf is probably safer in the long run.

library/std/src/sys/unix/fd.rs

adamreichold · 2020-08-05T08:07:19Z

@Amanieu Is there anything that is still needs of a resolution?

library/std/src/sys/unix/fd.rs

Amanieu · 2020-08-05T12:43:36Z

@bors r+

bors · 2020-08-05T12:43:37Z

📌 Commit 10203e90f836f4ef9310e028d37b123ea54f9a38 has been approved by Amanieu

bors · 2020-08-05T13:09:30Z

⌛ Testing commit 10203e90f836f4ef9310e028d37b123ea54f9a38 with merge 908d12265962911fcf0aeb6cedbacc693dd1c789...

bors · 2020-08-05T13:59:57Z

💔 Test failed - checks-actions

adamreichold · 2020-08-05T14:53:26Z

broken_heart Test failed - checks-actions

@Amanieu The build failed as Redox OS' libc does not define _SC_IOV_MAX. What do you think about adding a fallback like

#[cfg(target_os = "redox")]
fn max_iov() -> usize { 16 }

for this case?

Amanieu · 2020-08-05T14:54:04Z

Sounds good!

Both Linux and MacOS enforce limits on the vector count when performing vectored I/O via the readv and writev system calls and return EINVAL when these limits are exceeded. This changes the standard library to handle those limits as short reads and writes to avoid forcing its users to query these limits using platform specific mechanisms.

Keep the I/O vector count limit in a `SyncOnceCell` to avoid the overhead of repeatedly calling `sysconf` as these limits are guaranteed to not change during the lifetime of a process by POSIX.

All #[cfg(unix)] platforms follow the POSIX standard and define _SC_IOV_MAX so that we rely purely on POSIX semantics to determine the limits on I/O vector count.

adamreichold · 2020-08-05T15:07:58Z

Sounds good!

Added it and build tested on x86_64-unknown-redox. Does anybody know any other cfg(unix) targets up front that will need this before bors try this again?

adamreichold · 2020-08-05T15:13:25Z

Does anybody know any other cfg(unix) targets up front that will need this before bors try this again?

Grepping libc sources suggests target_env = "newlib" would be affected as well...

Amanieu · 2020-08-05T16:21:17Z

@bors r+

bors · 2020-08-05T16:21:19Z

📌 Commit 9073acd has been approved by Amanieu

Amanieu · 2020-08-05T16:21:27Z

@bors rollup=iffy

bors · 2020-08-05T17:59:00Z

⌛ Testing commit 9073acd with merge 52b179b...

bors · 2020-08-05T19:56:26Z

☀️ Test successful - checks-actions, checks-azure
Approved by: Amanieu
Pushing 52b179b to master...

@Amanieu

…r=Amanieu Use IOV_MAX and UIO_MAXIOV constants in limit vectored I/O Also updates the libc dependency to 0.2.77 (from 0.2.74) as the constants were only recently added. Related rust-lang#68042, rust-lang#75005 r? @Amanieu (also reviewed rust-lang#75005)

…Amanieu Use IOV_MAX and UIO_MAXIOV constants in limit vectored I/O Also updates the libc dependency to 0.2.77 (from 0.2.74) as the constants were only recently added. Related rust-lang#68042, rust-lang#75005 r? `@Amanieu` (also reviewed rust-lang#75005)

rust-highfive assigned hanna-kruppe Aug 1, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 1, 2020

adamreichold mentioned this pull request Aug 1, 2020

writev calls error out with EINVAL after update to 0.18 rustls/rustls#389

Closed

matklad reviewed Aug 1, 2020

View reviewed changes

library/std/src/sys/unix/fd.rs Outdated Show resolved Hide resolved

tesuji reviewed Aug 1, 2020

View reviewed changes

rust-highfive assigned Amanieu and unassigned hanna-kruppe Aug 1, 2020

tesuji reviewed Aug 1, 2020

View reviewed changes

library/std/src/sys/unix/fd.rs Outdated Show resolved Hide resolved

Amanieu reviewed Aug 1, 2020

View reviewed changes

adamreichold changed the title ~~Limit I/O vector count on Linux and macOS~~ Limit I/O vector count on Unix Aug 1, 2020

pickfire reviewed Aug 1, 2020

View reviewed changes

library/std/src/sys/unix/fd.rs Show resolved Hide resolved

Amanieu reviewed Aug 5, 2020

View reviewed changes

library/std/src/sys/unix/fd.rs Outdated Show resolved Hide resolved

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 5, 2020

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 5, 2020

adamreichold added 4 commits August 5, 2020 16:57

Memoize the I/O vector count limit

6672f7b

Keep the I/O vector count limit in a `SyncOnceCell` to avoid the overhead of repeatedly calling `sysconf` as these limits are guaranteed to not change during the lifetime of a process by POSIX.

Reduce synchronization overhead of I/O vector count memoization

87edccf

Rely only on POSIX semantics for I/O vector count

04a0114

All #[cfg(unix)] platforms follow the POSIX standard and define _SC_IOV_MAX so that we rely purely on POSIX semantics to determine the limits on I/O vector count.

Add fallback for cfg(unix) targets that do not define libc::_SC_IOV_MAX.

9073acd

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 5, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 5, 2020

bors merged commit 52b179b into rust-lang:master Aug 5, 2020

adamreichold deleted the limit-vector-count branch August 5, 2020 19:57

tmiasko mentioned this pull request Aug 6, 2020

Avoid unwrap_or_else in str indexing #75121

Merged

ecstatic-morse mentioned this pull request Aug 6, 2020

Keep stdout open in limit_vector_count test #75228

Merged

Thomasdezeeuw mentioned this pull request Sep 10, 2020

Use IOV_MAX and UIO_MAXIOV constants in limit vectored I/O #76561

Merged

cuviper added this to the 1.47.0 milestone May 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit I/O vector count on Unix #75005

Limit I/O vector count on Unix #75005

adamreichold commented Aug 1, 2020 •

edited

Loading

rust-highfive commented Aug 1, 2020

adamreichold commented Aug 1, 2020

tesuji Aug 1, 2020

tesuji Aug 1, 2020

adamreichold Aug 1, 2020 •

edited

Loading

tesuji Aug 1, 2020

Amanieu Aug 1, 2020

adamreichold Aug 1, 2020

hanna-kruppe commented Aug 1, 2020

Amanieu commented Aug 1, 2020 •

edited

Loading

adamreichold commented Aug 1, 2020

adamreichold commented Aug 1, 2020

Amanieu Aug 1, 2020

adamreichold Aug 1, 2020

Amanieu commented Aug 1, 2020

adamreichold commented Aug 5, 2020

Amanieu commented Aug 5, 2020

bors commented Aug 5, 2020

bors commented Aug 5, 2020

bors commented Aug 5, 2020

adamreichold commented Aug 5, 2020 •

edited

Loading

Amanieu commented Aug 5, 2020

adamreichold commented Aug 5, 2020

adamreichold commented Aug 5, 2020

Amanieu commented Aug 5, 2020

bors commented Aug 5, 2020

Amanieu commented Aug 5, 2020

bors commented Aug 5, 2020

bors commented Aug 5, 2020

Limit I/O vector count on Unix #75005

Limit I/O vector count on Unix #75005

Conversation

adamreichold commented Aug 1, 2020 • edited Loading

rust-highfive commented Aug 1, 2020

adamreichold commented Aug 1, 2020

tesuji Aug 1, 2020

Choose a reason for hiding this comment

tesuji Aug 1, 2020

Choose a reason for hiding this comment

adamreichold Aug 1, 2020 • edited Loading

Choose a reason for hiding this comment

tesuji Aug 1, 2020

Choose a reason for hiding this comment

Amanieu Aug 1, 2020

Choose a reason for hiding this comment

adamreichold Aug 1, 2020

Choose a reason for hiding this comment

hanna-kruppe commented Aug 1, 2020

Amanieu commented Aug 1, 2020 • edited Loading

adamreichold commented Aug 1, 2020

adamreichold commented Aug 1, 2020

Amanieu Aug 1, 2020

Choose a reason for hiding this comment

adamreichold Aug 1, 2020

Choose a reason for hiding this comment

Amanieu commented Aug 1, 2020

adamreichold commented Aug 5, 2020

Amanieu commented Aug 5, 2020

bors commented Aug 5, 2020

bors commented Aug 5, 2020

bors commented Aug 5, 2020

adamreichold commented Aug 5, 2020 • edited Loading

Amanieu commented Aug 5, 2020

adamreichold commented Aug 5, 2020

adamreichold commented Aug 5, 2020

Amanieu commented Aug 5, 2020

bors commented Aug 5, 2020

Amanieu commented Aug 5, 2020

bors commented Aug 5, 2020

bors commented Aug 5, 2020

adamreichold commented Aug 1, 2020 •

edited

Loading

adamreichold Aug 1, 2020 •

edited

Loading

Amanieu commented Aug 1, 2020 •

edited

Loading

adamreichold commented Aug 5, 2020 •

edited

Loading