Kernel: Tweak FileBackedFS API to avoid intermediary copies

read_block() and write_block() now accept the count (how many bytes to read
or write) and offset (where in the block to start; defaults to 0). Using these
new APIs, we can avoid doing copies between intermediary buffers in a lot more
cases. Hopefully this improves performance or something.

Kernel/FileSystem/Ext2FileSystem.cpp

@@ -155,7 +155,7 @@ InodeIdentifier Ext2FS::root_inode() const
     return { fsid(), EXT2_ROOT_INO };
 }
 
-bool Ext2FS::read_block_containing_inode(unsigned inode, unsigned& block_index, unsigned& offset, u8* buffer) const
+bool Ext2FS::find_block_containing_inode(unsigned inode, unsigned& block_index, unsigned& offset) const
 {
     LOCKER(m_lock);
     auto& super_block = this->super_block();
@@ -172,7 +172,7 @@ bool Ext2FS::read_block_containing_inode(unsigned inode, unsigned& block_index,
     block_index = bgd.bg_inode_table + (offset >> EXT2_BLOCK_SIZE_BITS(&super_block));
     offset &= block_size() - 1;
 
-    return read_block(block_index, buffer);
+    return true;
 }
 
 Ext2FS::BlockListShape Ext2FS::compute_block_list_shape(unsigned blocks)
@@ -285,7 +285,7 @@ bool Ext2FS::write_block_list_for_inode(InodeIndex inode_index, ext2_inode& e2in
             --remaining_blocks;
         }
         stream.fill_to_end(0);
-        bool success = write_block(e2inode.i_block[EXT2_IND_BLOCK], block_contents.data());
+        bool success = write_block(e2inode.i_block[EXT2_IND_BLOCK], block_contents.data(), block_size());
         ASSERT(success);
     }
 
@@ -319,10 +319,11 @@ bool Ext2FS::write_block_list_for_inode(InodeIndex inode_index, ext2_inode& e2in
             indirect_block_count++;
 
         auto dind_block_contents = ByteBuffer::create_uninitialized(block_size());
-        read_block(e2inode.i_block[EXT2_DIND_BLOCK], dind_block_contents.data());
         if (dind_block_new) {
             memset(dind_block_contents.data(), 0, dind_block_contents.size());
             dind_block_dirty = true;
+        } else {
+            read_block(e2inode.i_block[EXT2_DIND_BLOCK], dind_block_contents.data(), block_size());
         }
         auto* dind_block_as_pointers = (unsigned*)dind_block_contents.data();
 
@@ -340,10 +341,11 @@ bool Ext2FS::write_block_list_for_inode(InodeIndex inode_index, ext2_inode& e2in
             }
 
             auto ind_block_contents = ByteBuffer::create_uninitialized(block_size());
-            read_block(indirect_block_index, ind_block_contents.data());
             if (ind_block_new) {
                 memset(ind_block_contents.data(), 0, dind_block_contents.size());
                 ind_block_dirty = true;
+            } else {
+                read_block(indirect_block_index, ind_block_contents.data(), block_size());
             }
             auto* ind_block_as_pointers = (unsigned*)ind_block_contents.data();
 
@@ -368,7 +370,7 @@ bool Ext2FS::write_block_list_for_inode(InodeIndex inode_index, ext2_inode& e2in
             }
 
             if (ind_block_dirty) {
-                bool success = write_block(indirect_block_index, ind_block_contents.data());
+                bool success = write_block(indirect_block_index, ind_block_contents.data(), block_size());
                 ASSERT(success);
             }
         }
@@ -380,7 +382,7 @@ bool Ext2FS::write_block_list_for_inode(InodeIndex inode_index, ext2_inode& e2in
         }
 
         if (dind_block_dirty) {
-            bool success = write_block(e2inode.i_block[EXT2_DIND_BLOCK], dind_block_contents.data());
+            bool success = write_block(e2inode.i_block[EXT2_DIND_BLOCK], dind_block_contents.data(), block_size());
             ASSERT(success);
         }
     }
@@ -449,11 +451,12 @@ Vector<Ext2FS::BlockIndex> Ext2FS::block_list_for_inode_impl(const ext2_inode& e
     auto process_block_array = [&](unsigned array_block_index, auto&& callback) {
         if (include_block_list_blocks)
             callback(array_block_index);
-        auto array_block = ByteBuffer::create_uninitialized(block_size());
+        unsigned count = min(blocks_remaining, entries_per_block);
-        read_block(array_block_index, array_block.data());
+        size_t read_size = count * sizeof(__u32);
+        auto array_block = ByteBuffer::create_uninitialized(read_size);
+        read_block(array_block_index, array_block.data(), read_size, 0);
         ASSERT(array_block);
         auto* array = reinterpret_cast<const __u32*>(array_block.data());
-        unsigned count = min(blocks_remaining, entries_per_block);
         for (BlockIndex i = 0; i < count; ++i)
             callback(array[i]);
     };
@@ -537,7 +540,7 @@ void Ext2FS::flush_writes()
     }
     for (auto& cached_bitmap : m_cached_bitmaps) {
         if (cached_bitmap->dirty) {
-            write_block(cached_bitmap->bitmap_block_index, cached_bitmap->buffer.data());
+            write_block(cached_bitmap->bitmap_block_index, cached_bitmap->buffer.data(), block_size());
             cached_bitmap->dirty = false;
 #ifdef EXT2_DEBUG
             dbg() << "Flushed bitmap block " << cached_bitmap->bitmap_block_index;
@@ -638,12 +641,11 @@ RefPtr<Inode> Ext2FS::get_inode(InodeIdentifier inode) const
 
     unsigned block_index;
     unsigned offset;
-    u8 block[max_block_size];
+    if (!find_block_containing_inode(inode.index(), block_index, offset))
-    if (!read_block_containing_inode(inode.index(), block_index, offset, block))
         return {};
 
     auto new_inode = adopt(*new Ext2FSInode(const_cast<Ext2FS&>(*this), inode.index()));
-    memcpy(&new_inode->m_raw_inode, reinterpret_cast<ext2_inode*>(block + offset), sizeof(ext2_inode));
+    read_block(block_index, reinterpret_cast<u8*>(&new_inode->m_raw_inode), sizeof(ext2_inode), offset);
     m_inode_cache.set(inode.index(), new_inode);
     return new_inode;
 }
@@ -674,6 +676,8 @@ ssize_t Ext2FSInode::read_bytes(off_t offset, ssize_t count, u8* buffer, FileDes
         return -EIO;
     }
 
+    bool allow_cache = !description || !description->is_direct();
+
     const int block_size = fs().block_size();
 
     size_t first_block_logical_index = offset / block_size;
@@ -691,20 +695,16 @@ ssize_t Ext2FSInode::read_bytes(off_t offset, ssize_t count, u8* buffer, FileDes
     dbg() << "Ext2FS: Reading up to " << count << " bytes " << offset << " bytes into inode " << identifier() << " to " << (const void*)buffer;
 #endif
 
-    u8 block[max_block_size];
-
     for (size_t bi = first_block_logical_index; remaining_count && bi <= last_block_logical_index; ++bi) {
         auto block_index = m_block_list[bi];
         ASSERT(block_index);
-        bool success = fs().read_block(block_index, block, description);
+        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);
+        bool success = fs().read_block(block_index, out, num_bytes_to_copy, offset_into_block, allow_cache);
         if (!success) {
             klog() << "ext2fs: read_bytes: read_block(" << block_index << ") failed (lbi: " << bi << ")";
             return -EIO;
         }
-
-        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);
-        memcpy(out, block + offset_into_block, num_bytes_to_copy);
         remaining_count -= num_bytes_to_copy;
         nread += num_bytes_to_copy;
         out += num_bytes_to_copy;
@@ -789,6 +789,8 @@ ssize_t Ext2FSInode::write_bytes(off_t offset, ssize_t count, const u8* data, Fi
         }
     }
 
+    bool allow_cache = !description || !description->is_direct();
+
     const size_t block_size = fs().block_size();
     u64 old_size = size();
     u64 new_size = max(static_cast<u64>(offset) + count, (u64)size());
@@ -812,8 +814,6 @@ ssize_t Ext2FSInode::write_bytes(off_t offset, ssize_t count, const u8* data, Fi
 
     size_t offset_into_first_block = offset % block_size;
 
-    size_t last_logical_block_index_in_file = new_size / block_size;
-
     ssize_t nwritten = 0;
     size_t remaining_count = min((off_t)count, (off_t)new_size - offset);
     const u8* in = data;
@@ -822,35 +822,13 @@ ssize_t Ext2FSInode::write_bytes(off_t offset, ssize_t count, const u8* data, Fi
     dbg() << "Ext2FS: Writing " << count << " bytes " << offset << " bytes into inode " << identifier() << " from " << (const void*)data;
 #endif
 
-    auto buffer_block = ByteBuffer::create_uninitialized(block_size);
     for (size_t bi = first_block_logical_index; remaining_count && bi <= last_block_logical_index; ++bi) {
         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);
-
-        ByteBuffer block;
-        if (offset_into_block != 0 || num_bytes_to_copy != block_size) {
-            block = ByteBuffer::create_uninitialized(block_size);
-            bool success = fs().read_block(m_block_list[bi], block.data(), description);
-            if (!success) {
-                dbg() << "Ext2FS: In write_bytes, read_block(" << m_block_list[bi] << ") failed (bi: " << bi << ")";
-                return -EIO;
-            }
-        } else
-            block = buffer_block;
-
-        memcpy(block.data() + offset_into_block, in, num_bytes_to_copy);
-        if (bi == last_logical_block_index_in_file && num_bytes_to_copy < block_size) {
-            size_t padding_start = new_size % block_size;
-            size_t padding_bytes = block_size - padding_start;
-#ifdef EXT2_DEBUG
-            dbg() << "Ext2FS: Padding last block of file with zero x " << padding_bytes << " (new_size=" << new_size << ", offset_into_block=" << offset_into_block << ", num_bytes_to_copy=" << num_bytes_to_copy << ")";
-#endif
-            memset(block.data() + padding_start, 0, padding_bytes);
-        }
 #ifdef EXT2_DEBUG
         dbg() << "Ext2FS: Writing block " << m_block_list[bi] << " (offset_into_block: " << offset_into_block << ")";
 #endif
-        bool success = fs().write_block(m_block_list[bi], block.data(), description);
+        bool success = fs().write_block(m_block_list[bi], in, num_bytes_to_copy, offset_into_block, allow_cache);
         if (!success) {
             dbg() << "Ext2FS: write_block(" << m_block_list[bi] << ") failed (bi: " << bi << ")";
             ASSERT_NOT_REACHED();
@@ -1056,13 +1034,9 @@ bool Ext2FS::write_ext2_inode(unsigned inode, const ext2_inode& e2inode)
     LOCKER(m_lock);
     unsigned block_index;
     unsigned offset;
-    u8 block[max_block_size];
+    if (!find_block_containing_inode(inode, block_index, offset))
-    if (!read_block_containing_inode(inode, block_index, offset, block))
         return false;
-    memcpy(reinterpret_cast<ext2_inode*>(block + offset), &e2inode, inode_size());
+    return write_block(block_index, reinterpret_cast<const u8*>(&e2inode), inode_size(), offset);
-    bool success = write_block(block_index, block);
-    ASSERT(success);
-    return success;
 }
 
 Vector<Ext2FS::BlockIndex> Ext2FS::allocate_blocks(GroupIndex preferred_group_index, size_t count)
@@ -1287,7 +1261,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");
-    bool success = read_block(bitmap_block_index, block.data());
+    bool success = read_block(bitmap_block_index, block.data(), block_size());
     ASSERT(success);
     m_cached_bitmaps.append(make<CachedBitmap>(bitmap_block_index, move(block)));
     return *m_cached_bitmaps.last();

Kernel/FileSystem/Ext2FileSystem.h

@@ -123,7 +123,7 @@ private:
     unsigned inode_size() const;
 
     bool write_ext2_inode(InodeIndex, const ext2_inode&);
-    bool read_block_containing_inode(InodeIndex inode, BlockIndex& block_index, unsigned& offset, u8* buffer) const;
+    bool find_block_containing_inode(InodeIndex inode, BlockIndex& block_index, unsigned& offset) const;
 
     bool flush_super_block();
 

Kernel/FileSystem/FileBackedFileSystem.cpp

@@ -56,7 +56,7 @@ public:
         }
     }
 
-    ~DiskCache() {}
+    ~DiskCache() { }
 
     bool is_dirty() const { return m_dirty; }
     void set_dirty(bool b) { m_dirty = b; }
@@ -124,26 +124,31 @@ FileBackedFS::~FileBackedFS()
 {
 }
 
-bool FileBackedFS::write_block(unsigned index, const u8* data, FileDescription* description)
+bool FileBackedFS::write_block(unsigned index, const u8* data, size_t count, size_t offset, bool allow_cache)
 {
     ASSERT(m_logical_block_size);
+    ASSERT(offset + count <= block_size());
 #ifdef FBFS_DEBUG
     klog() << "FileBackedFileSystem::write_block " << index << ", size=" << data.size();
 #endif
 
-    bool allow_cache = !description || !description->is_direct();
-
     if (!allow_cache) {
         flush_specific_block_if_needed(index);
-        u32 base_offset = static_cast<u32>(index) * static_cast<u32>(block_size());
+        u32 base_offset = static_cast<u32>(index) * static_cast<u32>(block_size()) + offset;
         m_file_description->seek(base_offset, SEEK_SET);
-        auto nwritten = m_file_description->write(data, block_size());
+        auto nwritten = m_file_description->write(data, count);
-        ASSERT(nwritten == block_size());
+        if (nwritten < 0)
+            return false;
+        ASSERT(static_cast<size_t>(nwritten) == count);
         return true;
     }
 
     auto& entry = cache().get(index);
-    memcpy(entry.data, data, block_size());
+    if (count < block_size()) {
+        // Fill the cache first.
+        read_block(index, nullptr, block_size());
+    }
+    memcpy(entry.data + offset, data, count);
     entry.is_dirty = true;
     entry.has_data = true;
 
@@ -187,58 +192,62 @@ bool FileBackedFS::raw_write_blocks(unsigned index, size_t count, const u8* buff
     return true;
 }
 
-bool FileBackedFS::write_blocks(unsigned index, unsigned count, const u8* data, FileDescription* description)
+bool FileBackedFS::write_blocks(unsigned index, unsigned count, const u8* data, bool allow_cache)
 {
     ASSERT(m_logical_block_size);
 #ifdef FBFS_DEBUG
     klog() << "FileBackedFileSystem::write_blocks " << index << " x" << count;
 #endif
     for (unsigned i = 0; i < count; ++i)
-        write_block(index + i, data + i * block_size(), description);
+        write_block(index + i, data + i * block_size(), block_size(), 0, allow_cache);
     return true;
 }
 
-bool FileBackedFS::read_block(unsigned index, u8* buffer, FileDescription* description) const
+bool FileBackedFS::read_block(unsigned index, u8* buffer, size_t count, size_t offset, bool allow_cache) const
 {
     ASSERT(m_logical_block_size);
+    ASSERT(offset + count <= block_size());
 #ifdef FBFS_DEBUG
     klog() << "FileBackedFileSystem::read_block " << index;
 #endif
 
-    bool allow_cache = !description || !description->is_direct();
-
     if (!allow_cache) {
         const_cast<FileBackedFS*>(this)->flush_specific_block_if_needed(index);
-        u32 base_offset = static_cast<u32>(index) * static_cast<u32>(block_size());
+        u32 base_offset = static_cast<u32>(index) * static_cast<u32>(block_size()) + static_cast<u32>(offset);
-        const_cast<FileDescription&>(*m_file_description).seek(base_offset, SEEK_SET);
+        m_file_description->seek(base_offset, SEEK_SET);
-        auto nread = const_cast<FileDescription&>(*m_file_description).read(buffer, block_size());
+        auto nread = m_file_description->read(buffer, count);
-        ASSERT(nread == block_size());
+        if (nread < 0)
+            return false;
+        ASSERT(static_cast<size_t>(nread) == count);
         return true;
     }
 
     auto& entry = cache().get(index);
     if (!entry.has_data) {
         u32 base_offset = static_cast<u32>(index) * static_cast<u32>(block_size());
-        const_cast<FileDescription&>(*m_file_description).seek(base_offset, SEEK_SET);
+        m_file_description->seek(base_offset, SEEK_SET);
-        auto nread = const_cast<FileDescription&>(*m_file_description).read(entry.data, block_size());
+        auto nread = m_file_description->read(entry.data, block_size());
+        if (nread < 0)
+            return false;
+        ASSERT(static_cast<size_t>(nread) == block_size());
         entry.has_data = true;
-        ASSERT(nread == block_size());
     }
-    memcpy(buffer, entry.data, block_size());
+    if (buffer)
+        memcpy(buffer, entry.data + offset, count);
     return true;
 }
 
-bool FileBackedFS::read_blocks(unsigned index, unsigned count, u8* buffer, FileDescription* description) const
+bool FileBackedFS::read_blocks(unsigned index, unsigned count, u8* buffer, bool allow_cache) const
 {
     ASSERT(m_logical_block_size);
     if (!count)
         return false;
     if (count == 1)
-        return read_block(index, buffer, description);
+        return read_block(index, buffer, block_size(), 0, allow_cache);
     u8* out = buffer;
 
     for (unsigned i = 0; i < count; ++i) {
-        if (!read_block(index + i, out, description))
+        if (!read_block(index + i, out, block_size(), 0, allow_cache))
             return false;
         out += block_size();
     }

Kernel/FileSystem/FileBackedFileSystem.h

@@ -36,8 +36,6 @@ class FileBackedFS : public FS {
 public:
     virtual ~FileBackedFS() override;
 
-    virtual bool is_file_backed() const override { return true; }
-
     File& file() { return m_file_description->file(); }
     FileDescription& file_description() { return *m_file_description; }
     const File& file() const { return m_file_description->file(); }
@@ -52,8 +50,8 @@ public:
 protected:
     explicit FileBackedFS(FileDescription&);
 
-    bool read_block(unsigned index, u8* buffer, FileDescription* = nullptr) const;
+    bool read_block(unsigned index, u8* buffer, size_t count, size_t offset = 0, bool allow_cache = true) const;
-    bool read_blocks(unsigned index, unsigned count, u8* buffer, FileDescription* = nullptr) const;
+    bool read_blocks(unsigned index, unsigned count, u8* buffer, bool allow_cache = true) const;
 
     bool raw_read(unsigned index, u8* buffer);
     bool raw_write(unsigned index, const u8* buffer);
@@ -61,16 +59,18 @@ protected:
     bool raw_read_blocks(unsigned index, size_t count, u8* buffer);
     bool raw_write_blocks(unsigned index, size_t count, const u8* buffer);
 
-    bool write_block(unsigned index, const u8*, FileDescription* = nullptr);
+    bool write_block(unsigned index, const u8* buffer, size_t count, size_t offset = 0, bool allow_cache = true);
-    bool write_blocks(unsigned index, unsigned count, const u8*, FileDescription* = nullptr);
+    bool write_blocks(unsigned index, unsigned count, const u8*, bool allow_cache = true);
 
     size_t m_logical_block_size { 512 };
 
 private:
+    virtual bool is_file_backed() const override { return true; }
+
     DiskCache& cache() const;
     void flush_specific_block_if_needed(unsigned index);
 
-    NonnullRefPtr<FileDescription> m_file_description;
+    mutable NonnullRefPtr<FileDescription> m_file_description;
     mutable OwnPtr<DiskCache> m_cache;
 };