Skip to content

Conversation

pfmooney
Copy link
Collaborator

@pfmooney pfmooney commented Oct 3, 2025

This addresses considerable lock contention and thundering herd issues present in the block IO abstractions. Devices with multiple queues (read: NVMe), serviced by multiple workers, should be in much better shape when it comes to efficiently farming out work.

This addresses considerable lock contention and thundering herd issues
present in the block IO abstractions.  Devices with multiple queues
(read: NVMe), serviced by multiple workers, should be in much better
shape when it comes to efficiently farming out work.
@pfmooney
Copy link
Collaborator Author

pfmooney commented Oct 3, 2025

This should not be merged before the R17 release branch is cut. We'll want soak time, consider the scope of work. I wanted to get it up and available for folks to review.

@pfmooney pfmooney requested a review from iximeow October 3, 2025 02:48
Copy link
Member

@iximeow iximeow left a comment

Choose a reason for hiding this comment

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

i've mostly not looked at propolis/src/block yet but figured this first round of thoughts would be useful. on the whole this seems pretty reasonable! but I see that transmute in attachment.rs which I look forward to in.. round 2

Copy link
Member

@iximeow iximeow left a comment

Choose a reason for hiding this comment

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

somewhat fewer comments on the back half (plus what we've been talking about re. NoneProcessing)

Copy link
Collaborator Author

@pfmooney pfmooney left a comment

Choose a reason for hiding this comment

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

Thanks for looking through so far. I think I've at least touched on all the points you've raised.

size: u32,
nvme: &PciNvme,
mem: &MemCtx,
) -> Result<Arc<SubQueue>, NvmeError> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sold on returning the QueueId at the moment, but we definitely need some stronger checks here. I've included some additional logic around the admin queues, and fixed the part where we were not doing proper association during state import.

Copy link
Contributor

@leftwo leftwo left a comment

Choose a reason for hiding this comment

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

I have no further comments, thanks.

Copy link
Member

@iximeow iximeow left a comment

Choose a reason for hiding this comment

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

just getting back to earlier conversations and resolving where appropriate. if only there was some kind of collaborative forge where the review interface was amenable to conversations...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants