Handle transient ENOMEM in statx probing

[vinDelphini]

Fixes a FIXME in sys/fs/unix.rs.

Currently, if the statx availability probe fails with ENOMEM, statx is permanently disabled for the process. Since ENOMEM is transient, this change allows falling back to stat for the current call without updating STATX_SAVED_STATE to Unavailable, permitting statx to be retried later.

[rustbot]

r? @ibraheemdev

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

[terrarier2111]

Are you sure there are no other transient coditions (other than ENOMEM) that we should worry about here?
As the comment says "like" which indicates to me as an uninformed reader, that there may be more transient conditions to consider other than ENOMEM.
If so, removing this comment is not correct IMHO.

[vinDelphini]

ya true, deleting the comment was premature...

One concern I have is that caching STATX_STATE::Unavailable feels risky if the failure is transient, since that would effectively disable statx for the lifetime of the process.

1. Should Unavailable be cached only for ENOSYS, or also cases like EPERM (seccomp)?
2. For other errors that aren’t clearly “missing” or transient, is it better to avoid caching and just fall back to legacy stat for that call?
3. Any other transient errors you’d want handled explicitly?

If this needs a bit more discussion, I could open a GitHub issue first, please suggest, thanks mate! @terrarier2111 @ibraheemdev

[tgross35]

It's fine to discuss here since this is where we would see the changes anyway.

One concern I have is that caching STATX_STATE::Unavailable feels risky if the failure is transient, since that would effectively disable statx for the lifetime of the process.

There is nothing "risky" about using the stat fallback.

• For other errors that aren’t clearly “missing” or transient, is it better to avoid caching and just fall back to legacy stat for that call?
• Any other transient errors you’d want handled explicitly?

It's your PR, could you propose a reasonable solution here? :)

[vinDelphini]

• ENOMEM : don't cache anything, just fall back to stat for this call and retry the probe next time
• All other errors: Cache as Unavailable since we can't be sure (seccomp could return anything)
• EAGAIN comes to mind but I think it's unlikely from statx.

The updated code is in the latest commit (64c3e3f)

[tgross35]

I don't think it's the worst thing to allow a retry on ENOMEM, but this code changes more than just the ENOMEM handling.

As an alternative we could keep a retry counter and fall back if it fails a number of times, but things are going rather poorly if you get a repeated kernel OOM from an empty statx. I don't really think it's worth it.

@tbu- it looks like you've touched this code most recently, do you have any thoughts?

View changes since this review

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[asder8215]

I think it's a good idea to not cache that statx as Unavailable or Present on ENOMEM error if it's thrown out on our first uses of try_statx (as it's neither evidence itself that the syscall is unavailable or present on the system).

For handling this, maybe for that situation an error should be returned directly? I'm not too sure about the counter idea since it's not absolute that statx is unavailable. What do you think @tgross35, @vinDelphini?

[vinDelphini]

I think it's a good idea to not cache that statx as Unavailable or Present on ENOMEM error if it's thrown out on our first uses of try_statx (as it's neither evidence itself that the syscall is unavailable or present on the system).

For handling this, maybe for that situation an error should be returned directly? I'm not too sure about the counter idea since it's not absolute that statx is unavailable. What do you think @tgross35, @vinDelphini?

current approach doesn't cache anything on ENOMEM - it just returns None so we fall back to stat for that call, then retry the probe next time.

falling back to stat seems safer since it at least gives the user some result - curious what @tgross35 and @tbu- think!

[vinDelphini]

@rustbot ready

[tbu-]

I think I only fixed up comments around the statx stuff.

If I were to refactor it, I'd probably make sure we only test for the presence of statx after trying it with the passed parameters once and getting an error for it. But I think that's out of scope for this PR.