From fea3cb5ff9127d2f5524d8923157c5281d6b8340 Mon Sep 17 00:00:00 2001 From: Liav A Date: Sat, 20 Aug 2022 09:28:02 +0300 Subject: [PATCH] Kernel/FileSystem: Discard safely filesystems when unmounted last time This commit reached that goal of "safely discarding" a filesystem by doing the following: 1. Stop using the s_file_system_map HashMap as it was an unsafe measure to access pointers of FileSystems. Instead, make sure to register all FileSystems at the VFS layer, with an IntrusiveList, to avoid problems related to OOM conditions. 2. Make sure to cleanly remove the DiskCache object from a BlockBased filesystem, so the destructor of such object will not need to do that in the destruction point. 3. For ext2 filesystems, don't cache the root inode at m_inode_cache HashMap. The reason for this is that when unmounting an ext2 filesystem, we lookup at the cache to see if there's a reference to a cached inode and if that's the case, we fail with EBUSY. If we keep the m_root_inode also being referenced at the m_inode_cache map, we have 2 references to that object, which will lead to fail with EBUSY. Also, it's much simpler to always ask for a root inode and get it immediately from m_root_inode, instead of looking up the cache for that inode. --- Kernel/FileSystem/BlockBasedFileSystem.cpp | 18 +++++-- Kernel/FileSystem/BlockBasedFileSystem.h | 2 + Kernel/FileSystem/Ext2FileSystem.cpp | 23 +++++++- Kernel/FileSystem/Ext2FileSystem.h | 2 + Kernel/FileSystem/FileSystem.cpp | 32 ++--------- Kernel/FileSystem/FileSystem.h | 13 +---- Kernel/FileSystem/ISO9660FileSystem.cpp | 1 + Kernel/FileSystem/VirtualFileSystem.cpp | 62 +++++++++++++++++++--- Kernel/FileSystem/VirtualFileSystem.h | 4 ++ 9 files changed, 103 insertions(+), 54 deletions(-) diff --git a/Kernel/FileSystem/BlockBasedFileSystem.cpp b/Kernel/FileSystem/BlockBasedFileSystem.cpp index b343022ecf..4e78a5d3ed 100644 --- a/Kernel/FileSystem/BlockBasedFileSystem.cpp +++ b/Kernel/FileSystem/BlockBasedFileSystem.cpp @@ -27,7 +27,7 @@ public: , m_entries(move(entries_buffer)) { for (size_t i = 0; i < EntryCount; ++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(); m_clean_list.append(entries()[i]); } } @@ -72,7 +72,7 @@ public: // Not a single clean entry! Flush writes and try again. // NOTE: We want to make sure we only call FileBackedFileSystem flush here, // not some FileBackedFileSystem subclass flush! - m_fs.flush_writes_impl(); + m_fs->flush_writes_impl(); return ensure(block_index); } @@ -100,10 +100,10 @@ public: } private: - BlockBasedFileSystem& m_fs; - mutable HashMap m_hash; - mutable IntrusiveList<&CacheEntry::list_node> m_clean_list; + mutable NonnullRefPtr m_fs; mutable IntrusiveList<&CacheEntry::list_node> m_dirty_list; + mutable IntrusiveList<&CacheEntry::list_node> m_clean_list; + mutable HashMap m_hash; NonnullOwnPtr m_cached_block_data; NonnullOwnPtr m_entries; }; @@ -116,6 +116,14 @@ BlockBasedFileSystem::BlockBasedFileSystem(OpenFileDescription& file_description BlockBasedFileSystem::~BlockBasedFileSystem() = default; +void BlockBasedFileSystem::remove_disk_cache_before_last_unmount() +{ + VERIFY(m_lock.is_locked()); + m_cache.with_exclusive([&](auto& cache) { + cache.clear(); + }); +} + ErrorOr BlockBasedFileSystem::initialize_while_locked() { VERIFY(m_lock.is_locked()); diff --git a/Kernel/FileSystem/BlockBasedFileSystem.h b/Kernel/FileSystem/BlockBasedFileSystem.h index 817747a482..58893d5e10 100644 --- a/Kernel/FileSystem/BlockBasedFileSystem.h +++ b/Kernel/FileSystem/BlockBasedFileSystem.h @@ -41,6 +41,8 @@ protected: u64 m_logical_block_size { 512 }; + void remove_disk_cache_before_last_unmount(); + private: DiskCache& cache() const; void flush_specific_block_if_needed(BlockIndex index); diff --git a/Kernel/FileSystem/Ext2FileSystem.cpp b/Kernel/FileSystem/Ext2FileSystem.cpp index e36f63e5f5..15aa39c1e2 100644 --- a/Kernel/FileSystem/Ext2FileSystem.cpp +++ b/Kernel/FileSystem/Ext2FileSystem.cpp @@ -140,7 +140,7 @@ ErrorOr Ext2FS::initialize_while_locked() } } - m_root_inode = static_ptr_cast(TRY(get_inode({ fsid(), EXT2_ROOT_INO }))); + m_root_inode = TRY(build_root_inode()); return {}; } @@ -770,10 +770,29 @@ ErrorOr Ext2FSInode::flush_metadata() return {}; } +ErrorOr> Ext2FS::build_root_inode() const +{ + MutexLocker locker(m_lock); + BlockIndex block_index; + unsigned offset; + if (!find_block_containing_inode(EXT2_ROOT_INO, block_index, offset)) + return EINVAL; + + auto inode = TRY(adopt_nonnull_lock_ref_or_enomem(new (nothrow) Ext2FSInode(const_cast(*this), EXT2_ROOT_INO))); + + auto buffer = UserOrKernelBuffer::for_kernel_buffer(reinterpret_cast(&inode->m_raw_inode)); + TRY(read_block(block_index, &buffer, sizeof(ext2_inode), offset)); + return inode; +} + ErrorOr> Ext2FS::get_inode(InodeIdentifier inode) const { MutexLocker locker(m_lock); VERIFY(inode.fsid() == fsid()); + VERIFY(m_root_inode); + + if (inode.index() == EXT2_ROOT_INO) + return *m_root_inode; { auto it = m_inode_cache.find(inode.index()); @@ -1690,12 +1709,12 @@ unsigned Ext2FS::free_inode_count() const ErrorOr Ext2FS::prepare_to_clear_last_mount() { MutexLocker locker(m_lock); - for (auto& it : m_inode_cache) { if (it.value->ref_count() > 1) return EBUSY; } + BlockBasedFileSystem::remove_disk_cache_before_last_unmount(); m_inode_cache.clear(); m_root_inode = nullptr; return {}; diff --git a/Kernel/FileSystem/Ext2FileSystem.h b/Kernel/FileSystem/Ext2FileSystem.h index 9d22d04a42..88bcf278c3 100644 --- a/Kernel/FileSystem/Ext2FileSystem.h +++ b/Kernel/FileSystem/Ext2FileSystem.h @@ -118,6 +118,8 @@ private: u64 blocks_per_group() const; u64 inode_size() const; + ErrorOr> build_root_inode() const; + ErrorOr write_ext2_inode(InodeIndex, ext2_inode const&); bool find_block_containing_inode(InodeIndex, BlockIndex& block_index, unsigned& offset) const; diff --git a/Kernel/FileSystem/FileSystem.cpp b/Kernel/FileSystem/FileSystem.cpp index 09f3bfc143..47d88ef485 100644 --- a/Kernel/FileSystem/FileSystem.cpp +++ b/Kernel/FileSystem/FileSystem.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -16,30 +17,14 @@ namespace Kernel { static u32 s_lastFileSystemID; -static Singleton> s_file_system_map; - -static HashMap& all_file_systems() -{ - return *s_file_system_map; -} FileSystem::FileSystem() : m_fsid(++s_lastFileSystemID) { - s_file_system_map->set(m_fsid, this); } FileSystem::~FileSystem() { - s_file_system_map->remove(m_fsid); -} - -FileSystem* FileSystem::from_fsid(FileSystemID id) -{ - auto it = all_file_systems().find(id); - if (it != all_file_systems().end()) - return (*it).value; - return nullptr; } ErrorOr FileSystem::prepare_to_unmount() @@ -61,23 +46,12 @@ FileSystem::DirectoryEntryView::DirectoryEntryView(StringView n, InodeIdentifier void FileSystem::sync() { Inode::sync_all(); - - NonnullLockRefPtrVector file_systems; - { - InterruptDisabler disabler; - for (auto& it : all_file_systems()) - file_systems.append(*it.value); - } - - for (auto& fs : file_systems) - fs.flush_writes(); + VirtualFileSystem::the().sync_filesystems(); } void FileSystem::lock_all() { - for (auto& it : all_file_systems()) { - it.value->m_lock.lock(); - } + VirtualFileSystem::the().lock_all_filesystems(); } } diff --git a/Kernel/FileSystem/FileSystem.h b/Kernel/FileSystem/FileSystem.h index 4098884062..39ae416ca6 100644 --- a/Kernel/FileSystem/FileSystem.h +++ b/Kernel/FileSystem/FileSystem.h @@ -20,12 +20,12 @@ namespace Kernel { class FileSystem : public AtomicRefCounted { friend class Inode; + friend class VirtualFileSystem; public: virtual ~FileSystem(); FileSystemID fsid() const { return m_fsid; } - static FileSystem* from_fsid(FileSystemID); static void sync(); static void lock_all(); @@ -80,18 +80,9 @@ private: bool m_readonly { false }; SpinlockProtected m_attach_count { LockRank::FileSystem, 0 }; + IntrusiveListNode m_file_system_node; }; -inline FileSystem* InodeIdentifier::fs() // NOLINT(readability-make-member-function-const) const InodeIdentifiers should not be able to modify the FileSystem -{ - return FileSystem::from_fsid(m_fsid); -} - -inline FileSystem const* InodeIdentifier::fs() const -{ - return FileSystem::from_fsid(m_fsid); -} - } namespace AK { diff --git a/Kernel/FileSystem/ISO9660FileSystem.cpp b/Kernel/FileSystem/ISO9660FileSystem.cpp index 3061f868a8..e9fd2bc448 100644 --- a/Kernel/FileSystem/ISO9660FileSystem.cpp +++ b/Kernel/FileSystem/ISO9660FileSystem.cpp @@ -235,6 +235,7 @@ u8 ISO9660FS::internal_file_type_to_directory_entry_type(DirectoryEntryView cons ErrorOr ISO9660FS::prepare_to_clear_last_mount() { // FIXME: Do proper cleaning here. + BlockBasedFileSystem::remove_disk_cache_before_last_unmount(); return {}; } diff --git a/Kernel/FileSystem/VirtualFileSystem.cpp b/Kernel/FileSystem/VirtualFileSystem.cpp index 3d1f11bcc5..6fe7161b12 100644 --- a/Kernel/FileSystem/VirtualFileSystem.cpp +++ b/Kernel/FileSystem/VirtualFileSystem.cpp @@ -111,6 +111,30 @@ ErrorOr VirtualFileSystem::remount(Custody& mount_point, int new_flags) return {}; } +void VirtualFileSystem::sync_filesystems() +{ + NonnullLockRefPtrVector file_systems; + m_file_systems_list.with([&](auto const& list) { + for (auto& fs : list) + file_systems.append(fs); + }); + + for (auto& fs : file_systems) + fs.flush_writes(); +} + +void VirtualFileSystem::lock_all_filesystems() +{ + NonnullLockRefPtrVector file_systems; + m_file_systems_list.with([&](auto const& list) { + for (auto& fs : list) + file_systems.append(fs); + }); + + for (auto& fs : file_systems) + fs.m_lock.lock(); +} + ErrorOr VirtualFileSystem::unmount(Custody& mountpoint_custody) { auto& guest_inode = mountpoint_custody.inode(); @@ -125,11 +149,27 @@ ErrorOr VirtualFileSystem::unmount(Custody& mountpoint_custody) auto mountpoint_path = TRY(mount->absolute_path()); if (custody_path->view() != mountpoint_path->view()) continue; - TRY(mount->guest_fs().prepare_to_unmount()); - mount->guest_fs().mounted_count({}).with([&](auto& mounted_count) { - mounted_count--; + NonnullRefPtr fs = mount->guest_fs(); + TRY(fs->prepare_to_unmount()); + fs->mounted_count({}).with([&](auto& mounted_count) { + VERIFY(mounted_count > 0); + if (mounted_count == 1) { + dbgln("VirtualFileSystem: Unmounting file system {} for the last time...", fs->fsid()); + m_file_systems_list.with([&](auto& list) { + list.remove(*fs); + }); + if (fs->is_file_backed()) { + dbgln("VirtualFileSystem: Unmounting file backed file system {} for the last time...", fs->fsid()); + auto& file_backed_fs = static_cast(*fs); + m_file_backed_file_systems_list.with([&](auto& list) { + list.remove(file_backed_fs); + }); + } + } else { + mounted_count--; + } }); - dbgln("VirtualFileSystem: Unmounting file system {}...", mount->guest_fs().fsid()); + dbgln("VirtualFileSystem: Unmounting file system {}...", fs->fsid()); (void)mounts.unstable_take(i); return {}; } @@ -154,15 +194,20 @@ ErrorOr VirtualFileSystem::mount_root(FileSystem& fs) } m_root_inode = root_inode; - auto pseudo_path = TRY(static_cast(fs).file_description().pseudo_path()); - dmesgln("VirtualFileSystem: mounted root({}) from {} ({})", fs.fsid(), fs.class_name(), pseudo_path); - if (fs.is_file_backed()) { + auto pseudo_path = TRY(static_cast(fs).file_description().pseudo_path()); + dmesgln("VirtualFileSystem: mounted root({}) from {} ({})", fs.fsid(), fs.class_name(), pseudo_path); m_file_backed_file_systems_list.with([&](auto& list) { list.append(static_cast(fs)); }); + } else { + dmesgln("VirtualFileSystem: mounted root({}) from {}", fs.fsid(), fs.class_name()); } + m_file_systems_list.with([&](auto& fs_list) { + fs_list.append(fs); + }); + // Note: Actually add a mount for the filesystem and increment the filesystem mounted count m_mounts.with([&](auto& mounts) { new_mount->guest_fs().mounted_count({}).with([&](auto& mounted_count) { @@ -278,6 +323,9 @@ ErrorOr> VirtualFileSystem::find_already auto fs = TRY(callback(description)); VERIFY(fs->is_file_backed()); list.append(static_cast(*fs)); + m_file_systems_list.with([&](auto& fs_list) { + fs_list.append(*fs); + }); return static_ptr_cast(fs); })); } diff --git a/Kernel/FileSystem/VirtualFileSystem.h b/Kernel/FileSystem/VirtualFileSystem.h index 514217e1e1..87decc58e5 100644 --- a/Kernel/FileSystem/VirtualFileSystem.h +++ b/Kernel/FileSystem/VirtualFileSystem.h @@ -77,6 +77,9 @@ public: InodeIdentifier root_inode_id() const; + void sync_filesystems(); + void lock_all_filesystems(); + static void sync(); NonnullRefPtr root_custody(); @@ -106,6 +109,7 @@ private: SpinlockProtected, 16>> m_mounts { LockRank::None }; SpinlockProtected> m_file_backed_file_systems_list { LockRank::None }; + SpinlockProtected> m_file_systems_list { LockRank::FileSystem }; }; }