fs: Implement io-uring::Op<Statx> and use it to implement fs::try_exists and complete the todo's in read_uring#7791
fs: Implement io-uring::Op<Statx> and use it to implement fs::try_exists and complete the todo's in read_uring#7791vrtgs wants to merge 3 commits intotokio-rs:masterfrom
Conversation
vrtgs
force-pushed
the
io-uring-metadata
branch
10 times, most recently
from
a030621 to
d5876fc
Compare
vrtgs
force-pushed
the
io-uring-metadata
branch
from
d5876fc to
85be9f9
Compare
vrtgs
changed the title
vrtgs
force-pushed
the
io-uring-metadata
branch
2 times, most recently
from
b9b395f to
9f98fdf
Compare
vrtgs
force-pushed
the
io-uring-metadata
branch
from
9f98fdf to
972c7c1
Compare
| let size_hint: Option<usize> = size_hint | ||
| .ok() | ||
| .map(|m| usize::try_from(m.len()).unwrap_or(usize::MAX)); | ||
|
|
There was a problem hiding this comment.
Here if we obtain size_hint Err for whatever reason, we should start with a length 0 capacity and gradually increase it
| ) | ||
| .flags(flags as i32) | ||
| .mask(linux_raw_sys::general::STATX_BASIC_STATS) | ||
| .build(); |
There was a problem hiding this comment.
is there a guarantee that after this line the buffer will be initialized? Should we add a check after it completes? What if the kernel errored and the buffer still remains unint?
| fn complete(self, cqe: CqeResult) -> Self::Output { | ||
| let ret = cqe | ||
| .result | ||
| .map(|_| Metadata(*unsafe { box_assume_init(self.buffer) })); |
There was a problem hiding this comment.
I think we should add a check here to check if the buffer is init or not or a safety comment explaining why this is ok
There was a problem hiding this comment.
Alright I will check the return to be exactly 0, if it is 0 then yes there is a guarantee
| let mut buffer = box_new_uninit::<statx>(); | ||
|
|
||
| let flags: u32 = linux_raw_sys::general::AT_STATX_SYNC_AS_STAT | ||
| | (linux_raw_sys::general::AT_SYMLINK_NOFOLLOW * u32::from(!follow_symlinks)); |
There was a problem hiding this comment.
You can create an member function that takes in a flags: u32, path: &Path then you can have fn statx_follow_symlinks and fn statx_no_symlinks than have a vague bool argument which might confuse future readers and users
Yeah, I did realize that as well which is why I didn't rewrite tokio::fs::metadata |
|
I am very sorry for the weird behavior btw, the github app sucks I accidentally closed the pr, when I respond to any comment 2 comment show up, sorry for the inconvenience |
martin-g
changed the title
| // https://man7.org/linux/man-pages/man2/statx.2.html | ||
| let statx_op = opcode::Statx::new( | ||
| types::Fd(fd.as_raw_fd()), | ||
| c"".as_ptr(), |
There was a problem hiding this comment.
Hm. How does this build with 1.71 (current MSRV) ?!
https://github.com/vrtgs/tokio/blob/972c7c1c10efd2d7e5a7a7b2a273343beab450a2/tokio/Cargo.toml#L11
https://doc.rust-lang.org/edition-guide/rust-2021/c-string-literals.html says that c-string literals are available since Rust 1.77
| } | ||
|
|
||
| cfg_io_uring! { | ||
| async fn try_exists_uring(path: &Path) -> io::Result<bool> { |
There was a problem hiding this comment.
nit: IMO this function can be inlined at line 39 above
| # Requires `--cfg tokio_unstable` to enable. | ||
| [target.'cfg(all(tokio_unstable, target_os = "linux"))'.dependencies] | ||
| io-uring = { version = "0.7.6", default-features = false, optional = true } | ||
| linux-raw-sys = { version = "0.12.1", optional = true } |
There was a problem hiding this comment.
Is there a particular reason to not use libc instead ?
AFAIS it provides all the constants used in this PR from linux-raw-sys
There was a problem hiding this comment.
Please do not use linux-raw-sys.
|
BTW there was an earlier attempt for Statx support at #7616. |
| const MAX_READ_SIZE: usize = 64 * 1024 * 1024; | ||
|
|
||
| pub(crate) async fn read_uring(path: &Path) -> io::Result<Vec<u8>> { | ||
| let file = OpenOptions::new().read(true).open(path).await?; |
There was a problem hiding this comment.
I'd like to keep the OpenOptions, use it how you would without breaking/adding new changes and opening files with uring should work.
mattiapitossi
added
the
S-waiting-on-author
6 participants
Motivation
get us one step closer to real io-uring