Bladeren bron

Kernel: Fix broken disk cache behavior with O_DIRECT

When writing into a block via an O_DIRECT open file description, we
would first flush every dirty block *except* the one we're about to
write into.

The purpose of flushing is to ensure coherency when mixing direct and
indirect accesses to the same file. This patch fixes the issue by only
flushing the affected block.

Our behavior was totally backwards and could end up doing a lot of
unnecessary work while avoiding the work that actually mattered.
Andreas Kling 3 jaren geleden
bovenliggende
commit
a7b70c62d7
1 gewijzigde bestanden met toevoegingen van 21 en 18 verwijderingen
  1. 21 18
      Kernel/FileSystem/BlockBasedFileSystem.cpp

+ 21 - 18
Kernel/FileSystem/BlockBasedFileSystem.cpp

@@ -35,6 +35,7 @@ public:
     ~DiskCache() = default;
     ~DiskCache() = default;
 
 
     bool is_dirty() const { return !m_dirty_list.is_empty(); }
     bool is_dirty() const { return !m_dirty_list.is_empty(); }
+    bool entry_is_dirty(CacheEntry const& entry) const { return m_dirty_list.contains(entry); }
 
 
     void mark_all_clean()
     void mark_all_clean()
     {
     {
@@ -52,13 +53,20 @@ public:
         m_clean_list.prepend(entry);
         m_clean_list.prepend(entry);
     }
     }
 
 
+    CacheEntry* get(BlockBasedFileSystem::BlockIndex block_index) const
+    {
+        auto it = m_hash.find(block_index);
+        if (it == m_hash.end())
+            return nullptr;
+        auto& entry = const_cast<CacheEntry&>(*it->value);
+        VERIFY(entry.block_index == block_index);
+        return &entry;
+    }
+
     CacheEntry& ensure(BlockBasedFileSystem::BlockIndex block_index) const
     CacheEntry& ensure(BlockBasedFileSystem::BlockIndex block_index) const
     {
     {
-        if (auto it = m_hash.find(block_index); it != m_hash.end()) {
-            auto& entry = const_cast<CacheEntry&>(*it->value);
-            VERIFY(entry.block_index == block_index);
-            return entry;
-        }
+        if (auto* entry = get(block_index))
+            return *entry;
 
 
         if (m_clean_list.is_empty()) {
         if (m_clean_list.is_empty()) {
             // Not a single clean entry! Flush writes and try again.
             // Not a single clean entry! Flush writes and try again.
@@ -261,19 +269,14 @@ void BlockBasedFileSystem::flush_specific_block_if_needed(BlockIndex index)
     m_cache.with_exclusive([&](auto& cache) {
     m_cache.with_exclusive([&](auto& cache) {
         if (!cache->is_dirty())
         if (!cache->is_dirty())
             return;
             return;
-        Vector<CacheEntry*, 32> cleaned_entries;
-        cache->for_each_dirty_entry([&](CacheEntry& entry) {
-            if (entry.block_index != index) {
-                size_t base_offset = entry.block_index.value() * block_size();
-                auto entry_data_buffer = UserOrKernelBuffer::for_kernel_buffer(entry.data);
-                [[maybe_unused]] auto rc = file_description().write(base_offset, entry_data_buffer, block_size());
-                cleaned_entries.append(&entry);
-            }
-        });
-        // NOTE: We make a separate pass to mark entries clean since marking them clean
-        //       moves them out of the dirty list which would disturb the iteration above.
-        for (auto* entry : cleaned_entries)
-            cache->mark_clean(*entry);
+        auto* entry = cache->get(index);
+        if (!entry)
+            return;
+        if (!cache->entry_is_dirty(*entry))
+            return;
+        size_t base_offset = entry->block_index.value() * block_size();
+        auto entry_data_buffer = UserOrKernelBuffer::for_kernel_buffer(entry->data);
+        (void)file_description().write(base_offset, entry_data_buffer, block_size());
     });
     });
 }
 }