-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
fs: support io_uring in fs::write
#7567
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
Conversation
I made some changes in da5b9e1:
|
tokio/src/fs/write.rs
Outdated
while offset < total { | ||
// There is a cap on how many bytes we can write in a single uring write operation. | ||
// ref: https://github.com/axboe/liburing/discussions/497 | ||
let len = std::cmp::min(total - offset, u32::MAX as usize) as u32; |
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.
Could also make sense to move this logic inside Op::write_at
and have it accept usize. After all, it's indistinguishable to the caller from any other situation that writes the bytes partially. That way it's more natural for the caller as the caller expects usize for both input and output length.
let len = std::cmp::min(total - offset, u32::MAX as usize) as u32; | |
let len = u32::try_from(total - offset).unwrap_or(u32::MAX); |
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.
Did you mean having a dedicated variable for file_offset
here? I did so in c2ed659.
tokio/src/fs/write.rs
Outdated
let total: usize = buf.len(); | ||
let mut offset: usize = 0; | ||
while offset < total { |
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.
Since write_at
accepts u64 for file positions, let's use that here too.
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.
Moved that logic into write_at
function in c2ed659.
…ox692/add_uring_write
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.
Looks good to me!
let mut buf_offset: usize = 0; | ||
let mut file_offset: u64 = 0; | ||
while buf_offset < total { |
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.
These variables are always equal.
// There is a cap on how many bytes we can write in a single uring write operation. | ||
// ref: https://github.com/axboe/liburing/discussions/497 | ||
let len = u32::try_from(buf.as_ref().len() - buf_offset).unwrap_or(u32::MAX); | ||
|
||
let ptr = buf.as_ref()[buf_offset..buf_offset + len as usize].as_ptr(); |
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 is a super-nit, but I like this:
// There is a cap on how many bytes we can write in a single uring write operation. | |
// ref: https://github.com/axboe/liburing/discussions/497 | |
let len = u32::try_from(buf.as_ref().len() - buf_offset).unwrap_or(u32::MAX); | |
let ptr = buf.as_ref()[buf_offset..buf_offset + len as usize].as_ptr(); | |
let buf = owned_buf.as_ref(); | |
// There is a cap on how many bytes we can write in a single uring write operation. | |
// ref: https://github.com/axboe/liburing/discussions/497 | |
let len = u32::try_from(buf.len() - buf_offset).unwrap_or(u32::MAX); | |
let ptr = buf[buf_offset..buf_offset + len as usize].as_ptr(); |
use std::task::Waker; | ||
use std::{io, mem}; | ||
|
||
// This field isn't accessed directly, but it holds cancellation data, |
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 field isn't accessed directly, but it holds cancellation data, | |
// These fields aren't accessed directly, but they hold cancellation data, |
Add io-uring version of
fs::write
. I think the change itself is mostly straightforward. This PR might need to coordinate with #7547 regarding the usage of cfg.Related issue: #7266