Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix submission queue overflow on large blob writes #520

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

PThorpe92
Copy link
Contributor

@PThorpe92 PThorpe92 commented Dec 20, 2024

#406

From everything I have read, just adjusting the sub queue entries size should be fine... Obviously spin-locking on an entry to be processed is not exactly ideal here but with the entries sized increased it seems like that isn't a likely situation. More than a few examples I found are creating a ring with >= 256.

All the other possible improvements I have read about don't really apply to this use-case (batch submissions being the primary one I have seen)

Probably should add tests with large blob inserts, right now the only tests that are doing inserts is in shelltests.py

EDIT: Currently exploring the possible realistic solutions discussed on discord.

@penberg penberg requested a review from pereman2 December 20, 2024 07:32
@@ -49,7 +50,7 @@ struct InnerLinuxIO {

impl LinuxIO {
pub fn new() -> Result<Self> {
let ring = io_uring::IoUring::new(MAX_IOVECS as u32)?;
let ring = io_uring::IoUring::new(SUBQ_ENTRYS)?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spelling nit

Suggested change
let ring = io_uring::IoUring::new(SUBQ_ENTRYS)?;
let ring = io_uring::IoUring::new(SUBQ_ENTRIES)?;

Comment on lines +89 to +93
match res {
Ok(_) => break,
Err(_) => {
let _ = self.wait_for_completion();
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of this, we are ignoring the error and we are waiting for completion for everything. This feels kinda wrong honestly because that would mean that if you try to write a big blob it will for the connection to wait instead of returning control to it as we would expect from limbo.

In my opinion submit_entry should return submission is full and therefore there should be a state machine in write path that can resume long writes on next iteration.

cc: @penberg

@PThorpe92 PThorpe92 marked this pull request as draft December 20, 2024 13:32
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants