fix renaming encrypted directory and check compaction paused during c…

…heckpoint (tikv#338) * fix renaming encrypted directory Signed-off-by: tabokie <xy.tao@outlook.com> * fix build Signed-off-by: tabokie <xy.tao@outlook.com> * patch test manager Signed-off-by: tabokie <xy.tao@outlook.com> * fix build Signed-off-by: tabokie <xy.tao@outlook.com> * check compaction paused during checkpoint Signed-off-by: tabokie <xy.tao@outlook.com> * add comment Signed-off-by: tabokie <xy.tao@outlook.com> --------- Signed-off-by: tabokie <xy.tao@outlook.com>
tabokie · Jul 7, 2023 · 14f36f8 · 14f36f8
1 parent 8a9c10e
commit 14f36f8
Show file tree

Hide file tree

Showing 4 changed files with 30 additions and 7 deletions.
diff --git a/encryption/encryption.cc b/encryption/encryption.cc
@@ -527,23 +527,22 @@ Status KeyManagedEncryptedEnv::RenameFile(const std::string& src_fname,
  }
  s = target()->RenameFile(src_fname, dst_fname);
  if (s.ok()) {
- s = key_manager_->DeleteFile(src_fname);
+ s = key_manager_->DeleteFileExt(src_fname, dst_fname);
  } else {
  Status delete_status __attribute__((__unused__)) =
- key_manager_->DeleteFile(dst_fname);
+ key_manager_->DeleteFileExt(dst_fname, src_fname);
  assert(delete_status.ok());
  }
  return s;
 }
 
 Status KeyManagedEncryptedEnv::DeleteDir(const std::string& dname) {
- Status s = target()->DeleteDir(dname);
+ // We don't guarantee atomicity. Delete keys first.
+ Status s = key_manager_->DeleteFile(dname);
  if (!s.ok()) {
  return s;
  }
- // We don't use a dedicated `DeleteDir` function, because RocksDB already uses
- // `RenameFile` for both file and directory.
- return key_manager_->DeleteFile(dname);
+ return target()->DeleteDir(dname);
 }
 
 Env* NewKeyManagedEncryptedEnv(Env* base_env,

diff --git a/include/rocksdb/encryption.h b/include/rocksdb/encryption.h
@@ -55,9 +55,17 @@ class KeyManager {
  FileEncryptionInfo* file_info) = 0;
  virtual Status NewFile(const std::string& fname,
  FileEncryptionInfo* file_info) = 0;
+ // Used with both file and directory.
  virtual Status DeleteFile(const std::string& fname) = 0;
  virtual Status LinkFile(const std::string& src_fname,
  const std::string& dst_fname) = 0;
+ // Provide additional hint of physical file when the key name doesn't map to
+ // one. A typical use case of this is atomically deleting a directory by
+ // renaming it first.
+ virtual Status DeleteFileExt(const std::string& fname,
+ const std::string& /*physical_fname*/) {
+ return DeleteFile(fname);
+ }
 };
 
 // An Env with underlying files being encrypted. It holds a reference to an

diff --git a/test_util/testutil.h b/test_util/testutil.h
@@ -86,6 +86,18 @@ class TestKeyManager : public encryption::KeyManager {
  Status DeleteFile(const std::string& fname) override {
  std::lock_guard<std::mutex> l(mutex);
  file_set.erase(fname);
+ if (!fname.empty()) {
+ std::string copy = fname;
+ if (copy.back() != '/') {
+ copy.push_back('/');
+ }
+ auto begin = file_set.lower_bound(copy);
+ auto end = begin;
+ while (end != file_set.end() && end->compare(0, copy.size(), copy) == 0) {
+ end++;
+ }
+ file_set.erase(begin, end);
+ }
  return Status::OK();
  }
 

diff --git a/utilities/checkpoint/checkpoint_impl.cc b/utilities/checkpoint/checkpoint_impl.cc
@@ -310,7 +310,11 @@ Status CheckpointImpl::ExportColumnFamily(
  s = db_->GetEnv()->CreateDir(tmp_export_dir);
 
  if (s.ok()) {
- s = db_->Flush(ROCKSDB_NAMESPACE::FlushOptions(), handle);
+ auto opts = ROCKSDB_NAMESPACE::FlushOptions();
+ // In TiKV context: If tablet is to be destroyed, its background work will
+ // be paused. Manual flush can never make progress.
+ opts.check_if_compaction_disabled = true;
+ s = db_->Flush(opts, handle);
  }
 
  ColumnFamilyMetaData db_metadata;