From 4e85b74790de48895e682869414e43d4a12f409c Mon Sep 17 00:00:00 2001 From: Mike Kolupaev Date: Wed, 23 Mar 2016 09:14:56 -0700 Subject: [PATCH] Make WritableFileWrapper not screw up preallocation Summary: Without this diff, this is what happens to compaction output file if it's a subclass of WritableFileWrapper: - during compaction, all `PrepareWrite()` calls update `last_preallocated_block_` of the `WritableFileWrapper` itself, not of `target_`, since `PrepareWrite()` is not virtual, - `PrepareWrite()` calls `Allocate()`, which is virtual; it does `fallocate()` on `target_`, - after writing data, `target_->Close()` calls `GetPreallocationStatus()` of `target_`; it returns `last_preallocated_block_` of `target_`, which is zero because it was never touched before, - `target_->Close()` doesn't call `ftruncate()`; file remains big. This diff fixes it in a straightforward way, by making the methods virtual. `WritableFileWrapper` ends up having the useless fields `last_preallocated_block_` and `preallocation_block_size_`. I think ideally the preallocation logic should be outside `WritableFile`, the same way as `log_writer.h` and `file_reader_writer.h` moved some non-platform-specific logic out of Env, but that's probably not worth the effort now. Test Plan: `make -j check`; I'm going to deploy it on our test tier and see if it fixes space reclamation problem there Reviewers: yhchiang, sdong Reviewed By: sdong Subscribers: andrewkr, dhruba, sdong Differential Revision: https://reviews.facebook.net/D54681 --- include/rocksdb/env.h | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/include/rocksdb/env.h b/include/rocksdb/env.h index 7bdb6ee61..28bffe735 100644 --- a/include/rocksdb/env.h +++ b/include/rocksdb/env.h @@ -561,7 +561,7 @@ class WritableFile { * underlying storage of a file (generally via fallocate) if the Env * instance supports it. */ - void SetPreallocationBlockSize(size_t size) { + virtual void SetPreallocationBlockSize(size_t size) { preallocation_block_size_ = size; } @@ -597,7 +597,7 @@ class WritableFile { // of space on devices where it can result in less file // fragmentation and/or less waste from over-zealous filesystem // pre-allocation. - void PrepareWrite(size_t offset, size_t len) { + virtual void PrepareWrite(size_t offset, size_t len) { if (preallocation_block_size_ == 0) { return; } @@ -950,6 +950,13 @@ class WritableFileWrapper : public WritableFile { return target_->InvalidateCache(offset, length); } + virtual void SetPreallocationBlockSize(size_t size) override { + target_->SetPreallocationBlockSize(size); + } + virtual void PrepareWrite(size_t offset, size_t len) override { + target_->PrepareWrite(offset, len); + } + protected: Status Allocate(uint64_t offset, uint64_t len) override { return target_->Allocate(offset, len);