Optimize `Seek::stream_len` impl for `File`

[tbu-]

It uses the file metadata on Unix with a fallback for files incorrectly reported as zero-sized. It uses GetFileSizeEx on Windows.

This reduces the number of syscalls needed for determining the file size of an open file from 3 to 1.

[rustbot]

r? @ChrisDenton

rustbot has assigned @ChrisDenton. They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[rust-log-analyzer]

The job mingw-check failed! Check out the build log: (web) (plain)

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

    Checking std_detect v0.1.5 (/checkout/library/stdarch/crates/std_detect)
error[E0308]: mismatched types
   --> library/std/src/sys/pal/windows/fs.rs:506:14
    |
506 |           Some(cvt(unsafe { c::GetFileSizeEx(self.handle.as_raw_handle(), &mut result) })
    |  _________----_^
    | |         arguments to this enum variant are incorrect
    | |         arguments to this enum variant are incorrect
507 | |             .map(|_| result))
    | |____________________________^ expected `Result<u64, Error>`, found `Result<i64, Error>`
    = note: expected enum `core::result::Result<u64, _>`
               found enum `core::result::Result<i64, _>`
               found enum `core::result::Result<i64, _>`
help: the type constructed contains `core::result::Result<i64, io::error::Error>` due to the type of the argument passed
   --> library/std/src/sys/pal/windows/fs.rs:506:9
    |
506 |            Some(cvt(unsafe { c::GetFileSizeEx(self.handle.as_raw_handle(), &mut result) })
    |  __________^____-
    | | _________|
    | ||
507 | ||             .map(|_| result))
    | ||____________________________-^
    |                               this argument influences the type of `Some`
note: tuple variant defined here
   --> /checkout/library/core/src/option.rs:579:5
    |

[rust-log-analyzer]

The job x86_64-gnu-llvm-17 failed! Check out the build log: (web) (plain)

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

#16 exporting to docker image format
#16 sending tarball 29.8s done
#16 DONE 35.5s
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-17]
---
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-17', '--enable-llvm-link-shared', '--set', 'rust.thin-lto-import-instr-limit=10', '--set', 'change-id=99999999', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-17/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.thin-lto-import-instr-limit := 10
configure: change-id            := 99999999
---
   Compiling addr2line v0.21.0
error: function cannot return without recursing
   --> library/std/src/fs.rs:953:5
    |
953 |     fn stream_len(&mut self) -> io::Result<u64> {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot return without recursing
954 |         (&*self).stream_len()
    |         --------------------- recursive call site
    |
    = help: a `loop` may express intention better if this is on purpose
    = note: `-D unconditional-recursion` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(unconditional_recursion)]`

error[E0596]: cannot borrow data in a `&` reference as mutable
    |
    |
954 |         (&*self).stream_len()
    |         ^^^^^^^^ cannot borrow as mutable
For more information about this error, try `rustc --explain E0596`.
error: could not compile `std` (lib) due to 2 previous errors
Build completed unsuccessfully in 0:00:15
  local time: Sun May 19 15:52:39 UTC 2024

[bors]

:umbrella: The latest upstream changes (presumably #127360) made this pull request unmergeable. Please resolve the merge conflicts.

[the8472]

with a fallback for files incorrectly reported as zero-sized.

Alas, this is insufficient. There are other filesystems with questionable stat impls that return incorrect values that aren't 0. For example sysfs.

$ stat -c %s /sys/kernel/oops_count 
4096
$ wc -c /sys/kernel/oops_count 
2 /sys/kernel/oops_count

[tbu-]

There are other filesystems with questionable stat impls that return incorrect values that aren't 0. For example sysfs.

Amazing, I did not foresee this.

Alas, this is insufficient.

For this particular example, this doesn't look insufficient, since it doesn't regress anything:

use std::fs::File;
use std::io;
use std::io::Seek as _;
use std::io::SeekFrom;

fn main() -> io::Result<()> {
    let file = File::open("/sys/kernel/oops_count")?;
    println!("{}", (&file).seek(SeekFrom::End(0))?);
    Ok(())
}

This outputs 4096 on my machine, the same as the stat result.

[…]
openat(AT_FDCWD, "/sys/kernel/oops_count", O_RDONLY|O_CLOEXEC) = 3
lseek(3, 0, SEEK_END)                   = 4096
write(1, "4096\n", 5)                   = 5
close(3)                                = 0

[tbu-]

~~It seems that wc falls back to reading the entire file if the file size is a multiple of the page size (4096 on my machine). I don't think we want to do that in Rust, that sounds horribly inefficient for files that happen to be a multiple of 4096 in size.~~

https://github.com/coreutils/coreutils/blob/74ef0ac8a56b36ed3d0277c3876fefcbf434d0b6/src/wc.c#L357-L365

EDIT: Seems like I was incorrect. It uses some heuristic to lseek to a bit before the end of file and reads from there. For an 1 GiB large file, it does the following:

openat(AT_FDCWD, "a", O_RDONLY)         = 3
fstat(3, {st_mode=S_IFREG|0644, st_size=1073741824, ...}) = 0
lseek(3, 1073618943, SEEK_CUR)          = 1073618943
fadvise64(3, 0, 0, POSIX_FADV_SEQUENTIAL) = 0
read(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 16384) = 16384
read(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 16384) = 16384
read(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 16384) = 16384
read(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 16384) = 16384
read(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 16384) = 16384
read(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 16384) = 16384
read(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 16384) = 16384
read(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 16384) = 8193
read(3, "", 16384)                      = 0
fstat(1, {st_mode=S_IFCHR|0620, st_rdev=makedev(0x88, 0x3), ...}) = 0
write(1, "1073741824 a\n", 13)          = 13
close(3)                                = 0

[the8472]

Ok, maybe seeking is nonsense on those files too and they're only meant to be used via read and write.

But that still leaves FUSE drivers which can implement basically arbitrary behavior. stat and seek aren't required to be consistent there either and seek might be the method doing the right thing.

And any approach distinguishing good from bad filesystems would require additional syscalls.

[tbu-]

But that still leaves FUSE drivers which can implement basically arbitrary behavior. stat and seek aren't required to be consistent there either and seek might be the method doing the right thing.

What is Rust's platform support for bad FUSE drivers? Who decides which of the methods to trust (fstat vs lseek)?

I can kinda see not crashing the Rust program if a FUSE driver decides to return EBADF for close. But this seems like a case of garbage in, garbage out, and it's on the user to not use file systems that lie to programs.

[the8472]

FUSE is just the canary in the coal mine. sysfs shows that in-kernel filesystems do strange things too. If we have 2 examples already then I assume there's some network filesystem will also do weird things when weird remote machines are involved.

So I'd say the trust hierarchy when it comes to "how many bytes does this file contain is" read > seek > stat

It's unfortunate that statx has a return field that's supposed to indicate what is actually supported by the kernel, but they're not using it to signal that reporting the size is unsupported on sysfs.

[tbu-]

FUSE is just the canary in the coal mine. sysfs shows that in-kernel filesystems do strange things too. If we have 2 examples already then I assume there's some network filesystem will also do weird things when weird remote machines are involved.

I had assumed lseek would error out for procfs, but turns out it doesn't:

$ python -c 'import os; print(open("/proc/1/cmdline").seek(0, os.SEEK_END))'
0

So after checking again, it seems that even for procfs lseek offers no advantage over fstat. This means the two examples (procfs, sysfs) we found are not examples where the two methods behave differently.

[ChrisDenton]

Sorry, just catching up. Given the above, @the8472 are you happy that this is at least no worse than the status quo? I.e. if seek or stat is returning wrong or nonsense values then that's a problem in either case.

[the8472]

I think it would be better to only do this for regular files. Non-regular ones have weird behavior in general. E.g. directories have a size according to stat but reading or seeking would error, so they don't have a meaningful stream length.

Even with that restriction I think someone will still eventually run into issues around FUSE, network filesystems or weird filesystems.

That said, stream_len is an unstable API and it's a convenience method, so we don't necessarily have to provide ironclad guarantees. Maybe we should just rephrase the documentation of Seek::stream_len. Currently it says

This method is implemented using up to three seek operations.

this would be better

The default implementation uses up to three seek operations.

I mean, it always applies to trait methods that trait impls can override them and can provide different behavior, so this isn't special. But perhaps it's better to not suggest that implementations will only ever behave exactly like that.