From 5ef2b317ef1965121ab0090d86962d3eea4a357e Mon Sep 17 00:00:00 2001 From: Dan Engelbrecht Date: Mon, 9 May 2022 22:03:45 +0200 Subject: Make sure CacheBucket::PutStandaloneCacheValue cleans up the temp file if we fail to move the it into place --- zenserver/cache/structuredcachestore.cpp | 48 ++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) (limited to 'zenserver/cache/structuredcachestore.cpp') diff --git a/zenserver/cache/structuredcachestore.cpp b/zenserver/cache/structuredcachestore.cpp index 05c80c5bf..411717e61 100644 --- a/zenserver/cache/structuredcachestore.cpp +++ b/zenserver/cache/structuredcachestore.cpp @@ -1819,6 +1819,14 @@ ZenCacheDiskLayer::CacheBucket::PutStandaloneCacheValue(const IoHash& HashKey, c RetryCount--; } while (RetryCount > 0); + // Once we have called MoveTemporaryIntoPlace we no longer will automatically clean up the temp file + // as the file handle has already been closed + std::filesystem::remove(DataFile.GetPath(), Ec); + if (Ec) + { + ZEN_WARN("Failed to clean up temporary file '{}' for put in '{}', reason '{}'", DataFile.GetPath(), m_BucketDir, Ec.message()); + } + throw std::system_error(Ec, fmt::format("Failed to finalize file '{}' for put in '{}'", DataFilePath.ToUtf8(), m_BucketDir)); } @@ -2971,6 +2979,46 @@ TEST_CASE("z$.threadedinsert") // * doctest::skip(true)) } } +# if ZEN_PLATFORM_WINDOWS +TEST_CASE("z$.blocked.disklayer.put") +{ + // On Windows platform we can't overwrite a standalone file that + // is open for read at the same time. + // Make sure the retry path runs and we get an exception + + ScopedTemporaryDirectory TempDir; + + GcStorageSize CacheSize; + + const auto CreateCacheValue = [](size_t Size) -> CbObject { + std::vector Buf; + Buf.resize(Size); + + CbObjectWriter Writer; + Writer.AddBinary("Binary"sv, Buf.data(), Buf.size()); + return Writer.Save(); + }; + + CasGc Gc; + ZenCacheNamespace Zcs(Gc, TempDir.Path() / "cache"); + + CbObject CacheValue = CreateCacheValue(64 * 1024 + 64); + + IoBuffer Buffer = CacheValue.GetBuffer().AsIoBuffer(); + Buffer.SetContentType(ZenContentType::kCbObject); + + size_t Key = Buffer.Size(); + IoHash HashKey = IoHash::HashBuffer(&Key, sizeof(uint32_t)); + Zcs.Put("test_bucket", HashKey, {.Value = Buffer}); + + ZenCacheValue BufferGet; + CHECK(Zcs.Get("test_bucket", HashKey, BufferGet)); + + MemoryView ValueView = BufferGet.Value.GetView(); + CHECK_THROWS(Zcs.Put("test_bucket", HashKey, {.Value = Buffer})); +} +# endif + #endif void -- cgit v1.2.3 From 5a872e2c699b439e3e5e95fe1c1882c8a0ca92dd Mon Sep 17 00:00:00 2001 From: Dan Engelbrecht Date: Mon, 9 May 2022 22:32:17 +0200 Subject: Restore logic where we accept failed overwrite if resulting size is the same Correctly calculate the m_TotalSize difference when overwriting file --- zenserver/cache/structuredcachestore.cpp | 47 ++++++++++++++++++++++++++++++-- 1 file changed, 44 insertions(+), 3 deletions(-) (limited to 'zenserver/cache/structuredcachestore.cpp') diff --git a/zenserver/cache/structuredcachestore.cpp b/zenserver/cache/structuredcachestore.cpp index 411717e61..b6fd44742 100644 --- a/zenserver/cache/structuredcachestore.cpp +++ b/zenserver/cache/structuredcachestore.cpp @@ -1748,6 +1748,8 @@ ZenCacheDiskLayer::CacheBucket::PutStandaloneCacheValue(const IoHash& HashKey, c BuildPath(DataFilePath, HashKey); std::filesystem::path FsPath{DataFilePath.ToPath()}; + uint64_t OldFileSize = 0; + // We retry to open the file since it can be held open for read. // This happens if the server processes a Get request for the file or // if we are busy sending the file upstream @@ -1757,7 +1759,27 @@ ZenCacheDiskLayer::CacheBucket::PutStandaloneCacheValue(const IoHash& HashKey, c Ec.clear(); { RwLock::ExclusiveLockScope ValueLock(LockForHash(HashKey)); + + std::error_code ExistingEc; + OldFileSize = std::filesystem::file_size(FsPath, ExistingEc); + if (ExistingEc) + { + OldFileSize = 0; + } + DataFile.MoveTemporaryIntoPlace(FsPath, Ec); + + if (Ec && (!ExistingEc) && (OldFileSize == Value.Value.Size())) + { + ZEN_INFO( + "Failed to move temporary file '{}' to '{}'. Target file has same size, assuming concurrent write of same value, move " + "failed with reason '{}'", + DataFile.GetPath(), + FsPath.string(), + m_BucketDir, + Ec.message()); + return; + } } if (!Ec) @@ -1789,7 +1811,15 @@ ZenCacheDiskLayer::CacheBucket::PutStandaloneCacheValue(const IoHash& HashKey, c } m_SlogFile.Append({.Key = HashKey, .Location = Loc}); - m_TotalSize.fetch_add(Loc.Size(), std::memory_order::relaxed); + uint64_t NewFileSize = Loc.Size(); + if (OldFileSize <= NewFileSize) + { + m_TotalSize.fetch_add(NewFileSize - OldFileSize, std::memory_order::relaxed); + } + else + { + m_TotalSize.fetch_sub(OldFileSize - NewFileSize, std::memory_order::relaxed); + } return; } @@ -3014,8 +3044,19 @@ TEST_CASE("z$.blocked.disklayer.put") ZenCacheValue BufferGet; CHECK(Zcs.Get("test_bucket", HashKey, BufferGet)); - MemoryView ValueView = BufferGet.Value.GetView(); - CHECK_THROWS(Zcs.Put("test_bucket", HashKey, {.Value = Buffer})); + // Overwriting with a value of same size should go fine + Zcs.Put("test_bucket", HashKey, {.Value = Buffer}); + + CbObject CacheValue2 = CreateCacheValue(64 * 1024 + 64 + 1); + IoBuffer Buffer2 = CacheValue2.GetBuffer().AsIoBuffer(); + Buffer2.SetContentType(ZenContentType::kCbObject); + // Overwriting with different size should throw exception if file is held open + CHECK_THROWS(Zcs.Put("test_bucket", HashKey, {.Value = Buffer2})); + + BufferGet = ZenCacheValue{}; + + // Read access has been removed, we should now be able to overwrite it + Zcs.Put("test_bucket", HashKey, {.Value = Buffer2}); } # endif -- cgit v1.2.3 From 239e09c1df23e080c5d88cfb5d6af8eb63c232f9 Mon Sep 17 00:00:00 2001 From: Dan Engelbrecht Date: Mon, 9 May 2022 22:38:27 +0200 Subject: make test run on more platforms --- zenserver/cache/structuredcachestore.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) (limited to 'zenserver/cache/structuredcachestore.cpp') diff --git a/zenserver/cache/structuredcachestore.cpp b/zenserver/cache/structuredcachestore.cpp index b6fd44742..a4cab881f 100644 --- a/zenserver/cache/structuredcachestore.cpp +++ b/zenserver/cache/structuredcachestore.cpp @@ -3009,13 +3009,8 @@ TEST_CASE("z$.threadedinsert") // * doctest::skip(true)) } } -# if ZEN_PLATFORM_WINDOWS TEST_CASE("z$.blocked.disklayer.put") { - // On Windows platform we can't overwrite a standalone file that - // is open for read at the same time. - // Make sure the retry path runs and we get an exception - ScopedTemporaryDirectory TempDir; GcStorageSize CacheSize; @@ -3050,15 +3045,20 @@ TEST_CASE("z$.blocked.disklayer.put") CbObject CacheValue2 = CreateCacheValue(64 * 1024 + 64 + 1); IoBuffer Buffer2 = CacheValue2.GetBuffer().AsIoBuffer(); Buffer2.SetContentType(ZenContentType::kCbObject); - // Overwriting with different size should throw exception if file is held open +# if ZEN_PLATFORM_WINDOWS + // On Windows platform, overwriting with different size while we have + // it open for read should throw exception if file is held open CHECK_THROWS(Zcs.Put("test_bucket", HashKey, {.Value = Buffer2})); +# else + // Other platforms should handle overwrite just fine + Zcs.Put("test_bucket", HashKey, {.Value = Buffer2}); +# endif BufferGet = ZenCacheValue{}; // Read access has been removed, we should now be able to overwrite it Zcs.Put("test_bucket", HashKey, {.Value = Buffer2}); } -# endif #endif -- cgit v1.2.3 From e67a43514bfba97fae4bc4ccf42ca312ba1d01bb Mon Sep 17 00:00:00 2001 From: Dan Engelbrecht Date: Mon, 9 May 2022 23:31:29 +0200 Subject: happy path should be minimal work --- zenserver/cache/structuredcachestore.cpp | 46 ++++++++++++++++---------------- 1 file changed, 23 insertions(+), 23 deletions(-) (limited to 'zenserver/cache/structuredcachestore.cpp') diff --git a/zenserver/cache/structuredcachestore.cpp b/zenserver/cache/structuredcachestore.cpp index a4cab881f..c3904d40a 100644 --- a/zenserver/cache/structuredcachestore.cpp +++ b/zenserver/cache/structuredcachestore.cpp @@ -1725,6 +1725,8 @@ ZenCacheDiskLayer::UpdateAccessTimes(const zen::access_tracking::AccessTimes& Ac void ZenCacheDiskLayer::CacheBucket::PutStandaloneCacheValue(const IoHash& HashKey, const ZenCacheValue& Value) { + uint64_t NewFileSize = Value.Value.Size(); + TemporaryFile DataFile; std::error_code Ec; @@ -1739,7 +1741,7 @@ ZenCacheDiskLayer::CacheBucket::PutStandaloneCacheValue(const IoHash& HashKey, c { throw std::system_error(Ec, fmt::format("Failed to write payload ({} bytes) to temporary file '{}' for put in '{}'", - NiceBytes(Value.Value.Size()), + NiceBytes(NewFileSize), DataFile.GetPath().string(), m_BucketDir)); } @@ -1748,9 +1750,7 @@ ZenCacheDiskLayer::CacheBucket::PutStandaloneCacheValue(const IoHash& HashKey, c BuildPath(DataFilePath, HashKey); std::filesystem::path FsPath{DataFilePath.ToPath()}; - uint64_t OldFileSize = 0; - - // We retry to open the file since it can be held open for read. + // We retry to move the file since it can be held open for read. // This happens if the server processes a Get request for the file or // if we are busy sending the file upstream int RetryCount = 3; @@ -1760,25 +1760,24 @@ ZenCacheDiskLayer::CacheBucket::PutStandaloneCacheValue(const IoHash& HashKey, c { RwLock::ExclusiveLockScope ValueLock(LockForHash(HashKey)); - std::error_code ExistingEc; - OldFileSize = std::filesystem::file_size(FsPath, ExistingEc); - if (ExistingEc) - { - OldFileSize = 0; - } - DataFile.MoveTemporaryIntoPlace(FsPath, Ec); - if (Ec && (!ExistingEc) && (OldFileSize == Value.Value.Size())) + if (Ec) { - ZEN_INFO( - "Failed to move temporary file '{}' to '{}'. Target file has same size, assuming concurrent write of same value, move " - "failed with reason '{}'", - DataFile.GetPath(), - FsPath.string(), - m_BucketDir, - Ec.message()); - return; + std::error_code ExistingEc; + uint64_t OldFileSize = std::filesystem::file_size(FsPath, ExistingEc); + if (!ExistingEc && (OldFileSize == NewFileSize)) + { + ZEN_INFO( + "Failed to move temporary file '{}' to '{}'. Target file has same size, assuming concurrent write of same value, " + "move " + "failed with reason '{}'", + DataFile.GetPath(), + FsPath.string(), + m_BucketDir, + Ec.message()); + return; + } } } @@ -1795,9 +1794,10 @@ ZenCacheDiskLayer::CacheBucket::PutStandaloneCacheValue(const IoHash& HashKey, c EntryFlags |= DiskLocation::kCompressed; } - DiskLocation Loc(Value.Value.Size(), EntryFlags); + DiskLocation Loc(NewFileSize, EntryFlags); IndexEntry Entry = IndexEntry(Loc, GcClock::TickCount()); + uint64_t OldFileSize = 0; RwLock::ExclusiveLockScope _(m_IndexLock); if (auto It = m_Index.find(HashKey); It == m_Index.end()) { @@ -1807,11 +1807,11 @@ ZenCacheDiskLayer::CacheBucket::PutStandaloneCacheValue(const IoHash& HashKey, c else { // TODO: should check if write is idempotent and bail out if it is? - It.value() = Entry; + OldFileSize = It.value().Location.Size(); + It.value() = Entry; } m_SlogFile.Append({.Key = HashKey, .Location = Loc}); - uint64_t NewFileSize = Loc.Size(); if (OldFileSize <= NewFileSize) { m_TotalSize.fetch_add(NewFileSize - OldFileSize, std::memory_order::relaxed); -- cgit v1.2.3 From 5d15fa59655c79a0c8ad1b4c5d44b657aa07c29e Mon Sep 17 00:00:00 2001 From: Dan Engelbrecht Date: Tue, 10 May 2022 10:08:31 +0200 Subject: Make sure we clean up temp file in all scenarios --- zenserver/cache/structuredcachestore.cpp | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) (limited to 'zenserver/cache/structuredcachestore.cpp') diff --git a/zenserver/cache/structuredcachestore.cpp b/zenserver/cache/structuredcachestore.cpp index c3904d40a..ce55b24b6 100644 --- a/zenserver/cache/structuredcachestore.cpp +++ b/zenserver/cache/structuredcachestore.cpp @@ -1736,6 +1736,22 @@ ZenCacheDiskLayer::CacheBucket::PutStandaloneCacheValue(const IoHash& HashKey, c throw std::system_error(Ec, fmt::format("Failed to open temporary file for put in '{}'", m_BucketDir)); } + bool CleanUpTempFile = false; + auto __ = MakeGuard([&] { + if (CleanUpTempFile) + { + std::error_code Ec; + std::filesystem::remove(DataFile.GetPath(), Ec); + if (Ec) + { + ZEN_WARN("Failed to clean up temporary file '{}' for put in '{}', reason '{}'", + DataFile.GetPath(), + m_BucketDir, + Ec.message()); + } + } + }); + DataFile.WriteAll(Value.Value, Ec); if (Ec) { @@ -1762,6 +1778,10 @@ ZenCacheDiskLayer::CacheBucket::PutStandaloneCacheValue(const IoHash& HashKey, c DataFile.MoveTemporaryIntoPlace(FsPath, Ec); + // Once we have called MoveTemporaryIntoPlace automatic clean up the temp file + // will be disabled as the file handle has already been closed + CleanUpTempFile = Ec ? true : false; + if (Ec) { std::error_code ExistingEc; @@ -1849,14 +1869,6 @@ ZenCacheDiskLayer::CacheBucket::PutStandaloneCacheValue(const IoHash& HashKey, c RetryCount--; } while (RetryCount > 0); - // Once we have called MoveTemporaryIntoPlace we no longer will automatically clean up the temp file - // as the file handle has already been closed - std::filesystem::remove(DataFile.GetPath(), Ec); - if (Ec) - { - ZEN_WARN("Failed to clean up temporary file '{}' for put in '{}', reason '{}'", DataFile.GetPath(), m_BucketDir, Ec.message()); - } - throw std::system_error(Ec, fmt::format("Failed to finalize file '{}' for put in '{}'", DataFilePath.ToUtf8(), m_BucketDir)); } -- cgit v1.2.3