Fix POSIX LockFile after failure to create file (#8747)

Summary:
Failure to create the lock file (e.g. out of space) could
prevent future LockFile attempts in the same process on the same file
from succeeding.

Also added DEBUG code to fail assertion if PosixFileLock is destroyed
without using UnlockFile (which is a risk because FileLock is in the
public API with virtual destructor).

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

Test Plan: test added

Reviewed By: ajkr

Differential Revision: D30732543

Pulled By: pdillinger

fbshipit-source-id: 4c30a959566d91f778d6fad3fbbd5f3941b097c1

diff --git a/HISTORY.md b/HISTORY.md
index 913bf84ec9d890a6c18120e674766574cbd3dd63..0456014dbc3ec1fe797d1c51fbe9065bad9a4fbe 100644 (file)
--- a/HISTORY.md
+++ b/HISTORY.md
@@ -7,6 +7,7 @@
 * Fix a race in DumpStats() with column family destruction due to not taking a Ref on each entry while iterating the ColumnFamilySet.
 * Fix a race in item ref counting in LRUCache when promoting an item from the SecondaryCache.
 * Fix a race in BackupEngine if RateLimiter is reconfigured during concurrent Restore operations.
+* Fix a bug on POSIX in which failure to create a lock file (e.g. out of space) can prevent future LockFile attempts in the same process on the same file from succeeding.
 
 ### New Features
 * RemoteCompaction's interface now includes `db_name`, `db_id`, `session_id`, which could help the user uniquely identify compaction job between db instances and sessions.

diff --git a/env/env_test.cc b/env/env_test.cc
index d8403d3d32bc6cfdf3f535367852fb03c6bc583a..dea6bbf878757fa780a16e2898068eb404e4d967 100644 (file)
--- a/env/env_test.cc
+++ b/env/env_test.cc
@@ -45,6 +45,7 @@
 #include "rocksdb/convenience.h"
 #include "rocksdb/env.h"
 #include "rocksdb/env_encryption.h"
+#include "rocksdb/file_system.h"
 #include "rocksdb/system_clock.h"
 #include "test_util/sync_point.h"
 #include "test_util/testharness.h"
@@ -2644,6 +2645,28 @@ TEST_F(EnvTest, GenerateRawUniqueIdTrackRandomDeviceOnly) {
   t.Run();
 }
 
+TEST_F(EnvTest, FailureToCreateLockFile) {
+  auto env = Env::Default();
+  auto fs = env->GetFileSystem();
+  std::string dir = test::PerThreadDBPath(env, "lockdir");
+  std::string file = dir + "/lockfile";
+
+  // Ensure directory doesn't exist
+  ASSERT_OK(DestroyDir(env, dir));
+
+  // Make sure that we can acquire a file lock after the first attempt fails
+  FileLock* lock = nullptr;
+  ASSERT_NOK(fs->LockFile(file, IOOptions(), &lock, /*dbg*/ nullptr));
+  ASSERT_FALSE(lock);
+
+  ASSERT_OK(fs->CreateDir(dir, IOOptions(), /*dbg*/ nullptr));
+  ASSERT_OK(fs->LockFile(file, IOOptions(), &lock, /*dbg*/ nullptr));
+  ASSERT_OK(fs->UnlockFile(lock, IOOptions(), /*dbg*/ nullptr));
+
+  // Clean up
+  ASSERT_OK(DestroyDir(env, dir));
+}
+
 }  // namespace ROCKSDB_NAMESPACE
 
 int main(int argc, char** argv) {

diff --git a/env/fs_posix.cc b/env/fs_posix.cc
index a3e360806b619ec14140f2e71bf7ceed627ac843..6e967b2badc0a416c7419ea1a8b8b7b9b2ed7e3a 100644 (file)
--- a/env/fs_posix.cc
+++ b/env/fs_posix.cc
@@ -107,8 +107,18 @@ static int LockOrUnlock(int fd, bool lock) {
 
 class PosixFileLock : public FileLock {
  public:
-  int fd_;
+  int fd_ = /*invalid*/ -1;
   std::string filename;
+
+  void Clear() {
+    fd_ = -1;
+    filename.clear();
+  }
+
+  virtual ~PosixFileLock() override {
+    // Check for destruction without UnlockFile
+    assert(fd_ == -1);
+  }
 };
 
 int cloexec_flags(int flags, const EnvOptions* options) {
@@ -825,9 +835,6 @@ class PosixFileSystem : public FileSystem {
     if (fd < 0) {
       result = IOError("while open a file for lock", fname, errno);
     } else if (LockOrUnlock(fd, true) == -1) {
-      // if there is an error in locking, then remove the pathname from
-      // lockedfiles
-      locked_files.erase(fname);
       result = IOError("While lock file", fname, errno);
       close(fd);
     } else {
@@ -837,6 +844,12 @@ class PosixFileSystem : public FileSystem {
       my_lock->filename = fname;
       *lock = my_lock;
     }
+    if (!result.ok()) {
+      // If there is an error in locking, then remove the pathname from
+      // locked_files. (If we got this far, it did not exist in locked_files
+      // before this call.)
+      locked_files.erase(fname);
+    }
 
     mutex_locked_files.Unlock();
     return result;
@@ -856,6 +869,7 @@ class PosixFileSystem : public FileSystem {
       result = IOError("unlock", my_lock->filename, errno);
     }
     close(my_lock->fd_);
+    my_lock->Clear();
     delete my_lock;
     mutex_locked_files.Unlock();
     return result;

diff --git a/include/rocksdb/env.h b/include/rocksdb/env.h
index c32abc4f12eac2694422a88a1a3fe0f8ff0466ab..880046e5a0b0c7b78b1c460e1f4510fe03d108be 100644 (file)
--- a/include/rocksdb/env.h
+++ b/include/rocksdb/env.h
@@ -1209,7 +1209,9 @@ class Logger {
   InfoLogLevel log_level_;
 };
 
-// Identifies a locked file.
+// Identifies a locked file. Except in custom Env/Filesystem implementations,
+// the lifetime of a FileLock object should be managed only by LockFile() and
+// UnlockFile().
 class FileLock {
  public:
   FileLock() {}