- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 2.8k
          Implement uring in AsyncWrite for fs::File
          #7713
        
          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
base: master
Are you sure you want to change the base?
Conversation
| } | ||
|  | ||
| cfg_io_uring! { | ||
| struct BoxedOp<T>(Pin<Box<dyn Future<Output = T> + Send + Sync + 'static>>); | 
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.
Does the Future need to be Sync ?
I think it is not really necessary.
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 like it does need Sync, at least its required by a test https://github.com/RedKinda/tokio/blob/uring_file/tokio/tests/async_send_sync.rs#L651 - https://github.com/tokio-rs/tokio/actions/runs/18909744941/job/53976898682?pr=7713#step:6:248
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.
In principle you could avoid Sync using src/util/sync_wrapper.rs. But for this use-case, it does not matter.
        
          
                tokio/src/fs/file.rs
              
                Outdated
          
        
      | let handle = BoxedOp(Box::pin(async move { | ||
| let (r, buf, _fd) = op.await; | ||
| match r { | ||
| Ok(_n) => (Operation::Write(Ok(())), buf), | 
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.
I think there is a chance that the buffer is not fully drained here.
Write(Ok()) should be returned only if buf.is_empty().
I think you need to wrap the write in a loop and handle it with something like:
Ok(0) => break (Operation::Write(Err(io::ErrorKind::WriteZero.into())), buf),
Ok(_) if buf.is_empty() => break (Operation::Write(Ok(())), buf),
Ok(_) => continue, // more to write
Err(e) => break (Operation::Write(Err(e)), buf),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.
Good catch, thanks and fixed
        
          
                tokio/src/fs/file.rs
              
                Outdated
          
        
      | let handle = BoxedOp(Box::pin(async move { | ||
| let (r, buf, _fd) = op.await; | ||
| match r { | ||
| Ok(_n) => (Operation::Write(Ok(())), buf), | 
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.
Same as above - make sure the buf is empty before saying Ok().
Motivation
Currently IO operations for
fs::Fileis implemented usingspawn_blockingto perform IO, which is not great for an async runtime. This PR implements IO operations usingio_uring. Thefs::writefunction already usesio_uringfor async operations, and this PR extends the uring functionality intoimpl AsyncWrite for fs::File.For full discussion see here #7684 and subsequently on discord https://discord.com/channels/500028886025895936/810724255046172692/1427735341003051079
Solution
The
fs::Filestruct already implements an internal buffer when writing usingspawn_blocking, which is similar to whatio_uringrequires. Therefore inpoll_write, we can check ifio_uringis initialized and supported, and if not fall back to thespawn_blockingimplementation, without requiring any API changes.This PR only implements
AsyncWrite, and once this PR is accepted, I am happy to also open a PR forAsyncRead for Fileusing the same structure.