RGBCube/serenity
1
Fork 0
mirror of https://github.com/RGBCube/serenity synced 2025-05-24 21:05:06 +00:00
Code Issues Activity Actions

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

Browse source 
This reverts commit 092a13211a.

Fixes #4611.
This commit is contained in:
Andreas Kling 2020-12-29 00:05:37 +01:00
parent 13594b7146
commit 43d9fe15f9
3 changed files with 47 additions and 80 deletions
Download patch file Download diff file Expand all files Collapse all files

96
Kernel/FileSystem/BlockBasedFileSystem.cpp
View file

@ -32,12 +32,19 @@
namespace Kernel { namespace Kernel {
struct CacheEntry {
IntrusiveListNode list_node;
u32 block_index { 0 };
u8* data { nullptr };
bool has_data { false };
};
class DiskCache { class DiskCache {
public: public:
explicit DiskCache(BlockBasedFS& fs) explicit DiskCache(BlockBasedFS& fs)
: m_fs(fs) : m_fs(fs)
, m_cached_block_data(KBuffer::create_with_size(m_entry_count * m_fs.block_size())) , 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(BlockBasedFS::CacheEntry&))) , m_entries(KBuffer::create_with_size(m_entry_count * sizeof(CacheEntry)))
{ {
for (size_t i = 0; i < m_entry_count; ++i) { for (size_t i = 0; i < m_entry_count; ++i) {
entries()[i].data = m_cached_block_data.data() + i * m_fs.block_size(); entries()[i].data = m_cached_block_data.data() + i * m_fs.block_size();
@ -57,21 +64,21 @@ public:
m_dirty = false; m_dirty = false;
} }
void mark_dirty(BlockBasedFS::CacheEntry& entry) void mark_dirty(CacheEntry& entry)
{ {
m_dirty_list.prepend(entry); m_dirty_list.prepend(entry);
m_dirty = true; m_dirty = true;
} }
void mark_clean(BlockBasedFS::CacheEntry& entry) void mark_clean(CacheEntry& entry)
{ {
m_clean_list.prepend(entry); m_clean_list.prepend(entry);
} }
BlockBasedFS::CacheEntry& get(u32 block_index) const CacheEntry& get(u32 block_index) const
{ {
if (auto it = m_hash.find(block_index); it != m_hash.end()) { if (auto it = m_hash.find(block_index); it != m_hash.end()) {
auto& entry = const_cast<BlockBasedFS::CacheEntry&>(*it->value); auto& entry = const_cast<CacheEntry&>(*it->value);
ASSERT(entry.block_index == block_index); ASSERT(entry.block_index == block_index);
return entry; return entry;
} }
@ -97,8 +104,8 @@ public:
return new_entry; return new_entry;
} }
const BlockBasedFS::CacheEntry* entries() const { return (const BlockBasedFS::CacheEntry*)m_entries.data(); } const CacheEntry* entries() const { return (const CacheEntry*)m_entries.data(); }
BlockBasedFS::CacheEntry* entries() { return (BlockBasedFS::CacheEntry*)m_entries.data(); } CacheEntry* entries() { return (CacheEntry*)m_entries.data(); }
template<typename Callback> template<typename Callback>
void for_each_clean_entry(Callback callback) void for_each_clean_entry(Callback callback)
@ -117,9 +124,9 @@ public:
private: private:
BlockBasedFS& m_fs; BlockBasedFS& m_fs;
size_t m_entry_count { 10000 }; size_t m_entry_count { 10000 };
mutable HashMap<u32, BlockBasedFS::CacheEntry*> m_hash; mutable HashMap<u32, CacheEntry*> m_hash;
mutable IntrusiveList<BlockBasedFS::CacheEntry, &BlockBasedFS::CacheEntry::list_node> m_clean_list; mutable IntrusiveList<CacheEntry, &CacheEntry::list_node> m_clean_list;
mutable IntrusiveList<BlockBasedFS::CacheEntry, &BlockBasedFS::CacheEntry::list_node> m_dirty_list; mutable IntrusiveList<CacheEntry, &CacheEntry::list_node> m_dirty_list;
KBuffer m_cached_block_data; KBuffer m_cached_block_data;
KBuffer m_entries; KBuffer m_entries;
bool m_dirty { false }; bool m_dirty { false };
@ -154,48 +161,17 @@ int BlockBasedFS::write_block(unsigned index, const UserOrKernelBuffer& data, si
return 0; return 0;
} }
auto entry_or_error = cache_block(index); auto& entry = cache().get(index);
if (entry_or_error.is_error())
return -EIO;
if (!entry_or_error.value().has_data)
return -EIO;
if (count < block_size()) { if (count < block_size()) {
// Fill the cache first. // Fill the cache first.
if (!force_cache_block(index)) read_block(index, nullptr, block_size());
return -EIO;
} }
if (!data.read(entry_or_error.value().data + offset, count)) if (!data.read(entry.data + offset, count))
return -EFAULT; return -EFAULT;
cache().mark_dirty(entry_or_error.value()); cache().mark_dirty(entry);
// 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; entry.has_data = true;
return true; return 0;
}
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) bool BlockBasedFS::raw_read(unsigned index, UserOrKernelBuffer& buffer)
@ -249,7 +225,7 @@ int BlockBasedFS::write_blocks(unsigned index, unsigned count, const UserOrKerne
return 0; 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(m_logical_block_size);
ASSERT(offset + count <= block_size()); ASSERT(offset + count <= block_size());
@ -261,19 +237,25 @@ int BlockBasedFS::read_block(unsigned index, UserOrKernelBuffer& buffer, size_t
const_cast<BlockBasedFS*>(this)->flush_specific_block_if_needed(index); 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); u32 base_offset = static_cast<u32>(index) * static_cast<u32>(block_size()) + static_cast<u32>(offset);
file_description().seek(base_offset, SEEK_SET); 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()) if (nread.is_error())
return -EIO; return -EIO;
ASSERT(nread.value() == count); ASSERT(nread.value() == count);
return 0; return 0;
} }
auto entry_or_error = cache_block(index); auto& entry = cache().get(index);
if (entry_or_error.is_error()) 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; return -EIO;
if (!entry_or_error.value().has_data) ASSERT(nread.value() == block_size());
return -EIO; entry.has_data = true;
if (!buffer.write(entry_or_error.value().data + offset, count)) }
if (buffer && !buffer->write(entry.data + offset, count))
return -EFAULT; return -EFAULT;
return 0; return 0;
} }
@ -284,10 +266,10 @@ int BlockBasedFS::read_blocks(unsigned index, unsigned count, UserOrKernelBuffer
if (!count) if (!count)
return false; return false;
if (count == 1) 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; auto out = buffer;
for (unsigned i = 0; i < count; ++i) { 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) if (err < 0)
return err; return err;
out = out.offset(block_size()); out = out.offset(block_size());
@ -302,7 +284,7 @@ void BlockBasedFS::flush_specific_block_if_needed(unsigned index)
if (!cache().is_dirty()) if (!cache().is_dirty())
return; return;
Vector<CacheEntry*, 32> cleaned_entries; Vector<CacheEntry*, 32> cleaned_entries;
cache().for_each_dirty_entry([&](BlockBasedFS::CacheEntry& entry) { cache().for_each_dirty_entry([&](CacheEntry& entry) {
if (entry.block_index != index) { if (entry.block_index != index) {
u32 base_offset = static_cast<u32>(entry.block_index) * static_cast<u32>(block_size()); u32 base_offset = static_cast<u32>(entry.block_index) * static_cast<u32>(block_size());
file_description().seek(base_offset, SEEK_SET); file_description().seek(base_offset, SEEK_SET);
@ -324,7 +306,7 @@ void BlockBasedFS::flush_writes_impl()
if (!cache().is_dirty()) if (!cache().is_dirty())
return; return;
u32 count = 0; u32 count = 0;
cache().for_each_dirty_entry([&](BlockBasedFS::CacheEntry& entry) { cache().for_each_dirty_entry([&](CacheEntry& entry) {
u32 base_offset = static_cast<u32>(entry.block_index) * static_cast<u32>(block_size()); u32 base_offset = static_cast<u32>(entry.block_index) * static_cast<u32>(block_size());
file_description().seek(base_offset, SEEK_SET); file_description().seek(base_offset, SEEK_SET);
// FIXME: Should this error path be surfaced somehow? // FIXME: Should this error path be surfaced somehow?

17
Kernel/FileSystem/BlockBasedFileSystem.h
View file

@ -27,22 +27,10 @@
#pragma once #pragma once
#include <Kernel/FileSystem/FileBackedFileSystem.h> #include <Kernel/FileSystem/FileBackedFileSystem.h>
#include <Kernel/KResult.h>
namespace Kernel { namespace Kernel {
class DiskCache;
class BlockBasedFS : public FileBackedFS { 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: public:
virtual ~BlockBasedFS() override; virtual ~BlockBasedFS() override;
@ -54,10 +42,7 @@ public:
protected: protected:
explicit BlockBasedFS(FileDescription&); 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; int read_blocks(unsigned index, unsigned count, UserOrKernelBuffer& buffer, bool allow_cache = true) const;
bool raw_read(unsigned index, UserOrKernelBuffer& buffer); bool raw_read(unsigned index, UserOrKernelBuffer& buffer);

12
Kernel/FileSystem/Ext2FileSystem.cpp
View file

@ -349,7 +349,7 @@ bool Ext2FS::write_block_list_for_inode(InodeIndex inode_index, ext2_inode& e2in
dind_block_dirty = true; dind_block_dirty = true;
} else { } else {
auto buffer = UserOrKernelBuffer::for_kernel_buffer(dind_block_contents.data()); 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(); 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; ind_block_dirty = true;
} else { } else {
auto buffer = UserOrKernelBuffer::for_kernel_buffer(ind_block_contents.data()); 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(); 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); auto count = min(blocks_remaining, entries_per_block);
u32 array[count]; u32 array[count];
auto buffer = UserOrKernelBuffer::for_kernel_buffer((u8*)array); 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) for (BlockIndex i = 0; i < count; ++i)
callback(array[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 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)); 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); m_inode_cache.set(inode.index(), new_inode);
return 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 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); size_t num_bytes_to_copy = min(block_size - offset_into_block, remaining_count);
auto buffer_offset = buffer.offset(nread); 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) { if (err < 0) {
klog() << "ext2fs: read_bytes: read_block(" << block_index << ") failed (lbi: " << bi << ")"; klog() << "ext2fs: read_bytes: read_block(" << block_index << ") failed (lbi: " << bi << ")";
return err; 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 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()); 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); ASSERT(err >= 0);
m_cached_bitmaps.append(make<CachedBitmap>(bitmap_block_index, move(block))); m_cached_bitmaps.append(make<CachedBitmap>(bitmap_block_index, move(block)));
return *m_cached_bitmaps.last(); return *m_cached_bitmaps.last();