Skip to content

Commit

Permalink
Back out "Use fallocate for file size extension when supported" (face…
Browse files Browse the repository at this point in the history
…bookincubator#11480)

Summary:
Pull Request resolved: facebookincubator#11480

Original commit changeset: f690fb454d40

Original Phabricator Diff: D65316028

We observed crash

```
terminate called after throwing an instance of 'facebook::velox::VeloxRuntimeError'
what(): Exception: VeloxRuntimeError
Error Source: RUNTIME
Error Code: INVALID_STATE
Reason: (-1 vs. 0) fallocate failed in LocalWriteFile::truncate: No space left on device.
Retriable: False
Expression: ret == 0
Function: truncate
File: fbcode/velox/common/file/File.cpp
Line: 388
Stack trace:
```

Reviewed By: xiaoxmeng, zacw7

Differential Revision: D65647366

fbshipit-source-id: 34cbb6d83d1fc9040c263f76cea2bd44bf6fb222
  • Loading branch information
amitkdutta authored and facebook-github-bot committed Nov 8, 2024
1 parent c5232cd commit 31aa90c
Show file tree
Hide file tree
Showing 4 changed files with 12 additions and 30 deletions.
21 changes: 9 additions & 12 deletions velox/common/caching/SsdFile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -169,15 +169,11 @@ SsdFile::SsdFile(const Config& config)
writeFile_ = fs_->openFileForWrite(fileName_, fileOptions);
readFile_ = fs_->openFileForRead(fileName_);

// NOTE: checkpoint recovery will set 'numRegions_' and 'dataSize_'
// accordingly.
numRegions_ = 0;
dataSize_ = 0;

const auto maxFileSize = kRegionSize * maxRegions_;
if (writeFile_->size() != maxFileSize) {
// Initialize and pre-allocate (if possible) the data file with fixed space.
writeFile_->truncate(static_cast<int64_t>(maxFileSize));
const uint64_t size = writeFile_->size();
numRegions_ = std::min<int32_t>(size / kRegionSize, maxRegions_);
fileSize_ = numRegions_ * kRegionSize;
if ((size % kRegionSize > 0) || (size > numRegions_ * kRegionSize)) {
writeFile_->truncate(fileSize_);
}
// The existing regions in the file are writable.
writableRegions_.resize(numRegions_);
Expand Down Expand Up @@ -338,8 +334,10 @@ std::optional<std::pair<uint64_t, int32_t>> SsdFile::getSpace(
bool SsdFile::growOrEvictLocked() {
process::TraceContext trace("SsdFile::growOrEvictLocked");
if (numRegions_ < maxRegions_) {
const auto newSize = (numRegions_ + 1) * kRegionSize;
try {
dataSize_ = (numRegions_ + 1) * kRegionSize;
writeFile_->truncate(newSize);
fileSize_ = newSize;
writableRegions_.push_back(numRegions_);
regionSizes_[numRegions_] = 0;
erasedRegionSizes_[numRegions_] = 0;
Expand Down Expand Up @@ -450,7 +448,7 @@ void SsdFile::write(std::vector<CachePin>& pins) {
writeOffset += writeLength;
writeLength = 0;
}
VELOX_CHECK_GE(dataSize_, writeOffset);
VELOX_CHECK_GE(fileSize_, writeOffset);

{
std::lock_guard<std::shared_mutex> l(mutex_);
Expand Down Expand Up @@ -1009,7 +1007,6 @@ void SsdFile::readCheckpoint(std::ifstream& state) {
maxRegions_,
"Trying to start from checkpoint with a different capacity");
numRegions_ = readNumber<int32_t>(state);
dataSize_ = numRegions_ * kRegionSize;
std::vector<double> scores(maxRegions);
state.read(asChar(scores.data()), maxRegions_ * sizeof(double));
std::unordered_map<uint64_t, StringIdLease> idMap;
Expand Down
4 changes: 2 additions & 2 deletions velox/common/caching/SsdFile.h
Original file line number Diff line number Diff line change
Expand Up @@ -563,8 +563,8 @@ class SsdFile {
// File system.
std::shared_ptr<filesystems::FileSystem> fs_;

// The size of actual cached data in bytes. Must be multiple of kRegionSize.
uint64_t dataSize_{0};
// Size of the backing file in bytes. Must be multiple of kRegionSize.
uint64_t fileSize_{0};

// ReadFile for cache data file.
std::unique_ptr<ReadFile> readFile_;
Expand Down
2 changes: 1 addition & 1 deletion velox/common/caching/tests/SsdFileTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -647,7 +647,7 @@ TEST_F(SsdFileTest, recoverFromCheckpointWithChecksum) {
ASSERT_EQ(statsAfterRecover.entriesCached, stats.entriesCached);
} else {
ASSERT_EQ(statsAfterRecover.bytesCached, 0);
ASSERT_EQ(statsAfterRecover.regionsCached, 0);
ASSERT_EQ(statsAfterRecover.regionsCached, stats.regionsCached);
ASSERT_EQ(statsAfterRecover.entriesCached, 0);
}

Expand Down
15 changes: 0 additions & 15 deletions velox/common/file/File.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -377,21 +377,6 @@ void LocalWriteFile::write(
void LocalWriteFile::truncate(int64_t newSize) {
checkNotClosed(closed_);
VELOX_CHECK_GE(newSize, 0, "New size cannot be negative.");
#ifdef linux
if (newSize > size_) {
// Use fallocate to extend the file.
const auto ret = ::fallocate(fd_, 0, 0, newSize);
VELOX_CHECK_EQ(
ret,
0,
"fallocate failed in LocalWriteFile::truncate: {}.",
folly::errnoStr(errno));
size_ = newSize;
return;
}
#endif // linux

// Fallback to ftruncate.
const auto ret = ::ftruncate(fd_, newSize);
VELOX_CHECK_EQ(
ret,
Expand Down

0 comments on commit 31aa90c

Please sign in to comment.