Handle return code by io_uring_submit_and_wait() and io_uring_wait_cqe() #8311
Conversation
…e(). Summary: Right now return codes by io_uring_submit_and_wait() and io_uring_wait_cqe() are not handled. It is not the good practice. Although these two functions are not supposed to return non-0 values in normal exeuction, people suspect that they might return non-0 value when an interruption happens, and the code might cause hanging. Test Plan: Make sure at least normal test cases still pass.
siying
changed the title
|
@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@siying has updated the pull request. You must reimport the pull request before landing. |
|
@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@siying has updated the pull request. You must reimport the pull request before landing. |
|
@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@siying has updated the pull request. You must reimport the pull request before landing. |
|
@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@siying has updated the pull request. You must reimport the pull request before landing. |
|
@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
| } | ||
|
|
||
| TEST_F(EnvPosixTest, MultiReadIOUringError) { | ||
| // In this test we don't do aligned read, wo it doesn't work for |
There was a problem hiding this comment.
It is copied from line 1276...
There was a problem hiding this comment.
I just was not know what "wo" means. Write only or is it a typo?
There was a problem hiding this comment.
I don't know either...
| bool io_uring_wait_cqe_called = false; | ||
| ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->SetCallBack( |
There was a problem hiding this comment.
What happens on platforms that do not support io_uring?
There was a problem hiding this comment.
io_uring_wait_cqe_called will not be called and we won't check return value to be non-OK.
| Status s = file->MultiRead(reqs.data(), reqs.size()); | ||
| if (io_uring_wait_cqe_called) { | ||
| ASSERT_NOK(s); | ||
| } |
There was a problem hiding this comment.
This looks like it should fail the ASSERT_STATUS_CHECKED calls if the platform does not support io_uring.
There was a problem hiding this comment.
What does "fail the ASSERT_STATUS_CHECKED calls" mean?
There was a problem hiding this comment.
If you compile with ASSERT_STATUS_CHECKED=1, then a status code must be checked or it aborts with a stack trace. If the uring condition is false, then "s" is never checked.
There was a problem hiding this comment.
Ahh, good point. Since the tests are passing, I am going to do it as a follow up.
| Status s = file->MultiRead(reqs.data(), reqs.size()); | ||
| if (io_uring_submit_and_wait_called) { | ||
| ASSERT_NOK(s); | ||
| } |
There was a problem hiding this comment.
This looks like it should fail the ASSERT_STATUS_CHECKED calls if the platform does not support io_uring.
|
@siying has updated the pull request. You must reimport the pull request before landing. |
|
@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
This pull request has been merged in 60e5af8. |
Summary:
Right now return codes by io_uring_submit_and_wait() and io_uring_wait_cqe() are not handled. It is not the good practice. Although these two functions are not supposed to return non-0 values in normal exeuction, people suspect that they might return non-0 value when an interruption happens, and the code might cause hanging.
Test Plan: Make sure at least normal test cases still pass.