diff --git a/HISTORY.md b/HISTORY.md index 913bf84ec9d890a6c18120e674766574cbd3dd63..0456014dbc3ec1fe797d1c51fbe9065bad9a4fbe 100644 --- 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 --- 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 --- 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 --- 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() {}