Kernel: Convert read_block method to get a reference instead of pointer

BlockBasedFileSystem::read_block method should get a reference of
a UserOrKernelBuffer.

If we need to force caching a block, we will call other method to do so.

3 files changed, 80 insertions, 47 deletions

diff --git a/Kernel/FileSystem/BlockBasedFileSystem.cpp b/Kernel/FileSystem/BlockBasedFileSystem.cpp
index 103b28d5fc..071a798af8 100644
--- a/Kernel/FileSystem/BlockBasedFileSystem.cpp
+++ b/Kernel/FileSystem/BlockBasedFileSystem.cpp
@@ -32,19 +32,12 @@
 
 namespace Kernel {
 
-struct CacheEntry {
-    IntrusiveListNode list_node;
-    u32 block_index { 0 };
-    u8* data { nullptr };
-    bool has_data { false };
-};
-
 class DiskCache {
 public:
     explicit DiskCache(BlockBasedFS& fs)
         : m_fs(fs)
         , m_cached_block_data(KBuffer::create_with_size(m_entry_count * m_fs.block_size()))
-        , m_entries(KBuffer::create_with_size(m_entry_count * sizeof(CacheEntry)))
+        , m_entries(KBuffer::create_with_size(m_entry_count * sizeof(BlockBasedFS::CacheEntry&)))
     {
         for (size_t i = 0; i < m_entry_count; ++i) {
             entries()[i].data = m_cached_block_data.data() + i * m_fs.block_size();
@@ -64,21 +57,21 @@ public:
         m_dirty = false;
     }
 
-    void mark_dirty(CacheEntry& entry)
+    void mark_dirty(BlockBasedFS::CacheEntry& entry)
     {
         m_dirty_list.prepend(entry);
         m_dirty = true;
     }
 
-    void mark_clean(CacheEntry& entry)
+    void mark_clean(BlockBasedFS::CacheEntry& entry)
     {
         m_clean_list.prepend(entry);
     }
 
-    CacheEntry& get(u32 block_index) const
+    BlockBasedFS::CacheEntry& get(u32 block_index) const
     {
         if (auto it = m_hash.find(block_index); it != m_hash.end()) {
-            auto& entry = const_cast<CacheEntry&>(*it->value);
+            auto& entry = const_cast<BlockBasedFS::CacheEntry&>(*it->value);
             ASSERT(entry.block_index == block_index);
             return entry;
         }
@@ -104,8 +97,8 @@ public:
         return new_entry;
     }
 
-    const CacheEntry* entries() const { return (const CacheEntry*)m_entries.data(); }
-    CacheEntry* entries() { return (CacheEntry*)m_entries.data(); }
+    const BlockBasedFS::CacheEntry* entries() const { return (const BlockBasedFS::CacheEntry*)m_entries.data(); }
+    BlockBasedFS::CacheEntry* entries() { return (BlockBasedFS::CacheEntry*)m_entries.data(); }
 
     template<typename Callback>
     void for_each_clean_entry(Callback callback)
@@ -124,9 +117,9 @@ public:
 private:
     BlockBasedFS& m_fs;
     size_t m_entry_count { 10000 };
-    mutable HashMap<u32, CacheEntry*> m_hash;
-    mutable IntrusiveList<CacheEntry, &CacheEntry::list_node> m_clean_list;
-    mutable IntrusiveList<CacheEntry, &CacheEntry::list_node> m_dirty_list;
+    mutable HashMap<u32, BlockBasedFS::CacheEntry*> m_hash;
+    mutable IntrusiveList<BlockBasedFS::CacheEntry, &BlockBasedFS::CacheEntry::list_node> m_clean_list;
+    mutable IntrusiveList<BlockBasedFS::CacheEntry, &BlockBasedFS::CacheEntry::list_node> m_dirty_list;
     KBuffer m_cached_block_data;
     KBuffer m_entries;
     bool m_dirty { false };
@@ -161,19 +154,50 @@ int BlockBasedFS::write_block(unsigned index, const UserOrKernelBuffer& data, si
         return 0;
     }
 
-    auto& entry = cache().get(index);
+    auto entry_or_error = cache_block(index);
+    if (entry_or_error.is_error())
+        return -EIO;
+    if (!entry_or_error.value().has_data)
+        return -EIO;
+
     if (count < block_size()) {
         // Fill the cache first.
-        read_block(index, nullptr, block_size());
+        if (!force_cache_block(index))
+            return -EIO;
     }
-    if (!data.read(entry.data + offset, count))
+    if (!data.read(entry_or_error.value().data + offset, count))
         return -EFAULT;
 
-    cache().mark_dirty(entry);
-    entry.has_data = true;
+    cache().mark_dirty(entry_or_error.value());
+    // Update the actual entry and not the copy of it!
+    cache().get(index).has_data = true;
     return 0;
 }
 
+bool BlockBasedFS::force_cache_block(unsigned index) const
+{
+    auto& entry = cache().get(index);
+    u32 base_offset = static_cast<u32>(index) * static_cast<u32>(block_size());
+    file_description().seek(base_offset, SEEK_SET);
+    auto entry_data_buffer = UserOrKernelBuffer::for_kernel_buffer(entry.data);
+    auto nread = file_description().read(entry_data_buffer, block_size());
+    if (nread.is_error())
+        return false;
+    ASSERT(nread.value() == block_size());
+    entry.has_data = true;
+    return true;
+}
+
+KResultOr<BlockBasedFS::CacheEntry> BlockBasedFS::cache_block(unsigned index) const
+{
+    auto& entry = cache().get(index);
+    if (!entry.has_data) {
+        if (!force_cache_block(index))
+            return KResult(-EIO);
+    }
+    return entry;
+}
+
 bool BlockBasedFS::raw_read(unsigned index, UserOrKernelBuffer& buffer)
 {
     u32 base_offset = static_cast<u32>(index) * static_cast<u32>(m_logical_block_size);
@@ -225,7 +249,7 @@ int BlockBasedFS::write_blocks(unsigned index, unsigned count, const UserOrKerne
     return 0;
 }
 
-int BlockBasedFS::read_block(unsigned index, UserOrKernelBuffer* buffer, size_t count, size_t offset, bool allow_cache) const
+int BlockBasedFS::read_block(unsigned index, UserOrKernelBuffer& buffer, size_t count, size_t offset, bool allow_cache) const
 {
     ASSERT(m_logical_block_size);
     ASSERT(offset + count <= block_size());
@@ -237,25 +261,19 @@ int BlockBasedFS::read_block(unsigned index, UserOrKernelBuffer* buffer, size_t
         const_cast<BlockBasedFS*>(this)->flush_specific_block_if_needed(index);
         u32 base_offset = static_cast<u32>(index) * static_cast<u32>(block_size()) + static_cast<u32>(offset);
         file_description().seek(base_offset, SEEK_SET);
-        auto nread = file_description().read(*buffer, count);
+        auto nread = file_description().read(buffer, count);
         if (nread.is_error())
             return -EIO;
         ASSERT(nread.value() == count);
         return 0;
     }
 
-    auto& entry = cache().get(index);
-    if (!entry.has_data) {
-        u32 base_offset = static_cast<u32>(index) * static_cast<u32>(block_size());
-        file_description().seek(base_offset, SEEK_SET);
-        auto entry_data_buffer = UserOrKernelBuffer::for_kernel_buffer(entry.data);
-        auto nread = file_description().read(entry_data_buffer, block_size());
-        if (nread.is_error())
-            return -EIO;
-        ASSERT(nread.value() == block_size());
-        entry.has_data = true;
-    }
-    if (buffer && !buffer->write(entry.data + offset, count))
+    auto entry_or_error = cache_block(index);
+    if (entry_or_error.is_error())
+        return -EIO;
+    if (!entry_or_error.value().has_data)
+        return -EIO;
+    if (!buffer.write(entry_or_error.value().data + offset, count))
         return -EFAULT;
     return 0;
 }
@@ -266,10 +284,10 @@ int BlockBasedFS::read_blocks(unsigned index, unsigned count, UserOrKernelBuffer
     if (!count)
         return false;
     if (count == 1)
-        return read_block(index, &buffer, block_size(), 0, allow_cache);
+        return read_block(index, buffer, block_size(), 0, allow_cache);
     auto out = buffer;
     for (unsigned i = 0; i < count; ++i) {
-        auto err = read_block(index + i, &out, block_size(), 0, allow_cache);
+        auto err = read_block(index + i, out, block_size(), 0, allow_cache);
         if (err < 0)
             return err;
         out = out.offset(block_size());
@@ -284,7 +302,7 @@ void BlockBasedFS::flush_specific_block_if_needed(unsigned index)
     if (!cache().is_dirty())
         return;
     Vector<CacheEntry*, 32> cleaned_entries;
-    cache().for_each_dirty_entry([&](CacheEntry& entry) {
+    cache().for_each_dirty_entry([&](BlockBasedFS::CacheEntry& entry) {
         if (entry.block_index != index) {
             u32 base_offset = static_cast<u32>(entry.block_index) * static_cast<u32>(block_size());
             file_description().seek(base_offset, SEEK_SET);
@@ -306,7 +324,7 @@ void BlockBasedFS::flush_writes_impl()
     if (!cache().is_dirty())
         return;
     u32 count = 0;
-    cache().for_each_dirty_entry([&](CacheEntry& entry) {
+    cache().for_each_dirty_entry([&](BlockBasedFS::CacheEntry& entry) {
         u32 base_offset = static_cast<u32>(entry.block_index) * static_cast<u32>(block_size());
         file_description().seek(base_offset, SEEK_SET);
         // FIXME: Should this error path be surfaced somehow?
diff --git a/Kernel/FileSystem/BlockBasedFileSystem.h b/Kernel/FileSystem/BlockBasedFileSystem.h
index fcf096aec3..827d7ca927 100644
--- a/Kernel/FileSystem/BlockBasedFileSystem.h
+++ b/Kernel/FileSystem/BlockBasedFileSystem.h
@@ -27,10 +27,22 @@
 #pragma once
 
 #include <Kernel/FileSystem/FileBackedFileSystem.h>
+#include <Kernel/KResult.h>
 
 namespace Kernel {
 
+class DiskCache;
 class BlockBasedFS : public FileBackedFS {
+    friend class DiskCache;
+
+private:
+    struct CacheEntry {
+        IntrusiveListNode list_node;
+        u32 block_index { 0 };
+        u8* data { nullptr };
+        bool has_data { false };
+    };
+
 public:
     virtual ~BlockBasedFS() override;
 
@@ -42,7 +54,10 @@ public:
 protected:
     explicit BlockBasedFS(FileDescription&);
 
-    int read_block(unsigned index, UserOrKernelBuffer* buffer, size_t count, size_t offset = 0, bool allow_cache = true) const;
+    int read_block(unsigned index, UserOrKernelBuffer& buffer, size_t count, size_t offset = 0, bool allow_cache = true) const;
+    bool force_cache_block(unsigned index) const;
+    KResultOr<CacheEntry> cache_block(unsigned index) const;
+
     int read_blocks(unsigned index, unsigned count, UserOrKernelBuffer& buffer, bool allow_cache = true) const;
 
     bool raw_read(unsigned index, UserOrKernelBuffer& buffer);
diff --git a/Kernel/FileSystem/Ext2FileSystem.cpp b/Kernel/FileSystem/Ext2FileSystem.cpp
index 73a1c0b130..679107bd5a 100644
--- a/Kernel/FileSystem/Ext2FileSystem.cpp
+++ b/Kernel/FileSystem/Ext2FileSystem.cpp
@@ -349,7 +349,7 @@ bool Ext2FS::write_block_list_for_inode(InodeIndex inode_index, ext2_inode& e2in
             dind_block_dirty = true;
         } else {
             auto buffer = UserOrKernelBuffer::for_kernel_buffer(dind_block_contents.data());
-            read_block(e2inode.i_block[EXT2_DIND_BLOCK], &buffer, block_size());
+            read_block(e2inode.i_block[EXT2_DIND_BLOCK], buffer, block_size());
         }
         auto* dind_block_as_pointers = (unsigned*)dind_block_contents.data();
 
@@ -372,7 +372,7 @@ bool Ext2FS::write_block_list_for_inode(InodeIndex inode_index, ext2_inode& e2in
                 ind_block_dirty = true;
             } else {
                 auto buffer = UserOrKernelBuffer::for_kernel_buffer(ind_block_contents.data());
-                read_block(indirect_block_index, &buffer, block_size());
+                read_block(indirect_block_index, buffer, block_size());
             }
             auto* ind_block_as_pointers = (unsigned*)ind_block_contents.data();
 
@@ -491,7 +491,7 @@ Vector<Ext2FS::BlockIndex> Ext2FS::block_list_for_inode_impl(const ext2_inode& e
         auto count = min(blocks_remaining, entries_per_block);
         u32 array[count];
         auto buffer = UserOrKernelBuffer::for_kernel_buffer((u8*)array);
-        read_block(array_block_index, &buffer, sizeof(array), 0);
+        read_block(array_block_index, buffer, sizeof(array), 0);
         for (BlockIndex i = 0; i < count; ++i)
             callback(array[i]);
     };
@@ -684,7 +684,7 @@ RefPtr<Inode> Ext2FS::get_inode(InodeIdentifier inode) const
 
     auto new_inode = adopt(*new Ext2FSInode(const_cast<Ext2FS&>(*this), inode.index()));
     auto buffer = UserOrKernelBuffer::for_kernel_buffer(reinterpret_cast<u8*>(&new_inode->m_raw_inode));
-    read_block(block_index, &buffer, sizeof(ext2_inode), offset);
+    read_block(block_index, buffer, sizeof(ext2_inode), offset);
     m_inode_cache.set(inode.index(), new_inode);
     return new_inode;
 }
@@ -740,7 +740,7 @@ ssize_t Ext2FSInode::read_bytes(off_t offset, ssize_t count, UserOrKernelBuffer&
         size_t offset_into_block = (bi == first_block_logical_index) ? offset_into_first_block : 0;
         size_t num_bytes_to_copy = min(block_size - offset_into_block, remaining_count);
         auto buffer_offset = buffer.offset(nread);
-        int err = fs().read_block(block_index, &buffer_offset, num_bytes_to_copy, offset_into_block, allow_cache);
+        int err = fs().read_block(block_index, buffer_offset, num_bytes_to_copy, offset_into_block, allow_cache);
         if (err < 0) {
             klog() << "ext2fs: read_bytes: read_block(" << block_index << ") failed (lbi: " << bi << ")";
             return err;
@@ -1352,7 +1352,7 @@ Ext2FS::CachedBitmap& Ext2FS::get_bitmap_block(BlockIndex bitmap_block_index)
 
     auto block = KBuffer::create_with_size(block_size(), Region::Access::Read | Region::Access::Write, "Ext2FS: Cached bitmap block");
     auto buffer = UserOrKernelBuffer::for_kernel_buffer(block.data());
-    int err = read_block(bitmap_block_index, &buffer, block_size());
+    int err = read_block(bitmap_block_index, buffer, block_size());
     ASSERT(err >= 0);
     m_cached_bitmaps.append(make<CachedBitmap>(bitmap_block_index, move(block)));
     return *m_cached_bitmaps.last();