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

@@ -1,10 +1,9 @@
 #pragma once
 
-#include <BAN/HashMap.h>
 #include <BAN/String.h>
-#include <BAN/StringView.h>
+#include <BAN/Vector.h>
 #include <kernel/FS/FileSystem.h>
-#include <kernel/Storage/StorageController.h>
+#include <kernel/SpinLock.h>
 
 namespace Kernel
 {
@@ -16,9 +15,10 @@ namespace Kernel
 		static VirtualFileSystem& get();
 		virtual ~VirtualFileSystem() {};
 
-		virtual BAN::RefPtr<Inode> root_inode() override  { return m_root_fs->root_inode(); }
+		virtual BAN::RefPtr<Inode> root_inode() override { return m_root_fs->root_inode(); }
 
 		BAN::ErrorOr<void> mount(BAN::StringView, BAN::StringView);
+		BAN::ErrorOr<void> mount(FileSystem*, BAN::StringView);
 
 		struct File
 		{
@@ -29,7 +29,6 @@ namespace Kernel
 
 	private:
 		VirtualFileSystem() = default;
-		BAN::ErrorOr<void> mount(FileSystem*, BAN::StringView);
 
 		struct MountPoint
 		{
@@ -39,9 +38,9 @@ namespace Kernel
 		MountPoint* mount_point_for_inode(BAN::RefPtr<Inode>);
 
 	private:
+		SpinLock						m_lock;
 		FileSystem*						m_root_fs = nullptr;
 		BAN::Vector<MountPoint>			m_mount_points;
-		BAN::Vector<StorageController*>	m_storage_controllers;
 	};
 
 }

kernel/include/kernel/Process.h

@@ -41,7 +41,7 @@ namespace Kernel
 
 		BAN::ErrorOr<BAN::Vector<BAN::String>> read_directory_entries(int);
 
-		BAN::String working_directory() const;
+		BAN::ErrorOr<BAN::String> working_directory() const;
 		BAN::ErrorOr<void> set_working_directory(BAN::StringView);
 
 		static BAN::RefPtr<Process> current() { return Thread::current().process(); }

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;
 

kernel/kernel/FS/VirtualFileSystem.cpp

@@ -1,11 +1,10 @@
 #include <BAN/ScopeGuard.h>
 #include <BAN/StringView.h>
-#include <BAN/Vector.h>
 #include <kernel/Device.h>
 #include <kernel/FS/Ext2.h>
 #include <kernel/FS/VirtualFileSystem.h>
+#include <kernel/LockGuard.h>
 #include <kernel/PCI.h>
-#include <kernel/Storage/ATAController.h>
 
 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> 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> VirtualFileSystem::file_from_absolute_path(BAN::StringView path)
 	{
+		LockGuard _(m_lock);
+
 		ASSERT(path.front() == '/');
 
 		auto inode = root_inode();

kernel/kernel/Process.cpp

@@ -20,9 +20,9 @@ namespace Kernel
 
 	BAN::ErrorOr<void> 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<int> 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<size_t> 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<void> 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<void> Process::mount(BAN::StringView partition, BAN::StringView path)
 	{
-		LockGuard _(m_lock);
 		TRY(VirtualFileSystem::get().mount(partition, path));
 		return {};
 	}
 
 	BAN::ErrorOr<void> 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<void> 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<BAN::Vector<BAN::String>> 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<BAN::String> Process::working_directory() const
 	{
+		BAN::String result;
+
 		LockGuard _(m_lock);
-		return m_working_directory;
+		TRY(result.append(m_working_directory));
+		
+		return result;
 	}
 
 	BAN::ErrorOr<void> 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<BAN::String> 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<void> 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<int> 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;

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)); });

kernel/kernel/Storage/ATAController.cpp

@@ -1,4 +1,5 @@
 #include <kernel/IO.h>
+#include <kernel/LockGuard.h>
 #include <kernel/Storage/ATAController.h>
 
 #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