Skip to content
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.

Sign up for GitHub

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

Jump to bottom

statx probe: ENOSYS might come from a faulty FUSE driver #123928

Merged
merged 1 commit into from
Apr 15, 2024
Merged

statx probe: ENOSYS might come from a faulty FUSE driver #123928

merged 1 commit into from
Apr 15, 2024

Conversation

tbu-
Copy link
Contributor

@tbu- tbu- commented Apr 14, 2024

Do the availability check regardless of the error returned from statx.

CC #122079 (comment)

@tbu-
statx probe: ENOSYS might come from a faulty FUSE driver
2325b81 
Do the availability check regardless of the error returned from `statx`.

CC rust-lang#122079 (comment)
@rustbot
Copy link
Collaborator

rustbot commented Apr 14, 2024

r? @workingjubilee

rustbot has assigned @workingjubilee.
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

@rustbot rustbot added O-unix Operating system: Unix-like S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 14, 2024
@tbu- tbu- mentioned this pull request Apr 14, 2024
Merged
workingjubilee
workingjubilee reviewed Apr 14, 2024
View reviewed changes
Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine, but I would prefer the comments be changed. Also, it genuinely seems like it might affect compiler runtime, given how many files the compiler touches, so I'm going to perf it.

library/std/src/sys/pal/unix/fs.rs
@@ -193,20 +193,17 @@ cfg_has_statx! {{
return Some(Err(err));
}

// Availability not checked yet.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This initial line is still relevant. Please restore it.

library/std/src/sys/pal/unix/fs.rs
@@ -193,20 +193,17 @@ cfg_has_statx! {{
return Some(Err(err));
}

// Availability not checked yet.
// `ENOSYS` might come from a faulty FUSE driver.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment only makes sense in light of the immediately previous version. Please explain it as if the reader has encountered the first time. So: Why are you starting off by mentioning ENOSYS here? Instead say something like "Syscalls can return errors from things other than the kernel per se, like a filesystem driver, so we can't assume this is relevant for all files."

@workingjubilee
Copy link
Member

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 14, 2024
@the8472
Copy link
Member

the8472 commented Apr 14, 2024

given how many files the compiler touches,

STATX_SAVED_STATE is a static, so the probe is only done once per process lifetime.

@bors
Copy link
Contributor

bors commented Apr 14, 2024

⌛ Trying commit 2325b81 with merge 48ee069...

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 14, 2024
@bors
Auto merge of rust-lang#123928 - tbu-:pr_statx_enosys, r=<try>
48ee069 
`statx` probe: `ENOSYS` might come from a faulty FUSE driver

Do the availability check regardless of the error returned from `statx`.

CC rust-lang#122079 (comment)
@bors
Copy link
Contributor

bors commented Apr 14, 2024

☀️ Try build successful - checks-actions
Build commit: 48ee069 (48ee0695fc72b431024db9b714a81c3c9b3f1588)

@rust-timer

This comment has been minimized.

Sign in to view
@workingjubilee
Copy link
Member

Then it should be fine, and I'm just being paranoid, I would think.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (48ee069): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

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)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 0.6% [0.6%, 0.6%] 1

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
Regressions ❌
(secondary)		 1.8% [1.8%, 1.8%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 677.423s -> 676.569s (-0.13%)
Artifact size: 316.04 MiB -> 316.02 MiB (-0.01%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 15, 2024
@workingjubilee
Copy link
Member

cool. I was in fact just being paranoid.

@bors rollup=always r+

@bors
Copy link
Contributor

bors commented Apr 15, 2024

📌 Commit 2325b81 has been approved by workingjubilee

It is now in the queue for this repository.

@bors 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 Apr 15, 2024
@bors
Copy link
Contributor

bors commented Apr 15, 2024

⌛ Testing commit 2325b81 with merge 9db7a74...

@workingjubilee
Copy link
Member

...I sure did space out on my own review notes huh. Well whatever.

@bors
Copy link
Contributor

bors commented Apr 15, 2024

☀️ Test successful - checks-actions
Approved by: workingjubilee
Pushing 9db7a74 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 15, 2024
@bors bors merged commit 9db7a74 into rust-lang:master Apr 15, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Apr 15, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (9db7a74): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

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
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 -0.9% [-0.9%, -0.9%] 1
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -0.9% [-0.9%, -0.9%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 677.574s -> 677.058s (-0.08%)
Artifact size: 315.94 MiB -> 315.97 MiB (0.01%)

sunfishcode added a commit to bytecodealliance/rustix that referenced this pull request Apr 21, 2024
@sunfishcode
Probe for statx availability even when statx returns ENOSYS.
1ca954f 
Following rust-lang/rust#123928, check whether the system supports
`statx` even when the initial call returns `NOSYS`, because that can
come from a faulty FUSE driver.
@sunfishcode sunfishcode mentioned this pull request Apr 21, 2024
Merged
sunfishcode added a commit to bytecodealliance/rustix that referenced this pull request Apr 21, 2024
@sunfishcode
Probe for statx availability even when statx returns ENOSYS. (#…
95a8302 
…1048)

Following rust-lang/rust#123928, check whether the system supports
`statx` even when the initial call returns `NOSYS`, because that can
come from a faulty FUSE driver.
tbu- added a commit to tbu-/rust that referenced this pull request Apr 27, 2024
@tbu-
Elaborate in comment about statx probe
adebad1 
As requested by @workingjubilee in
rust-lang#123928 (comment).
@tbu- tbu- mentioned this pull request Apr 27, 2024
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 27, 2024
@matthiaskrgr
Rollup merge of rust-lang#124443 - tbu-:pr_statx_enosys_comment, r=wo…
4501931 
…rkingjubilee

Elaborate in comment about `statx` probe

As requested by `@workingjubilee` in rust-lang#123928 (comment).
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 27, 2024
@rust-timer
Unrolled build for rust-lang#124443
0eb290d 
Rollup merge of rust-lang#124443 - tbu-:pr_statx_enosys_comment, r=workingjubilee

Elaborate in comment about `statx` probe

As requested by `@workingjubilee` in rust-lang#123928 (comment).
mattstam pushed a commit to succinctlabs/rust that referenced this pull request Aug 1, 2024
@tbu- @mattstam
Elaborate in comment about statx probe
4bc70ed 
As requested by @workingjubilee in
rust-lang/rust#123928 (comment).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@workingjubilee workingjubilee workingjubilee left review comments

Assignees

@workingjubilee workingjubilee

Labels
merged-by-bors This PR was explicitly merged by bors. O-unix Operating system: Unix-like S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.79.0
Development

Successfully merging this pull request may close these issues.
6 participants
@tbu- @rustbot @workingjubilee @rust-timer @the8472 @bors