book/rocksdb
1
0
Fork 0
mirror of https://github.com/facebook/rocksdb.git synced 2025-04-20 03:20:00 +08:00
Code Issues Packages Projects Releases Wiki Activity

Update test implementations for MultiRead with fs_scratch reuse (#13195)

Browse Source 
Summary:
This is a follow up to https://github.com/facebook/rocksdb/issues/13189. As mentioned in the description in the previous PR, to guard against similar bugs in the future, we should update our test implementations to reflect the real-world assumptions that we can make about `fs_scratch` when we issue reads with the filesystem buffer reuse optimization. The current test implementations reinforce the misconception that `fs_scratch` points to the same place as `result.data()` (i.e. to the start of the valid data buffer for the read result). `fs_scratch` can point to any arbitrary data structure, but for our purposes, I think we achieve what we want if we just have it point to a `Slice` which wraps the underlying result buffer inside one of its class variables.

Pull Request resolved: https://github.com/facebook/rocksdb/pull/13195

Test Plan: Existing unit tests test the same functionality but in an improved way with this change.

Reviewed By: hx235

Differential Revision: D66896380

Pulled By: archang19

fbshipit-source-id: 377e67ec70427716f2b7b7388d99b78003c01eb0
This commit is contained in:
Andrew Chang 2024-12-17 16:36:04 -08:00 committed by Facebook GitHub Bot
parent 6ae3412244
commit d957e1a33a
3 changed files with 43 additions and 15 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

18
cache/tiered_secondary_cache_test.cc vendored
View File

@ -951,12 +951,20 @@ TEST_F(DBTieredSecondaryCacheTest, FSBufferTest) {
IODebugContext* dbg) override {
for (size_t i = 0; i < num_reqs; ++i) {
FSReadRequest& req = reqs[i];
FSAllocationPtr buffer(new char[req.len], [](void* ptr) {
delete[] static_cast<char*>(ptr);
});
req.fs_scratch = std::move(buffer);
// See https://github.com/facebook/rocksdb/pull/13195 for why we
// want to set up our test implementation for FSAllocationPtr this
// way.
char* internalData = new char[req.len];
req.status = Read(req.offset, req.len, options, &req.result,
static_cast<char*>(req.fs_scratch.get()), dbg);
internalData, dbg);
Slice* internalSlice = new Slice(internalData, req.len);
FSAllocationPtr internalPtr(internalSlice, [](void* ptr) {
delete[] static_cast<const char*>(
static_cast<Slice*>(ptr)->data_);
delete static_cast<Slice*>(ptr);
});
req.fs_scratch = std::move(internalPtr);
}
return IOStatus::OK();
}

18
db/db_io_failure_test.cc
View File

@ -97,12 +97,20 @@ class CorruptionFS : public FileSystemWrapper {
for (size_t i = 0; i < num_reqs; ++i) {
FSReadRequest& req = reqs[i];
if (fs_.fs_buffer_) {
FSAllocationPtr buffer(new char[req.len], [](void* ptr) {
delete[] static_cast<char*>(ptr);
});
req.fs_scratch = std::move(buffer);
// See https://github.com/facebook/rocksdb/pull/13195 for why we
// want to set up our test implementation for FSAllocationPtr this
// way.
char* internalData = new char[req.len];
req.status = Read(req.offset, req.len, options, &req.result,
static_cast<char*>(req.fs_scratch.get()), dbg);
internalData, dbg);
Slice* internalSlice = new Slice(internalData, req.len);
FSAllocationPtr internalPtr(internalSlice, [](void* ptr) {
delete[] static_cast<const char*>(
static_cast<Slice*>(ptr)->data_);
delete static_cast<Slice*>(ptr);
});
req.fs_scratch = std::move(internalPtr);
} else {
req.status = Read(req.offset, req.len, options, &req.result,
req.scratch, dbg);

22
file/prefetch_test.cc
View File

@ -3316,12 +3316,24 @@ class FSBufferPrefetchTest : public testing::Test,
IODebugContext* dbg) override {
for (size_t i = 0; i < num_reqs; ++i) {
FSReadRequest& req = reqs[i];
FSAllocationPtr buffer(new char[req.len], [](void* ptr) {
delete[] static_cast<char*>(ptr);
});
req.fs_scratch = std::move(buffer);
// We cannot assume that fs_scratch points to the start of
// the read data. We can have the FSAllocationPtr point to a
// wrapper around the result buffer in our test implementation so
// that we can catch whenever we incorrectly make this assumption.
// See https://github.com/facebook/rocksdb/pull/13189 for more
// context.
char* internalData = new char[req.len];
req.status = Read(req.offset, req.len, options, &req.result,
static_cast<char*>(req.fs_scratch.get()), dbg);
internalData, dbg);
Slice* internalSlice = new Slice(internalData, req.len);
FSAllocationPtr internalPtr(internalSlice, [](void* ptr) {
delete[] static_cast<const char*>(
static_cast<Slice*>(ptr)->data_);
delete static_cast<Slice*>(ptr);
});
req.fs_scratch = std::move(internalPtr);
}
return IOStatus::OK();
}