-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
RandomAccessFileReader::MultiRead() should not return read bytes not read #8941
Conversation
…read Summary: Right now, if underlying read returns fewer bytes than asked for, RandomAccessFileReader::MultiRead() still returns those in the buffer to upper layer. This can be a surprise to upper layer. This is unlikely to cause incorrect data. To cause incorrect data, checksum checking in upper layer should pass with short reads, whose chance is low. Test Plan: Run stress tests for a while
@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -296,8 +296,14 @@ IOStatus RandomAccessFileReader::MultiRead(const IOOptions& opts, | |||
r.status = fs_r.status; | |||
if (r.status.ok()) { | |||
uint64_t offset = r.offset - fs_r.offset; | |||
size_t len = std::min(r.len, static_cast<size_t>(fs_r.len - offset)); | |||
r.result = Slice(fs_r.scratch + offset, len); | |||
if (fs_r.result.size() <= offset) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
This pull request has been merged in fcce1f2. |
…read (#8941) Summary: Right now, if underlying read returns fewer bytes than asked for, RandomAccessFileReader::MultiRead() still returns those in the buffer to upper layer. This can be a surprise to upper layer. This is unlikely to cause incorrect data. To cause incorrect data, checksum checking in upper layer should pass with short reads, whose chance is low. Pull Request resolved: #8941 Test Plan: Run stress tests for a while Reviewed By: anand1976 Differential Revision: D31085780 fbshipit-source-id: 999adf2d6c2712f1323d14bb68b678df59969973
…read (#8941) Summary: Right now, if underlying read returns fewer bytes than asked for, RandomAccessFileReader::MultiRead() still returns those in the buffer to upper layer. This can be a surprise to upper layer. This is unlikely to cause incorrect data. To cause incorrect data, checksum checking in upper layer should pass with short reads, whose chance is low. Pull Request resolved: facebook/rocksdb#8941 Test Plan: Run stress tests for a while Reviewed By: anand1976 Differential Revision: D31085780 fbshipit-source-id: 999adf2d6c2712f1323d14bb68b678df59969973
Summary: Right now, if underlying read returns fewer bytes than asked for, RandomAccessFileReader::MultiRead() still returns those in the buffer to upper layer. This can be a surprise to upper layer.
This is unlikely to cause incorrect data. To cause incorrect data, checksum checking in upper layer should pass with short reads, whose chance is low.
Test Plan: Run stress tests for a while