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

be:spdk: process spdk events before reusing controller #114

Merged
merged 1 commit into from
Oct 3, 2022

Conversation

LiadOz
Copy link
Contributor

@LiadOz LiadOz commented Jun 12, 2022

Signed-off-by: Liad Oz liadozil@gmail.com

@LiadOz
Copy link
Contributor Author

LiadOz commented Jun 12, 2022

resolves #109

@safl safl added the pr-ready-for-test PR is ready for functional tests label Jun 13, 2022
@safl
Copy link
Member

safl commented Jun 13, 2022

Hi @LiadOz,
LGTM, CI is spinning up to check for any regressions.
Thanks,
Simon

@safl
Copy link
Member

safl commented Jun 13, 2022

Looks like the test xnvme_tests_enum open --count 4 fails with this change.
The test is here tests/enum.c. In the failing scenario it attempts to open the same PCIe address multiple times, this is the use-case the cref was made for, as we cannot have multiple driver instances, we have to share and re-use the one we have previously attached to. I am currently traveling, so can't dive into the code, however @birkelund and @Baekalfen did you see a similar thing / issues when references counting in #111 ? Something that might be applicable here?

@birkelund
Copy link
Collaborator

Looks like the test xnvme_tests_enum open --count 4 fails with this change. The test is here tests/enum.c. In the failing scenario it attempts to open the same PCIe address multiple times, this is the use-case the cref was made for, as we cannot have multiple driver instances, we have to share and re-use the one we have previously attached to. I am currently traveling, so can't dive into the code, however @birkelund and @Baekalfen did you see a similar thing / issues when references counting in #111 ? Something that might be applicable here?

Test is OK for the be-vfio backend.

@LiadOz
Copy link
Contributor Author

LiadOz commented Jun 21, 2022

Is there a way to see the logs for this test? I suspect the controller might be detached when using it similar to #107, before this change assuming the controller was detached it gets to spdk_nvme_ctrlr_get_ns which returns 0 and then creates a new controller.

lib/xnvme_be_spdk_dev.c Outdated Show resolved Hide resolved
Copy link
Contributor

@karlowich karlowich left a comment

Choose a reason for hiding this comment

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

LGTM!

@karlowich karlowich force-pushed the loz/AUTOINFRA-755 branch 2 times, most recently from c7a653f to 928f3f2 Compare September 23, 2022 07:39
@safl
Copy link
Member

safl commented Sep 29, 2022

@LiadOz this is looking great thank you. Can you redo this as a rebase to avoid the merge-commit?

@LiadOz LiadOz force-pushed the loz/AUTOINFRA-755 branch 2 times, most recently from 2aa8053 to 7e2fd5b Compare October 3, 2022 04:56
Signed-off-by: Liad Oz <liadozil@gmail.com>
Copy link
Member

@safl safl left a comment

Choose a reason for hiding this comment

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

Great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-ready-for-test PR is ready for functional tests
Development

Successfully merging this pull request may close these issues.

4 participants