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.

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

Use statx on Linux #61386

Closed
wants to merge 1 commit into from
Closed

Conversation

ariasuni
Copy link
Contributor

Fix #59743

Using statx on Linux notably gives us access to stx_btime so we can return a value for Metadata::created().

I implemented the feature but need time to improve the code:

  • Update the
  • Find how to fallback in an efficient way when glibc version < 2.28
    • stat64 and statx returns different structs, and it seems that right now it’s not possible to know the glibc version at compile-time. I can make FileAttr an enum which can be either one.
    • Should I use sys::os::glibc_version() or something like
      if !__cxa_thread_atexit_impl.is_null() {
  • I added st_{r,}dev_{major,minor}, but I’m not sure it’s the proper way to do it.
  • Being able to access all fields of statx as functions like the other information is probably something that you’d like to have.

@rust-highfive
Copy link
Collaborator

r? @KodrAus

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 31, 2019
@xen0n
Copy link
Contributor

xen0n commented Jun 5, 2019

If you get -ENOSYS when calling statx(2) first, that means the syscall is not implemented so you can fallback to stat(2). You probably don't want to detect for this every time stat is called, though.

@bors
Copy link
Contributor

bors commented Jun 7, 2019

☔ The latest upstream changes (presumably #61408) made this pull request unmergeable. Please resolve the merge conflicts.

@ariasuni
Copy link
Contributor Author

@xen0n Do you suggest using the syscall instead of the glibc wrapper?

You probably don't want to detect for this every time stat is called, though.

But I don’t see how to “save” the fact that statx is available or not. I would prefer being able to detect that at compile-time but it doesn’t seem possible as far as I know (which is not that much since I am new at contributing to Rust).

@oxalica
Copy link
Contributor

oxalica commented Jun 17, 2019

For syscall to statx, I have already made a wrapper statx-sys which may helps. So we can keep dependency to the old version of glibc.

@ariasuni I think this can be simply achieved by setting an atomic static flag for the first ENOSYS.

But I don’t see how to “save” the fact that statx is available or not. I would prefer being able to detect that at compile-time but it doesn’t seem possible as far as I know (which is not that much since I am new at contributing to Rust).

@Mark-Simulacrum Mark-Simulacrum added S-inactive and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 9, 2019
@cuviper
Copy link
Member

cuviper commented Aug 2, 2019

For tracking ENOSYS, look at how copy_file_range is handled. Note that it also calls manually through libc::syscall, avoiding the problem of libc not having the new symbol at all.

@ariasuni
Copy link
Contributor Author

ariasuni commented Sep 7, 2019

The problem I see is that if I use statx for the existing interface, I’ll need to convert stat64 struct (when statx is unavailable) to the statx struct or vice-versa.

Here are problematic extracts of the code I modified:

pub struct FileAttr {
    #[cfg(not(target_os = "linux"))]
    stat: stat64,
    #[cfg(target_os = "linux")]
    stat: statx,
}

[]

// I don’t think I can do anything here, because I can only return one type
#[cfg(target_os = "linux")]
impl AsInner<statx> for FileAttr {
    fn as_inner(&self) -> &statx { &self.stat }
}

impl MetadataExt for Metadata {
    #[allow(deprecated)]
    fn as_raw_stat(&self) -> &raw::stat {
        unsafe {
            // this doesn’t work, and I’m not sure I can make it work
            // because I need to convert the struct from `statx` to `raw::stat`
            // but I can’t return a ref to something that I’ll create in the fn…
            &*(self.as_inner().as_inner() as *const libc::statx
                                          as *const raw::stat)
        }
    }
    []
}

I don’t want to spend time on this if I can’t resolve the issues around compatibility.

@ariasuni
Copy link
Contributor Author

ariasuni commented Oct 3, 2019

My current understanding is that since we’re exposing the underlying type in the API and returning a reference to it, we just can’t change the implementation from stat64 to statx.

It’s maybe something we can fix with epoch but as such, it seems to be a lost cause (or it’s too complex for me to do).

@alexcrichton
Copy link
Member

@ariasuni FWIW my personal take on how we could land this today would be:

  • On Unix Metadata is an enum of stat and statx. We dynamically detect at runtime which is available (favoring statx via syscall if the kernel supports it)
  • The as_raw_stat function just panics if the internal variant is statx
  • All accessors will match on whether we got stat or statx to return the right file.

While it's "bad" to panic I don't think anyone's calling as_raw_stat anyway so I'd hate for that to block this feature.

@oxalica
Copy link
Contributor

oxalica commented Oct 4, 2019

@alexcrichton

The as_raw_stat function just panics if the internal variant is statx

It's deprecated, but it's still a breaking change :(

One of my idea is to convert representation from statx to stat with extra fields (like btime) when the evil as_raw_stat is called. This seems acceptable, but make all of other access functions to perform an extra atomic load operation, since we have Metadata: Sync.

Note that struct stat is 144 bytes while struct statx is 256 bytes. It is zero-cost to append extra fields into stat representation. But having both of them will make the struct too big.

There is also a quite simple way that, just use struct FileAttr { stat: stat64, btime: Option<..>, }, and convert statx to stat64 to fill in the fields if the syscall to statx succeeds.
I think the main cost here is the syscall itself, so we can just copy (rearrange) 144+ bytes for every call.

Centril added a commit to Centril/rust that referenced this pull request Oct 9, 2019
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
Centril added a commit to Centril/rust that referenced this pull request Oct 15, 2019
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
Centril added a commit to Centril/rust that referenced this pull request Oct 16, 2019
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
Centril added a commit to Centril/rust that referenced this pull request Oct 16, 2019
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
Centril added a commit to Centril/rust that referenced this pull request Oct 17, 2019
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
bors added a commit that referenced this pull request Oct 19, 2019
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
@crlf0710
Copy link
Member

@rustbot modify labels to -S-inactive +S-inactive-closed

@rustbot rustbot added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-inactive labels Mar 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metadata::created() not working on Linux with statx