-
Notifications
You must be signed in to change notification settings - Fork 12.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prefer statx on linux if available #65094
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (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. |
I think the doc needs to be updated, see here:
It could be changed to something like:
|
This is looking good to me, thanks! My main comment would be to make the platform-specific code here a bit more maintainable. Would it be possible to instead of duplicating each function only include Linux-specific code like so: #[cfg(target_os = "linux")]
{
// statx code...
}
// the current implementation as a fallback That should hopefully make it a bit more clear that this is the same as the current code, only it has a Linux-specific fallback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few final comments, but otherwise r=me, thanks!
src/libstd/sys/unix/fs.rs
Outdated
stat.st_ctime_nsec = buf.stx_ctime.tv_nsec as i64; | ||
|
||
let extra = StatxExtraFields { | ||
stx_mask: buf.stx_mask, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This stx_mask
field, according to the man page, looks to indicate whether fields are actually available or not. This is used for the creation time, but should this also be taken into account for all the fields above as well? (presumably for them we'd return an error if they aren't available?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we need to check buf.statx_mask & STATX_BASIC_STATS == STATX_BASIC_STATS
?
But what would be returned by stat64
for the case of missing basic fields?
I haven't seen anything about it in the man page of stat
, including the ERRORS
section.
For man page of statx
,
If a filesystem does not support a field or if it has an unrepre‐
sentable value (for instance, a file with an exotic type), then the
mask bit corresponding to that field will be cleared in stx_mask even
if the user asked for it and a dummy value will be filled in for com‐
patibility purposes if one is available (e.g., a dummy UID and GID
may be specified to mount under some circumstances).
Is this the same behavior as stat
which never return a "partial result"?
BTW: I tested to statx
a file in mounted FAT32 filesystem which has no permission support. The result has all STATX_BASIC_STATS
set in stx_mode
, and the file mode, uid and gid are just what specified in mount
. And stat64
also returns the dummy result without any error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok, this sounds reasonable to me, thanks for the clarification!
@bors: r+ |
📌 Commit 21b4577 has been approved by |
Prefer statx on linux if available This PR make `metadata`-related functions try to invoke `statx` first on Linux if available, making `std::fs::Metadata::created` work on Linux with `statx` supported. It follows the discussion in rust-lang#61386 , and will fix rust-lang#59743 The implementation of this PR is simply converting `struct statx` into `struct stat64` with extra fields for `btime` if `statx` succeeds, since other fields are not currently used. --- I also did a separated benchmark for `fs::metadata`, `stat64`, `statx`, and `statx` with conversion to `stat64`. It shows that `statx` with conversion is even more faster than pure `statx`. I think it's due to `sizeof stat64 == 114` but `sizeof statx == 256`. Anyway, the bare implementation of `statx` with conversion is only about 0.2% slower than the original impl (`stat64`-family). With heap-allocation counted (~8.5% of total cost), the difference between `stat` and `statx` (with or without conversion) is just nothing. Therefore, I think it is not urgent to use bare `struct statx` as underlying representation now. There is no need to break `std::os::linux::fs::MetadataExt::as_raw_stat` (rust-lang#61386 (comment)) [Separated bare benchmarks](https://gist.github.com/oxalica/c4073ecb202c599fe41b7f15f86dc79c): ``` metadata_ok time: [529.41 ns 529.77 ns 530.19 ns] metadata_err time: [538.71 ns 539.39 ns 540.35 ns] stat64_ok time: [484.32 ns 484.53 ns 484.75 ns] stat64_err time: [481.77 ns 482.00 ns 482.24 ns] statx_ok time: [488.07 ns 488.35 ns 488.62 ns] statx_err time: [487.74 ns 488.00 ns 488.27 ns] statx_cvt_ok time: [485.05 ns 485.28 ns 485.53 ns] statx_cvt_err time: [485.23 ns 485.45 ns 485.67 ns] ``` r? @alexcrichton
⌛ Testing commit 21b4577 with merge c0ae792d7ff091accab2257c10fa4bd2b256ca25... |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
💔 Test failed - checks-azure |
Fixed type conversions from Also squashed and rebased to master. |
@bors: r+ |
📌 Commit 4c56b1c2f3b881789d84df26f4366994983f8212 has been approved by |
⌛ Testing commit 4c56b1c2f3b881789d84df26f4366994983f8212 with merge 550eb83a2f5ed85655cffa2a221b6198bdc480d8... |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
📌 Commit c3bbdc5 has been approved by |
Prefer statx on linux if available This PR make `metadata`-related functions try to invoke `statx` first on Linux if available, making `std::fs::Metadata::created` work on Linux with `statx` supported. It follows the discussion in rust-lang#61386 , and will fix rust-lang#59743 The implementation of this PR is simply converting `struct statx` into `struct stat64` with extra fields for `btime` if `statx` succeeds, since other fields are not currently used. --- I also did a separated benchmark for `fs::metadata`, `stat64`, `statx`, and `statx` with conversion to `stat64`. It shows that `statx` with conversion is even more faster than pure `statx`. I think it's due to `sizeof stat64 == 114` but `sizeof statx == 256`. Anyway, the bare implementation of `statx` with conversion is only about 0.2% slower than the original impl (`stat64`-family). With heap-allocation counted (~8.5% of total cost), the difference between `stat` and `statx` (with or without conversion) is just nothing. Therefore, I think it is not urgent to use bare `struct statx` as underlying representation now. There is no need to break `std::os::linux::fs::MetadataExt::as_raw_stat` (rust-lang#61386 (comment)) [Separated bare benchmarks](https://gist.github.com/oxalica/c4073ecb202c599fe41b7f15f86dc79c): ``` metadata_ok time: [529.41 ns 529.77 ns 530.19 ns] metadata_err time: [538.71 ns 539.39 ns 540.35 ns] stat64_ok time: [484.32 ns 484.53 ns 484.75 ns] stat64_err time: [481.77 ns 482.00 ns 482.24 ns] statx_ok time: [488.07 ns 488.35 ns 488.62 ns] statx_err time: [487.74 ns 488.00 ns 488.27 ns] statx_cvt_ok time: [485.05 ns 485.28 ns 485.53 ns] statx_cvt_err time: [485.23 ns 485.45 ns 485.67 ns] ``` r? @alexcrichton
Failed in #65530 (comment), @bors r- rollup=never |
@alexcrichton |
@bors: r+ |
📌 Commit 2ee45c9 has been approved by |
Prefer statx on linux if available This PR make `metadata`-related functions try to invoke `statx` first on Linux if available, making `std::fs::Metadata::created` work on Linux with `statx` supported. It follows the discussion in #61386 , and will fix #59743 The implementation of this PR is simply converting `struct statx` into `struct stat64` with extra fields for `btime` if `statx` succeeds, since other fields are not currently used. --- I also did a separated benchmark for `fs::metadata`, `stat64`, `statx`, and `statx` with conversion to `stat64`. It shows that `statx` with conversion is even more faster than pure `statx`. I think it's due to `sizeof stat64 == 114` but `sizeof statx == 256`. Anyway, the bare implementation of `statx` with conversion is only about 0.2% slower than the original impl (`stat64`-family). With heap-allocation counted (~8.5% of total cost), the difference between `stat` and `statx` (with or without conversion) is just nothing. Therefore, I think it is not urgent to use bare `struct statx` as underlying representation now. There is no need to break `std::os::linux::fs::MetadataExt::as_raw_stat` (#61386 (comment)) [Separated bare benchmarks](https://gist.github.com/oxalica/c4073ecb202c599fe41b7f15f86dc79c): ``` metadata_ok time: [529.41 ns 529.77 ns 530.19 ns] metadata_err time: [538.71 ns 539.39 ns 540.35 ns] stat64_ok time: [484.32 ns 484.53 ns 484.75 ns] stat64_err time: [481.77 ns 482.00 ns 482.24 ns] statx_ok time: [488.07 ns 488.35 ns 488.62 ns] statx_err time: [487.74 ns 488.00 ns 488.27 ns] statx_cvt_ok time: [485.05 ns 485.28 ns 485.53 ns] statx_cvt_err time: [485.23 ns 485.45 ns 485.67 ns] ``` r? @alexcrichton
☀️ Test successful - checks-azure |
I think this might be broken on some filesystems on Linux. Simple |
Actually, I think docker may have a cgroup or BPF filter or so installed that returns EPERM on non-whitelisted syscalls. This prevents the fallback from operating, because that only triggers on ENOSYS. Host kernel: 4.15.0-65-generic docker version
|
Try statx for all linux-gnu target. After rust-lang/libc#1577, which is contained in `libc` 0.2.66, provides `SYS_statx` for all Linux platform, so we can try to use `statx` for ~all Linux target~ all linux-gnu targets. Unfortunately, `struct statx` and `fn statx` is not a part of public interface of musl (currently), ~we still need to invoke it through `syscall`~ we does **not** support statx for musl or other libc impls currently. Previous PR: rust-lang#65094 cc @alexcrichton
This PR make
metadata
-related functions try to invokestatx
first on Linux if available,making
std::fs::Metadata::created
work on Linux withstatx
supported.It follows the discussion in #61386 , and will fix #59743
The implementation of this PR is simply converting
struct statx
intostruct stat64
withextra fields for
btime
ifstatx
succeeds, since other fields are not currently used.I also did a separated benchmark for
fs::metadata
,stat64
,statx
, andstatx
with conversion tostat64
.It shows that
statx
with conversion is even more faster than purestatx
.I think it's due to
sizeof stat64 == 114
butsizeof statx == 256
.Anyway, the bare implementation of
statx
with conversion is only about 0.2% slower than the original impl (stat64
-family).With heap-allocation counted (~8.5% of total cost), the difference between
stat
andstatx
(with or without conversion) is just nothing.Therefore, I think it is not urgent to use bare
struct statx
as underlying representation now.There is no need to break
std::os::linux::fs::MetadataExt::as_raw_stat
(#61386 (comment))Separated bare benchmarks:
r? @alexcrichton