Skip to content
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

Log fallback due to lack of resources as info rather than error. #590

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

EMachin3
Copy link

Attempting to resolve issue #499.

@EMachin3 EMachin3 requested review from a team as code owners December 28, 2024 19:43
@EMachin3
Copy link
Author

@microsoft-github-policy-service agree

@smalis-msft
Copy link
Contributor

Thanks for your contribution, and welcome!

@@ -819,7 +836,7 @@ impl<T: DeviceBacking> DriverWorkerTask<T> {
cpu: u32,
) -> anyhow::Result<IoIssuer> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll leave it up to the storage folks to decide if they want to take this, but I think our preferred approach would be to convert these functions to returning full thiserror enums for all the different cases and then just matching on that, instead of using downcast_ref.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed - many thanks for the contribution @EMancin3, and for the feedback here, @smalis-msft.

I am in favor of making a code change to reduce the severity of this message. I also agree with Steven's feedback. (Tho, while I'm confident in the storage space, I'm still learning Rust and I'm open to a conversation).

I would suggest we generalize this ... :

#[derive(Debug, Error)]
pub enum DeviceCapabilityError {
    #[error("no more io queues available, max {0}", usize)]
    NoMoreIoQueues,
}

Since anyhow::Result is a shortcut for Result<T, anyhow::Error> and anyhow::Error implements std::error::Error, we should be able to then do:

    async fn create_io_queue(
        &mut self,
        state: &mut WorkerState,
        cpu: u32,
    ) -> Result<IoIssuer, Error> {

...

        if self.io.len() >= state.max_io_queues as usize {
            return Err(DeviceCapabilityError::NoMoreIoQueues(state.max_io_queues))
        }

and then in the calling code match on Error type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with @mattkur.

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.

4 participants