From ee4e9b807ab6bf80bb460079f72e30d0d90d7f02 Mon Sep 17 00:00:00 2001 From: Liav A Date: Fri, 21 Apr 2023 14:48:11 +0300 Subject: [PATCH] Kernel: Protect internal structures in InodeWatcher with spinlocks This was the last change that was needed to be able boot with the flag of LOCK_IN_CRITICAL_DEBUG. That flag is not always enabled because there are still other issues in which we hold a spinlock and still try to lock a mutex. Instead of using one global mutex we can protect internal structures of the InodeWatcher class with SpinlockProtected wrappers. This in turn allows the InodeWatcher code to be called from other parts in the kernel while holding a prior spinlock properly. --- Kernel/FileSystem/InodeWatcher.cpp | 147 +++++++++++++++-------------- Kernel/FileSystem/InodeWatcher.h | 14 ++- 2 files changed, 85 insertions(+), 76 deletions(-) diff --git a/Kernel/FileSystem/InodeWatcher.cpp b/Kernel/FileSystem/InodeWatcher.cpp index 8a62d4c95c..4daf649633 100644 --- a/Kernel/FileSystem/InodeWatcher.cpp +++ b/Kernel/FileSystem/InodeWatcher.cpp @@ -23,18 +23,19 @@ InodeWatcher::~InodeWatcher() bool InodeWatcher::can_read(OpenFileDescription const&, u64) const { - MutexLocker locker(m_lock); - return !m_queue.is_empty(); + return m_queue.with([](auto& queue) { return !queue.is_empty(); }); } ErrorOr InodeWatcher::read(OpenFileDescription&, u64, UserOrKernelBuffer& buffer, size_t buffer_size) { - MutexLocker locker(m_lock); - if (m_queue.is_empty()) - // can_read will catch the blocking case. - return EAGAIN; + auto event = TRY(m_queue.with([](auto& queue) -> ErrorOr { + if (queue.is_empty()) { + // can_read will catch the blocking case. + return EAGAIN; + } - auto event = m_queue.dequeue(); + return queue.dequeue(); + })); size_t bytes_to_write = sizeof(InodeWatcherEvent); if (event.path) @@ -68,106 +69,110 @@ ErrorOr InodeWatcher::read(OpenFileDescription&, u64, UserOrKernelBuffer ErrorOr InodeWatcher::close() { - MutexLocker locker(m_lock); + m_watch_maps.with([this](auto& watch_maps) { + for (auto& entry : watch_maps.wd_to_watches) { + auto& inode = const_cast(entry.value->inode); + inode.unregister_watcher({}, *this); + } - for (auto& entry : m_wd_to_watches) { - auto& inode = const_cast(entry.value->inode); - inode.unregister_watcher({}, *this); - } - - m_wd_to_watches.clear(); - m_inode_to_watches.clear(); + watch_maps.inode_to_watches.clear(); + watch_maps.wd_to_watches.clear(); + }); return {}; } ErrorOr> InodeWatcher::pseudo_path(OpenFileDescription const&) const { - return KString::formatted("InodeWatcher:({})", m_wd_to_watches.size()); + return m_watch_maps.with([](auto& watch_maps) -> ErrorOr> { + return KString::formatted("InodeWatcher:({})", watch_maps.wd_to_watches.size()); + }); } void InodeWatcher::notify_inode_event(Badge, InodeIdentifier inode_id, InodeWatcherEvent::Type event_type, StringView name) { - MutexLocker locker(m_lock); + m_watch_maps.with([this, inode_id, event_type, name](auto& watch_maps) { + auto it = watch_maps.inode_to_watches.find(inode_id); + if (it == watch_maps.inode_to_watches.end()) + return; - auto it = m_inode_to_watches.find(inode_id); - if (it == m_inode_to_watches.end()) - return; + auto& watcher = *it->value; + if (!(watcher.event_mask & static_cast(event_type))) + return; - auto& watcher = *it->value; - if (!(watcher.event_mask & static_cast(event_type))) - return; + m_queue.with([watcher, event_type, name](auto& queue) { + OwnPtr path; + if (!name.is_null()) + path = KString::try_create(name).release_value_but_fixme_should_propagate_errors(); + queue.enqueue({ watcher.wd, event_type, move(path) }); + }); + }); - OwnPtr path; - if (!name.is_null()) - path = KString::try_create(name).release_value_but_fixme_should_propagate_errors(); - m_queue.enqueue({ watcher.wd, event_type, move(path) }); evaluate_block_conditions(); } ErrorOr InodeWatcher::register_inode(Inode& inode, unsigned event_mask) { - MutexLocker locker(m_lock); + return m_watch_maps.with([this, &inode, event_mask](auto& watch_maps) -> ErrorOr { + if (watch_maps.inode_to_watches.find(inode.identifier()) != watch_maps.inode_to_watches.end()) + return EEXIST; - if (m_inode_to_watches.find(inode.identifier()) != m_inode_to_watches.end()) - return EEXIST; + int wd = -1; + do { + wd = m_wd_counter.value(); - int wd; - do { - wd = m_wd_counter.value(); + m_wd_counter++; + if (m_wd_counter.has_overflow()) + m_wd_counter = 1; + } while (watch_maps.wd_to_watches.find(wd) != watch_maps.wd_to_watches.end()); - m_wd_counter++; - if (m_wd_counter.has_overflow()) - m_wd_counter = 1; - } while (m_wd_to_watches.find(wd) != m_wd_to_watches.end()); + auto description = TRY(WatchDescription::create(wd, inode, event_mask)); - auto description = TRY(WatchDescription::create(wd, inode, event_mask)); + TRY(watch_maps.inode_to_watches.try_set(inode.identifier(), description.ptr())); + auto set_result = watch_maps.wd_to_watches.try_set(wd, move(description)); + if (set_result.is_error()) { + watch_maps.inode_to_watches.remove(inode.identifier()); + return set_result.release_error(); + } - TRY(m_inode_to_watches.try_set(inode.identifier(), description.ptr())); - auto set_result = m_wd_to_watches.try_set(wd, move(description)); - if (set_result.is_error()) { - m_inode_to_watches.remove(inode.identifier()); - return set_result.release_error(); - } + auto register_result = inode.register_watcher({}, *this); + if (register_result.is_error()) { + watch_maps.inode_to_watches.remove(inode.identifier()); + watch_maps.wd_to_watches.remove(wd); + return register_result.release_error(); + } - auto register_result = inode.register_watcher({}, *this); - if (register_result.is_error()) { - m_inode_to_watches.remove(inode.identifier()); - m_wd_to_watches.remove(wd); - return register_result.release_error(); - } - - return wd; + return wd; + }); } ErrorOr InodeWatcher::unregister_by_wd(int wd) { - MutexLocker locker(m_lock); + TRY(m_watch_maps.with([this, wd](auto& watch_maps) -> ErrorOr { + auto it = watch_maps.wd_to_watches.find(wd); + if (it == watch_maps.wd_to_watches.end()) + return ENOENT; - auto it = m_wd_to_watches.find(wd); - if (it == m_wd_to_watches.end()) - return ENOENT; - - auto& inode = it->value->inode; - inode.unregister_watcher({}, *this); - - m_inode_to_watches.remove(inode.identifier()); - m_wd_to_watches.remove(it); + auto& inode = it->value->inode; + inode.unregister_watcher({}, *this); + watch_maps.inode_to_watches.remove(inode.identifier()); + watch_maps.wd_to_watches.remove(it); + return {}; + })); return {}; } void InodeWatcher::unregister_by_inode(Badge, InodeIdentifier identifier) { - MutexLocker locker(m_lock); + m_watch_maps.with([identifier](auto& watch_maps) { + auto it = watch_maps.inode_to_watches.find(identifier); + if (it == watch_maps.inode_to_watches.end()) + return; - auto it = m_inode_to_watches.find(identifier); - if (it == m_inode_to_watches.end()) - return; - - // NOTE: no need to call unregister_watcher here, the Inode calls us. - - m_inode_to_watches.remove(identifier); - m_wd_to_watches.remove(it->value->wd); + // NOTE: no need to call unregister_watcher here, the Inode calls us. + watch_maps.inode_to_watches.remove(identifier); + watch_maps.wd_to_watches.remove(it->value->wd); + }); } } diff --git a/Kernel/FileSystem/InodeWatcher.h b/Kernel/FileSystem/InodeWatcher.h index fb01da67e4..8b8f23cec4 100644 --- a/Kernel/FileSystem/InodeWatcher.h +++ b/Kernel/FileSystem/InodeWatcher.h @@ -15,6 +15,8 @@ #include #include #include +#include +#include namespace Kernel { @@ -63,20 +65,22 @@ public: private: explicit InodeWatcher() { } - mutable Mutex m_lock; - struct Event { int wd { 0 }; InodeWatcherEvent::Type type { InodeWatcherEvent::Type::Invalid }; OwnPtr path; }; - CircularQueue m_queue; + SpinlockProtected, LockRank::None> m_queue; Checked m_wd_counter { 1 }; // NOTE: These two hashmaps provide two different ways of reaching the same // watch description, so they will overlap. - HashMap> m_wd_to_watches; - HashMap m_inode_to_watches; + struct WatchMaps { + HashMap> wd_to_watches; + HashMap inode_to_watches; + }; + + mutable SpinlockProtected m_watch_maps; }; }