From eef3631a5a29e0adf93b2064538dc672885a4693 Mon Sep 17 00:00:00 2001 From: Bananymous Date: Thu, 30 Mar 2023 22:02:16 +0300 Subject: [PATCH] Kernel: Improve locking in Process, VFS and ATAController We used to block on all process access. This meant that shell reading the keyboard input would block all VFS access making disk accesses practically impossible. We now block only when it is necessary :) --- kernel/include/kernel/FS/VirtualFileSystem.h | 11 +- kernel/include/kernel/Process.h | 2 +- kernel/include/kernel/Storage/ATAController.h | 1 + kernel/kernel/FS/VirtualFileSystem.cpp | 11 +- kernel/kernel/Process.cpp | 112 ++++++++++-------- kernel/kernel/Shell.cpp | 8 +- kernel/kernel/Storage/ATAController.cpp | 5 + 7 files changed, 90 insertions(+), 60 deletions(-) diff --git a/kernel/include/kernel/FS/VirtualFileSystem.h b/kernel/include/kernel/FS/VirtualFileSystem.h index 18463c03..49e0b324 100644 --- a/kernel/include/kernel/FS/VirtualFileSystem.h +++ b/kernel/include/kernel/FS/VirtualFileSystem.h @@ -1,10 +1,9 @@ #pragma once -#include #include -#include +#include #include -#include +#include namespace Kernel { @@ -16,9 +15,10 @@ namespace Kernel static VirtualFileSystem& get(); virtual ~VirtualFileSystem() {}; - virtual BAN::RefPtr root_inode() override { return m_root_fs->root_inode(); } + virtual BAN::RefPtr root_inode() override { return m_root_fs->root_inode(); } BAN::ErrorOr mount(BAN::StringView, BAN::StringView); + BAN::ErrorOr mount(FileSystem*, BAN::StringView); struct File { @@ -29,7 +29,6 @@ namespace Kernel private: VirtualFileSystem() = default; - BAN::ErrorOr mount(FileSystem*, BAN::StringView); struct MountPoint { @@ -39,9 +38,9 @@ namespace Kernel MountPoint* mount_point_for_inode(BAN::RefPtr); private: + SpinLock m_lock; FileSystem* m_root_fs = nullptr; BAN::Vector m_mount_points; - BAN::Vector m_storage_controllers; }; } \ No newline at end of file diff --git a/kernel/include/kernel/Process.h b/kernel/include/kernel/Process.h index 5d4facf7..7b8988ef 100644 --- a/kernel/include/kernel/Process.h +++ b/kernel/include/kernel/Process.h @@ -41,7 +41,7 @@ namespace Kernel BAN::ErrorOr> read_directory_entries(int); - BAN::String working_directory() const; + BAN::ErrorOr working_directory() const; BAN::ErrorOr set_working_directory(BAN::StringView); static BAN::RefPtr current() { return Thread::current().process(); } diff --git a/kernel/include/kernel/Storage/ATAController.h b/kernel/include/kernel/Storage/ATAController.h index 9ced08cf..2496f7b9 100644 --- a/kernel/include/kernel/Storage/ATAController.h +++ b/kernel/include/kernel/Storage/ATAController.h @@ -87,6 +87,7 @@ namespace Kernel private: ATABus m_buses[2]; const PCIDevice& m_pci_device; + SpinLock m_lock; friend class ATADevice; diff --git a/kernel/kernel/FS/VirtualFileSystem.cpp b/kernel/kernel/FS/VirtualFileSystem.cpp index 85bfe4a3..95ac7ead 100644 --- a/kernel/kernel/FS/VirtualFileSystem.cpp +++ b/kernel/kernel/FS/VirtualFileSystem.cpp @@ -1,11 +1,10 @@ #include #include -#include #include #include #include +#include #include -#include namespace Kernel { @@ -45,9 +44,11 @@ namespace Kernel auto partition_file = TRY(file_from_absolute_path(partition)); if (partition_file.inode->inode_type() != Inode::InodeType::Device) return BAN::Error::from_c_string("Not a partition"); + Device* device = (Device*)partition_file.inode.ptr(); if (device->device_type() != Device::DeviceType::Partition) return BAN::Error::from_c_string("Not a partition"); + auto* file_system = TRY(Ext2FS::create(*(Partition*)device)); return mount(file_system, target); } @@ -57,12 +58,16 @@ namespace Kernel auto file = TRY(file_from_absolute_path(path)); if (!file.inode->mode().ifdir()) return BAN::Error::from_errno(ENOTDIR); + + LockGuard _(m_lock); TRY(m_mount_points.push_back({ file, file_system })); + return {}; } VirtualFileSystem::MountPoint* VirtualFileSystem::mount_point_for_inode(BAN::RefPtr inode) { + ASSERT(m_lock.is_locked()); for (MountPoint& mount : m_mount_points) if (*mount.host.inode == *inode) return &mount; @@ -71,6 +76,8 @@ namespace Kernel BAN::ErrorOr VirtualFileSystem::file_from_absolute_path(BAN::StringView path) { + LockGuard _(m_lock); + ASSERT(path.front() == '/'); auto inode = root_inode(); diff --git a/kernel/kernel/Process.cpp b/kernel/kernel/Process.cpp index efbe7b6f..9d580a14 100644 --- a/kernel/kernel/Process.cpp +++ b/kernel/kernel/Process.cpp @@ -20,9 +20,9 @@ namespace Kernel BAN::ErrorOr Process::add_thread(entry_t entry, void* data) { - LockGuard _(m_lock); - Thread* thread = TRY(Thread::create(entry, data, this)); + + LockGuard _(m_lock); TRY(m_threads.push_back(thread)); if (auto res = Scheduler::get().add_thread(thread); res.is_error()) { @@ -43,8 +43,6 @@ namespace Kernel BAN::ErrorOr Process::open(BAN::StringView path, int flags) { - LockGuard _(m_lock); - if (flags != O_RDONLY) return BAN::Error::from_errno(ENOTSUP); @@ -52,6 +50,7 @@ namespace Kernel auto file = TRY(VirtualFileSystem::get().file_from_absolute_path(absolute_path)); + LockGuard _(m_lock); int fd = TRY(get_free_fd()); auto& open_file_description = m_open_files[fd]; open_file_description.inode = file.inode; @@ -73,63 +72,70 @@ namespace Kernel BAN::ErrorOr Process::read(int fd, void* buffer, size_t count) { - LockGuard _(m_lock); + m_lock.lock(); TRY(validate_fd(fd)); - auto& open_fd = open_file_description(fd); - if (!(open_fd.flags & O_RDONLY)) + auto open_fd_copy = open_file_description(fd); + m_lock.unlock(); + + if (!(open_fd_copy.flags & O_RDONLY)) return BAN::Error::from_errno(EBADF); - size_t n_read = TRY(open_fd.inode->read(open_fd.offset, buffer, count)); - open_fd.offset += n_read; + size_t n_read = TRY(open_fd_copy.inode->read(open_fd_copy.offset, buffer, count)); + open_fd_copy.offset += n_read; + + m_lock.lock(); + MUST(validate_fd(fd)); + open_file_description(fd) = open_fd_copy; + m_lock.unlock(); + return n_read; } BAN::ErrorOr Process::creat(BAN::StringView path, mode_t mode) { - LockGuard _(m_lock); auto absolute_path = TRY(absolute_path_of(path)); - while (absolute_path.sv().back() != '/') + while (absolute_path.back() != '/') absolute_path.pop_back(); - auto parent_inode = TRY(VirtualFileSystem::get().file_from_absolute_path(absolute_path)); + + auto parent_file = TRY(VirtualFileSystem::get().file_from_absolute_path(absolute_path)); if (path.count('/') > 0) return BAN::Error::from_c_string("You can only create files to current working directory"); - TRY(parent_inode.inode->create_file(path, mode)); + TRY(parent_file.inode->create_file(path, mode)); + return {}; } BAN::ErrorOr Process::mount(BAN::StringView partition, BAN::StringView path) { - LockGuard _(m_lock); TRY(VirtualFileSystem::get().mount(partition, path)); return {}; } BAN::ErrorOr Process::fstat(int fd, struct stat* out) { - LockGuard _(m_lock); - + m_lock.lock(); TRY(validate_fd(fd)); - const auto& open_fd = open_file_description(fd); + auto open_fd_copy = open_file_description(fd); + m_lock.unlock(); - out->st_dev = open_fd.inode->dev(); - out->st_ino = open_fd.inode->ino(); - out->st_mode = open_fd.inode->mode().mode; - out->st_nlink = open_fd.inode->nlink(); - out->st_uid = open_fd.inode->uid(); - out->st_gid = open_fd.inode->gid(); - out->st_rdev = open_fd.inode->rdev(); - out->st_size = open_fd.inode->size(); - out->st_atim = open_fd.inode->atime(); - out->st_mtim = open_fd.inode->mtime(); - out->st_ctim = open_fd.inode->ctime(); - out->st_blksize = open_fd.inode->blksize(); - out->st_blocks = open_fd.inode->blocks(); + out->st_dev = open_fd_copy.inode->dev(); + out->st_ino = open_fd_copy.inode->ino(); + out->st_mode = open_fd_copy.inode->mode().mode; + out->st_nlink = open_fd_copy.inode->nlink(); + out->st_uid = open_fd_copy.inode->uid(); + out->st_gid = open_fd_copy.inode->gid(); + out->st_rdev = open_fd_copy.inode->rdev(); + out->st_size = open_fd_copy.inode->size(); + out->st_atim = open_fd_copy.inode->atime(); + out->st_mtim = open_fd_copy.inode->mtime(); + out->st_ctim = open_fd_copy.inode->ctime(); + out->st_blksize = open_fd_copy.inode->blksize(); + out->st_blocks = open_fd_copy.inode->blocks(); return {}; } BAN::ErrorOr Process::stat(BAN::StringView path, struct stat* out) { - LockGuard _(m_lock); int fd = TRY(open(path, O_RDONLY)); auto ret = fstat(fd, out); MUST(close(fd)); @@ -138,30 +144,41 @@ namespace Kernel BAN::ErrorOr> Process::read_directory_entries(int fd) { - LockGuard _(m_lock); + m_lock.lock(); TRY(validate_fd(fd)); - auto& open_fd = open_file_description(fd); - auto result = TRY(open_fd.inode->read_directory_entries(open_fd.offset)); - open_fd.offset++; + auto open_fd_copy = open_file_description(fd); + m_lock.unlock(); + + auto result = TRY(open_fd_copy.inode->read_directory_entries(open_fd_copy.offset)); + open_fd_copy.offset++; + + m_lock.lock(); + MUST(validate_fd(fd)); + open_file_description(fd) = open_fd_copy; + m_lock.unlock(); + return result; } - BAN::String Process::working_directory() const + BAN::ErrorOr Process::working_directory() const { + BAN::String result; + LockGuard _(m_lock); - return m_working_directory; + TRY(result.append(m_working_directory)); + + return result; } BAN::ErrorOr Process::set_working_directory(BAN::StringView path) { - LockGuard _(m_lock); - BAN::String absolute_path = TRY(absolute_path_of(path)); auto file = TRY(VirtualFileSystem::get().file_from_absolute_path(absolute_path)); if (!file.inode->mode().ifdir()) return BAN::Error::from_errno(ENOTDIR); + LockGuard _(m_lock); m_working_directory = BAN::move(file.canonical_path); return {}; @@ -169,24 +186,23 @@ namespace Kernel BAN::ErrorOr Process::absolute_path_of(BAN::StringView path) const { - LockGuard _(m_lock); - if (path.empty()) - return m_working_directory; + return working_directory(); BAN::String absolute_path; if (path.front() != '/') { + LockGuard _(m_lock); TRY(absolute_path.append(m_working_directory)); - if (m_working_directory.sv().back() != '/') - TRY(absolute_path.push_back('/')); } + if (!absolute_path.empty() && absolute_path.back() != '/') + TRY(absolute_path.push_back('/')); TRY(absolute_path.append(path)); return absolute_path; } BAN::ErrorOr Process::validate_fd(int fd) { - LockGuard _(m_lock); + ASSERT(m_lock.is_locked()); if (fd < 0 || m_open_files.size() <= (size_t)fd || !m_open_files[fd].inode) return BAN::Error::from_errno(EBADF); return {}; @@ -194,16 +210,14 @@ namespace Kernel Process::OpenFileDescription& Process::open_file_description(int fd) { - LockGuard _(m_lock); - + ASSERT(m_lock.is_locked()); MUST(validate_fd(fd)); return m_open_files[fd]; } BAN::ErrorOr Process::get_free_fd() { - LockGuard _(m_lock); - + ASSERT(m_lock.is_locked()); for (size_t fd = 0; fd < m_open_files.size(); fd++) if (!m_open_files[fd].inode) return fd; diff --git a/kernel/kernel/Shell.cpp b/kernel/kernel/Shell.cpp index 965b85e1..429661a9 100644 --- a/kernel/kernel/Shell.cpp +++ b/kernel/kernel/Shell.cpp @@ -77,7 +77,7 @@ namespace Kernel break; case 'w': { - auto working_directory = Process::current()->working_directory(); + auto working_directory = TRY(Process::current()->working_directory()); TRY(m_prompt.append(working_directory)); m_prompt_length += working_directory.size(); break; @@ -379,7 +379,11 @@ argument_done: if (arguments.size() > 2) return BAN::Error::from_c_string("usage: 'ls [path]'"); - BAN::String path = (arguments.size() == 2) ? arguments[1] : Process::current()->working_directory(); + BAN::String path; + if (arguments.size() == 2) + TRY(path.append(arguments[1])); + else + TRY(path.append(TRY(Process::current()->working_directory()))); int fd = TRY(Process::current()->open(path, O_RDONLY)); BAN::ScopeGuard _([fd] { MUST(Process::current()->close(fd)); }); diff --git a/kernel/kernel/Storage/ATAController.cpp b/kernel/kernel/Storage/ATAController.cpp index 230360d0..d083f39d 100644 --- a/kernel/kernel/Storage/ATAController.cpp +++ b/kernel/kernel/Storage/ATAController.cpp @@ -1,4 +1,5 @@ #include +#include #include #define ATA_PRIMARY 0 @@ -203,6 +204,8 @@ namespace Kernel if (lba + sector_count > device->lba_count) return BAN::Error::from_c_string("Attempted to read outside of the device boundaries"); + LockGuard _(m_lock); + if (lba < (1 << 28)) { // LBA28 @@ -233,6 +236,8 @@ namespace Kernel if (lba + sector_count > device->lba_count) return BAN::Error::from_c_string("Attempted to read outside of the device boundaries"); + LockGuard _(m_lock); + if (lba < (1 << 28)) { // LBA28