From 2871df6f0d131f4777847665d9cbc0ca93e3e752 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Thu, 4 Mar 2021 15:08:37 +0100 Subject: [PATCH] Kernel: Stop trying to keep InodeVMObject in sync with disk changes As it turns out, Dr. POSIX doesn't require that post-mmap() changes to a file are reflected in the memory mappings. So we don't actually have to care about the file size changing (or the contents.) IIUC, as long as all the MAP_SHARED mappings that refer to the same inode are in sync, we're good. This means that VMObjects don't need resizing capabilities. I'm sure there are ways we can take advantage of this fact. --- Kernel/FileSystem/Ext2FileSystem.cpp | 5 ----- Kernel/FileSystem/Inode.cpp | 14 ------------ Kernel/FileSystem/Inode.h | 2 -- Kernel/FileSystem/TmpFS.cpp | 13 ----------- Kernel/VM/InodeVMObject.cpp | 32 ---------------------------- Kernel/VM/InodeVMObject.h | 3 --- 6 files changed, 69 deletions(-) diff --git a/Kernel/FileSystem/Ext2FileSystem.cpp b/Kernel/FileSystem/Ext2FileSystem.cpp index 4235078576f..75b58a10ca3 100644 --- a/Kernel/FileSystem/Ext2FileSystem.cpp +++ b/Kernel/FileSystem/Ext2FileSystem.cpp @@ -889,7 +889,6 @@ ssize_t Ext2FSInode::write_bytes(off_t offset, ssize_t count, const UserOrKernel 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(offset) + count, (u64)size()); auto resize_result = resize(new_size); @@ -930,10 +929,6 @@ ssize_t Ext2FSInode::write_bytes(off_t offset, ssize_t count, const UserOrKernel } dbgln_if(EXT2_VERY_DEBUG, "Ext2FS: After write, i_size={}, i_blocks={} ({} blocks in list)", m_raw_inode.i_size, m_raw_inode.i_blocks, m_block_list.size()); - - if (old_size != new_size) - inode_size_changed(old_size, new_size); - inode_contents_changed(offset, count, data); return nwritten; } diff --git a/Kernel/FileSystem/Inode.cpp b/Kernel/FileSystem/Inode.cpp index 8858fd4a6d5..d89e9411b6a 100644 --- a/Kernel/FileSystem/Inode.cpp +++ b/Kernel/FileSystem/Inode.cpp @@ -137,20 +137,6 @@ void Inode::will_be_destroyed() flush_metadata(); } -void Inode::inode_contents_changed(off_t offset, ssize_t size, const UserOrKernelBuffer& data) -{ - LOCKER(m_lock); - if (auto shared_vmobject = this->shared_vmobject()) - shared_vmobject->inode_contents_changed({}, offset, size, data); -} - -void Inode::inode_size_changed(size_t old_size, size_t new_size) -{ - LOCKER(m_lock); - if (auto shared_vmobject = this->shared_vmobject()) - shared_vmobject->inode_size_changed({}, old_size, new_size); -} - int Inode::set_atime(time_t) { return -ENOTIMPL; diff --git a/Kernel/FileSystem/Inode.h b/Kernel/FileSystem/Inode.h index b87eb18f1cf..d7906b9c09e 100644 --- a/Kernel/FileSystem/Inode.h +++ b/Kernel/FileSystem/Inode.h @@ -129,8 +129,6 @@ public: protected: Inode(FS& fs, InodeIndex); void set_metadata_dirty(bool); - void inode_contents_changed(off_t, ssize_t, const UserOrKernelBuffer&); - void inode_size_changed(size_t old_size, size_t new_size); KResult prepare_to_write_data(); void did_add_child(const InodeIdentifier&); diff --git a/Kernel/FileSystem/TmpFS.cpp b/Kernel/FileSystem/TmpFS.cpp index 7fc7c4efbfa..b2328cf5d50 100644 --- a/Kernel/FileSystem/TmpFS.cpp +++ b/Kernel/FileSystem/TmpFS.cpp @@ -204,13 +204,10 @@ ssize_t TmpFSInode::write_bytes(off_t offset, ssize_t size, const UserOrKernelBu m_metadata.size = new_size; set_metadata_dirty(true); set_metadata_dirty(false); - inode_size_changed(old_size, new_size); } if (!buffer.read(m_content->data() + offset, size)) // TODO: partial reads? return -EFAULT; - inode_contents_changed(offset, size, buffer); - return size; } @@ -353,18 +350,8 @@ KResult TmpFSInode::truncate(u64 size) m_content = move(tmp); } - size_t old_size = m_metadata.size; m_metadata.size = size; notify_watchers(); - - if (old_size != (size_t)size) { - inode_size_changed(old_size, size); - if (m_content) { - auto buffer = UserOrKernelBuffer::for_kernel_buffer(m_content->data()); - inode_contents_changed(0, size, buffer); - } - } - return KSuccess; } diff --git a/Kernel/VM/InodeVMObject.cpp b/Kernel/VM/InodeVMObject.cpp index 77f02d0af5c..30bb0f629ed 100644 --- a/Kernel/VM/InodeVMObject.cpp +++ b/Kernel/VM/InodeVMObject.cpp @@ -71,38 +71,6 @@ size_t InodeVMObject::amount_dirty() const return count * PAGE_SIZE; } -void InodeVMObject::inode_size_changed(Badge, size_t old_size, size_t new_size) -{ - dbgln("VMObject::inode_size_changed: ({}:{}) {} -> {}", m_inode->fsid(), m_inode->index(), old_size, new_size); - - InterruptDisabler disabler; - - auto new_page_count = page_round_up(new_size) / PAGE_SIZE; - m_physical_pages.resize(new_page_count); - - m_dirty_pages.grow(new_page_count, false); - - // FIXME: Consolidate with inode_contents_changed() so we only do a single walk. - for_each_region([](auto& region) { - region.remap(); - }); -} - -void InodeVMObject::inode_contents_changed(Badge, off_t offset, [[maybe_unused]] ssize_t size, [[maybe_unused]] const UserOrKernelBuffer& data) -{ - InterruptDisabler disabler; - VERIFY(offset >= 0); - - // FIXME: Only invalidate the parts that actually changed. - for (auto& physical_page : m_physical_pages) - physical_page = nullptr; - - // FIXME: Consolidate with inode_size_changed() so we only do a single walk. - for_each_region([](auto& region) { - region.remap(); - }); -} - int InodeVMObject::release_all_clean_pages() { LOCKER(m_paging_lock); diff --git a/Kernel/VM/InodeVMObject.h b/Kernel/VM/InodeVMObject.h index 035663aa12e..d00c8538b89 100644 --- a/Kernel/VM/InodeVMObject.h +++ b/Kernel/VM/InodeVMObject.h @@ -39,9 +39,6 @@ public: Inode& inode() { return *m_inode; } const Inode& inode() const { return *m_inode; } - void inode_contents_changed(Badge, off_t, ssize_t, const UserOrKernelBuffer&); - void inode_size_changed(Badge, size_t old_size, size_t new_size); - size_t amount_dirty() const; size_t amount_clean() const;