don't splice from files into pipes in io::copy

[the8472]

This fixes potential data ordering issue where a write performed after a copy operation could become visible in the copy even though it signaled completion.

I assumed that by not setting SPLICE_F_MOVE we would be safe and the kernel would do a copy in kernel space and we could avoid the read-write syscall and copy-to/from-userspace costs. But apparently that flag only makes a difference when splicing from a pipe, but not when splicing into it.

Context: https://lkml.org/lkml/2023/2/9/673

[rustbot]

r? @m-ou-se

(rustbot has picked a reviewer for you, use r? to override)

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[ChrisDenton]

Reading through that thread it makes sense but.. that seems unfortunate? This could be a soundness issue, right?

Does this need a test?

[the8472]

I don't think rust code can see bytes in memory change under its nose, so there's no soundness issue with non-deterministic bytes.
But it could be an ordering issue leading to logic bugs where one copies data and then modifies the source and the modified version becomes visible on the other side instead of the expected original.

Thinking about it, this shouldn't affect sockets because their buffers can't be modified, so splicing from a socket to a pipe could still be ok. The linked mail also describes this as case 1.

And yeah, I'll add a test, it's subtle enough.

@rustbot author

[the8472]

Oh bloody, sendfile has the same issue:

 If out_fd refers to a socket or pipe with zero-copy support,
callers must ensure the transferred portions of the file referred
to by in_fd remain unmodified until the reader on the other end
of out_fd has consumed the transferred data.

Removing that optimization for the File -> Socket case removes the main benefit of sendfile that people have asked for. 🙁

[the8472]

@rustbot ready

added a test and checked that it fails without the fix.

[ChrisDenton]

Thank you for investigating this.

@bors r+

[bors]

📌 Commit 171ccb5 has been approved by ChrisDenton

It is now in the queue for this repository.

[bors]

⌛ Testing commit 171ccb5 with merge 4f418e71c2c591d3f3b2811ddf62cebac5a3b7ba...

[bors]

💔 Test failed - checks-actions

[ChrisDenton]

Hm, STATUS_ILLEGAL_INSTRUCTION on i686-pc-windows-msvc looks a bit worrying but it should not have anything to do with this PR.

@bors retry

[bors]

⌛ Testing commit 171ccb5 with merge a29dada...

[bors]

☀️ Test successful - checks-actions
Approved by: ChrisDenton
Pushing a29dada to master...

[rust-timer]

Finished benchmarking commit (a29dada): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.4%	[1.4%, 1.4%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.6%	[0.6%, 0.6%]	1
Regressions ❌ (secondary)	1.6%	[1.6%, 1.6%]	1
Improvements ✅ (primary)	-0.7%	[-0.7%, -0.7%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.1%	[-0.7%, 0.6%]	2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.6%	[0.6%, 0.6%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.7%	[-0.7%, -0.7%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.1%	[-0.7%, 0.6%]	2

[rust-log-analyzer]

The job i686-msvc-1 failed! Check out the build log: (web)(plain)

Click to see the possible cause of the failure (guessed by this bot)

 Compiling sourcegen v0.0.0 (C:\a\rust\rust\src\tools\rust-analyzer\crates\sourcegen)
[RUSTC-TIMING] sourcegen test:false 3.667
Compiling proc-macro-srv-cli v0.0.0 (C:\a\rust\rust\src\tools\rust-analyzer\crates\proc-macro-srv-cli)
[RUSTC-TIMING] rust_analyzer_proc_macro_srv test:false 0.976
Assertion failed: !KeyInfoT::isEqual(Val, EmptyKey) && !KeyInfoT::isEqual(Val, TombstoneKey) && "Empty/Tombstone value shouldn't be inserted into map!", file C:\a\rust\rust\src\llvm-project\llvm\include\llvm/ADT/DenseMap.h, line 624
error: could not compile `hir-ty`
Caused by:
Caused by:
process didn't exit successfully: `C:\a\rust\rust\build\bootstrap\debug\rustc --crate-name hir_ty --edition=2021 crates\hir-ty\src\lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --crate-type lib --emit=dep-info,metadata,link -C opt-level=3 -C embed-bitcode=no -C debuginfo=0 -C debug-assertions=on -Zunstable-options --check-cfg values(feature) --check-cfg names() --check-cfg values() -C metadata=91277913358a0422 -C extra-filename=-91277913358a0422 --out-dir C:\a\rust\rust\build\i686-pc-windows-msvc\stage2-tools\i686-pc-windows-msvc\release\deps --target i686-pc-windows-msvc -L dependency=C:\a\rust\rust\build\i686-pc-windows-msvc\stage2-tools\i686-pc-windows-msvc\release\deps -L dependency=C:\a\rust\rust\build\i686-pc-windows-msvc\stage2-tools\release\deps --extern arrayvec=C:\a\rust\rust\build\i686-pc-windows-msvc\stage2-tools\i686-pc-windows-msvc\release\deps\libarrayvec-9d6091a37119fcba.rmeta --extern base_db=C:\a\rust\rust\build\i686-pc-windows-msvc\stage2-tools\i686-pc-windows-msvc\release\deps\libbase_db-f10262e2745c3ae3.rmeta --extern bitflags=C:\a\rust\rust\build\i686-pc-windows-msvc\stage2-tools\i686-pc-windows-msvc\release\deps\libbitflags-dee732d3861c88b9.rmeta --extern chalk_derive=C:\a\rust\rust\build\i686-pc-windows-msvc\stage2-tools\release\deps\chalk_derive-74fe33590d67bb02.dll --extern chalk_ir=C:\a\rust\rust\build\i686-pc-windows-msvc\stage2-tools\i686-pc-windows-msvc\release\deps\libchalk_ir-c1e2d9a03988523e.rmeta --extern chalk_recursive=C:\a\rust\rust\build\i686-pc-windows-msvc\stage2-tools\i686-pc-windows-msvc\release\deps\libchalk_recursive-ff61d111d47ed187.rmeta --extern chalk_solve=C:\a\rust\rust\build\i686-pc-windows-msvc\stage2-tools\i686-pc-windows-msvc\release\deps\libchalk_solve-8753f40861d45017.rmeta --extern cov_mark=C:\a\rust\rust\build\i686-pc-windows-msvc\stage2-tools\i686-pc-windows-msvc\release\deps\libcov_mark-cb482023ffa296a4.rmeta --extern either=C:\a\rust\rust\build\i686-pc-windows-msvc\stage2-tools\i686-pc-windows-msvc\release\deps\libeither-52470f2aa7c714f1.rmeta --extern ena=C:\a\rust\rust\build\i686-pc-windows-msvc\stage2-tools\i686-pc-windows-msvc\release\deps\libena-a3265d6f37d7312f.rmeta --extern hir_def=C:\a\rust\rust\build\i686-pc-windows-msvc\stage2-tools\i686-pc-windows-msvc\release\deps\libhir_def-68051d7002233a5e.rmeta --extern hir_expand=C:\a\rust\rust\build\i686-pc-windows-msvc\stage2-tools\i686-pc-windows-msvc\release\deps\libhir_expand-066e10b1cf041501.rmeta --extern rustc_index=C:\a\rust\rust\build\i686-pc-windows-msvc\stage2-tools\i686-pc-windows-msvc\release\deps\libhkalbasi_rustc_ap_rustc_index-50488986d1e2acaa.rmeta --extern intern=C:\a\rust\rust\build\i686-pc-windows-msvc\stage2-tools\i686-pc-windows-msvc\release\deps\libintern-5ff99a8a902b6291.rmeta --extern itertools=C:\a\rust\rust\build\i686-pc-windows-msvc\stage2-tools\i686-pc-windows-msvc\release\deps\libitertools-53668c30ac2c4538.rmeta --extern la_arena=C:\a\rust\rust\build\i686-pc-windows-msvc\stage2-tools\i686-pc-windows-msvc\release\deps\libla_arena-55a88d685f7b2b4c.rmeta --extern limit=C:\a\rust\rust\build\i686-pc-windows-msvc\stage2-tools\i686-pc-windows-msvc\release\deps\liblimit-ac07f608f496cd98.rmeta --extern once_cell=C:\a\rust\rust\build\i686-pc-windows-msvc\stage2-tools\i686-pc-windows-msvc\release\deps\libonce_cell-c4fab29fec5d0f5d.rmeta --extern profile=C:\a\rust\rust\build\i686-pc-windows-msvc\stage2-tools\i686-pc-windows-msvc\release\deps\libprofile-d2f6d4b9b0075f24.rmeta --extern rustc_hash=C:\a\rust\rust\build\i686-pc-windows-msvc\stage2-tools\i686-pc-windows-msvc\release\deps\librustc_hash-9951669e3cef155d.rmeta --extern scoped_tls=C:\a\rust\rust\build\i686-pc-windows-msvc\stage2-tools\i686-pc-windows-msvc\release\deps\libscoped_tls-130876785c1f2e9c.rmeta --extern smallvec=C:\a\rust\rust\build\i686-pc-windows-msvc\stage2-tools\i686-pc-windows-msvc\release\deps\libsmallvec-a756a535f31976a3.rmeta --extern stdx=C:\a\rust\rust\build\i686-pc-windows-msvc\stage2-tools\i686-pc-windows-msvc\release\deps\libstdx-99e203d50d6741af.rmeta --extern syntax=C:\a\rust\rust\build\i686-pc-windows-msvc\stage2-tools\i686-pc-windows-msvc\release\deps\libsyntax-0f3d1f69b6409078.rmeta --extern tracing=C:\a\rust\rust\build\i686-pc-windows-msvc\stage2-tools\i686-pc-windows-msvc\release\deps\libtracing-268a7a2d20d16c24.rmeta --extern typed_arena=C:\a\rust\rust\build\i686-pc-windows-msvc\stage2-tools\i686-pc-windows-msvc\release\deps\libtyped_arena-757c6870dba4e216.rmeta -Csymbol-mangling-version=v0 -Zunstable-options --check-cfg=values(bootstrap) -Zmacro-backtrace -Csplit-debuginfo=packed -Ctarget-feature=+crt-static -Z binary-dep-depinfo -L native=C:\Users\runneradmin\.cargo\registry\src\index.crates.io-1cd66030c949c28d\windows_i686_msvc-0.42.1\lib` (exit code: 0xc000001d, STATUS_ILLEGAL_INSTRUCTION)
[RUSTC-TIMING] xtask test:false 23.027
[RUSTC-TIMING] hir_def test:false 265.730
[RUSTC-TIMING] ide_assists test:false 180.159
[RUSTC-TIMING] project_model test:false 72.604
[RUSTC-TIMING] project_model test:false 72.604
[RUSTC-TIMING] ide test:false 128.652
[RUSTC-TIMING] rust_analyzer test:false 85.970
Build completed unsuccessfully in 1:02:02
make: *** [Makefile:68: ci-subset-1] Error 1

[NobodyXu]

For optimization reverted here, I think we could add it back with an intermediate pipe?

I.e. first splice to an intermediate pipe with F_MOVE, then splice to the destination pipe without F_MOVE?

The intermediate pipe can be cached and reused, given that we can make sure there's no panicking within io::copy

[the8472]

If that works at all it'd still negate some of the advantages of splice. More syscalls, more copying. It still might provide some more throughput because pipes would be a larger buffer than our 8k on-stack buffer. Maybe worth a try.

In theory using sendfile/splice from read-only files might be ok, but then the question is in which state the file or file descriptor must be so that we could consider it sufficiently read-only.
Another theoretical option would be to wait for the socket send buffer to drain. Which would be fine for sending huge files, but not when using io::copy to send some chunk of a smaller file.

[NobodyXu]

Maybe worth a try.

Thanks, I'd try opening a PR for this.

but then the question is in which state the file or file descriptor must be so that we could consider it sufficiently read-only.

Unless it's /dev/zero or on an immutable system, I suppose the file can be changed at anytime.

I thought about if CoW filesystem is immune to this, but then I think that's impossible because the mmap abstraction provides by the kernel guarantees it will see updates.

That reminds me, that on CoW fs, copy_file_range can be very cheap as it requires no copy only reflink, we could also create a O_TMPFILE and use it to achieve zero-copy.

Another theoretical option would be to wait for the socket send buffer to drain.

I think the best fix would be for the Linux kernel to fix this, if they would provide a flag to opt-in to CoW behavior.

It'd also be great if mmap has such flags, currently it says with MMAP_PRIVATE read-only mmap, it is unspecified if changes to the original file would affect the mmap.

[NobodyXu]

@the8472 I think sealed files can be used here, if the file has flag F_SEAL_WRITE (prevent modifying of existing contents, shrinking and appending is allowed) and F_SEAL_SEAL (not changing of seals).

[NobodyXu]

So to sum up, I think we can add back optimization via:

• if the file is on squashfs/erofs, then it's immutable (detectable via statfs
• if the is sealed with F_SEAL_WRITE (prevent modifying of existing contents, shrinking and appending is allowed) and F_SEAL_SEAL (not changing of seals), detectable via fcntl(fd, F_GET_SEALS)
• if it's on read-only btrfs subvolume (e.g. snapshot), likewise we can do the same for zfs, nilfs, bcachefs
• If the file is /dev/zero, or from other pseudo fs (e.g. procfs, sysfe), then I think we can assume it's immutable

Otherwise, we'd fallback to:

• read + write if the file is really small
• splice to an intermediate pipe with F_MOVE and then splice to the pipe without F_MOVE if the file is more than 1-2 pages
• if the fs supports reflink and file is really large, then we could create a O_TMPFILE on the system, copy_file_range to it and then splice it

Also, if the file is a named fifo, then just a splice without F_MOVE would work fine.

[NobodyXu]

@the8472 opened #128300