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

kv/RocksDBStore: abort if rocksdb EIO, don't return incorrect result #15862

Merged
merged 1 commit into from Jun 26, 2017

Conversation

Projects
None yet
3 participants
@yuyuyu101
Member

yuyuyu101 commented Jun 23, 2017

If the underlying disk is missing, the current logic won't check the actual
reason why Get/Set failed, it will result to client get a empty key/value
pair which is not expected. The correct logic should be abort
right now. Otherwise, it will leads to undefined behavior.

Signed-off-by: Haomai Wang haomai@xsky.com

@yuyuyu101 yuyuyu101 requested review from liewegas and yehudasa Jun 23, 2017

@@ -716,6 +716,9 @@ int RocksDBStore::get(
std::string value;
std::string bound = combine_strings(prefix, *i);
auto status = db->Get(rocksdb::ReadOptions(), rocksdb::Slice(bound), &value);
if (s.IsIOError())

This comment has been minimized.

@tchaikov

tchaikov Jun 23, 2017

Contributor

should be after the if clause

else if (status.IsIOError()) {
 ...
}
@@ -739,6 +742,8 @@ int RocksDBStore::get(
s = db->Get(rocksdb::ReadOptions(), rocksdb::Slice(k), &value);
if (s.ok()) {
out->append(value);
} else if (s.IsIOError()) {

This comment has been minimized.

@tchaikov

tchaikov Jun 23, 2017

Contributor

shall we be more explicit here? and with more info before bailing out:

} else if (s.IsNotFound()) {
  r = -ENOENT;
} else {
  ceph_abort_msg(s.ToString());
}

This comment has been minimized.

@tchaikov

tchaikov Jun 23, 2017

Contributor

i mean, we can take this opportunity to be more explicit about handling -ENOENT.

@liewegas liewegas added the needs-qa label Jun 23, 2017

@liewegas

This comment has been minimized.

Member

liewegas commented Jun 23, 2017

does not compile

(*out)[*i].append(value);
} else if (s.IsIOError()) {

This comment has been minimized.

@tchaikov

tchaikov Jun 24, 2017

Contributor

s/s/status/

kv/RocksDBStore: abort if rocksdb EIO, don't return incorrect result
If the underlying disk is missing, the current logic won't check the actual
reason why Get/Set failed, it will result to client get a empty key/value
pair which is not expected. The correct logic should be abort
right now. Otherwise, it will leads to undefined behavior.

Signed-off-by: Haomai Wang <haomai@xsky.com>
@yuyuyu101

This comment has been minimized.

Member

yuyuyu101 commented Jun 24, 2017

done

@tchaikov

This comment has been minimized.

@tchaikov tchaikov merged commit 203ee0e into ceph:master Jun 26, 2017

4 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
default Build finished.
Details
make check make check succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment