issue-1751: [Filestore] WriteBackCache should not crash on corruption

Author: Andrei Nasonov

cloud/filestore/libs/diagnostics/critical_events.h

@@ -41,6 +41,7 @@ namespace NCloud::NFileStore{
     xxx(WriteBackCacheCorruptionError)                                         \
     xxx(WriteBackCacheDataLossError)                                           \
     xxx(WriteBackCacheImpossibleState)                                         \
+    xxx(WriteBackCacheInvalidConfiguration)                                    \
     xxx(WriteBackCacheWritingNotAllowedInDrainingMode)                         \
     xxx(ErrorWasSentToTheGuest)                                                \
     xxx(DirectoryHandlesStorageError)                                          \

cloud/filestore/libs/vfs_fuse/write_back_cache/persistent_storage.cpp

@@ -42,12 +42,10 @@ class TFileRingBufferStorage: public IPersistentStorage
         , LogTag(std::move(logTag))
     {
         SetCounters();
-    }
 
-    NProto::TError Init()
-    {
         if (Storage.IsCorrupted()) {
-            return MakeError(E_FAIL, "Data structure is corrupted");
+            // Reporting corrupted state is handled by TFileRingBuffer
+            return;
         }
 
         NJsonWriter::TBuf json;
@@ -65,19 +63,23 @@ class TFileRingBufferStorage: public IPersistentStorage
             .EndObject();
 
         STORAGE_INFO(
-            LogTag << " WriteBackCache has been initialized " << json.Str());
-
-        return {};
+            LogTag << " WriteBackCache storage has been initialized "
+                   << json.Str());
     }
 
     bool Empty() const override
     {
         return Storage.Empty();
     }
 
-    void Visit(const TVisitor& visitor) override
+    bool IsCorrupted() const override
     {
-        Storage.Visit(
+        return Storage.IsCorrupted();
+    }
+
+    NProto::TError Visit(const TVisitor& visitor) override
+    {
+        return Storage.Visit(
             [&visitor](ui32 checksum, ui32 tag, TStringBuf entry)
             {
                 Y_UNUSED(checksum);
@@ -97,27 +99,23 @@ class TFileRingBufferStorage: public IPersistentStorage
         return Storage.Alloc(size);
     }
 
-    void Commit() override
+    NProto::TError Commit() override
     {
         auto res = Storage.Commit();
-        Y_ENSURE(
-            !HasError(res),
-            "Failed to commit allocation: " << FormatError(res));
         SetCounters();
+        return res;
     }
 
-    void Free(const void* ptr) override
+    NProto::TError Free(const void* ptr) override
     {
         auto res = Storage.Free(ptr);
-        Y_ENSURE(
-            !HasError(res),
-            "Failed to free pointer " << ptr << ": " << FormatError(res));
         SetCounters();
+        return res;
     }
 
-    void SetTag(const void* ptr, ui32 tag) override
+    NProto::TError SetTag(const void* ptr, ui32 tag) override
     {
-        Storage.SetTag(ptr, tag);
+        return Storage.SetTag(ptr, tag);
     }
 
     void UpdateStats() const override
@@ -144,24 +142,17 @@ class TFileRingBufferStorage: public IPersistentStorage
 
 ////////////////////////////////////////////////////////////////////////////////
 
-TResultOrError<IPersistentStoragePtr> CreateFileRingBufferPersistentStorage(
+IPersistentStoragePtr CreateFileRingBufferPersistentStorage(
     IPersistentStorageStatsPtr stats,
     TPersistentStorageConfig config,
     TLog log,
     TString logTag)
 {
-    auto storage = std::make_shared<TFileRingBufferStorage>(
+    return std::make_shared<TFileRingBufferStorage>(
         std::move(stats),
         std::move(config),
         std::move(log),
         std::move(logTag));
-
-    auto error = storage->Init();
-    if (HasError(error)) {
-        return error;
-    }
-
-    return static_cast<IPersistentStoragePtr>(storage);
 }
 
 }   // namespace NCloud::NFileStore::NFuse::NWriteBackCache

cloud/filestore/libs/vfs_fuse/write_back_cache/persistent_storage.h

@@ -22,14 +22,24 @@ struct IPersistentStorage
     virtual ~IPersistentStorage() = default;
 
     virtual bool Empty() const = 0;
+    virtual bool IsCorrupted() const = 0;
 
-    // Enumerates the contents of the persistent storage in the allocation order
-    virtual void Visit(const TVisitor& visitor) = 0;
+    /**
+     * Enumerates the contents of the persistent storage in the allocation order
+     *
+     * Does not visit anything and returns an error if the buffer is corrupted.
+     */
+    virtual NProto::TError Visit(const TVisitor& visitor) = 0;
 
     /**
-     * Gets the maximum possible buffer size that can be allocated.
-     * Alloc is guaranteed to succeed for any size <=
-     * MaxSupportedAllocationByteCount when the storage is empty.
+     * Returns the number of bytes that can be successfully allocated by
+     * PushBack and Alloc for an empty buffer without exceeding the capacity.
+     *
+     * Returns zero if the buffer is corrupted.
+     *
+     * Note: the purpose of this method is to provide a guarantee that an
+     * allocation of this size will eventually succeed. Allocations of higher
+     * sizes will fail with an error.
      */
     virtual ui64 GetMaxSupportedAllocationByteCount() const = 0;
 
@@ -39,29 +49,43 @@ struct IPersistentStorage
      * On successful allocation, returns a pointer to the buffer in persistent
      * storage. The caller should fill the buffer and call Commit.
      *
-     * Returns nullptr if there is not enough free space in the storage.
+     * On failure, returns nullptr if the buffer is full or an error if
+     * allocation is not possible due to corruption or invalid argument.
      *
-     * Returns an error if allocation is not possible due to other reasons.
+     * Note: only one allocation is possible at a time. Repeated Alloc will
+     * return an error.
      */
     [[nodiscard]] virtual TResultOrError<char*> Alloc(size_t size) = 0;
 
     /**
-     * Commits previous memory allocation
+     * Commits previously allocated memory buffer.
+     *
+     * Once committed, it is not allowed to modify the contents of the allocated
+     * entry. If there is a need to augment the allocation with additional data,
+     * SetTag can be used.
      *
      * Memory that was allocated but not committed will be lost at buffer
      * recreation.
      *
-     * Returns true if the commit was successful.
-     * Returns false if Alloc was not called.
+     * An error is returned if there is no incomplete allocation or the buffer
+     * is corrupted.
      */
-    virtual void Commit() = 0;
+    virtual NProto::TError Commit() = 0;
 
-    // Frees a previously allocated buffer.
-    virtual void Free(const void* ptr) = 0;
+    /**
+     * Frees a previously allocated and committed buffer.
+     *
+     * An error is returned if the pointer is invalid or the buffer is corrupted
+     */
+    virtual NProto::TError Free(const void* ptr) = 0;
 
-    // Once committed, entries should remain immutable.
-    // But it is possible to assign a small mutable tag to the entry.
-    virtual void SetTag(const void* ptr, ui32 tag) = 0;
+    /**
+     * Sets the tag value associated with the allocation.
+     *
+     * Returns an error if the pointer is invalid, the tag value exceeds the
+     * maximal supported value or if the buffer is corrupted.
+     */
+    virtual NProto::TError SetTag(const void* ptr, ui32 tag) = 0;
 
     virtual void UpdateStats() const = 0;
 };
@@ -79,7 +103,7 @@ struct TPersistentStorageConfig
 
 ////////////////////////////////////////////////////////////////////////////////
 
-TResultOrError<IPersistentStoragePtr> CreateFileRingBufferPersistentStorage(
+IPersistentStoragePtr CreateFileRingBufferPersistentStorage(
     IPersistentStorageStatsPtr stats,
     TPersistentStorageConfig config,
     TLog log,

cloud/filestore/libs/vfs_fuse/write_back_cache/persistent_storage_ut.cpp

@@ -41,20 +41,15 @@ struct TBootstrap
 
     NProto::TError Initialize()
     {
-        auto res = CreateFileRingBufferPersistentStorage(
+        Storage = CreateFileRingBufferPersistentStorage(
             Stats,
             {.FilePath = TempFile.GetName(),
              .DataCapacity = DefaultCapacity,
              .MetadataCapacity = 0},
             Log,
             "[tag]");
 
-        if (HasError(res)) {
-            return res.GetError();
-        }
-
-        Storage = res.ExtractResult();
-        return {};
+        return MakeError(Storage->IsCorrupted() ? E_FAIL : S_OK);
     }
 
     void Deinitialize()
@@ -89,9 +84,9 @@ struct TBootstrap
         return HasError(allocationResult);
     }
 
-    void Free(const void* ptr) const
+    NProto::TError Free(const void* ptr) const
     {
-        Storage->Free(ptr);
+        return Storage->Free(ptr);
     }
 
     TString Dump() const
@@ -157,7 +152,7 @@ Y_UNIT_TEST_SUITE(TPersistentStorageTest)
         UNIT_ASSERT_VALUES_EQUAL(0, stats.EntryCount->Get());
     }
 
-    Y_UNIT_TEST(ShouldThrowOnDoubleFree)
+    Y_UNIT_TEST(ShouldReturnErrorOnDoubleFree)
     {
         TBootstrap b;
 
@@ -170,11 +165,11 @@ Y_UNIT_TEST_SUITE(TPersistentStorageTest)
         const auto* ptr2 = b.Alloc("567890");
         UNIT_ASSERT(ptr2);
 
-        b.Free(ptr2);
-        UNIT_ASSERT_EXCEPTION(b.Free(ptr2), yexception);
+        UNIT_ASSERT(!HasError(b.Free(ptr2)));
+        UNIT_ASSERT(HasError(b.Free(ptr2)));
 
-        b.Free(ptr1);
-        UNIT_ASSERT_EXCEPTION(b.Free(ptr1), yexception);
+        UNIT_ASSERT(!HasError(b.Free(ptr1)));
+        UNIT_ASSERT(HasError(b.Free(ptr1)));
     }
 
     Y_UNIT_TEST(ShouldValidateAllocationSize)

cloud/filestore/libs/vfs_fuse/write_back_cache/read_response_builder_ut.cpp

@@ -37,14 +37,15 @@ class TBootstrap: private IQueuedOperationsProcessor
         : Stats(CreateWriteBackCacheStats())
         , State(
               *this,
+              CreateTestStorage(Stats),
               std::make_shared<TTestTimer>(),
               Stats->GetWriteBackCacheStateStats(),
               Stats->GetWriteDataRequestManagerStats(),
               Stats->GetNodeStateHolderStats(),
               TFlushBatchLimits{},
               "[tag]")
     {
-        State.Init(CreateTestStorage(Stats));
+        State.Init();
 
         Write(2, "ABCD");
         Write(10, "IJKL");

cloud/filestore/libs/vfs_fuse/write_back_cache/test/test_persistent_storage.cpp

@@ -13,11 +13,17 @@ bool TTestStorage::Empty() const
     return Data.empty();
 }
 
-void TTestStorage::Visit(const TVisitor& visitor)
+bool TTestStorage::IsCorrupted() const
+{
+    return false;
+}
+
+NProto::TError TTestStorage::Visit(const TVisitor& visitor)
 {
     for (const auto& it: List) {
         visitor(it.Tag, it.Data);
     }
+    return {};
 }
 
 ui64 TTestStorage::GetMaxSupportedAllocationByteCount() const
@@ -43,25 +49,29 @@ TResultOrError<char*> TTestStorage::Alloc(size_t size)
     return res;
 }
 
-void TTestStorage::Commit()
-{}
+NProto::TError TTestStorage::Commit()
+{
+    return {};
+}
 
-void TTestStorage::Free(const void* ptr)
+NProto::TError TTestStorage::Free(const void* ptr)
 {
     auto it = Data.find(ptr);
     Y_ENSURE(it != Data.end(), "Double free detected");
 
     Data.erase(it);
 
     SetStats();
+    return {};
 }
 
-void TTestStorage::SetTag(const void* ptr, ui32 tag)
+NProto::TError TTestStorage::SetTag(const void* ptr, ui32 tag)
 {
     auto it = Data.find(ptr);
     Y_ENSURE(it != Data.end(), "Entry not found");
 
     it->second->Tag = tag;
+    return {};
 }
 
 void TTestStorage::UpdateStats() const

cloud/filestore/libs/vfs_fuse/write_back_cache/test/test_persistent_storage.h

@@ -29,12 +29,13 @@ class TTestStorage: public IPersistentStorage
     explicit TTestStorage(IPersistentStorageStatsPtr stats);
 
     bool Empty() const override;
-    void Visit(const TVisitor& visitor) override;
+    bool IsCorrupted() const override;
+    NProto::TError Visit(const TVisitor& visitor) override;
     ui64 GetMaxSupportedAllocationByteCount() const override;
     TResultOrError<char*> Alloc(size_t size) override;
-    void Commit() override;
-    void Free(const void* ptr) override;
-    void SetTag(const void* ptr, ui32 tag) override;
+    NProto::TError Commit() override;
+    NProto::TError Free(const void* ptr) override;
+    NProto::TError SetTag(const void* ptr, ui32 tag) override;
     void UpdateStats() const override;
 
     void SetCapacity(size_t capacity);